[PATCH] ovl: relax lookup error on mismatch origin ftype

[PATCH] ovl: relax lookup error on mismatch origin ftype

[Amir Goldstein]

We get occasional reports of lookup errors due to mismatched
origin ftype from users that re-format a lower squashfs image.

Commit 13c6ad0f45fd ("ovl: document lower modification caveats")
tries to discourage the practice of re-formating lower layers and
describes the expected behavior as undefined.

Commit b0e0f69731cd ("ovl: restrict lower null uuid for "xino=auto"")
limits the configurations in which origin file handles are followed.

In addition to these measures, change the behavior in case of detecting
a mismatch origin ftype in lookup to issue a warning, not follow origin,
but not fail the lookup operation either.

That should make overall more users happy without any big consequences.

Link: https://lore.kernel.org/linux-unionfs/CAOQ4uxgPq9E9xxwU2CDyHy-_yCZZeymg+3n+-6AqkGGE1YtwvQ@mail.gmail.com/
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---

Miklos,

I am getting tired of dealing with lower squashfs related reports.
How about this?

It passes the xfstests quick tests and no, I do not have a reproducer
for origin mismatch, so will wait for Byron to test the patch.

Thanks,
Amir.

 fs/overlayfs/namei.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 71e264e2f16b..850c0a37f1f0 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -392,7 +392,7 @@ int ovl_check_origin_fh(struct ovl_fs *ofs, struct ovl_fh *fh, bool connected,
 			    upperdentry, d_inode(upperdentry)->i_mode & S_IFMT,
 			    d_inode(origin)->i_mode & S_IFMT);
 	dput(origin);
-	return -EIO;
+	return -EINVAL;
 }
 
 static int ovl_check_origin(struct ovl_fs *ofs, struct dentry *upperdentry,
@@ -408,7 +408,7 @@ static int ovl_check_origin(struct ovl_fs *ofs, struct dentry *upperdentry,
 	kfree(fh);
 
 	if (err) {
-		if (err == -ESTALE)
+		if (err == -ESTALE || err == -EINVAL)
 			return 0;
 		return err;
 	}
-- 
2.25.1

Re: [PATCH] ovl: relax lookup error on mismatch origin ftype

[Amir Goldstein]

On Fri, Apr 30, 2021 at 12:59 PM www <ouyangxuan10@163.com> wrote:
>
> Hi Amir,
>
>
> Thank you very much for your help.  I have another question to clarify.
>
> >> 3. If we upgrade overlayfs separately, we are not very good at verifying that we have solved this problem, because the recurrence probability of this problem is very low. So I want to ask, how can we quickly reproduce this problem?
>
> >Re-creating a lower squashfs after files have been copied to upper should
> >reproduce the problem quite often.
>
> Does the re-creating lower squashfs here mean that remount squashfs?
> I've tested dozens of times in the remount way, but I haven't found this problem again?
> My test steps are:
> 1). umount lower squashfs;
> 2). modify the file in upper dir;
> 3). mount lower squanshfs;
> 4). restart service(it will re-parse the modified file)  or reboot the system and the problem didn't happen.
>

No. That's not what I mean by re-creating lower fs.
What I mean is that overlayfs is the file is question is in the squashfs
image and has been copied up.

I don't know where the squashfs image you are using comes from
but I am guessing you have replaced it with a new squashfs image.
In the new squashfs image, files have different inode numbers.

I reckon this behavior is common for OpenWRT where the system
image is being upgraded.

>
> At 2021-04-27 18:28:26, "Amir Goldstein" <amir73il@gmail.com> wrote:
> >We get occasional reports of lookup errors due to mismatched
> >origin ftype from users that re-format a lower squashfs image.
> >
> >Commit 13c6ad0f45fd ("ovl: document lower modification caveats")
> >tries to discourage the practice of re-formating lower layers and
> >describes the expected behavior as undefined.
> >
> >Commit b0e0f69731cd ("ovl: restrict lower null uuid for "xino=auto"")
> >limits the configurations in which origin file handles are followed.
> >
> >In addition to these measures, change the behavior in case of detecting
> >a mismatch origin ftype in lookup to issue a warning, not follow origin,
> >but not fail the lookup operation either.
> >
> >That should make overall more users happy without any big consequences.
> >
>
> We can't do that. When overlayfs reports an error, there may be some very important files that cannot be correctly parsed, resulting in some important services can not be started smoothly, which directly affects the use of the whole system.

