[PATCH] ovl: fix failed copyup of fileattr on a symlink

[PATCH] ovl: fix failed copyup of fileattr on a symlink

[Amir Goldstein]

Some local filesystems support setting persistent fileattr flags
(e.g. FS_NOATIME_FL) on directories and regular files via ioctl.
Some of those persistent fileattr flags are reflected to vfs as
in-memory inode flags (e.g. S_NOATIME).

Overlayfs uses the in-memory inode flags (e.g. S_NOATIME) on a lower file
as an indication that a the lower file may have persistent inode fileattr
flags (e.g. FS_NOATIME_FL) that need to be copied to upper file.

However, in some cases, the S_NOATIME in-memory flag could be a false
indication for persistent FS_NOATIME_FL fileattr. For example, with NFS
and FUSE lower fs, as was the case in the two bug reports, the S_NOATIME
flag is set unconditionally for all inodes.

Users cannot set persistent fileattr flags on symlinks and special files,
but in some local fs, such as ext4/btrfs/tmpfs, the FS_NOATIME_FL fileattr
flag are inheritted to symlinks and special files from parent directory.

In both cases described above, when lower symlink has the S_NOATIME flag,
overlayfs will try to copy the symlink's fileattrs and fail with error
ENOXIO, because it could not open the symlink for the ioctl security hook.

To solve this failure, do not attempt to copyup fileattrs for anything
other than directories and regular files.

Reported-by: Ruiwen Zhao <ruiwen@google.com>
Link: https://lore.kernel.org/r/CAKd=y5Hpg7J2gxrFT02F94o=FM9QvGp=kcH1Grctx8HzFYvpiA@mail.gmail.com/
Fixes: 72db82115d2b ("ovl: copy up sync/noatime fileattr flags")
Cc: <stable@vger.kernel.org> # v5.15
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---

Hi Miklos,

Do you agree with this solution?

See fstests at:
https://github.com/amir73il/xfstests/blob/overlayfs-devel/tests/overlay/082

Thanks,
Amir.

 fs/overlayfs/copy_up.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index bae404a1bad4..d1761ec5866a 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -618,7 +618,8 @@ static int ovl_copy_up_metadata(struct ovl_copy_up_ctx *c, struct dentry *temp)
 	if (err)
 		return err;
 
