| Message ID | 20230611132732.1502040-3-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Handle notifications on overlayfs fake path files | expand |
On Sun, Jun 11, 2023 at 04:27:31PM +0300, Amir Goldstein wrote: > Overlayfs knows the real path of underlying dentries. Add an optional > struct vfsmount out argument to ->d_real(), so callers could compose the > real path. > > Add a helper f_real_path() that uses this new interface to return the > real path of f_inode, for overlayfs internal files whose f_path if a > "fake" overlayfs path and f_inode is the underlying real inode. I really don't like this ->d_real nagic. Most callers of it really can't ever be on overlayfs. So I'd suggest we do an audit of the callers of file_dentry and drop all the pointless ones first, and improve the documentation on when to use it.
On Mon, Jun 12, 2023 at 7:36 AM Christoph Hellwig <hch@infradead.org> wrote: > > On Sun, Jun 11, 2023 at 04:27:31PM +0300, Amir Goldstein wrote: > > Overlayfs knows the real path of underlying dentries. Add an optional > > struct vfsmount out argument to ->d_real(), so callers could compose the > > real path. > > > > Add a helper f_real_path() that uses this new interface to return the > > real path of f_inode, for overlayfs internal files whose f_path if a > > "fake" overlayfs path and f_inode is the underlying real inode. > > I really don't like this ->d_real nagic. Most callers of it > really can't ever be on overlayfs. Which callers are you referring to? > So I'd suggest we do an audit > of the callers of file_dentry and drop all the pointless ones > first, and improve the documentation on when to use it. Well, v3 is trying to reduce ->d_real() magic and the step after introducing the alternative path container is to convert file_dentry() to use the stored real_path instead of ->d_real(). But I agree that the documentation about this black magic is missing. Will try to improve that with the move to the "fake" file container. Thanks, Amir.
On Mon, Jun 12, 2023 at 09:28:40AM +0300, Amir Goldstein wrote: > On Mon, Jun 12, 2023 at 7:36 AM Christoph Hellwig <hch@infradead.org> wrote: > > > > On Sun, Jun 11, 2023 at 04:27:31PM +0300, Amir Goldstein wrote: > > > Overlayfs knows the real path of underlying dentries. Add an optional > > > struct vfsmount out argument to ->d_real(), so callers could compose the > > > real path. > > > > > > Add a helper f_real_path() that uses this new interface to return the > > > real path of f_inode, for overlayfs internal files whose f_path if a > > > "fake" overlayfs path and f_inode is the underlying real inode. > > > > I really don't like this ->d_real nagic. Most callers of it > > really can't ever be on overlayfs. > > Which callers are you referring to? Most users of file_dentry are inside file systems and will never see the overlayfs path. > > So I'd suggest we do an audit > > of the callers of file_dentry and drop all the pointless ones > > first, and improve the documentation on when to use it. > > Well, v3 is trying to reduce ->d_real() magic and the step > after introducing the alternative path container is to convert > file_dentry() to use the stored real_path instead of ->d_real(). Yeah, that makes this comment kinda irrelevant.
On Mon, Jun 12, 2023 at 9:36 AM Christoph Hellwig <hch@infradead.org> wrote: > > On Mon, Jun 12, 2023 at 09:28:40AM +0300, Amir Goldstein wrote: > > On Mon, Jun 12, 2023 at 7:36 AM Christoph Hellwig <hch@infradead.org> wrote: > > > > > > On Sun, Jun 11, 2023 at 04:27:31PM +0300, Amir Goldstein wrote: > > > > Overlayfs knows the real path of underlying dentries. Add an optional > > > > struct vfsmount out argument to ->d_real(), so callers could compose the > > > > real path. > > > > > > > > Add a helper f_real_path() that uses this new interface to return the > > > > real path of f_inode, for overlayfs internal files whose f_path if a > > > > "fake" overlayfs path and f_inode is the underlying real inode. > > > > > > I really don't like this ->d_real nagic. Most callers of it > > > really can't ever be on overlayfs. > > > > Which callers are you referring to? > > Most users of file_dentry are inside file systems and will never > see the overlayfs path. > Ay ay ay. I suspected that this is what you meant and I do not blame you. There is no documentation and it is hard to understand what is going on even harder to understand why that is going on... Before "ovl: stack file ops" series, a file opened from ovl (over xfs) path would have ovl f_path and xfs f_inode, as well as xfs f_ops, so indeed xfs code had to be careful, but so did a lot of generic vfs code. After ovl stacked f_ops, a file opened from ovl (over xfs) path has an ovl f_path and an ovl f_inode, so a lot of hacks could be removed from generic vfs code (e.g. locks_inode() macro). Alas, for every ovl file, there is an "internal/real" file (stashed in file->f_private) which is opened by open_with_fake_path(). This "internal/real" file has ovl f_path and xfs f_inode, so xfs could not get rid of file_dentry() just yet. Currently, the reason for the fake f_path hack is that the same internal file is assigned as ->vm_file in ovl_mmap(), ovl does not implement stacked aops and some users of ->vm_file need the "fake" ovl path. Anyway, the first step is to introduce the ovl internal file container. Next, we will use the real_path for file_dentry() and we can see if we can get rid of some of these file_dentry() uses. Thanks, Amir.
diff --git a/Documentation/filesystems/locking.rst b/Documentation/filesystems/locking.rst index aa1a233b0fa8..a6063b0c79fd 100644 --- a/Documentation/filesystems/locking.rst +++ b/Documentation/filesystems/locking.rst @@ -29,7 +29,8 @@ prototypes:: char *(*d_dname)((struct dentry *dentry, char *buffer, int buflen); struct vfsmount *(*d_automount)(struct path *path); int (*d_manage)(const struct path *, bool); - struct dentry *(*d_real)(struct dentry *, const struct inode *); + struct dentry *(*d_real)(struct dentry *, const struct inode *, + struct vfsmount **pmnt); locking rules: diff --git a/Documentation/filesystems/vfs.rst b/Documentation/filesystems/vfs.rst index 769be5230210..edafe824fca4 100644 --- a/Documentation/filesystems/vfs.rst +++ b/Documentation/filesystems/vfs.rst @@ -1264,7 +1264,8 @@ defined: char *(*d_dname)(struct dentry *, char *, int); struct vfsmount *(*d_automount)(struct path *); int (*d_manage)(const struct path *, bool); - struct dentry *(*d_real)(struct dentry *, const struct inode *); + struct dentry *(*d_real)(struct dentry *, const struct inode *, + struct vfsmount **pmnt); }; ``d_revalidate`` diff --git a/fs/file_table.c b/fs/file_table.c index d64d3933f3e4..fa187ceb54d6 100644 --- a/fs/file_table.c +++ b/fs/file_table.c @@ -215,6 +215,29 @@ struct file *alloc_empty_file_internal(int flags, const struct cred *cred) return f; } +/** + * f_real_path - return the real path of an internal file with fake path + * + * @file: The file to query + * + * If f_path is on a union/overlay and f_inode is not, then return the + * underlying real path of f_inode. + * Otherwise return f_path (by value). + */ +struct path f_real_path(const struct file *f) +{ + struct path path; + + if (!(f->f_mode & FMODE_INTERNAL) || + (d_inode(f->f_path.dentry) == f->f_inode)) + return f->f_path; + + path.mnt = f->f_path.mnt; + path.dentry = d_real(f->f_path.dentry, f->f_inode, &path.mnt); + return path; +} +EXPORT_SYMBOL(f_real_path); + /** * alloc_file - allocate and initialize a 'struct file' * diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c index d9be5d318e1b..591c77b33ff3 100644 --- a/fs/overlayfs/super.c +++ b/fs/overlayfs/super.c @@ -60,9 +60,11 @@ MODULE_PARM_DESC(metacopy, "Default to on or off for the metadata only copy up feature"); static struct dentry *ovl_d_real(struct dentry *dentry, - const struct inode *inode) + const struct inode *inode, + struct vfsmount **pmnt) { struct dentry *real = NULL, *lower; + struct path realpath; /* It's an overlay file */ if (inode && d_inode(dentry) == inode) @@ -74,12 +76,13 @@ static struct dentry *ovl_d_real(struct dentry *dentry, goto bug; } - real = ovl_dentry_upper(dentry); + ovl_path_upper(dentry, &realpath); + real = realpath.dentry; if (real && (inode == d_inode(real))) - return real; + goto found; if (real && !inode && ovl_has_upperdata(d_inode(dentry))) - return real; + goto found; /* * Best effort lazy lookup of lowerdata for !inode case to return @@ -90,16 +93,22 @@ static struct dentry *ovl_d_real(struct dentry *dentry, * when setting the uprobe. */ ovl_maybe_lookup_lowerdata(dentry); - lower = ovl_dentry_lowerdata(dentry); + ovl_path_lowerdata(dentry, &realpath); + lower = realpath.dentry; if (!lower) goto bug; - real = lower; /* Handle recursion */ - real = d_real(real, inode); + real = d_real(lower, inode, &realpath.mnt); + + if (inode && inode != d_inode(real)) + goto bug; + +found: + if (pmnt) + *pmnt = realpath.mnt; + return real; - if (!inode || inode == d_inode(real)) - return real; bug: WARN(1, "%s(%pd4, %s:%lu): real dentry (%p/%lu) not found\n", __func__, dentry, inode ? inode->i_sb->s_id : "NULL", diff --git a/include/linux/dcache.h b/include/linux/dcache.h index 6b351e009f59..78a54d175662 100644 --- a/include/linux/dcache.h +++ b/include/linux/dcache.h @@ -139,7 +139,8 @@ struct dentry_operations { char *(*d_dname)(struct dentry *, char *, int); struct vfsmount *(*d_automount)(struct path *); int (*d_manage)(const struct path *, bool); - struct dentry *(*d_real)(struct dentry *, const struct inode *); + struct dentry *(*d_real)(struct dentry *, const struct inode *, + struct vfsmount **); } ____cacheline_aligned; /* @@ -564,6 +565,7 @@ static inline struct dentry *d_backing_dentry(struct dentry *upper) * d_real - Return the real dentry * @dentry: the dentry to query * @inode: inode to select the dentry from multiple layers (can be NULL) + * @pmnt: returns the real mnt in case @dentry is not real * * If dentry is on a union/overlay, then return the underlying, real dentry. * Otherwise return the dentry itself. @@ -571,10 +573,11 @@ static inline struct dentry *d_backing_dentry(struct dentry *upper) * See also: Documentation/filesystems/vfs.rst */ static inline struct dentry *d_real(struct dentry *dentry, - const struct inode *inode) + const struct inode *inode, + struct vfsmount **pmnt) { if (unlikely(dentry->d_flags & DCACHE_OP_REAL)) - return dentry->d_op->d_real(dentry, inode); + return dentry->d_op->d_real(dentry, inode, pmnt); else return dentry; } @@ -589,7 +592,7 @@ static inline struct dentry *d_real(struct dentry *dentry, static inline struct inode *d_real_inode(const struct dentry *dentry) { /* This usage of d_real() results in const dentry */ - return d_backing_inode(d_real((struct dentry *) dentry, NULL)); + return d_inode(d_real((struct dentry *) dentry, NULL, NULL)); } struct name_snapshot { diff --git a/include/linux/fs.h b/include/linux/fs.h index 13eec1e8ca86..d0129e9e0ae5 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -1042,7 +1042,7 @@ static inline struct inode *file_inode(const struct file *f) static inline struct dentry *file_dentry(const struct file *file) { - return d_real(file->f_path.dentry, file_inode(file)); + return d_real(file->f_path.dentry, file_inode(file), NULL); } struct fasync_struct { @@ -2354,6 +2354,8 @@ extern struct file *dentry_create(const struct path *path, int flags, umode_t mode, const struct cred *cred); extern struct file * open_with_fake_path(const struct path *, int, struct inode*, const struct cred *); +extern struct path f_real_path(const struct file *f); + static inline struct file *file_clone_open(struct file *file) { return dentry_open(&file->f_path, file->f_flags, file->f_cred);
Overlayfs knows the real path of underlying dentries. Add an optional struct vfsmount out argument to ->d_real(), so callers could compose the real path. Add a helper f_real_path() that uses this new interface to return the real path of f_inode, for overlayfs internal files whose f_path if a "fake" overlayfs path and f_inode is the underlying real inode. Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- Documentation/filesystems/locking.rst | 3 ++- Documentation/filesystems/vfs.rst | 3 ++- fs/file_table.c | 23 +++++++++++++++++++++++ fs/overlayfs/super.c | 27 ++++++++++++++++++--------- include/linux/dcache.h | 11 +++++++---- include/linux/fs.h | 4 +++- 6 files changed, 55 insertions(+), 16 deletions(-)