| Message ID | 20250217003020.3170652-2-neilb@suse.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | VFS: minor improvements to a couple of interfaces | expand |
On Mon, 17 Feb 2025 11:27:20 +1100, NeilBrown wrote: > No callers of kern_path_locked() or user_path_locked_at() want a > negative dentry. So change them to return -ENOENT instead. This > simplifies callers. > > This results in a subtle change to bcachefs in that an ioctl will now > return -ENOENT in preference to -EXDEV. I believe this restores the > behaviour to what it was prior to > Commit bbe6a7c899e7 ("bch2_ioctl_subvolume_destroy(): fix locking") > > [...] Applied to the vfs-6.15.async.dir branch of the vfs/vfs.git tree. Patches in the vfs-6.15.async.dir branch should appear in linux-next soon. Please report any outstanding bugs that were missed during review in a new review to the original patch series allowing us to drop it. It's encouraged to provide Acked-bys and Reviewed-bys even though the patch has now been applied. If possible patch trailers will be updated. Note that commit hashes shown below are subject to change due to rebase, trailer updates or similar. If in doubt, please check the listed branch. tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git branch: vfs-6.15.async.dir [1/2] VFS: change kern_path_locked() and user_path_locked_at() to never return negative dentry https://git.kernel.org/vfs/vfs/c/a97b8bfbb9f1 [2/2] VFS: add common error checks to lookup_one_qstr_excl() https://git.kernel.org/vfs/vfs/c/20c2c1baa9ab
On Mon, 2025-02-17 at 11:27 +1100, NeilBrown wrote: > No callers of kern_path_locked() or user_path_locked_at() want a > negative dentry. So change them to return -ENOENT instead. This > simplifies callers. > > This results in a subtle change to bcachefs in that an ioctl will now > return -ENOENT in preference to -EXDEV. I believe this restores the > behaviour to what it was prior to > Commit bbe6a7c899e7 ("bch2_ioctl_subvolume_destroy(): fix locking") > > Signed-off-by: NeilBrown <neilb@suse.de> > Link: https://lore.kernel.org/r/20250207034040.3402438-2-neilb@suse.de > Acked-by: Paul Moore <paul@paul-moore.com> > Signed-off-by: Christian Brauner <brauner@kernel.org> > --- > Documentation/filesystems/porting.rst | 8 ++++ > drivers/base/devtmpfs.c | 65 +++++++++++++-------------- > fs/bcachefs/fs-ioctl.c | 4 -- > fs/namei.c | 4 ++ > kernel/audit_watch.c | 12 ++--- > 5 files changed, 48 insertions(+), 45 deletions(-) > > diff --git a/Documentation/filesystems/porting.rst b/Documentation/filesystems/porting.rst > index 1639e78e3146..2ead47e20677 100644 > --- a/Documentation/filesystems/porting.rst > +++ b/Documentation/filesystems/porting.rst > @@ -1157,3 +1157,11 @@ in normal case it points into the pathname being looked up. > NOTE: if you need something like full path from the root of filesystem, > you are still on your own - this assists with simple cases, but it's not > magic. > + > +--- > + > +** recommended** > + > +kern_path_locked() and user_path_locked() no longer return a negative > +dentry so this doesn't need to be checked. If the name cannot be found, > +ERR_PTR(-ENOENT) is returned. > diff --git a/drivers/base/devtmpfs.c b/drivers/base/devtmpfs.c > index b848764ef018..c9e34842139f 100644 > --- a/drivers/base/devtmpfs.c > +++ b/drivers/base/devtmpfs.c > @@ -245,15 +245,12 @@ static int dev_rmdir(const char *name) > dentry = kern_path_locked(name, &parent); > if (IS_ERR(dentry)) > return PTR_ERR(dentry); > - if (d_really_is_positive(dentry)) { > - if (d_inode(dentry)->i_private == &thread) > - err = vfs_rmdir(&nop_mnt_idmap, d_inode(parent.dentry), > - dentry); > - else > - err = -EPERM; > - } else { > - err = -ENOENT; > - } > + if (d_inode(dentry)->i_private == &thread) > + err = vfs_rmdir(&nop_mnt_idmap, d_inode(parent.dentry), > + dentry); > + else > + err = -EPERM; > + > dput(dentry); > inode_unlock(d_inode(parent.dentry)); > path_put(&parent); > @@ -310,6 +307,8 @@ static int handle_remove(const char *nodename, struct device *dev) > { > struct path parent; > struct dentry *dentry; > + struct kstat stat; > + struct path p; > int deleted = 0; > int err; > > @@ -317,32 +316,28 @@ static int handle_remove(const char *nodename, struct device *dev) > if (IS_ERR(dentry)) > return PTR_ERR(dentry); > > - if (d_really_is_positive(dentry)) { > - struct kstat stat; > - struct path p = {.mnt = parent.mnt, .dentry = dentry}; > - err = vfs_getattr(&p, &stat, STATX_TYPE | STATX_MODE, > - AT_STATX_SYNC_AS_STAT); > - if (!err && dev_mynode(dev, d_inode(dentry), &stat)) { > - struct iattr newattrs; > - /* > - * before unlinking this node, reset permissions > - * of possible references like hardlinks > - */ > - newattrs.ia_uid = GLOBAL_ROOT_UID; > - newattrs.ia_gid = GLOBAL_ROOT_GID; > - newattrs.ia_mode = stat.mode & ~0777; > - newattrs.ia_valid = > - ATTR_UID|ATTR_GID|ATTR_MODE; > - inode_lock(d_inode(dentry)); > - notify_change(&nop_mnt_idmap, dentry, &newattrs, NULL); > - inode_unlock(d_inode(dentry)); > - err = vfs_unlink(&nop_mnt_idmap, d_inode(parent.dentry), > - dentry, NULL); > - if (!err || err == -ENOENT) > - deleted = 1; > - } > - } else { > - err = -ENOENT; > + p.mnt = parent.mnt; > + p.dentry = dentry; > + err = vfs_getattr(&p, &stat, STATX_TYPE | STATX_MODE, > + AT_STATX_SYNC_AS_STAT); > + if (!err && dev_mynode(dev, d_inode(dentry), &stat)) { > + struct iattr newattrs; > + /* > + * before unlinking this node, reset permissions > + * of possible references like hardlinks > + */ > + newattrs.ia_uid = GLOBAL_ROOT_UID; > + newattrs.ia_gid = GLOBAL_ROOT_GID; > + newattrs.ia_mode = stat.mode & ~0777; > + newattrs.ia_valid = > + ATTR_UID|ATTR_GID|ATTR_MODE; > + inode_lock(d_inode(dentry)); > + notify_change(&nop_mnt_idmap, dentry, &newattrs, NULL); > + inode_unlock(d_inode(dentry)); > + err = vfs_unlink(&nop_mnt_idmap, d_inode(parent.dentry), > + dentry, NULL); > + if (!err || err == -ENOENT) > + deleted = 1; > } > dput(dentry); > inode_unlock(d_inode(parent.dentry)); > diff --git a/fs/bcachefs/fs-ioctl.c b/fs/bcachefs/fs-ioctl.c > index 15725b4ce393..595b57fabc9a 100644 > --- a/fs/bcachefs/fs-ioctl.c > +++ b/fs/bcachefs/fs-ioctl.c > @@ -511,10 +511,6 @@ static long bch2_ioctl_subvolume_destroy(struct bch_fs *c, struct file *filp, > ret = -EXDEV; > goto err; > } > - if (!d_is_positive(victim)) { > - ret = -ENOENT; > - goto err; > - } > ret = __bch2_unlink(dir, victim, true); > if (!ret) { > fsnotify_rmdir(dir, victim); > diff --git a/fs/namei.c b/fs/namei.c > index 3ab9440c5b93..fb6da3ca0ca5 100644 > --- a/fs/namei.c > +++ b/fs/namei.c > @@ -2741,6 +2741,10 @@ static struct dentry *__kern_path_locked(int dfd, struct filename *name, struct > } > inode_lock_nested(path->dentry->d_inode, I_MUTEX_PARENT); > d = lookup_one_qstr_excl(&last, path->dentry, 0); > + if (!IS_ERR(d) && d_is_negative(d)) { > + dput(d); > + d = ERR_PTR(-ENOENT); > + } > if (IS_ERR(d)) { > inode_unlock(path->dentry->d_inode); > path_put(path); > diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c > index 7f358740e958..367eaf2c78b7 100644 > --- a/kernel/audit_watch.c > +++ b/kernel/audit_watch.c > @@ -350,11 +350,10 @@ static int audit_get_nd(struct audit_watch *watch, struct path *parent) > struct dentry *d = kern_path_locked(watch->path, parent); > if (IS_ERR(d)) > return PTR_ERR(d); > - if (d_is_positive(d)) { > - /* update watch filter fields */ > - watch->dev = d->d_sb->s_dev; > - watch->ino = d_backing_inode(d)->i_ino; > - } > + /* update watch filter fields */ > + watch->dev = d->d_sb->s_dev; > + watch->ino = d_backing_inode(d)->i_ino; > + > inode_unlock(d_backing_inode(parent->dentry)); > dput(d); > return 0; > @@ -419,10 +418,11 @@ int audit_add_watch(struct audit_krule *krule, struct list_head **list) > /* caller expects mutex locked */ > mutex_lock(&audit_filter_mutex); > > - if (ret) { > + if (ret && ret != -ENOENT) { > audit_put_watch(watch); > return ret; > } > + ret = 0; > > /* either find an old parent or attach a new one */ > parent = audit_find_parent(d_backing_inode(parent_path.dentry)); Reviewed-by: Jeff Layton <jlayton@kernel.org>
diff --git a/Documentation/filesystems/porting.rst b/Documentation/filesystems/porting.rst index 1639e78e3146..2ead47e20677 100644 --- a/Documentation/filesystems/porting.rst +++ b/Documentation/filesystems/porting.rst @@ -1157,3 +1157,11 @@ in normal case it points into the pathname being looked up. NOTE: if you need something like full path from the root of filesystem, you are still on your own - this assists with simple cases, but it's not magic. + +--- + +** recommended** + +kern_path_locked() and user_path_locked() no longer return a negative +dentry so this doesn't need to be checked. If the name cannot be found, +ERR_PTR(-ENOENT) is returned. diff --git a/drivers/base/devtmpfs.c b/drivers/base/devtmpfs.c index b848764ef018..c9e34842139f 100644 --- a/drivers/base/devtmpfs.c +++ b/drivers/base/devtmpfs.c @@ -245,15 +245,12 @@ static int dev_rmdir(const char *name) dentry = kern_path_locked(name, &parent); if (IS_ERR(dentry)) return PTR_ERR(dentry); - if (d_really_is_positive(dentry)) { - if (d_inode(dentry)->i_private == &thread) - err = vfs_rmdir(&nop_mnt_idmap, d_inode(parent.dentry), - dentry); - else - err = -EPERM; - } else { - err = -ENOENT; - } + if (d_inode(dentry)->i_private == &thread) + err = vfs_rmdir(&nop_mnt_idmap, d_inode(parent.dentry), + dentry); + else + err = -EPERM; + dput(dentry); inode_unlock(d_inode(parent.dentry)); path_put(&parent); @@ -310,6 +307,8 @@ static int handle_remove(const char *nodename, struct device *dev) { struct path parent; struct dentry *dentry; + struct kstat stat; + struct path p; int deleted = 0; int err; @@ -317,32 +316,28 @@ static int handle_remove(const char *nodename, struct device *dev) if (IS_ERR(dentry)) return PTR_ERR(dentry); - if (d_really_is_positive(dentry)) { - struct kstat stat; - struct path p = {.mnt = parent.mnt, .dentry = dentry}; - err = vfs_getattr(&p, &stat, STATX_TYPE | STATX_MODE, - AT_STATX_SYNC_AS_STAT); - if (!err && dev_mynode(dev, d_inode(dentry), &stat)) { - struct iattr newattrs; - /* - * before unlinking this node, reset permissions - * of possible references like hardlinks - */ - newattrs.ia_uid = GLOBAL_ROOT_UID; - newattrs.ia_gid = GLOBAL_ROOT_GID; - newattrs.ia_mode = stat.mode & ~0777; - newattrs.ia_valid = - ATTR_UID|ATTR_GID|ATTR_MODE; - inode_lock(d_inode(dentry)); - notify_change(&nop_mnt_idmap, dentry, &newattrs, NULL); - inode_unlock(d_inode(dentry)); - err = vfs_unlink(&nop_mnt_idmap, d_inode(parent.dentry), - dentry, NULL); - if (!err || err == -ENOENT) - deleted = 1; - } - } else { - err = -ENOENT; + p.mnt = parent.mnt; + p.dentry = dentry; + err = vfs_getattr(&p, &stat, STATX_TYPE | STATX_MODE, + AT_STATX_SYNC_AS_STAT); + if (!err && dev_mynode(dev, d_inode(dentry), &stat)) { + struct iattr newattrs; + /* + * before unlinking this node, reset permissions + * of possible references like hardlinks + */ + newattrs.ia_uid = GLOBAL_ROOT_UID; + newattrs.ia_gid = GLOBAL_ROOT_GID; + newattrs.ia_mode = stat.mode & ~0777; + newattrs.ia_valid = + ATTR_UID|ATTR_GID|ATTR_MODE; + inode_lock(d_inode(dentry)); + notify_change(&nop_mnt_idmap, dentry, &newattrs, NULL); + inode_unlock(d_inode(dentry)); + err = vfs_unlink(&nop_mnt_idmap, d_inode(parent.dentry), + dentry, NULL); + if (!err || err == -ENOENT) + deleted = 1; } dput(dentry); inode_unlock(d_inode(parent.dentry)); diff --git a/fs/bcachefs/fs-ioctl.c b/fs/bcachefs/fs-ioctl.c index 15725b4ce393..595b57fabc9a 100644 --- a/fs/bcachefs/fs-ioctl.c +++ b/fs/bcachefs/fs-ioctl.c @@ -511,10 +511,6 @@ static long bch2_ioctl_subvolume_destroy(struct bch_fs *c, struct file *filp, ret = -EXDEV; goto err; } - if (!d_is_positive(victim)) { - ret = -ENOENT; - goto err; - } ret = __bch2_unlink(dir, victim, true); if (!ret) { fsnotify_rmdir(dir, victim); diff --git a/fs/namei.c b/fs/namei.c index 3ab9440c5b93..fb6da3ca0ca5 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -2741,6 +2741,10 @@ static struct dentry *__kern_path_locked(int dfd, struct filename *name, struct } inode_lock_nested(path->dentry->d_inode, I_MUTEX_PARENT); d = lookup_one_qstr_excl(&last, path->dentry, 0); + if (!IS_ERR(d) && d_is_negative(d)) { + dput(d); + d = ERR_PTR(-ENOENT); + } if (IS_ERR(d)) { inode_unlock(path->dentry->d_inode); path_put(path); diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c index 7f358740e958..367eaf2c78b7 100644 --- a/kernel/audit_watch.c +++ b/kernel/audit_watch.c @@ -350,11 +350,10 @@ static int audit_get_nd(struct audit_watch *watch, struct path *parent) struct dentry *d = kern_path_locked(watch->path, parent); if (IS_ERR(d)) return PTR_ERR(d); - if (d_is_positive(d)) { - /* update watch filter fields */ - watch->dev = d->d_sb->s_dev; - watch->ino = d_backing_inode(d)->i_ino; - } + /* update watch filter fields */ + watch->dev = d->d_sb->s_dev; + watch->ino = d_backing_inode(d)->i_ino; + inode_unlock(d_backing_inode(parent->dentry)); dput(d); return 0; @@ -419,10 +418,11 @@ int audit_add_watch(struct audit_krule *krule, struct list_head **list) /* caller expects mutex locked */ mutex_lock(&audit_filter_mutex); - if (ret) { + if (ret && ret != -ENOENT) { audit_put_watch(watch); return ret; } + ret = 0; /* either find an old parent or attach a new one */ parent = audit_find_parent(d_backing_inode(parent_path.dentry));
No callers of kern_path_locked() or user_path_locked_at() want a negative dentry. So change them to return -ENOENT instead. This simplifies callers. This results in a subtle change to bcachefs in that an ioctl will now return -ENOENT in preference to -EXDEV. I believe this restores the behaviour to what it was prior to Commit bbe6a7c899e7 ("bch2_ioctl_subvolume_destroy(): fix locking") Signed-off-by: NeilBrown <neilb@suse.de> Link: https://lore.kernel.org/r/20250207034040.3402438-2-neilb@suse.de Acked-by: Paul Moore <paul@paul-moore.com> Signed-off-by: Christian Brauner <brauner@kernel.org> --- Documentation/filesystems/porting.rst | 8 ++++ drivers/base/devtmpfs.c | 65 +++++++++++++-------------- fs/bcachefs/fs-ioctl.c | 4 -- fs/namei.c | 4 ++ kernel/audit_watch.c | 12 ++--- 5 files changed, 48 insertions(+), 45 deletions(-)