In the ovl_check_origin()  and ovl_index_upper()function, the
PTR_ERR function was potentially passed a null pointer.
To fix this issue, separated the null pointer check and the error
pointer check, ensuring that PTR_ERR is only called with a valid
error pointer.

Fix below smatch warning.
smatch warnings:
fs/overlayfs/namei.c:479 ovl_check_origin() warn: passing zero to 'PTR_ERR'
fs/overlayfs/namei.c:581 ovl_index_upper() warn: passing zero to 'ERR_CAST'

Fixes: ad1d615cec1c ("ovl: use directory index entries for consistency verification")
Fixes: e8f9e5b780b0 ("ovl: verify directory index entries on mount")
Signed-off-by: Charles Han <hanchunchao@inspur.com>
---
 fs/overlayfs/namei.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 0b8b28392eb7..bc917b56e2b1 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -475,7 +475,9 @@ static int ovl_check_origin(struct ovl_fs *ofs, struct dentry *upperdentry,
 	struct ovl_fh *fh = ovl_get_fh(ofs, upperdentry, OVL_XATTR_ORIGIN);
 	int err;
 
-	if (IS_ERR_OR_NULL(fh))
+	if (!fh)
+		return -ENODATA;
+	else if (IS_ERR(fh))
 		return PTR_ERR(fh);
 
 	err = ovl_check_origin_fh(ofs, fh, false, upperdentry, stackp);
@@ -577,7 +579,9 @@ struct dentry *ovl_index_upper(struct ovl_fs *ofs, struct dentry *index,
 		return dget(index);
 
 	fh = ovl_get_fh(ofs, index, OVL_XATTR_UPPER);
-	if (IS_ERR_OR_NULL(fh))
+	if (!fh)
+		return ERR_PTR(-ENODATA);
+	else if (IS_ERR(fh))
 		return ERR_CAST(fh);
 
 	upper = ovl_decode_real_fh(ofs, fh, ovl_upper_mnt(ofs), connected);
-- 
2.43.0

On Mon, May 19, 2025 at 6:11 AM Charles Han <hanchunchao@inspur.com> wrote:
>
> In the ovl_check_origin()  and ovl_index_upper()function, the
> PTR_ERR function was potentially passed a null pointer.
> To fix this issue, separated the null pointer check and the error
> pointer check, ensuring that PTR_ERR is only called with a valid
> error pointer.
>
> Fix below smatch warning.
> smatch warnings:
> fs/overlayfs/namei.c:479 ovl_check_origin() warn: passing zero to 'PTR_ERR'
> fs/overlayfs/namei.c:581 ovl_index_upper() warn: passing zero to 'ERR_CAST'
>
> Fixes: ad1d615cec1c ("ovl: use directory index entries for consistency verification")
> Fixes: e8f9e5b780b0 ("ovl: verify directory index entries on mount")
> Signed-off-by: Charles Han <hanchunchao@inspur.com>
> ---
>  fs/overlayfs/namei.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index 0b8b28392eb7..bc917b56e2b1 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -475,7 +475,9 @@ static int ovl_check_origin(struct ovl_fs *ofs, struct dentry *upperdentry,
>         struct ovl_fh *fh = ovl_get_fh(ofs, upperdentry, OVL_XATTR_ORIGIN);
>         int err;
>
> -       if (IS_ERR_OR_NULL(fh))
> +       if (!fh)
> +               return -ENODATA;

Not good. This is changing behavior.

> +       else if (IS_ERR(fh))
>                 return PTR_ERR(fh);

PTR_ERR_OR_ZERO if you must.

>
>         err = ovl_check_origin_fh(ofs, fh, false, upperdentry, stackp);
> @@ -577,7 +579,9 @@ struct dentry *ovl_index_upper(struct ovl_fs *ofs, struct dentry *index,
>                 return dget(index);
>
>         fh = ovl_get_fh(ofs, index, OVL_XATTR_UPPER);
> -       if (IS_ERR_OR_NULL(fh))
> +       if (!fh)
> +               return ERR_PTR(-ENODATA);
> +       else if (IS_ERR(fh))
>                 return ERR_CAST(fh);

I don't see what's wrong with casting a NULL pointer.
This looks like a dubious smatch warning.

We could add ERR_OR_NULL_CAST() but it seems pointless.

Thanks,
Amir.

On Mon, May 19, 2025 at 08:43:16AM +0200, Amir Goldstein wrote:
> > @@ -577,7 +579,9 @@ struct dentry *ovl_index_upper(struct ovl_fs *ofs, struct dentry *index,
> >                 return dget(index);
> >
> >         fh = ovl_get_fh(ofs, index, OVL_XATTR_UPPER);
> > -       if (IS_ERR_OR_NULL(fh))
> > +       if (!fh)
> > +               return ERR_PTR(-ENODATA);
> > +       else if (IS_ERR(fh))
> >                 return ERR_CAST(fh);
> 
> I don't see what's wrong with casting a NULL pointer.
> This looks like a dubious smatch warning.
> 
> We could add ERR_OR_NULL_CAST() but it seems pointless.

The ERR_OR_NULL_CAST() define is kind of a horrible name but I can't
think of a better one...  To me, it's easy enough to just ignore
false positives.

This warning obviously does have false positives, but over half the
time it points to real bugs like:

	p = kmalloc(sizeof(*p));
	if (!p)
		return PTR_ERR(p);

Or copy and paste bugs etc.  There are lots of ways to get it wrong.
These warnings are always worth checking.

People should generally assume that static checker warnings in old code
are false positives.  We're good at fixing all the real bugs.  I didn't
report this warning because I knew the code was correct but I did look at
it on Feb 22, 2019.  It can sometimes be helpful to look up the warning
on lore.  We had a discussion about this one in 2020-04-21:

https://lore.kernel.org/all/?q=ovl_index_upper%20carpenter

regards,
dan carpenter