[PATCH] ovl: fix NULL pointer dereference

[PATCH] ovl: fix NULL pointer dereference

[Christoph Fritz]

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

Re: [PATCH] ovl: fix NULL pointer dereference

[Christoph Fritz]

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

Re: [PATCH] ovl: fix NULL pointer dereference

[Christoph Fritz]

> 
> [    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;

Re: [PATCH] ovl: fix NULL pointer dereference

[Kevin Locke]

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/

Re: [PATCH] ovl: fix NULL pointer dereference

[Christoph Fritz]

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

Re: [PATCH] ovl: fix NULL pointer dereference

[Kevin Locke]

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/

Re: [PATCH] ovl: fix NULL pointer dereference

[Christoph Fritz]

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

Re: [PATCH] ovl: fix NULL pointer dereference

[Miklos Szeredi]

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

Re: [PATCH] ovl: fix NULL pointer dereference

[Miklos Szeredi]

[-- 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;