On Thu, May 28, 2020 at 8:35 PM Vivek Goyal <vgoyal@xxxxxxxxxx> wrote:
>
> On Wed, May 27, 2020 at 11:11:38PM +0300, Amir Goldstein wrote:
>
> [..]
> > > > OR we don't check metacopy xattr in ovl_get_inode().
> > > >
> > > > In ovl_lookup() we already checked metacopy xattr.
> > > > No reason to check it again in this subtle context.
> > > >
> > > > In ovl_lookup() can store value of upper metacopy and after we get
> > > > the inode, set the OVL_UPPERDATA inode flag according to
> > > > upperdentry && !uppermetacopy.
> > > >
> > > > That would be consistent with ovl_obtain_alias() which sets the
> > > > OVL_UPPERDATA inode flag after getting the inode.
> > >
> > > Hi Amir,
> > >
> > > This patch implements what you are suggesting. Compile tested only.
> > > Does this look ok?
> > >
> >
> > It looks correct.
> >
> > > May be I don't need to split it up in lmetacopy and umetacopy. Ideally,
> > > lookup in lower layers should stop if an upper regular file is not
> > > metacopy. IOW, (upperdentry && !metacopy) might be sufficient check.
> > > Will look closely if this patch looks fine.
> > >
> >
> > I would stick uppermetacopy much like upperredirect and upperopaque.
>
> Ok, I introduced uppermetacopy and lowermetacopy. I need to make
> sure that I don't following metacopy file to lower layer if
> metacopy feature is off. This check should be done both for upper
> and lower metcopy files.
You don't need lowermetacopy for that. you can check d.metacopy
directly.
>
> >
> > This test:
> >
> > if (metacopy) {
> > /*
> > * Found a metacopy dentry but did not find corresponding
> > * data dentry
> > */
> > if (d.metacopy) {
> >
> > Is equivalent to if (d.metacopy) {
>
> Agreed. Updated the patch.
>
> >
> > I am not sure about:
> > if (ctr && (!upperdentry || (!d.is_dir && !metacopy)))
> > origin = stack[0].dentry;
> >
> > I will let you figure it out, but it feels like it is actually testing
> > !uppermetacopy
>
> Yes this is testing !uppermetacopy. I really want to simplify it a bit
> or atleast document it a bit that why metacopy case is different. Upper,
> regular files done't go through lower layer loop but upper metacopy
> files do. That's one difference which introduces some interesting
> code changes.
>
> - lower layer lookup loop already sets "origin" for metacopy files if
> indexing is on. This does not happen for regular non-metacopy files
> so they need to set origin here explicitly.
>
> if index feature is off, then we will not set "origin" for metacopy
> files in lower layer loop. But do we really need to set it given
> index is off and we don't want to lookup index.
>
> - We don't want to set origin if upper never had xattr ORIGIN. For
> regular files, ctr will be 0 or 1 if ORIGIN xattr was found on
> upper. But for metacopy upper files, ctr can be non-zero even
> if ORGIN xattr was not found. So that's another reason that
> we check for upper metacopy here.
>
> Difference between the case of regular and metacopy is subtle and
> I think this should be simplified otherwise its very easy to break
> it.
>
> I will spend some time on this after fixing the issue at hand. /me
> always gets lost in the mage of index and origin. There seem to
> be so many permutation and combination and its not clear to me
> when metacopy file is different than regular file w.r.t origin
> and index. It will be nice if we can minimize this difference and
> document it well so that future modifications are easy.
I agree it should be simplified.
If you cannot figure out how, let me know and I will try.
>
> Here is V2 of the patch. I added changelog. Also updated it to
> set OVL_UPPERDATA in ovl_instantiate(). This is creating a new
> file, so it can't be metacopy and should set OVL_UPPERDATA.
>
> Miklos and Amir, please let me know what do you think about this
> patch. I ran xfstetests overlay tests and these pass (except two
> which fail even without the patch and are meant to fail.).
>
> Thanks
> Vivek
>
>
> Subject: overlayfs: Initialize OVL_UPPERDATA in ovl_lookup()
>
> Currently ovl_get_inode() initializes OVL_UPPERDATA flag and for that it
> has to call ovl_check_metacopy_xattr() and check if metacopy xattr is
> present or not.
>
> yangerkun reported sometimes underlying filesystem might return -EIO
> and in that case error handling path does not cleanup properly leading
> to various warnings.
>
> Run generic/461 with ext4 upper/lower layer sometimes may trigger the
> bug as below(linux 4.19):
>
> [ 551.001349] overlayfs: failed to get metacopy (-5)
> [ 551.003464] overlayfs: failed to get inode (-5)
> [ 551.004243] overlayfs: cleanup of 'd44/fd51' failed (-5)
> [ 551.004941] overlayfs: failed to get origin (-5)
> [ 551.005199] ------------[ cut here ]------------
> [ 551.006697] WARNING: CPU: 3 PID: 24674 at fs/inode.c:1528 iput+0x33b/0x400
> ...
> [ 551.027219] Call Trace:
> [ 551.027623] ovl_create_object+0x13f/0x170
> [ 551.028268] ovl_create+0x27/0x30
> [ 551.028799] path_openat+0x1a35/0x1ea0
> [ 551.029377] do_filp_open+0xad/0x160
> [ 551.029944] ? vfs_writev+0xe9/0x170
> [ 551.030499] ? page_counter_try_charge+0x77/0x120
> [ 551.031245] ? __alloc_fd+0x160/0x2a0
> [ 551.031832] ? do_sys_open+0x189/0x340
> [ 551.032417] ? get_unused_fd_flags+0x34/0x40
> [ 551.033081] do_sys_open+0x189/0x340
> [ 551.033632] __x64_sys_creat+0x24/0x30
> [ 551.034219] do_syscall_64+0xd5/0x430
> [ 551.034800] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> One solution is to improve error handling and call iget_failed() if error
> is encountered. Amir thinks that this path is little intricate and there
> is not real need to check and initialize OVL_UPPERDATA in ovl_get_inode().
> Instead caller of ovl_get_inode() can initialize this state. And this
> will avoid double checking of metacopy xattr lookup in ovl_lookup()
> and ovl_get_inode().
>
> OVL_UPPERDATA is inode flag. So I was little concerned that initializing
> it outside ovl_get_inode() might have some races. But this is one way
> transition. That is once a file has been fully copied up, it can't go
> back to metacopy file again. And that seems to help avoid races. So
> as of now I can't see any races w.r.t OVL_UPPERDATA being set wrongly. So
> move settingof OVL_UPPERDATA inside the callers of ovl_get_inode().
> ovl_obtain_alias() already does it. So only two callers now left
> are ovl_lookup() and ovl_instantiate().
>
> metacopy variable has been split into two variables, lowermetacopy
> and uppermetacopy. It just makes it easier to understand whether
> metacopy if set on lower or upper. We need to set OVL_UPPERDATA
> only in case of uppermetacopy.
>
> Reported-by: yangerkun <yangerkun@xxxxxxxxxx>
> Suggested-by: Amir Goldstein <amir73il@xxxxxxxxx>
> Signed-off-by: Vivek Goyal <vgoyal@xxxxxxxxxx>
> ---
> fs/overlayfs/dir.c | 2 ++
> fs/overlayfs/inode.c | 11 +----------
> fs/overlayfs/namei.c | 25 ++++++++++++-------------
> 3 files changed, 15 insertions(+), 23 deletions(-)
>
> Index: redhat-linux/fs/overlayfs/namei.c
> ===================================================================
> --- redhat-linux.orig/fs/overlayfs/namei.c 2020-05-28 10:51:57.838556592 -0400
> +++ redhat-linux/fs/overlayfs/namei.c 2020-05-28 12:11:36.876964037 -0400
> @@ -823,7 +823,7 @@ struct dentry *ovl_lookup(struct inode *
> struct dentry *this;
> unsigned int i;
> int err;
> - bool metacopy = false;
> + bool uppermetacopy=false, lowermetacopy=false;
spaces around = and no need for lowermetacopy
> struct ovl_lookup_data d = {
> .sb = dentry->d_sb,
> .name = dentry->d_name,
> @@ -869,7 +869,7 @@ struct dentry *ovl_lookup(struct inode *
> goto out_put_upper;
>
> if (d.metacopy)
> - metacopy = true;
> + uppermetacopy = true;
> }
>
> if (d.redirect) {
> @@ -941,7 +941,7 @@ struct dentry *ovl_lookup(struct inode *
> }
>
> if (d.metacopy)
> - metacopy = true;
> + lowermetacopy = true;
> /*
> * Do not store intermediate metacopy dentries in chain,
> * except top most lower metacopy dentry
> @@ -982,16 +982,13 @@ struct dentry *ovl_lookup(struct inode *
> }
> }
>
> - if (metacopy) {
> - /*
> - * Found a metacopy dentry but did not find corresponding
> - * data dentry
> - */
> - if (d.metacopy) {
> - err = -EIO;
> - goto out_put;
> - }
> + /* Found a metacopy dentry but did not find corresponding data dentry */
> + if (d.metacopy) {
> + err = -EIO;
> + goto out_put;
> + }
>
> + if (lowermetacopy || uppermetacopy) {
> err = -EPERM;
> if (!ofs->config.metacopy) {
> pr_warn_ratelimited("refusing to follow metacopy origin for (%pd2)\n",
Move that test up to where setting metacopy = true for lower layers
similar to "refusing to follow redirect" and make it:
if (uppermetacopy || d.metacopy) {
Then you got rid of lowermetacopy.
> @@ -1023,7 +1020,7 @@ struct dentry *ovl_lookup(struct inode *
> *
> * Always lookup index of non-dir non-metacopy and non-upper.
> */
> - if (ctr && (!upperdentry || (!d.is_dir && !metacopy)))
> + if (ctr && (!upperdentry || (!d.is_dir && !uppermetacopy)))
> origin = stack[0].dentry;
>
I think this should be:
* Always lookup index of non-dir and non-upper.
*/
if (!origin && ctr && (!upperdentry || !d.is_dir))
origin = stack[0].dentry;
uppermetacopy is guaranteed to either have origin already set or
exit with an an error for ovl_verify_origin().
HOWEVER, if we set origin to lower, which turns out to be a lower
metacopy, we then skip this layer to the next one, but origin remains
set on the skipped layer dentry, which we had already dput().
Ay ay ay!
I think it would be best to move the check
* Do not store intermediate metacopy dentries in chain,
to right after ovl_lookup_layer(), before the ovl_fix_origin() and
ovl_verify_origin() checks.
Thanks,
Amir.
