linux-unionfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] ovl: fix NULL pointer dereference
@ 2022-01-12 18:33 Christoph Fritz
  2022-01-12 18:46 ` Christoph Fritz
  2022-01-14 15:59 ` Miklos Szeredi
  0 siblings, 2 replies; 9+ messages in thread
From: Christoph Fritz @ 2022-01-12 18:33 UTC (permalink / raw)
  To: Miklos Szeredi; +Cc: Kevin Locke, linux-unionfs

This patch is fixing a NULL pointer dereference to get a recently
introduced warning message working.

Fixes: 5b0a414d06c3 ("ovl: fix filattr copy-up failure")
Signed-off-by: Christoph Fritz <chf.fritz@googlemail.com>
---
 fs/overlayfs/copy_up.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index b193d08a3dc3..347b06479663 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -145,7 +145,7 @@ static int ovl_copy_fileattr(struct inode *inode, struct path *old,
 		if (err == -ENOTTY || err == -EINVAL)
 			return 0;
 		pr_warn("failed to retrieve lower fileattr (%pd2, err=%i)\n",
-			old, err);
+			old->dentry, err);
 		return err;
 	}
 
@@ -168,7 +168,7 @@ static int ovl_copy_fileattr(struct inode *inode, struct path *old,
 	err = ovl_real_fileattr_get(new, &newfa);
 	if (err) {
 		pr_warn("failed to retrieve upper fileattr (%pd2, err=%i)\n",
-			new, err);
+			new->dentry, err);
 		return err;
 	}
 
-- 
2.30.2



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] ovl: fix NULL pointer dereference
  2022-01-12 18:33 [PATCH] ovl: fix NULL pointer dereference Christoph Fritz
@ 2022-01-12 18:46 ` Christoph Fritz
  2022-01-12 20:28   ` Christoph Fritz
  2022-01-12 20:58   ` Kevin Locke
  2022-01-14 15:59 ` Miklos Szeredi
  1 sibling, 2 replies; 9+ messages in thread
From: Christoph Fritz @ 2022-01-12 18:46 UTC (permalink / raw)
  To: Miklos Szeredi; +Cc: Kevin Locke, linux-unionfs

Hello Miklos

On Wed, 2022-01-12 at 19:33 +0100, Christoph Fritz wrote:
> This patch is fixing a NULL pointer dereference to get a recently
> introduced warning message working.

With that patch applied, a lot of these are popping up now:

