ovl: Simplify ovl_lookup_real_one()

The primary purpose of this patch is to remove the locking from
ovl_lookup_real_one() as part of centralising all locking of directories
for name operations.

The locking here isn't needed. By performing consistency tests after
the lookup we can be sure that the result of the lookup was valid at
least for a moment, which is all the original code promised.

lookup_noperm_unlocked() is used for the lookup and it will take the
lock if needed only where it is needed.

Also:
- don't take a reference to real->d_parent. The parent is
only use for a pointer comparison, and no reference is needed for
that.
- Several "if" statements have a "goto" followed by "else" - the
else isn't needed: the following statement can directly follow
the "if" as a new statement
- Use a consistent pattern of setting "err" before performing a test
and possibly going to "fail".
- remove the "out" label (now that we don't need to dput(parent) or
unlock) and simply return from fail:.

Reviewed-by: Jeff Layton <jlayton@kernel.org>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: NeilBrown <neil@brown.name>
Link: https://patch.msgid.link/20260224222542.3458677-10-neilb@ownmail.net
Signed-off-by: Christian Brauner <brauner@kernel.org>

authored by

NeilBrown and committed by

Christian Brauner 3 months ago 2c3a9eb1 336faf5d

+37 -42

1 changed file

expand all

overlayfs

export.c

+37 -42

fs/overlayfs/export.c

··· 349 349 return NULL; 350 350 } 351 351 352 - /* 353 - * Lookup a child overlay dentry to get a connected overlay dentry whose real 354 - * dentry is @real. If @real is on upper layer, we lookup a child overlay 355 - * dentry with the same name as the real dentry. Otherwise, we need to consult 356 - * index for lookup. 352 + /** 353 + * ovl_lookup_real_one - Lookup a child overlay dentry to get an overlay dentry whose real dentry is given 354 + * @connected: parent overlay dentry 355 + * @real: given child real dentry 356 + * @layer: layer in which @real exists 357 + * 358 + * 359 + * Lookup a child overlay dentry in @connected with the same name as the @real 360 + * dentry. Then check that the parent of the result is the real dentry for 361 + * @connected, and @real is the real dentry for the result. 362 + * 363 + * Returns: 364 + * %-ECHILD if the parent of @real is no longer the real dentry for @connected. 365 + * %-ESTALE if @real is not the real dentry of the found dentry. 366 + * Otherwise the found dentry is returned. 357 367 */ 358 368 static struct dentry *ovl_lookup_real_one(struct dentry *connected, 359 369 struct dentry *real, 360 370 const struct ovl_layer *layer) 361 371 { 362 - struct inode *dir = d_inode(connected); 363 - struct dentry *this, *parent = NULL; 372 + struct dentry *this; 364 373 struct name_snapshot name; 365 374 int err; 366 375 367 376 /* 368 - * Lookup child overlay dentry by real name. The dir mutex protects us 369 - * from racing with overlay rename. If the overlay dentry that is above 370 - * real has already been moved to a parent that is not under the 371 - * connected overlay dir, we return -ECHILD and restart the lookup of 372 - * connected real path from the top. 373 - */ 374 - inode_lock_nested(dir, I_MUTEX_PARENT); 375 - err = -ECHILD; 376 - parent = dget_parent(real); 377 - if (ovl_dentry_real_at(connected, layer->idx) != parent) 378 - goto fail; 379 - 380 - /* 381 - * We also need to take a snapshot of real dentry name to protect us 377 + * We need to take a snapshot of real dentry name to protect us 382 378 * from racing with underlying layer rename. In this case, we don't 383 379 * care about returning ESTALE, only from dereferencing a free name 384 380 * pointer because we hold no lock on the real dentry. 385 381 */ 386 382 take_dentry_name_snapshot(&name, real); 387 - /* 388 - * No idmap handling here: it's an internal lookup. 389 - */ 390 - this = lookup_noperm(&name.name, connected); 383 + this = lookup_noperm_unlocked(&name.name, connected); 391 384 release_dentry_name_snapshot(&name); 392 - err = PTR_ERR(this); 393 - if (IS_ERR(this)) { 394 - goto fail; 395 - } else if (!this || !this->d_inode) { 396 - dput(this); 397 - err = -ENOENT; 398 - goto fail; 399 - } else if (ovl_dentry_real_at(this, layer->idx) != real) { 400 - dput(this); 401 - err = -ESTALE; 402 - goto fail; 403 - } 404 385 405 - out: 406 - dput(parent); 407 - inode_unlock(dir); 386 + err = -ECHILD; 387 + if (ovl_dentry_real_at(connected, layer->idx) != real->d_parent) 388 + goto fail; 389 + 390 + err = PTR_ERR(this); 391 + if (IS_ERR(this)) 392 + goto fail; 393 + 394 + err = -ENOENT; 395 + if (!this || !this->d_inode) 396 + goto fail; 397 + 398 + err = -ESTALE; 399 + if (ovl_dentry_real_at(this, layer->idx) != real) 400 + goto fail; 401 + 408 402 return this; 409 403 410 404 fail: 411 405 pr_warn_ratelimited("failed to lookup one by real (%pd2, layer=%d, connected=%pd2, err=%i)\n", 412 406 real, layer->idx, connected, err); 413 - this = ERR_PTR(err); 414 - goto out; 407 + if (!IS_ERR(this)) 408 + dput(this); 409 + return ERR_PTR(err); 415 410 } 416 411 417 412 static struct dentry *ovl_lookup_real(struct super_block *sb,

Configure Feed

Configure Feed