Well, that is the purpose of my patch -
When overlayfs reports the warning it will NOT prevent reading the
file, so it should NOT make the whole system unusable.

Thanks,
Amir.

Re: Re: [PATCH] ovl: relax lookup error on mismatch origin ftype

[Amir Goldstein]

On Thu, May 6, 2021 at 12:49 PM www <ouyangxuan10@163.com> wrote:
>
> Hi Amir,
>
>
>
> At 2021-05-01 00:16:28, "Amir Goldstein" <amir73il@gmail.com> wrote:
> >On Fri, Apr 30, 2021 at 12:59 PM www <ouyangxuan10@163.com> wrote:
> >>
> >> Hi Amir,
> >>
> >>
> >> Thank you very much for your help.  I have another question to clarify.
> >>
> >> >> 3. If we upgrade overlayfs separately, we are not very good at verifying that we have solved this problem, because the recurrence probability of this problem is very low. So I want to ask, how can we quickly reproduce this problem?
> >>
> >> >Re-creating a lower squashfs after files have been copied to upper should
> >> >reproduce the problem quite often.
> >>
> >> Does the re-creating lower squashfs here mean that remount squashfs?
> >> I've tested dozens of times in the remount way, but I haven't found this problem again?
> >> My test steps are:
> >> 1). umount lower squashfs;
> >> 2). modify the file in upper dir;
> >> 3). mount lower squanshfs;
> >> 4). restart service(it will re-parse the modified file)  or reboot the system and the problem didn't happen.
> >>
> >
> >No. That's not what I mean by re-creating lower fs.
> >What I mean is that overlayfs is the file is question is in the squashfs
> >image and has been copied up.
> >
> >I don't know where the squashfs image you are using comes from
> >but I am guessing you have replaced it with a new squashfs image.
> >In the new squashfs image, files have different inode numbers.
> >
> >I reckon this behavior is common for OpenWRT where the system
> >image is being upgraded.
> >
> https://github.com/openbmc/openbmc/blob/master/meta-phosphor/recipes-phosphor/initrdscripts/files/obmc-init.sh   (init file)
> ...
> line 376: mount  /run/image-rofs /run/initramfs/ro -t  squanshfs -o ro,loop          (line 358, set copy-base-filesystem-to-ram=y)
> ...
> line 416: mkdir -p $upper/var/log      --- new add line, not in the file
> line 417: mount -t overlayfs -o lowerdir=run/initramfs/ro,upperdir=run/initramfs/rw/cow,workdir=run/initramfs/rw/work  cow  /root
>  ...
> I would like to ask if the newly added line(line 416)  or "set set copy-base-filesystem-to-ram=y" causes this problem?(This folder is created and the value is set every time the system starts, but the probability of problems is not high)
> There are no other changes.
>
>

I don't know.

I am not sure what wasn't clear in my answer.

The lowerfs image, namely /run/image-rofs in your script can be downloaded
from tftp or from the web. I have no idea what actually happens in your setup.

Every time you run this init script this lower fs image may change, while the
upper f2fs rwfs does not change.
When that happens, it MAY cause the reported issue.

Thanks,
Amir.

Re: Re: Re: [PATCH] ovl: relax lookup error on mismatch origin ftype

[Amir Goldstein]