[    7.132514] overlayfs: failed to retrieve upper fileattr (index/#26, err=-25)
[    7.141520] overlayfs: failed to retrieve upper fileattr (index/#27, err=-25)
[    8.699070] overlayfs: failed to retrieve upper fileattr (index/#7, err=-25)
[    8.715804] overlayfs: failed to retrieve upper fileattr (index/#8, err=-25)
[    8.723218] overlayfs: failed to retrieve upper fileattr (index/#9, err=-25)
[    8.829887] overlayfs: failed to retrieve upper fileattr (index/#43, err=-25)
[    9.387676] overlayfs: failed to retrieve upper fileattr (index/#a, err=-25)
[    9.667531] overlayfs: failed to retrieve upper fileattr (index/#b, err=-25)
[    9.874005] overlayfs: failed to retrieve upper fileattr (index/#c, err=-25)
[    9.934664] overlayfs: failed to retrieve upper fileattr (index/#58, err=-25)
[    9.942036] overlayfs: failed to retrieve upper fileattr (index/#59, err=-25)
[    9.949406] overlayfs: failed to retrieve upper fileattr (index/#60, err=-25)
[    9.956738] overlayfs: failed to retrieve upper fileattr (index/#61, err=-25)
[   10.311610] overlayfs: failed to retrieve upper fileattr (index/#d, err=-25)
[   10.712019] overlayfs: failed to retrieve upper fileattr (index/#e, err=-25)
[   31.901577] overlayfs: failed to retrieve upper fileattr (index/#64, err=-25)

These have been -ENOIOCTLCMD errors but got (falsely?) converted to
-ENOTTY by the recently introduced commit 5b0a414d06c3 ("ovl: fix filattr copy-up failure"):

+	if (err == -ENOIOCTLCMD)
+		err = -ENOTTY;

Any ideas?

Thanks
 -- Christoph


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] ovl: fix NULL pointer dereference
  2022-01-12 18:46 ` Christoph Fritz
@ 2022-01-12 20:28   ` Christoph Fritz
  2022-01-14 16:01     ` Miklos Szeredi
  2022-01-12 20:58   ` Kevin Locke
  1 sibling, 1 reply; 9+ messages in thread
From: Christoph Fritz @ 2022-01-12 20:28 UTC (permalink / raw)
  To: Miklos Szeredi; +Cc: Kevin Locke, linux-unionfs

> 
> [    9.956738] overlayfs: failed to retrieve upper fileattr
> (index/#61, err=-25)
> [   10.311610] overlayfs: failed to retrieve upper fileattr
> (index/#d, err=-25)
> [   10.712019] overlayfs: failed to retrieve upper fileattr
> (index/#e, err=-25)
> [   31.901577] overlayfs: failed to retrieve upper fileattr
> (index/#64, err=-25)
> 
> These have been -ENOIOCTLCMD errors but got (falsely?) converted to
> -ENOTTY by the recently introduced commit 5b0a414d06c3 ("ovl: fix
> filattr copy-up failure"):
> 
> +       if (err == -ENOIOCTLCMD)
> +               err = -ENOTTY;
> 
> Any ideas?
> 

Doing the same "quirk" for upper fileattr seems to fix the issues, but
I have no clue about any other implications:

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 347b06479663..1e69bc000dd8 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -167,6 +167,8 @@ static int ovl_copy_fileattr(struct inode *inode, struct path *old,
 
 	err = ovl_real_fileattr_get(new, &newfa);
 	if (err) {
+		if (err == -ENOTTY || err == -EINVAL)
+			return 0;
 		pr_warn("failed to retrieve upper fileattr (%pd2, err=%i)\n",
 			new->dentry, err);
 		return err;






^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] ovl: fix NULL pointer dereference
  2022-01-12 18:46 ` Christoph Fritz
  2022-01-12 20:28   ` Christoph Fritz
@ 2022-01-12 20:58   ` Kevin Locke
  2022-01-12 21:06     ` Christoph Fritz
  1 sibling, 1 reply; 9+ messages in thread
From: Kevin Locke @ 2022-01-12 20:58 UTC (permalink / raw)
  To: Christoph Fritz; +Cc: Miklos Szeredi, linux-unionfs

Hi Christoph,

On Wed, 2022-01-12 at 19:46 +0100, Christoph Fritz wrote:
> On Wed, 2022-01-12 at 19:33 +0100, Christoph Fritz wrote:
>> This patch is fixing a NULL pointer dereference to get a recently
>> introduced warning message working.

Good catch!

> With that patch applied, a lot of these are popping up now:
> 
> [    7.132514] overlayfs: failed to retrieve upper fileattr (index/#26, err=-25)
>
> [...]
> 
> These have been -ENOIOCTLCMD errors but got (falsely?) converted to
> -ENOTTY by the recently introduced commit 5b0a414d06c3 ("ovl: fix filattr copy-up failure"):

Which filesystem are you using for upper (and lower) in your mount?
Presumably the upper doesn't support file attributes, if it returns
-ENOIOCTLCMD?  If so, I guess the question would be what behavior
overlayfs should have when attributes can't be copied from lower to
upper, which is actually a question Miklos raised when that commit was
being worked on![^1]  Perhaps your use case can help inform a good
answer.

Cheers,
Kevin

[^1]: https://lore.kernel.org/linux-unionfs/CAJfpegsRo3e-9B64W37YrmvDcjo0QB9t+coAW3mO6TSqdROz2w@mail.gmail.com/

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] ovl: fix NULL pointer dereference
  2022-01-12 20:58   ` Kevin Locke
@ 2022-01-12 21:06     ` Christoph Fritz
  2022-01-12 22:25       ` Kevin Locke
  0 siblings, 1 reply; 9+ messages in thread
From: Christoph Fritz @ 2022-01-12 21:06 UTC (permalink / raw)
  To: Kevin Locke; +Cc: Miklos Szeredi, linux-unionfs

Hi Kevin
> > These have been -ENOIOCTLCMD errors but got (falsely?) converted to
> > -ENOTTY by the recently introduced commit 5b0a414d06c3 ("ovl: fix
> > filattr copy-up failure"):
> 
> Which filesystem are you using for upper (and lower) in your mount?


It's tmpfs.

> Presumably the upper doesn't support file attributes, if it returns
> -ENOIOCTLCMD?


Tmpfs does support file attributes.

Thanks
  -- Christoph



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] ovl: fix NULL pointer dereference
  2022-01-12 21:06     ` Christoph Fritz
@ 2022-01-12 22:25       ` Kevin Locke
  2022-01-13 23:21         ` Christoph Fritz
  0 siblings, 1 reply; 9+ messages in thread
From: Kevin Locke @ 2022-01-12 22:25 UTC (permalink / raw)
  To: Christoph Fritz; +Cc: Miklos Szeredi, linux-unionfs

On Wed, 2022-01-12 at 22:06 +0100, Christoph Fritz wrote:
>>> These have been -ENOIOCTLCMD errors but got (falsely?) converted to
>>> -ENOTTY by the recently introduced commit 5b0a414d06c3 ("ovl: fix
>>> filattr copy-up failure"):
>> 
>> Which filesystem are you using for upper (and lower) in your mount?
> 
> 
> It's tmpfs.
> 
>> Presumably the upper doesn't support file attributes, if it returns
>> -ENOIOCTLCMD?
> 
> 
> Tmpfs does support file attributes.

Although tmpfs can support extended attributes (attr(1)/xattr(7)) with
CONFIG_TMPFS_XATTR, I'm not aware of support for traditional
attributes (chattr(1)).  I'm also not able to reproduce the error
message you mentioned with extended attributes.  With your patch[^1]
applied to 5.16, I ran the following:

    mkdir lower upwork overlay
    mount -t tmpfs - lower
    mount -t tmpfs - upwork
    mkdir upwork/upper upwork/work
    touch lower/file.txt
    setfacl -m 'u:0:rwx' lower/file.txt
    mount -t overlay -o "lowerdir=$PWD/lower,upperdir=$PWD/upwork/upper,workdir=$PWD/upwork/work" - overlay
    mv overlay/file.txt overlay/file2.txt
    getfattr -d -m '.*' overlay/file2.txt

This copied file.txt from lower to upper with the
system.posix_acl_access extended attribute and did not print any
messages from overlayfs to the kernel log.

Could you provide a minimal, reproducible example for the log messages
you mentioned observing?

Thanks,
Kevin

[^1]: https://lore.kernel.org/linux-unionfs/10d8ed194b934c298713ad7f0958329b46573dd1.camel@googlemail.com/

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] ovl: fix NULL pointer dereference
  2022-01-12 22:25       ` Kevin Locke
@ 2022-01-13 23:21         ` Christoph Fritz
  0 siblings, 0 replies; 9+ messages in thread
From: Christoph Fritz @ 2022-01-13 23:21 UTC (permalink / raw)
  To: Kevin Locke; +Cc: Miklos Szeredi, linux-unionfs


> Could you provide a minimal, reproducible example for the log messages
> you mentioned observing?

Overlay gets configured in an initramfs, after switching root and while
booting these new error messages happen.



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] ovl: fix NULL pointer dereference
  2022-01-12 18:33 [PATCH] ovl: fix NULL pointer dereference Christoph Fritz
  2022-01-12 18:46 ` Christoph Fritz
@ 2022-01-14 15:59 ` Miklos Szeredi
  1 sibling, 0 replies; 9+ messages in thread
From: Miklos Szeredi @ 2022-01-14 15:59 UTC (permalink / raw)
  To: chf.fritz; +Cc: Kevin Locke, overlayfs

On Wed, 12 Jan 2022 at 19:33, Christoph Fritz <chf.fritz@googlemail.com> wrote:
>
> This patch is fixing a NULL pointer dereference to get a recently
> introduced warning message working.

Thanks, applied.

Miklos

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] ovl: fix NULL pointer dereference
  2022-01-12 20:28   ` Christoph Fritz
@ 2022-01-14 16:01     ` Miklos Szeredi
  0 siblings, 0 replies; 9+ messages in thread
From: Miklos Szeredi @ 2022-01-14 16:01 UTC (permalink / raw)
  To: chf.fritz; +Cc: Kevin Locke, overlayfs

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

On Wed, 12 Jan 2022 at 21:28, Christoph Fritz <chf.fritz@googlemail.com> wrote:
>
> >
> > [    9.956738] overlayfs: failed to retrieve upper fileattr
> > (index/#61, err=-25)
> > [   10.311610] overlayfs: failed to retrieve upper fileattr
> > (index/#d, err=-25)
> > [   10.712019] overlayfs: failed to retrieve upper fileattr
> > (index/#e, err=-25)
> > [   31.901577] overlayfs: failed to retrieve upper fileattr
> > (index/#64, err=-25)
> >
> > These have been -ENOIOCTLCMD errors but got (falsely?) converted to
> > -ENOTTY by the recently introduced commit 5b0a414d06c3 ("ovl: fix
> > filattr copy-up failure"):
> >
> > +       if (err == -ENOIOCTLCMD)
> > +               err = -ENOTTY;
> >
> > Any ideas?
> >
>
> Doing the same "quirk" for upper fileattr seems to fix the issues, but
> I have no clue about any other implications:
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index 347b06479663..1e69bc000dd8 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -167,6 +167,8 @@ static int ovl_copy_fileattr(struct inode *inode, struct path *old,
>
>         err = ovl_real_fileattr_get(new, &newfa);
>         if (err) {
> +               if (err == -ENOTTY || err == -EINVAL)
> +                       return 0;
>                 pr_warn("failed to retrieve upper fileattr (%pd2, err=%i)\n",
>                         new->dentry, err);
>                 return err;
>

Can you please test the attached patch?

It still prints one warning message to inform the user about this
situation, but otherwise it should revert to the old behavior, like
your suggested patch.

Both patches pushed to:

  git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git#overlayfs-next

Thanks,
Miklos

[-- Attachment #2: ovl-dont-fail-copy-up-if-no-fileattr-support-on-upper.patch --]
[-- Type: text/x-patch, Size: 1569 bytes --]

From: Miklos Szeredi <mszeredi@redhat.com>
Subject: ovl: don't fail copy up if no fileattr support on upper

Christoph Fritz is reporting that failure to copy up fileattr when upper
doesn't support fileattr or xattr results in a regression.

Return success in these failure cases; this reverts overlayfs to the old
behavior.

Add a pr_warn_once() in these cases to still let the user know about the
copy up failures.

Reported-by: Christoph Fritz <chf.fritz@googlemail.com>
Fixes: 72db82115d2b ("ovl: copy up sync/noatime fileattr flags")
Cc: <stable@vger.kernel.org> # v5.15
Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/overlayfs/copy_up.c |   12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -157,7 +157,9 @@ static int ovl_copy_fileattr(struct inod
 	 */
 	if (oldfa.flags & OVL_PROT_FS_FLAGS_MASK) {
 		err = ovl_set_protattr(inode, new->dentry, &oldfa);
-		if (err)
+		if (err == -EPERM)
+			pr_warn_once("copying fileattr: no xattr on upper\n");
+		else if (err)
 			return err;
 	}
 
@@ -167,6 +169,14 @@ static int ovl_copy_fileattr(struct inod
 
 	err = ovl_real_fileattr_get(new, &newfa);
 	if (err) {
+		/*
+		 * Returning an error if upper doesn't support fileattr will
+		 * result in a regression, so revert to the old behavior.
+		 */
+		if (err == -ENOTTY || err == -EINVAL) {
+			pr_warn_once("copying fileattr: no support on upper\n");
+			return 0;
+		}
 		pr_warn("failed to retrieve upper fileattr (%pd2, err=%i)\n",
 			new->dentry, err);
 		return err;

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2022-01-14 16:01 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-12 18:33 [PATCH] ovl: fix NULL pointer dereference Christoph Fritz
2022-01-12 18:46 ` Christoph Fritz
2022-01-12 20:28   ` Christoph Fritz
2022-01-14 16:01     ` Miklos Szeredi
2022-01-12 20:58   ` Kevin Locke
2022-01-12 21:06     ` Christoph Fritz
2022-01-12 22:25       ` Kevin Locke
2022-01-13 23:21         ` Christoph Fritz
2022-01-14 15:59 ` Miklos Szeredi

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).