On Mon, Oct 7, 2024 at 5:12 AM Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
>
> On Sun, Oct 06, 2024 at 10:23:58AM +0200, Amir Goldstein wrote:
> > Stop using struct fd to return a real file from ovl_real_fdget_path(),
> > because we no longer return a temporary file object and the callers
> > always get a borrowed file reference.
> >
> > Rename the helper to ovl_real_file_path(), return a borrowed reference
> > of the real file that is referenced from the overlayfs file or an error.
> >
> > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
> > ---
> > fs/overlayfs/file.c | 70 +++++++++++++++++++++++++--------------------
> > 1 file changed, 39 insertions(+), 31 deletions(-)
> >
> > diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
> > index 42f9bbdd65b4..ead805e9f2d6 100644
> > --- a/fs/overlayfs/file.c
> > +++ b/fs/overlayfs/file.c
>
> > +static struct file *ovl_upper_file(const struct file *file, bool data)
> > {
> > struct dentry *dentry = file_dentry(file);
> > struct path realpath;
> > @@ -193,12 +204,11 @@ static int ovl_upper_fdget(const struct file *file, struct fd *real, bool data)
> > else
> > type = ovl_path_real(dentry, &realpath);
> >
> > - real->word = 0;
> > /* Not interested in lower nor in upper meta if data was requested */
> > if (!OVL_TYPE_UPPER(type) || (data && OVL_TYPE_MERGE(type)))
> > - return 0;
> > + return NULL;
> >
> > - return ovl_real_fdget_path(file, real, &realpath);
> > + return ovl_real_file_path(file, &realpath);
>
> AFAICS, we should never get NULL from ovl_real_file_path() now.
>
> > static int ovl_open(struct inode *inode, struct file *file)
> > @@ -455,7 +465,7 @@ static ssize_t ovl_splice_write(struct pipe_inode_info *pipe, struct file *out,
> >
> > static int ovl_fsync(struct file *file, loff_t start, loff_t end, int datasync)
> > {
> > - struct fd real;
> > + struct file *realfile;
> > const struct cred *old_cred;
> > int ret;
> >
> > @@ -463,19 +473,17 @@ static int ovl_fsync(struct file *file, loff_t start, loff_t end, int datasync)
> > if (ret <= 0)
> > return ret;
> >
> > - ret = ovl_upper_fdget(file, &real, datasync);
> > - if (ret || fd_empty(real))
> > - return ret;
> > + realfile = ovl_upper_file(file, datasync);
> > + if (IS_ERR_OR_NULL(realfile))
> > + return PTR_ERR(realfile);
>
> ... if so, the only source of NULL here would be the checks for OVL_TYPE_...
> in ovl_upper_file(). Which has no other callers...
>
> > /* Don't sync lower file for fear of receiving EROFS error */
> > - if (file_inode(fd_file(real)) == ovl_inode_upper(file_inode(file))) {
> > + if (file_inode(realfile) == ovl_inode_upper(file_inode(file))) {
>
> Can that _not_ be true after the same checks in ovl_upper_file()?
It should always be true. I will remove the check and rename the variable to
upperfile to clarify further.
Thanks for the thorough review!
Amir.