On Fri, May 7, 2021 at 6:51 AM www <ouyangxuan10@163.com> wrote:
>
> Hi Amir，
>
>
>
>
> At 2021-05-06 18:25:51, "Amir Goldstein" <amir73il@gmail.com> wrote:
> >On Thu, May 6, 2021 at 12:49 PM www <ouyangxuan10@163.com> wrote:
> >>
> >> Hi Amir,
> >>
> >>
> >>
> >> At 2021-05-01 00:16:28, "Amir Goldstein" <amir73il@gmail.com> wrote:
> >> >On Fri, Apr 30, 2021 at 12:59 PM www <ouyangxuan10@163.com> wrote:
> >> >>
> >> >> Hi Amir,
> >> >>
> >> >>
> >> >> Thank you very much for your help.  I have another question to clarify.
> >> >>
> >> >> >> 3. If we upgrade overlayfs separately, we are not very good at verifying that we have solved this problem, because the recurrence probability of this problem is very low. So I want to ask, how can we quickly reproduce this problem?
> >> >>
> >> >> >Re-creating a lower squashfs after files have been copied to upper should
> >> >> >reproduce the problem quite often.
> >> >>
> >> >> Does the re-creating lower squashfs here mean that remount squashfs?
> >> >> I've tested dozens of times in the remount way, but I haven't found this problem again?
> >> >> My test steps are:
> >> >> 1). umount lower squashfs;
> >> >> 2). modify the file in upper dir;
> >> >> 3). mount lower squanshfs;
> >> >> 4). restart service(it will re-parse the modified file)  or reboot the system and the problem didn't happen.
> >> >>
> >> >
> >> >No. That's not what I mean by re-creating lower fs.
> >> >What I mean is that overlayfs is the file is question is in the squashfs
> >> >image and has been copied up.
> >> >
> >> >I don't know where the squashfs image you are using comes from
> >> >but I am guessing you have replaced it with a new squashfs image.
> >> >In the new squashfs image, files have different inode numbers.
> >> >
> >> >I reckon this behavior is common for OpenWRT where the system
> >> >image is being upgraded.
> >> >
> >> https://github.com/openbmc/openbmc/blob/master/meta-phosphor/recipes-phosphor/initrdscripts/files/obmc-init.sh   (init file)
> >> ...
> >> line 376: mount  /run/image-rofs /run/initramfs/ro -t  squanshfs -o ro,loop          (line 358, set copy-base-filesystem-to-ram=y)
> >> ...
> >> line 416: mkdir -p $upper/var/log      --- new add line, not in the file
> >> line 417: mount -t overlayfs -o lowerdir=run/initramfs/ro,upperdir=run/initramfs/rw/cow,workdir=run/initramfs/rw/work  cow  /root
> >>  ...
> >> I would like to ask if the newly added line(line 416)  or "set set copy-base-filesystem-to-ram=y" causes this problem?(This folder is created and the value is set every time the system starts, but the probability of problems is not high)
> >> There are no other changes.
> >>
> >>
> >
> >I don't know.
> >
> >I am not sure what wasn't clear in my answer.
> >
> >The lowerfs image, namely /run/image-rofs in your script can be downloaded
> >from tftp or from the web. I have no idea what actually happens in your setup.
> >
> the /run/image-rofs image is copied form flash.
> line 359:  test ! -e /run/image-rofs && ! cp $rodev /run/image-rofs        -- the $rodev is mtd4
>
>
> >Every time you run this init script this lower fs image may change, while the
> >upper f2fs rwfs does not change.
> >When that happens, it MAY cause the reported issue.
> >
> the /run/image-rofs is squashfs, readonly, cannot be modified.
> Do you mean that when I upgrade the new firmware, rofs (squashfs) has been updated (rofs has been modified), is it easy to have this problem?
>

YES!

Thanks,
Amir.

Re: [PATCH] ovl: relax lookup error on mismatch origin ftype

[Miklos Szeredi]