-	if (inode->i_flags & OVL_COPY_I_FLAGS_MASK) {
+	if (inode->i_flags & OVL_COPY_I_FLAGS_MASK &&
+	    (S_ISREG(c->stat.mode) || S_ISDIR(c->stat.mode))) {
 		/*
 		 * Copy the fileattr inode flags that are the source of already
 		 * copied i_flags
-- 
2.34.1

Re: [PATCH] ovl: fix failed copyup of fileattr on a symlink

[Miklos Szeredi]

On Mon, 4 Sept 2023 at 15:24, Amir Goldstein <amir73il@gmail.com> wrote:
>
> Some local filesystems support setting persistent fileattr flags
> (e.g. FS_NOATIME_FL) on directories and regular files via ioctl.
> Some of those persistent fileattr flags are reflected to vfs as
> in-memory inode flags (e.g. S_NOATIME).
>
> Overlayfs uses the in-memory inode flags (e.g. S_NOATIME) on a lower file
> as an indication that a the lower file may have persistent inode fileattr
> flags (e.g. FS_NOATIME_FL) that need to be copied to upper file.
>
> However, in some cases, the S_NOATIME in-memory flag could be a false
> indication for persistent FS_NOATIME_FL fileattr. For example, with NFS
> and FUSE lower fs, as was the case in the two bug reports, the S_NOATIME
> flag is set unconditionally for all inodes.
>
> Users cannot set persistent fileattr flags on symlinks and special files,
> but in some local fs, such as ext4/btrfs/tmpfs, the FS_NOATIME_FL fileattr
> flag are inheritted to symlinks and special files from parent directory.
>
> In both cases described above, when lower symlink has the S_NOATIME flag,
> overlayfs will try to copy the symlink's fileattrs and fail with error
> ENOXIO, because it could not open the symlink for the ioctl security hook.
>
> To solve this failure, do not attempt to copyup fileattrs for anything
> other than directories and regular files.
>
> Reported-by: Ruiwen Zhao <ruiwen@google.com>
> Link: https://lore.kernel.org/r/CAKd=y5Hpg7J2gxrFT02F94o=FM9QvGp=kcH1Grctx8HzFYvpiA@mail.gmail.com/
> Fixes: 72db82115d2b ("ovl: copy up sync/noatime fileattr flags")
> Cc: <stable@vger.kernel.org> # v5.15
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>
> Hi Miklos,
>
> Do you agree with this solution?

It's good enough.   Linux might add API's in the future that allow
querying and setting fileattr on symlinks, but we can deal with that
later.

Thanks,
Miklos

Re: [PATCH] ovl: fix failed copyup of fileattr on a symlink

[Ruiwen Zhao]

On Mon, Sep 4, 2023 at 6:44 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Mon, 4 Sept 2023 at 15:24, Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > Some local filesystems support setting persistent fileattr flags
> > (e.g. FS_NOATIME_FL) on directories and regular files via ioctl.
> > Some of those persistent fileattr flags are reflected to vfs as
> > in-memory inode flags (e.g. S_NOATIME).
> >
> > Overlayfs uses the in-memory inode flags (e.g. S_NOATIME) on a lower file
> > as an indication that a the lower file may have persistent inode fileattr
> > flags (e.g. FS_NOATIME_FL) that need to be copied to upper file.
> >
> > However, in some cases, the S_NOATIME in-memory flag could be a false
> > indication for persistent FS_NOATIME_FL fileattr. For example, with NFS
> > and FUSE lower fs, as was the case in the two bug reports, the S_NOATIME
> > flag is set unconditionally for all inodes.
> >
> > Users cannot set persistent fileattr flags on symlinks and special files,
> > but in some local fs, such as ext4/btrfs/tmpfs, the FS_NOATIME_FL fileattr
> > flag are inheritted to symlinks and special files from parent directory.
> >
> > In both cases described above, when lower symlink has the S_NOATIME flag,
> > overlayfs will try to copy the symlink's fileattrs and fail with error
> > ENOXIO, because it could not open the symlink for the ioctl security hook.
> >
> > To solve this failure, do not attempt to copyup fileattrs for anything
> > other than directories and regular files.
> >
> > Reported-by: Ruiwen Zhao <ruiwen@google.com>
> > Link: https://lore.kernel.org/r/CAKd=y5Hpg7J2gxrFT02F94o=FM9QvGp=kcH1Grctx8HzFYvpiA@mail.gmail.com/
> > Fixes: 72db82115d2b ("ovl: copy up sync/noatime fileattr flags")
> > Cc: <stable@vger.kernel.org> # v5.15
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >
> > Hi Miklos,
> >
> > Do you agree with this solution?
>
> It's good enough.   Linux might add API's in the future that allow
> querying and setting fileattr on symlinks, but we can deal with that
> later.
>
> Thanks,
> Miklos

Thanks Amir for sending the fix! The fix looks good but I am not sure
how to LGTM it. (Still new to the kernel review process)

I believe the fix will be merged to the master branch first. Can we
backport it to 5.15, considering this is a regression and 5.15 is an
LTS version?

Thanks,
Ruiwen

Re: [PATCH] ovl: fix failed copyup of fileattr on a symlink

[Amir Goldstein]

On Mon, Sep 4, 2023 at 9:40 PM Ruiwen Zhao <ruiwen@google.com> wrote:
>
> On Mon, Sep 4, 2023 at 6:44 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Mon, 4 Sept 2023 at 15:24, Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > Some local filesystems support setting persistent fileattr flags
> > > (e.g. FS_NOATIME_FL) on directories and regular files via ioctl.
> > > Some of those persistent fileattr flags are reflected to vfs as
> > > in-memory inode flags (e.g. S_NOATIME).
> > >
> > > Overlayfs uses the in-memory inode flags (e.g. S_NOATIME) on a lower file
> > > as an indication that a the lower file may have persistent inode fileattr
> > > flags (e.g. FS_NOATIME_FL) that need to be copied to upper file.
> > >
> > > However, in some cases, the S_NOATIME in-memory flag could be a false
> > > indication for persistent FS_NOATIME_FL fileattr. For example, with NFS
> > > and FUSE lower fs, as was the case in the two bug reports, the S_NOATIME
> > > flag is set unconditionally for all inodes.
> > >
> > > Users cannot set persistent fileattr flags on symlinks and special files,
> > > but in some local fs, such as ext4/btrfs/tmpfs, the FS_NOATIME_FL fileattr
> > > flag are inheritted to symlinks and special files from parent directory.
> > >
> > > In both cases described above, when lower symlink has the S_NOATIME flag,
> > > overlayfs will try to copy the symlink's fileattrs and fail with error
> > > ENOXIO, because it could not open the symlink for the ioctl security hook.
> > >
> > > To solve this failure, do not attempt to copyup fileattrs for anything
> > > other than directories and regular files.
> > >
> > > Reported-by: Ruiwen Zhao <ruiwen@google.com>
> > > Link: https://lore.kernel.org/r/CAKd=y5Hpg7J2gxrFT02F94o=FM9QvGp=kcH1Grctx8HzFYvpiA@mail.gmail.com/
> > > Fixes: 72db82115d2b ("ovl: copy up sync/noatime fileattr flags")
> > > Cc: <stable@vger.kernel.org> # v5.15
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > ---
> > >
> > > Hi Miklos,
> > >
> > > Do you agree with this solution?
> >
> > It's good enough.   Linux might add API's in the future that allow
> > querying and setting fileattr on symlinks, but we can deal with that
> > later.
> >
> > Thanks,
> > Miklos
>
> Thanks Amir for sending the fix! The fix looks good but I am not sure
> how to LGTM it. (Still new to the kernel review process)
>
> I believe the fix will be merged to the master branch first. Can we
> backport it to 5.15, considering this is a regression and 5.15 is an
> LTS version?
>

The commit, when it gets merged will include:

    Reported-by: Ruiwen Zhao <ruiwen@google.com>
    Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217850
    Fixes: 72db82115d2b ("ovl: copy up sync/noatime fileattr flags")
    Cc: <stable@vger.kernel.org> # v5.15

So it should be automatically picked for stable bots
and by bugzilla I suppose.

Please let me know if you tested my patch and I will change to
Reported-and-tested-by:

Thanks,
Amir.

Re: [PATCH] ovl: fix failed copyup of fileattr on a symlink

[Ruiwen Zhao]

On Mon, Sep 4, 2023 at 11:57 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Mon, Sep 4, 2023 at 9:40 PM Ruiwen Zhao <ruiwen@google.com> wrote:
> >
> > On Mon, Sep 4, 2023 at 6:44 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > >
> > > On Mon, 4 Sept 2023 at 15:24, Amir Goldstein <amir73il@gmail.com> wrote:
> > > >
> > > > Some local filesystems support setting persistent fileattr flags
> > > > (e.g. FS_NOATIME_FL) on directories and regular files via ioctl.
> > > > Some of those persistent fileattr flags are reflected to vfs as
> > > > in-memory inode flags (e.g. S_NOATIME).
> > > >
> > > > Overlayfs uses the in-memory inode flags (e.g. S_NOATIME) on a lower file
> > > > as an indication that a the lower file may have persistent inode fileattr
> > > > flags (e.g. FS_NOATIME_FL) that need to be copied to upper file.
> > > >
> > > > However, in some cases, the S_NOATIME in-memory flag could be a false
> > > > indication for persistent FS_NOATIME_FL fileattr. For example, with NFS
> > > > and FUSE lower fs, as was the case in the two bug reports, the S_NOATIME
> > > > flag is set unconditionally for all inodes.
> > > >
> > > > Users cannot set persistent fileattr flags on symlinks and special files,
> > > > but in some local fs, such as ext4/btrfs/tmpfs, the FS_NOATIME_FL fileattr
> > > > flag are inheritted to symlinks and special files from parent directory.
> > > >
> > > > In both cases described above, when lower symlink has the S_NOATIME flag,
> > > > overlayfs will try to copy the symlink's fileattrs and fail with error
> > > > ENOXIO, because it could not open the symlink for the ioctl security hook.
> > > >
> > > > To solve this failure, do not attempt to copyup fileattrs for anything
> > > > other than directories and regular files.
> > > >
> > > > Reported-by: Ruiwen Zhao <ruiwen@google.com>
> > > > Link: https://lore.kernel.org/r/CAKd=y5Hpg7J2gxrFT02F94o=FM9QvGp=kcH1Grctx8HzFYvpiA@mail.gmail.com/
> > > > Fixes: 72db82115d2b ("ovl: copy up sync/noatime fileattr flags")
> > > > Cc: <stable@vger.kernel.org> # v5.15
> > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > > ---
> > > >
> > > > Hi Miklos,
> > > >
> > > > Do you agree with this solution?
> > >
> > > It's good enough.   Linux might add API's in the future that allow
> > > querying and setting fileattr on symlinks, but we can deal with that
> > > later.
> > >
> > > Thanks,
> > > Miklos
> >
> > Thanks Amir for sending the fix! The fix looks good but I am not sure
> > how to LGTM it. (Still new to the kernel review process)
> >
> > I believe the fix will be merged to the master branch first. Can we
> > backport it to 5.15, considering this is a regression and 5.15 is an
> > LTS version?
> >
>
> The commit, when it gets merged will include:
>
>     Reported-by: Ruiwen Zhao <ruiwen@google.com>
>     Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217850
>     Fixes: 72db82115d2b ("ovl: copy up sync/noatime fileattr flags")
>     Cc: <stable@vger.kernel.org> # v5.15
>
> So it should be automatically picked for stable bots
> and by bugzilla I suppose.
>
> Please let me know if you tested my patch and I will change to
> Reported-and-tested-by:
>
> Thanks,
> Amir.

Thanks! I tested this patch with the same repro step mentioned before
and can confirm it fixed the issue. Also checked dmesg and can confirm
there is no error. I think we can merge the fix.

ruiwen@instance-1:/tmp # mv merged/home/ruiwen/foolink
merged/home/ruiwen/foolink2
ruiwen@instance-1:/tmp # ls -l merged/home/ruiwen/ -l
total 0
-rw-r--r-- 1 root root 0 Sep  6 00:07 foo
lrwxrwxrwx 1 root root 3 Sep  6 00:07 foolink2 -> foo



Thanks,
Ruiwen