On Mon, Oct 7, 2024 at 5:42 AM Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
>
> On Mon, Oct 07, 2024 at 04:03:13AM +0100, Al Viro wrote:
>
> > > Hmm... That still feels awkward. Question: can we reach that code with
> > > * non-NULL upperfile
> > > * false ovl_is_real_file(upperfile, realpath)
> > > * true ovl_is_real_file(realfile, realpath)
> > > Is that really possible?
> >
> > read() from metacopied file after fsync(), with the data still in lower
> > layer? Or am I misreading that?
>
> Unless I'm misreading that thing, the logics would be
>
> If what we are asked for is where the data used to be at open time
> just use what we'd opened back then and be done with that.
>
> Either we'd been copied up since open, or it's a metadata fsync of
> a metacopied file; it doesn't matter which, since the upper layer
> file will be the same either way.
>
> If it hadn't been opened and stashed into the backing_file, do so.
>
> If we end up using the reference stashed by somebody else (either
> by finding it there in the first place, or by having cmpxchg tell
> us we'd lost the race), verify that it _is_ in the right place;
> it really should be, short of an equivalent of fs corruption
> (== somebody fucking around with the upper layer under us).
>
> Is that what's going on there? If so, I think your current version is
> correct, but I'd probably put it in a different way:
>
> if (!ovl_is_real_file(realfile, realpath) {
> /*
> * the place we want is not where the data used to be at
> * open time; either we'd been copied up, or it's an fsync
> * of metacopied file. Should be the same location either
> * way...
> */
> struct file *upperfile = backing_file_private(realfile);
> struct file *old;
>
> if (!upperfile) { /* nobody opened it yet */
> upperfile = ovl_open_realfile(file, realpath);
> if (IS_ERR(upperfile))
> return upperfile;
> old = cmpxchg_release(backing_file_private_ptr(realfile),
> NULL, upperfile);
> if (old) { /* but they did while we were opening it */
> fput(upperfile);
> upperfile = old;
> }
> }
> /*
> * stashed file must have been from the right place, unless
> * someone's been corrupting the upper layer.
> */
> if (WARN_ON_ONCE(!ovl_is_real_file(upperfile, realpath)))
> return ERR_PTR(-EIO);
> realfile = upperfile;
> }
I like. I will use that.
Thanks,
Amir.