On Tue, 27 Apr 2021 at 12:28, Amir Goldstein <amir73il@gmail.com> wrote:
>
> We get occasional reports of lookup errors due to mismatched
> origin ftype from users that re-format a lower squashfs image.
>
> Commit 13c6ad0f45fd ("ovl: document lower modification caveats")
> tries to discourage the practice of re-formating lower layers and
> describes the expected behavior as undefined.
>
> Commit b0e0f69731cd ("ovl: restrict lower null uuid for "xino=auto"")
> limits the configurations in which origin file handles are followed.
>
> In addition to these measures, change the behavior in case of detecting
> a mismatch origin ftype in lookup to issue a warning, not follow origin,
> but not fail the lookup operation either.
>
> That should make overall more users happy without any big consequences.
>
> Link: https://lore.kernel.org/linux-unionfs/CAOQ4uxgPq9E9xxwU2CDyHy-_yCZZeymg+3n+-6AqkGGE1YtwvQ@mail.gmail.com/
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>
> Miklos,
>
> I am getting tired of dealing with lower squashfs related reports.
> How about this?
>
> It passes the xfstests quick tests and no, I do not have a reproducer
> for origin mismatch, so will wait for Byron to test the patch.

Pushed a simplified variant that just changes the
ovl_check_origin_fh() return value from -EIO to -ESTALE.   Do you see
a problem with this?

Thanks,
Miklos



