On Sat, Oct 5, 2024 at 9:49 PM Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
>
> 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?
No, I meant I created a new helper ovl_upper_file() that only ovl_fsync()
uses and can return NULL.
All the rest of the callers are using the helper ovl_real_file() which cannot
return NULL.
I will post it.
>
> > 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
Correct.
> 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...
Yeh, that's a bit more clear without to goto, but I would not remove
the if (upperfile) assertion. Actually the first if has a bug.
It assumes that if the upperfile is stashed then it must be used, but
this is incorrect.
I have made the following change:
static bool ovl_is_real_file(const struct file *realfile,
const struct path *realpath)
{
return file_inode(realfile) == d_inode(realpath->dentry);
}
...
/*
* Usually, if we operated on a stashed upperfile once, all following
* operations will operate on the stashed upperfile, but there is one
* exception - ovl_fsync(datasync = false) can populate the stashed
* upperfile to perform fsync on upper metadata inode. In this case,
* following read/write operations will not use the stashed upperfile.
*/
if (upperfile && likely(ovl_is_real_file(upperfile, realpath))) {
realfile = upperfile;
goto checkflags;
}
/*
* 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(!ovl_is_real_file(realfile, realpath))) {
struct file *old;
/* Either stashed realfile or upperfile must match realinode */
if (WARN_ON_ONCE(upperfile))
return ERR_PTR(-EIO);
...
/* Stashed upperfile that won the race must match realinode */
if (WARN_ON_ONCE(!ovl_is_real_file(upperfile, realpath)))
return ERR_PTR(-EIO);
realfile = upperfile;
}
checkflags:
...
What happens is that we have two slots for stashing real files
and there are subtle assumptions in the code that
1. We will never be requested to open a real file for more than
two inodes (lower and upper)
2. If we are requested to open two inodes, we will always be
requested to open the lower inode first
3. IOW if we are requested to open the upper inode first, we
will not be requested to open a different inode
Therefore, the names realfile/upperfile are a bit misleading.
If file is opened after copyup, then the realfile in ->private_data
is the upper file and stashed upperfile is NULL.
I will post the revised version.
Thanks,
Amir.
