Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
Message-ID: <0c1ed836c5a306331e1b2a97217c3c9f7e1fb701.camel@mailo.com>
Subject: Re: [PATCH] gfs2: fix 'passing zero to ERR_PTR()' warning
From:   drv <drv@mailo.com>
To:     Dan Carpenter <dan.carpenter@linaro.org>
Cc:     Bob Peterson <rpeterso@redhat.com>,
        Andreas Gruenbacher <agruenba@redhat.com>,
        gfs2@lists.linux.dev, linux-kernel@vger.kernel.org,
        linux-kernel-mentees@lists.linuxfoundation.org,
        Dan Carpenter <error27@gmail.com>
Date:   Fri, 29 Sep 2023 13:09:11 +0530
In-Reply-To: <a2d702b0-e819-47b0-a945-c2e38a162381@kadam.mountain>
References: <ZRXA7n0wD83zhPxC@runicha.com>
         <a2d702b0-e819-47b0-a945-c2e38a162381@kadam.mountain>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.4-2 
MIME-Version: 1.0
Precedence: bulk

On Fri, 2023-09-29 at 10:06 +0300, Dan Carpenter wrote:
> On Thu, Sep 28, 2023 at 11:37:42PM +0530, Deepak R Varma wrote:
> > Resolve the following Smatch static checker warning:
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0fs/gfs2/acl.c:54 __gfs2=
_get_acl() warn: passing zero to
> > 'ERR_PTR'
> >=20
> > by returning NULL when an extended attribute length is zero,
> > instead of
> > passing on zero to the ERR_PTR().
> >=20
> > Signed-off-by: Deepak R Varma <drv@mailo.com>
> > ---
>=20
> Passing zero to ERR_PTR() is not a bug.
>=20
> You're patch doesn't change how the code works at all, right?=C2=A0 So
> it's
> like a cleanup patch.=C2=A0 But the code was nicer in the original.
>=20
> This is just a false positive.=C2=A0 Ignore static checker false
> positives.
> Fix the checker instead.=C2=A0 Although in this case, I can't think of an
> easy way fix the checker.=C2=A0 Perhaps don't print a warning if the
> callers
> check for NULL?
>=20
> The passing zero to ERR_PTR() warning is actually a pretty good
> heuristic.=C2=A0 90% of the time in new code this is a real bug.=C2=A0 Bu=
t in
> old
> code then probably it's 0% real bugs because we've been reviewing
> these
> warnings for over a decade.
>=20
> I have a blog which might be useful.
> https://staticthinking.wordpress.com/2022/08/01/mixing-error-pointers-and=
-null/
>=20
> When I'm reviewing this patch I think:
> 1) Does gfs2_xattr_acl_get() return zero?=C2=A0 And it does.
> 2) Does that look intentional.=C2=A0 It's harder to tell because there
> aren't
> =C2=A0=C2=A0 comments and it looks like it might be a missing error code.=
=C2=A0 But
> =C2=A0=C2=A0 when you read it closely then actually it does look intentio=
nal.
> =C2=A0=C2=A0 In terms of Smatch, I consider it "intentional" if there is =
an
> =C2=A0=C2=A0 "error =3D 0;" within 5 lines for the goto.=C2=A0 (Other lan=
guages like
> Rust
> =C2=A0=C2=A0 are better than C because they force everyone to follow the =
rules.
> =C2=A0=C2=A0 #trolling).
> 3) Do the callers of __gfs2_get_acl() check for NULL and they do.
>=20
> So this code is fine.
>=20
> I hope this helps you in your review process.=C2=A0 1)=C2=A0 Ignore old
> warnings.
> 2)=C2=A0 Ignore false positives.=C2=A0 3)=C2=A0 If you think it is a bug,=
 then try
> to
> figure out how it will cause a crash.=C2=A0 Look at the caller etc.
>=20
Hi Dan,
Thank you for the review, feedback and guidance. This is really
helpful.

regards,
deepak.

> regards,
> dan carpenter
>=20