Re: [PATCH 1/4] ovl: do not open non-data lower file for fsync

[Al Viro <viro@zeniv.linux.org.uk>]

On Sat, Oct 05, 2024 at 08:30:23AM +0200, Amir Goldstein wrote:

> I understand your concern, but honestly, I am not sure that returning
> struct fderr is fundamentally different from checking IS_ERR_OR_NULL.
> 
> What I can do is refactor the helpers differently so that ovl_fsync() will
> call ovl_file_upper() to clarify that it may return NULL, just like

ovl_dentry_upper(), you mean?

> ovl_{dentry,inode,path}_upper() and all the other callers will
> call ovl_file_real() which cannot return NULL, because it returns
> either lower or upper file, just like ovl_{inode,path}_real{,data}().

OK...  One thing I'm not happy about is the control (and data) flow in there:
stashed_upper:
        if (upperfile && file_inode(upperfile) == d_inode(realpath.dentry))
                realfile = upperfile;

        /*
         * If realfile is lower and has been copied up since we'd opened it,
         * open the real upper file and stash it in backing_file_private().
         */
        if (unlikely(file_inode(realfile) != d_inode(realpath.dentry))) {
                struct file *old;

                /* Stashed upperfile has a mismatched inode */
                if (unlikely(upperfile))
                        return ERR_PTR(-EIO);

                upperfile = ovl_open_realfile(file, &realpath);
                if (IS_ERR(upperfile))
                        return upperfile;

                old = cmpxchg_release(backing_file_private_ptr(realfile), NULL,
                                      upperfile);
                if (old) {
                        fput(upperfile);
                        upperfile = old;
                }

                goto stashed_upper;
        }
Unless I'm misreading that, the value of realfile seen inside the second
if is always the original; reassignment in the first if might as well had
been followed by goto past the second one.  What's more, if you win the
race in the second if, you'll have upperfile != NULL and its file_inode()
matching realpath.dentry->d_inode (you'd better, or you have a real problem
in backing_file_open()).  So that branch to stashed_upper in case old == NULL
might as well had been "realfile = upperfile;".  Correct?  In case old != NULL
we go there with upperfile != NULL.  If it (i.e. old) has the right file_inode(),
we are done; otherwise it's going to hit ERR_PTR(-EIO) in the second if.

So it seems to be equivalent to this:
        if (unlikely(file_inode(realfile) != d_inode(realpath.dentry))) {
	        /*
		 * If realfile is lower and has been copied up since we'd
		 * opened it, we need the real upper file opened.  Whoever gets
		 * there first stashes the result in backing_file_private().
		 */
		struct file *upperfile = backing_file_private(realfile);
		if (unlikely(!upperfile)) {
			struct file *old;

			upperfile = ovl_open_realfile(file, &realpath);
			if (IS_ERR(upperfile))
				return upperfile;

			old = cmpxchg_release(backing_file_private_ptr(realfile), NULL,
					      upperfile);
			if (old) {
				fput(upperfile);
				upperfile = old;
			}
		}
		// upperfile reference is owned by realfile at that point
		if (unlikely(file_inode(upperfile) != d_inode(realpath.dentry)))
			/* Stashed upperfile has a mismatched inode */
			return ERR_PTR(-EIO);
		realfile = upperfile;
	}
Or am I misreading it?  Seems to be more straightforward that way...