On 17 Apr 2024, at 11:08, Dan Carpenter wrote:
> On Wed, Apr 17, 2024 at 09:51:48AM -0400, Benjamin Coddington wrote:
>> 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 mystery.
>>>>> -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' = '0'
>>>>>
>>>>> fs/nfs/nfs4state.c
>>>>> 2115 static int nfs4_try_migration(struct nfs_server *server, const struct cred *cred)
>>>>> 2116 {
>>>>> 2117 struct nfs_client *clp = server->nfs_client;
>>>>> 2118 struct nfs4_fs_locations *locations = 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.major,
>>>>> 2125 (unsigned long long)server->fsid.minor,
>>>>> 2126 clp->cl_hostname);
>>>>> 2127
>>>>> 2128 result = 0;
>>>>> ^^^^^^^^^^^
>>>>>
>>>>> 2129 page = alloc_page(GFP_KERNEL);
>>>>> 2130 locations = kmalloc(sizeof(struct nfs4_fs_locations), GFP_KERNEL);
>>>>> 2131 if (page == NULL || locations == NULL) {
>>>>> 2132 dprintk("<-- %s: no memory\n", __func__);
>>>>> 2133 goto out;
>>>>> ^^^^^^^^
>>>>> Success.
>>>>>
>>>>> 2134 }
>>>>> 2135 locations->fattr = nfs_alloc_fattr();
>>>>> 2136 if (locations->fattr == 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 will
>>>> 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 = 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.
>
> To me ignoring it is fine or adding a comment is even better, but I also
> think adding a bunch of "ret = 0;" assignments should not be as
> controversial as people make it out to be.
>
> It's just a style debate, right? The compiler knows that ret is already
> zero and it's going to optimize them away. So it doesn't affect the
> compiled code.
>
> You could add a comment /* ret is zero intentionally */ or you could
> just add a "ret = 0;". Neither affects the compile code. But to me, I
> would prefer the code, because when I see the comment, then I
> immediately start scrolling back to see if ret is really zero. I like
> when the code looks deliberate. When you see a "ret = 0;" there isn't
> any question about the author's intent.
>
> But again, I don't feel strongly about this.
I think we could refactor to try the allocation into a local variable, that
should make smatch happier. Something like:
diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index 662e86ea3a2d..5b452411e8fd 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -2116,6 +2116,7 @@ static int nfs4_try_migration(struct nfs_server *server, const struct cred *cred
{
struct nfs_client *clp = server->nfs_client;
struct nfs4_fs_locations *locations = NULL;
+ struct nfs_fattr *fattr;
struct inode *inode;
struct page *page;
int status, result;
@@ -2125,19 +2126,16 @@ static int nfs4_try_migration(struct nfs_server *server, const struct cred *cred
(unsigned long long)server->fsid.minor,
clp->cl_hostname);
- result = 0;
page = alloc_page(GFP_KERNEL);
locations = kmalloc(sizeof(struct nfs4_fs_locations), GFP_KERNEL);
- if (page == NULL || locations == NULL) {
- dprintk("<-- %s: no memory\n", __func__);
- goto out;
- }
- locations->fattr = nfs_alloc_fattr();
- if (locations->fattr == NULL) {
+ fattr = nfs_alloc_fattr();
+ if (page == NULL || locations == NULL || fattr == NULL) {
dprintk("<-- %s: no memory\n", __func__);
+ result = 0;
goto out;
}
+ locations->fattr = fattr;
inode = d_inode(server->super->s_root);
result = nfs4_proc_get_locations(server, NFS_FH(inode), locations,
page, cred);
I don't have a great way to test this code, though. Seems mechanically
sane.
Ben
