Received-SPF: pass (google.com: domain of linux-nfs+bounces-2879-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
From: Benjamin Coddington <bcodding@redhat.com>
To: Dan Carpenter <dan.carpenter@linaro.org>
Cc: linux-nfs@vger.kernel.org, Chuck Lever III <chuck.lever@oracle.com>
Subject: Re: [bug report] NFSv4: Fix free of uninitialized nfs4_label on
 referral lookup.
Date: Wed, 17 Apr 2024 09:51:48 -0400
Message-ID: <F0002E44-B2E9-4DE3-BF3B-771F814A8EE1@redhat.com>
In-Reply-To: <7c4df27b-9698-4d49-a35f-9395b75348d3@moroto.mountain>
References: <ae03a217-e643-4127-bb4a-4993ad6a9d00@moroto.mountain>
 <13EE0F08-5567-48B8-A7C2-88A086FBDA89@redhat.com>
 <7c4df27b-9698-4d49-a35f-9395b75348d3@moroto.mountain>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain
Content-Transfer-Encoding: quoted-printable

On 17 Apr 2024, at 8:40, Dan Carpenter wrote:

> On Wed, Apr 17, 2024 at 08:00:04AM -0400, Benjamin Coddington wrote:
>> On 15 Apr 2024, at 4:08, Dan Carpenter wrote:
>>
>>> [ Why is Smatch only complaining now, 2 years later??? It is a myster=
y.
>>>   -dan ]
>>>
>>> Hello Benjamin Coddington,
>>
>> Hi Dan!
>>
>>> Commit c3ed222745d9 ("NFSv4: Fix free of uninitialized nfs4_label on
>>> referral lookup.") from May 14, 2022 (linux-next), leads to the
>>> following Smatch static checker warning:
>>>
>>> 	fs/nfs/nfs4state.c:2138 nfs4_try_migration()
>>> 	warn: missing error code here? 'nfs_alloc_fattr()' failed. 'result' =
=3D '0'
>>>
>>> fs/nfs/nfs4state.c
>>>     2115 static int nfs4_try_migration(struct nfs_server *server, con=
st struct cred *cred)
>>>     2116 {
>>>     2117         struct nfs_client *clp =3D server->nfs_client;
>>>     2118         struct nfs4_fs_locations *locations =3D NULL;
>>>     2119         struct inode *inode;
>>>     2120         struct page *page;
>>>     2121         int status, result;
>>>     2122
>>>     2123         dprintk("--> %s: FSID %llx:%llx on \"%s\"\n", __func=
__,
>>>     2124                         (unsigned long long)server->fsid.maj=
or,
>>>     2125                         (unsigned long long)server->fsid.min=
or,
>>>     2126                         clp->cl_hostname);
>>>     2127
>>>     2128         result =3D 0;
>>>                  ^^^^^^^^^^^
>>>
>>>     2129         page =3D alloc_page(GFP_KERNEL);
>>>     2130         locations =3D kmalloc(sizeof(struct nfs4_fs_location=
s), GFP_KERNEL);
>>>     2131         if (page =3D=3D NULL || locations =3D=3D NULL) {
>>>     2132                 dprintk("<-- %s: no memory\n", __func__);
>>>     2133                 goto out;
>>>                          ^^^^^^^^
>>> Success.
>>>
>>>     2134         }
>>>     2135         locations->fattr =3D nfs_alloc_fattr();
>>>     2136         if (locations->fattr =3D=3D NULL) {
>>>     2137                 dprintk("<-- %s: no memory\n", __func__);
>>> --> 2138                 goto out;
>>>                          ^^^^^^^^^
>>> Here too.
>>
>> My patch was following the precedent set by c9fdeb280b8cc.  I believe =
the
>> idea is that the function can fail without an error and the client wil=
l
>> retry the next time the server says -NFS4ERR_MOVED.
>>
>> Is there a way to appease smatch here?  I don't have a lot of smatch
>> smarts.
>
> Generally, I tell people to just ignore it.  Anyone with questions can
> look up this email thread.
>
> But if you really wanted to silence it, Smatch counts it as intentional=

> if the "result =3D 0;" is within five lines of the goto out.

Good to know!  In this case, I think the maintainers would show annoyance=

with that sort of patch.  A comment here about the successful return code=
 on
an allocation failure would have avoided this, and I probably should have=

recognized this patch might create an issue and inserted one.  Thanks for=

the report.

Ben