Re: [PATCH v2 2/4] ovl: stash upper real file in backing_file struct

[Al Viro <viro@xxxxxxxxxxxxxxxxxx>]

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;
	}