linux-unionfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Miklos Szeredi <miklos@szeredi.hu>
To: Kevin Locke <kevin@kevinlocke.name>, Amir Goldstein <amir73il@gmail.com>
Cc: overlayfs <linux-unionfs@vger.kernel.org>,
	Miklos Szeredi <mszeredi@redhat.com>
Subject: Re: [Regression] ovl: rename(2) EINVAL if lower doesn't support fileattrs
Date: Mon, 18 Oct 2021 10:47:43 +0200	[thread overview]
Message-ID: <CAJfpegsRo3e-9B64W37YrmvDcjo0QB9t+coAW3mO6TSqdROz2w@mail.gmail.com> (raw)
In-Reply-To: <YWyLigrybF6yzf6Y@kevinlocke.name>

[-- Attachment #1: Type: text/plain, Size: 2355 bytes --]

On Sun, 17 Oct 2021 at 22:53, Kevin Locke <kevin@kevinlocke.name> 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

[-- Attachment #2: ovl-fix-fileattr-copy-up-failure.patch --]
[-- Type: text/x-patch, Size: 1270 bytes --]

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)

  reply	other threads:[~2021-10-18  8:47 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20210910001820.174272-1-sashal@kernel.org>
2021-09-10  0:17 ` [PATCH AUTOSEL 5.13 40/88] ovl: copy up sync/noatime fileattr flags Sasha Levin
2021-10-17 20:46   ` [Regression] ovl: rename(2) EINVAL if lower doesn't support fileattrs Kevin Locke
2021-10-18  8:47     ` Miklos Szeredi [this message]
2021-10-18 14:02       ` Kevin Locke
2021-09-10  0:17 ` [PATCH AUTOSEL 5.13 41/88] ovl: skip checking lower file's i_writecount on truncate Sasha Levin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAJfpegsRo3e-9B64W37YrmvDcjo0QB9t+coAW3mO6TSqdROz2w@mail.gmail.com \
    --to=miklos@szeredi.hu \
    --cc=amir73il@gmail.com \
    --cc=kevin@kevinlocke.name \
    --cc=linux-unionfs@vger.kernel.org \
    --cc=mszeredi@redhat.com \
    --subject='Re: [Regression] ovl: rename(2) EINVAL if lower doesn'\''t support fileattrs' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).