On Sun, 17 Oct 2021 at 22:53, Kevin Locke <kevin@xxxxxxxxxxxxxxx> wrote:
>
> Hi all,
>
> With 5.15-rc5 or torvalds master (d999ade1cc86), attempting to rename
> a file fails with -EINVAL on an overlayfs mount with a lower
> filesystem that returns -EINVAL for ioctl(FS_IOC_GETFLAGS). For
> example, with ntfs-3g:
>
> mkdir lower upper work overlay
> dd if=/dev/zero of=ntfs.raw bs=1M count=2
> mkntfs -F ntfs.raw
> mount ntfs.raw lower
> touch lower/file.txt
> mount -t overlay -o "lowerdir=$PWD/lower,upperdir=$PWD/upper,workdir=$PWD/work" - overlay
> mv overlay/file.txt overlay/file2.txt
>
> mv fails and (misleadingly) prints
>
> mv: cannot move 'overlay/file.txt' to a subdirectory of itself, 'overlay/file2.txt'
>
> which strace(1) reveals to be due to rename(2) returning -22
> (-EINVAL). A bit of digging revealed that -EINVAL is coming from
> vfs_fileattr_get() with the following stack:
>
> ovl_real_fileattr_get.cold+0x9/0x12 [overlay]
> ovl_copy_up_inode+0x1b5/0x280 [overlay]
> ovl_copy_up_one+0xaf1/0xee0 [overlay]
> ovl_copy_up_flags+0xab/0xf0 [overlay]
> ovl_rename+0x149/0x850 [overlay]
> ? privileged_wrt_inode_uidgid+0x47/0x60
> ? generic_permission+0x90/0x200
> ? ovl_permission+0x70/0x120 [overlay]
> vfs_rename+0x619/0x9d0
> do_renameat2+0x3c0/0x570
> __x64_sys_renameat2+0x4b/0x60
> do_syscall_64+0x3b/0xc0
> entry_SYSCALL_64_after_hwframe+0x44/0xae
>
> This issue does not occur on 5.14. I've bisected the regression to
> 72db82115d2b.
This is clearly a regression. Not trivial how far the fix should go, though.
One option is to just ignore all errors from ovl_copy_fileattr(),
which would solve this and similar issues. However that would result
in missing the cases when the attributes were really meant to be
copied up, but failed to do so for some reason.
If vfs_fileattr_get() fails with ENOIOCTLCMD or ENOTTY on lower, that
obviously means we need to return success (lower fs does not support
fileattr). As ntfs-3g seems to return EINVAL that needs to be added
too.
More interesting question is what to do with get/set failures on
upper. My feeling is that for now we should try to return errors
(even ENOTTY), but should print a warning in the kernel log. If that
turns out to regress some use cases, then that needs to fixed as well.
Untested patch attached.
Thanks,
Miklos
diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 4e7d5bfa2949..ed0a0ce6deda 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -140,12 +140,21 @@ static int ovl_copy_fileattr(struct inode *inode, struct path *old,
int err;
err = ovl_real_fileattr_get(old, &oldfa);
- if (err)
+ if (err) {
+ /* Ntfs-3g return -EINVAL for "no fileattr support" */
+ if (err == -ENOTTY || err == -EINVAL)
+ return 0;
+ pr_warn("failed to retrieve fileattr (%pd2, err=%i)\n",
+ old, err);
return err;
+ }
err = ovl_real_fileattr_get(new, &newfa);
- if (err)
+ if (err) {
+ pr_warn("failed to retrieve fileattr (%pd2, err=%i)\n",
+ old, err);
return err;
+ }
/*
* We cannot set immutable and append-only flags on upper inode,
diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 832b17589733..1f36158c7dbe 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -610,7 +610,10 @@ int ovl_real_fileattr_get(struct path *realpath, struct fileattr *fa)
if (err)
return err;
- return vfs_fileattr_get(realpath->dentry, fa);
+ err = vfs_fileattr_get(realpath->dentry, fa);
+ if (err == -ENOIOCTLCMD)
+ err = -ENOTTY;
+ return err;
}
int ovl_fileattr_get(struct dentry *dentry, struct fileattr *fa)