>
> Thanks,
> Amir.
>
>  fs/overlayfs/namei.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index 71e264e2f16b..850c0a37f1f0 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -392,7 +392,7 @@ int ovl_check_origin_fh(struct ovl_fs *ofs, struct ovl_fh *fh, bool connected,
>                             upperdentry, d_inode(upperdentry)->i_mode & S_IFMT,
>                             d_inode(origin)->i_mode & S_IFMT);
>         dput(origin);
> -       return -EIO;
> +       return -EINVAL;
>  }
>
>  static int ovl_check_origin(struct ovl_fs *ofs, struct dentry *upperdentry,
> @@ -408,7 +408,7 @@ static int ovl_check_origin(struct ovl_fs *ofs, struct dentry *upperdentry,
>         kfree(fh);
>
>         if (err) {
> -               if (err == -ESTALE)
> +               if (err == -ESTALE || err == -EINVAL)
>                         return 0;
>                 return err;
>         }
> --
> 2.25.1
>

Re: [PATCH] ovl: relax lookup error on mismatch origin ftype

[Amir Goldstein]

On Wed, Jul 21, 2021 at 4:10 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Tue, 27 Apr 2021 at 12:28, Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > We get occasional reports of lookup errors due to mismatched
> > origin ftype from users that re-format a lower squashfs image.
> >
> > Commit 13c6ad0f45fd ("ovl: document lower modification caveats")
> > tries to discourage the practice of re-formating lower layers and
> > describes the expected behavior as undefined.
> >
> > Commit b0e0f69731cd ("ovl: restrict lower null uuid for "xino=auto"")
> > limits the configurations in which origin file handles are followed.
> >
> > In addition to these measures, change the behavior in case of detecting
> > a mismatch origin ftype in lookup to issue a warning, not follow origin,
> > but not fail the lookup operation either.
> >
> > That should make overall more users happy without any big consequences.
> >
> > Link: https://lore.kernel.org/linux-unionfs/CAOQ4uxgPq9E9xxwU2CDyHy-_yCZZeymg+3n+-6AqkGGE1YtwvQ@mail.gmail.com/
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >
> > Miklos,
> >
> > I am getting tired of dealing with lower squashfs related reports.
> > How about this?
> >
> > It passes the xfstests quick tests and no, I do not have a reproducer
> > for origin mismatch, so will wait for Byron to test the patch.
>
> Pushed a simplified variant that just changes the
> ovl_check_origin_fh() return value from -EIO to -ESTALE.   Do you see
> a problem with this?
>

The only difference is if ovl_fh_to_dentry() also emits a warning,
but since ovl_check_origin_fh() already has a warning I think that
your simplified version is fine.

Thanks,
Amir.

Re: [PATCH] ovl: relax lookup error on mismatch origin ftype

[Miklos Szeredi]

On Wed, 21 Jul 2021 at 16:17, Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Wed, Jul 21, 2021 at 4:10 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Tue, 27 Apr 2021 at 12:28, Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > We get occasional reports of lookup errors due to mismatched
> > > origin ftype from users that re-format a lower squashfs image.
> > >
> > > Commit 13c6ad0f45fd ("ovl: document lower modification caveats")
> > > tries to discourage the practice of re-formating lower layers and
> > > describes the expected behavior as undefined.
> > >
> > > Commit b0e0f69731cd ("ovl: restrict lower null uuid for "xino=auto"")
> > > limits the configurations in which origin file handles are followed.
> > >
> > > In addition to these measures, change the behavior in case of detecting
> > > a mismatch origin ftype in lookup to issue a warning, not follow origin,
> > > but not fail the lookup operation either.
> > >
> > > That should make overall more users happy without any big consequences.
> > >
> > > Link: https://lore.kernel.org/linux-unionfs/CAOQ4uxgPq9E9xxwU2CDyHy-_yCZZeymg+3n+-6AqkGGE1YtwvQ@mail.gmail.com/
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > ---
> > >
> > > Miklos,
> > >
> > > I am getting tired of dealing with lower squashfs related reports.
> > > How about this?
> > >
> > > It passes the xfstests quick tests and no, I do not have a reproducer
> > > for origin mismatch, so will wait for Byron to test the patch.
> >
> > Pushed a simplified variant that just changes the
> > ovl_check_origin_fh() return value from -EIO to -ESTALE.   Do you see
> > a problem with this?
> >
>
> The only difference is if ovl_fh_to_dentry() also emits a warning,
> but since ovl_check_origin_fh() already has a warning I think that
> your simplified version is fine.

Yeah, you are right, new version could have one less warning in the
->fh_to_dentry() case, but I agree that that should not be a problem.

Thanks,
Miklos

Re: [PATCH] ovl: relax lookup error on mismatch origin ftype

[Amir Goldstein]

On Wed, Jul 21, 2021 at 5:33 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Wed, 21 Jul 2021 at 16:17, Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Wed, Jul 21, 2021 at 4:10 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > >
> > > On Tue, 27 Apr 2021 at 12:28, Amir Goldstein <amir73il@gmail.com> wrote:
> > > >
> > > > We get occasional reports of lookup errors due to mismatched
> > > > origin ftype from users that re-format a lower squashfs image.
> > > >
> > > > Commit 13c6ad0f45fd ("ovl: document lower modification caveats")
> > > > tries to discourage the practice of re-formating lower layers and
> > > > describes the expected behavior as undefined.
> > > >
> > > > Commit b0e0f69731cd ("ovl: restrict lower null uuid for "xino=auto"")
> > > > limits the configurations in which origin file handles are followed.
> > > >
> > > > In addition to these measures, change the behavior in case of detecting
> > > > a mismatch origin ftype in lookup to issue a warning, not follow origin,
> > > > but not fail the lookup operation either.
> > > >
> > > > That should make overall more users happy without any big consequences.
> > > >
> > > > Link: https://lore.kernel.org/linux-unionfs/CAOQ4uxgPq9E9xxwU2CDyHy-_yCZZeymg+3n+-6AqkGGE1YtwvQ@mail.gmail.com/
> > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > > ---
> > > >
> > > > Miklos,
> > > >
> > > > I am getting tired of dealing with lower squashfs related reports.
> > > > How about this?
> > > >
> > > > It passes the xfstests quick tests and no, I do not have a reproducer
> > > > for origin mismatch, so will wait for Byron to test the patch.
> > >
> > > Pushed a simplified variant that just changes the
> > > ovl_check_origin_fh() return value from -EIO to -ESTALE.   Do you see
> > > a problem with this?
> > >
> >
> > The only difference is if ovl_fh_to_dentry() also emits a warning,
> > but since ovl_check_origin_fh() already has a warning I think that
> > your simplified version is fine.
>
> Yeah, you are right, new version could have one less warning in the
> ->fh_to_dentry() case, but I agree that that should not be a problem.
>

Technically, there is also a change in vfs api in the error returned to
exportfs_decode_fh_raw(), but both callers seem to convert EINVAL
to ESTALE anyway...

Thanks,
Amir.