| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-id: <174977057764.608730.14894222705301006129@noble.neil.brown.name>
Date: Fri, 13 Jun 2025 09:22:57 +1000
From: "NeilBrown" <neil@...wn.name>
To: "Amir Goldstein" <amir73il@...il.com>
Cc: "Alexander Viro" <viro@...iv.linux.org.uk>,
"Christian Brauner" <brauner@...nel.org>, "Jan Kara" <jack@...e.cz>,
"David Howells" <dhowells@...hat.com>, "Tyler Hicks" <code@...icks.com>,
"Chuck Lever" <chuck.lever@...cle.com>, "Jeff Layton" <jlayton@...nel.org>,
"Miklos Szeredi" <miklos@...redi.hu>, "Kees Cook" <kees@...nel.org>,
"Joel Granados" <joel.granados@...nel.org>,
"Namjae Jeon" <linkinjeon@...nel.org>, "Steve French" <smfrench@...il.com>,
"Sergey Senozhatsky" <senozhatsky@...omium.org>, netfs@...ts.linux.dev,
linux-kernel@...r.kernel.org, ecryptfs@...r.kernel.org,
linux-fsdevel@...r.kernel.org, linux-nfs@...r.kernel.org,
linux-unionfs@...r.kernel.org, linux-cifs@...r.kernel.org
Subject: Re: [PATCH 1/2] VFS: change old_dir and new_dir in struct renamedata
to dentrys
On Thu, 12 Jun 2025, Amir Goldstein wrote:
> On Thu, Jun 12, 2025 at 12:59 AM NeilBrown <neil@...wn.name> wrote:
> >
> > all users of 'struct renamedata' have the dentry for the old and new
> > directories, and often have no use for the inode except to store it in
> > the renamedata.
> >
> > This patch changes struct renamedata to hold the dentry, rather than
> > the inode, for the old and new directories, and changes callers to
> > match.
> >
> > This results in the removal of several local variables and several
> > dereferences of ->d_inode at the cost of adding ->d_inode dereferences
> > to vfs_rename().
> >
> > Signed-off-by: NeilBrown <neil@...wn.name>
> > ---
> > fs/cachefiles/namei.c | 4 ++--
> > fs/ecryptfs/inode.c | 4 ++--
> > fs/namei.c | 6 +++---
> > fs/nfsd/vfs.c | 7 ++-----
> > fs/overlayfs/copy_up.c | 6 +++---
> > fs/overlayfs/dir.c | 16 ++++++++--------
> > fs/overlayfs/overlayfs.h | 6 +++---
> > fs/overlayfs/readdir.c | 2 +-
> > fs/overlayfs/super.c | 2 +-
> > fs/overlayfs/util.c | 2 +-
> > fs/smb/server/vfs.c | 4 ++--
> > include/linux/fs.h | 4 ++--
> > 12 files changed, 30 insertions(+), 33 deletions(-)
> >
> > diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c
> > index aecfc5c37b49..053fc28b5423 100644
> > --- a/fs/cachefiles/namei.c
> > +++ b/fs/cachefiles/namei.c
> > @@ -388,10 +388,10 @@ int cachefiles_bury_object(struct cachefiles_cache *cache,
> > } else {
> > struct renamedata rd = {
> > .old_mnt_idmap = &nop_mnt_idmap,
> > - .old_dir = d_inode(dir),
> > + .old_dir = dir,
> > .old_dentry = rep,
> > .new_mnt_idmap = &nop_mnt_idmap,
> > - .new_dir = d_inode(cache->graveyard),
> > + .new_dir = cache->graveyard,
> > .new_dentry = grave,
> > };
> > trace_cachefiles_rename(object, d_inode(rep)->i_ino, why);
> > diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
> > index 493d7f194956..c9fec8b7e000 100644
> > --- a/fs/ecryptfs/inode.c
> > +++ b/fs/ecryptfs/inode.c
> > @@ -635,10 +635,10 @@ ecryptfs_rename(struct mnt_idmap *idmap, struct inode *old_dir,
> > }
> >
> > rd.old_mnt_idmap = &nop_mnt_idmap;
> > - rd.old_dir = d_inode(lower_old_dir_dentry);
> > + rd.old_dir = lower_old_dir_dentry;
> > rd.old_dentry = lower_old_dentry;
> > rd.new_mnt_idmap = &nop_mnt_idmap;
> > - rd.new_dir = d_inode(lower_new_dir_dentry);
> > + rd.new_dir = lower_new_dir_dentry;
> > rd.new_dentry = lower_new_dentry;
> > rc = vfs_rename(&rd);
> > if (rc)
> > diff --git a/fs/namei.c b/fs/namei.c
> > index 019073162b8a..5b0be8bca50d 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -5007,7 +5007,7 @@ SYSCALL_DEFINE2(link, const char __user *, oldname, const char __user *, newname
> > int vfs_rename(struct renamedata *rd)
> > {
> > int error;
> > - struct inode *old_dir = rd->old_dir, *new_dir = rd->new_dir;
> > + struct inode *old_dir = d_inode(rd->old_dir), *new_dir = d_inode(rd->new_dir);
> > struct dentry *old_dentry = rd->old_dentry;
> > struct dentry *new_dentry = rd->new_dentry;
> > struct inode **delegated_inode = rd->delegated_inode;
> > @@ -5266,10 +5266,10 @@ int do_renameat2(int olddfd, struct filename *from, int newdfd,
> > if (error)
> > goto exit5;
> >
> > - rd.old_dir = old_path.dentry->d_inode;
> > + rd.old_dir = old_path.dentry;
> > rd.old_dentry = old_dentry;
> > rd.old_mnt_idmap = mnt_idmap(old_path.mnt);
> > - rd.new_dir = new_path.dentry->d_inode;
> > + rd.new_dir = new_path.dentry;
> > rd.new_dentry = new_dentry;
> > rd.new_mnt_idmap = mnt_idmap(new_path.mnt);
> > rd.delegated_inode = &delegated_inode;
> > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> > index cd689df2ca5d..3c87fbd22c57 100644
> > --- a/fs/nfsd/vfs.c
> > +++ b/fs/nfsd/vfs.c
> > @@ -1864,7 +1864,6 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen,
> > struct svc_fh *tfhp, char *tname, int tlen)
> > {
> > struct dentry *fdentry, *tdentry, *odentry, *ndentry, *trap;
> > - struct inode *fdir, *tdir;
> > int type = S_IFDIR;
> > __be32 err;
> > int host_err;
> > @@ -1880,10 +1879,8 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen,
> > goto out;
> >
> > fdentry = ffhp->fh_dentry;
> > - fdir = d_inode(fdentry);
> >
> > tdentry = tfhp->fh_dentry;
> > - tdir = d_inode(tdentry);
> >
> > err = nfserr_perm;
> > if (!flen || isdotent(fname, flen) || !tlen || isdotent(tname, tlen))
> > @@ -1944,10 +1941,10 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen,
> > } else {
> > struct renamedata rd = {
> > .old_mnt_idmap = &nop_mnt_idmap,
> > - .old_dir = fdir,
> > + .old_dir = fdentry,
> > .old_dentry = odentry,
> > .new_mnt_idmap = &nop_mnt_idmap,
> > - .new_dir = tdir,
> > + .new_dir = tdentry,
> > .new_dentry = ndentry,
> > };
> > int retries;
> > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > index d7310fcf3888..8a3c0d18ec2e 100644
> > --- a/fs/overlayfs/copy_up.c
> > +++ b/fs/overlayfs/copy_up.c
> > @@ -563,7 +563,7 @@ static int ovl_create_index(struct dentry *dentry, const struct ovl_fh *fh,
> > if (IS_ERR(index)) {
> > err = PTR_ERR(index);
> > } else {
> > - err = ovl_do_rename(ofs, dir, temp, dir, index, 0);
> > + err = ovl_do_rename(ofs, indexdir, temp, indexdir, index, 0);
> > dput(index);
> > }
> > out:
> > @@ -762,7 +762,7 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
> > {
> > struct ovl_fs *ofs = OVL_FS(c->dentry->d_sb);
> > struct inode *inode;
> > - struct inode *udir = d_inode(c->destdir), *wdir = d_inode(c->workdir);
> > + struct inode *wdir = d_inode(c->workdir);
> > struct path path = { .mnt = ovl_upper_mnt(ofs) };
> > struct dentry *temp, *upper, *trap;
> > struct ovl_cu_creds cc;
> > @@ -829,7 +829,7 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
> > if (IS_ERR(upper))
> > goto cleanup;
> >
> > - err = ovl_do_rename(ofs, wdir, temp, udir, upper, 0);
> > + err = ovl_do_rename(ofs, c->workdir, temp, c->destdir, upper, 0);
> > dput(upper);
> > if (err)
> > goto cleanup;
> > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> > index fe493f3ed6b6..4fc221ea6480 100644
> > --- a/fs/overlayfs/dir.c
> > +++ b/fs/overlayfs/dir.c
> > @@ -107,7 +107,7 @@ static struct dentry *ovl_whiteout(struct ovl_fs *ofs)
> > }
> >
> > /* Caller must hold i_mutex on both workdir and dir */
> > -int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct inode *dir,
> > +int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct dentry *dir,
> > struct dentry *dentry)
> > {
> > struct inode *wdir = ofs->workdir->d_inode;
> > @@ -123,7 +123,7 @@ int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct inode *dir,
> > if (d_is_dir(dentry))
> > flags = RENAME_EXCHANGE;
> >
> > - err = ovl_do_rename(ofs, wdir, whiteout, dir, dentry, flags);
> > + err = ovl_do_rename(ofs, ofs->workdir, whiteout, dir, dentry, flags);
> > if (err)
> > goto kill_whiteout;
> > if (flags)
> > @@ -384,7 +384,7 @@ static struct dentry *ovl_clear_empty(struct dentry *dentry,
> > if (err)
> > goto out_cleanup;
> >
> > - err = ovl_do_rename(ofs, wdir, opaquedir, udir, upper, RENAME_EXCHANGE);
> > + err = ovl_do_rename(ofs, workdir, opaquedir, upperdir, upper, RENAME_EXCHANGE);
> > if (err)
> > goto out_cleanup;
> >
> > @@ -491,14 +491,14 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
> > if (err)
> > goto out_cleanup;
> >
> > - err = ovl_do_rename(ofs, wdir, newdentry, udir, upper,
> > + err = ovl_do_rename(ofs, workdir, newdentry, upperdir, upper,
> > RENAME_EXCHANGE);
> > if (err)
> > goto out_cleanup;
> >
> > ovl_cleanup(ofs, wdir, upper);
> > } else {
> > - err = ovl_do_rename(ofs, wdir, newdentry, udir, upper, 0);
> > + err = ovl_do_rename(ofs, workdir, newdentry, upperdir, upper, 0);
> > if (err)
> > goto out_cleanup;
> > }
> > @@ -774,7 +774,7 @@ static int ovl_remove_and_whiteout(struct dentry *dentry,
> > goto out_dput_upper;
> > }
> >
> > - err = ovl_cleanup_and_whiteout(ofs, d_inode(upperdir), upper);
> > + err = ovl_cleanup_and_whiteout(ofs, upperdir, upper);
> > if (err)
> > goto out_d_drop;
> >
> > @@ -1246,8 +1246,8 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
> > if (err)
> > goto out_dput;
> >
> > - err = ovl_do_rename(ofs, old_upperdir->d_inode, olddentry,
> > - new_upperdir->d_inode, newdentry, flags);
> > + err = ovl_do_rename(ofs, old_upperdir, olddentry,
> > + new_upperdir, newdentry, flags);
> > if (err)
> > goto out_dput;
> >
> > diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> > index 8baaba0a3fe5..65f9d51bed7c 100644
> > --- a/fs/overlayfs/overlayfs.h
> > +++ b/fs/overlayfs/overlayfs.h
> > @@ -353,8 +353,8 @@ static inline int ovl_do_remove_acl(struct ovl_fs *ofs, struct dentry *dentry,
> > return vfs_remove_acl(ovl_upper_mnt_idmap(ofs), dentry, acl_name);
> > }
> >
> > -static inline int ovl_do_rename(struct ovl_fs *ofs, struct inode *olddir,
> > - struct dentry *olddentry, struct inode *newdir,
> > +static inline int ovl_do_rename(struct ovl_fs *ofs, struct dentry *olddir,
> > + struct dentry *olddentry, struct dentry *newdir,
> > struct dentry *newdentry, unsigned int flags)
> > {
> > int err;
> > @@ -826,7 +826,7 @@ static inline void ovl_copyflags(struct inode *from, struct inode *to)
> >
> > /* dir.c */
> > extern const struct inode_operations ovl_dir_inode_operations;
> > -int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct inode *dir,
> > +int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct dentry *dir,
> > struct dentry *dentry);
> > struct ovl_cattr {
> > dev_t rdev;
> > diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
> > index 474c80d210d1..68cca52ae2ac 100644
> > --- a/fs/overlayfs/readdir.c
> > +++ b/fs/overlayfs/readdir.c
> > @@ -1235,7 +1235,7 @@ int ovl_indexdir_cleanup(struct ovl_fs *ofs)
> > * Whiteout orphan index to block future open by
> > * handle after overlay nlink dropped to zero.
> > */
> > - err = ovl_cleanup_and_whiteout(ofs, dir, index);
> > + err = ovl_cleanup_and_whiteout(ofs, indexdir, index);
> > } else {
> > /* Cleanup orphan index entries */
> > err = ovl_cleanup(ofs, dir, index);
> > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> > index e19940d649ca..cf99b276fdfb 100644
> > --- a/fs/overlayfs/super.c
> > +++ b/fs/overlayfs/super.c
> > @@ -580,7 +580,7 @@ static int ovl_check_rename_whiteout(struct ovl_fs *ofs)
> >
> > /* Name is inline and stable - using snapshot as a copy helper */
> > take_dentry_name_snapshot(&name, temp);
> > - err = ovl_do_rename(ofs, dir, temp, dir, dest, RENAME_WHITEOUT);
> > + err = ovl_do_rename(ofs, workdir, temp, workdir, dest, RENAME_WHITEOUT);
> > if (err) {
> > if (err == -EINVAL)
> > err = 0;
> > diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> > index dcccb4b4a66c..2b4754c645ee 100644
> > --- a/fs/overlayfs/util.c
> > +++ b/fs/overlayfs/util.c
> > @@ -1115,7 +1115,7 @@ static void ovl_cleanup_index(struct dentry *dentry)
> > } else if (ovl_index_all(dentry->d_sb)) {
> > /* Whiteout orphan index to block future open by handle */
> > err = ovl_cleanup_and_whiteout(OVL_FS(dentry->d_sb),
> > - dir, index);
> > + indexdir, index);
> > } else {
> > /* Cleanup orphan index entries */
> > err = ovl_cleanup(ofs, dir, index);
> > diff --git a/fs/smb/server/vfs.c b/fs/smb/server/vfs.c
> > index ba45e809555a..b8d913c61623 100644
> > --- a/fs/smb/server/vfs.c
> > +++ b/fs/smb/server/vfs.c
> > @@ -764,10 +764,10 @@ int ksmbd_vfs_rename(struct ksmbd_work *work, const struct path *old_path,
> > }
> >
> > rd.old_mnt_idmap = mnt_idmap(old_path->mnt),
> > - rd.old_dir = d_inode(old_parent),
> > + rd.old_dir = old_parent,
> > rd.old_dentry = old_child,
> > rd.new_mnt_idmap = mnt_idmap(new_path.mnt),
> > - rd.new_dir = new_path.dentry->d_inode,
> > + rd.new_dir = new_path.dentry,
> > rd.new_dentry = new_dentry,
> > rd.flags = flags,
> > rd.delegated_inode = NULL,
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index 16f40a6f8264..9a83904c9d4a 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -2016,10 +2016,10 @@ int vfs_unlink(struct mnt_idmap *, struct inode *, struct dentry *,
> > */
> > struct renamedata {
> > struct mnt_idmap *old_mnt_idmap;
> > - struct inode *old_dir;
> > + struct dentry *old_dir;
> > struct dentry *old_dentry;
> > struct mnt_idmap *new_mnt_idmap;
> > - struct inode *new_dir;
> > + struct dentry *new_dir;
> > struct dentry *new_dentry;
> > struct inode **delegated_inode;
> > unsigned int flags;
> > --
>
> It bothers me a bit that we are keeping the field name while changing its type.
>
> There is a wide convention in vfs methods and helpers of using
> struct inode *dir
> as the parent directory inode
> and often (but not always) using
> struct dentry *parent
> as the parent dentry
>
> How do you feel about making struct renamedata use:
>
> struct dentry *old_parent;
> struct dentry *new_parent;
>
> I don't think it will add any churn beyond what this patch already does.
I think that is an excellent idea - thanks.
Particularly as the kernel-doc documentation for struct renamedata
describe the fields as "parent of source" and "parent of destination".
I'll resubmit with that change and the reviewed-bys.
Thanks,
NeilBrown
Powered by blists - more mailing lists