Re: [PATCH] ovl: fix NULL pointer dereference

[Miklos Szeredi <miklos@szeredi.hu>]

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