* [bug report] NFSv4: Fix free of uninitialized nfs4_label on referral lookup.
@ 2024-04-15 8:08 Dan Carpenter
2024-04-17 12:00 ` Benjamin Coddington
0 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2024-04-15 8:08 UTC (permalink / raw)
To: bcodding; +Cc: linux-nfs
[ Why is Smatch only complaining now, 2 years later??? It is a mystery.
-dan ]
Hello Benjamin Coddington,
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.
2139 }
2140
2141 inode = d_inode(server->super->s_root);
2142 result = nfs4_proc_get_locations(server, NFS_FH(inode), locations,
2143 page, cred);
2144 if (result) {
2145 dprintk("<-- %s: failed to retrieve fs_locations: %d\n",
2146 __func__, result);
2147 goto out;
2148 }
2149
2150 result = -NFS4ERR_NXIO;
2151 if (!locations->nlocations)
2152 goto out;
2153
2154 if (!(locations->fattr->valid & NFS_ATTR_FATTR_V4_LOCATIONS)) {
2155 dprintk("<-- %s: No fs_locations data, migration skipped\n",
2156 __func__);
2157 goto out;
2158 }
2159
2160 status = nfs4_begin_drain_session(clp);
2161 if (status != 0) {
2162 result = status;
2163 goto out;
2164 }
2165
2166 status = nfs4_replace_transport(server, locations);
2167 if (status != 0) {
2168 dprintk("<-- %s: failed to replace transport: %d\n",
2169 __func__, status);
2170 goto out;
2171 }
2172
2173 result = 0;
2174 dprintk("<-- %s: migration succeeded\n", __func__);
2175
2176 out:
2177 if (page != NULL)
2178 __free_page(page);
2179 if (locations != NULL)
2180 kfree(locations->fattr);
2181 kfree(locations);
2182 if (result) {
2183 pr_err("NFS: migration recovery failed (server %s)\n",
2184 clp->cl_hostname);
2185 set_bit(NFS_MIG_FAILED, &server->mig_status);
2186 }
2187 return result;
2188 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] NFSv4: Fix free of uninitialized nfs4_label on referral lookup.
2024-04-15 8:08 [bug report] NFSv4: Fix free of uninitialized nfs4_label on referral lookup Dan Carpenter
@ 2024-04-17 12:00 ` Benjamin Coddington
2024-04-17 12:40 ` Dan Carpenter
0 siblings, 1 reply; 7+ messages in thread
From: Benjamin Coddington @ 2024-04-17 12:00 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-nfs, Chuck Lever III
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.
Ben
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] NFSv4: Fix free of uninitialized nfs4_label on referral lookup.
2024-04-17 12:00 ` Benjamin Coddington
@ 2024-04-17 12:40 ` Dan Carpenter
2024-04-17 13:51 ` Benjamin Coddington
0 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2024-04-17 12:40 UTC (permalink / raw)
To: Benjamin Coddington; +Cc: linux-nfs, Chuck Lever III
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.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] NFSv4: Fix free of uninitialized nfs4_label on referral lookup.
2024-04-17 12:40 ` Dan Carpenter
@ 2024-04-17 13:51 ` Benjamin Coddington
2024-04-17 15:08 ` Dan Carpenter
0 siblings, 1 reply; 7+ messages in thread
From: Benjamin Coddington @ 2024-04-17 13:51 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-nfs, Chuck Lever III
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.
Ben
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] NFSv4: Fix free of uninitialized nfs4_label on referral lookup.
2024-04-17 13:51 ` Benjamin Coddington
@ 2024-04-17 15:08 ` Dan Carpenter
2024-04-17 18:30 ` Benjamin Coddington
0 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2024-04-17 15:08 UTC (permalink / raw)
To: Benjamin Coddington; +Cc: linux-nfs, Chuck Lever III
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.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] NFSv4: Fix free of uninitialized nfs4_label on referral lookup.
2024-04-17 15:08 ` Dan Carpenter
@ 2024-04-17 18:30 ` Benjamin Coddington
2024-04-17 18:52 ` Dan Carpenter
0 siblings, 1 reply; 7+ messages in thread
From: Benjamin Coddington @ 2024-04-17 18:30 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-nfs, Chuck Lever III
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [bug report] NFSv4: Fix free of uninitialized nfs4_label on referral lookup.
2024-04-17 18:30 ` Benjamin Coddington
@ 2024-04-17 18:52 ` Dan Carpenter
0 siblings, 0 replies; 7+ messages in thread
From: Dan Carpenter @ 2024-04-17 18:52 UTC (permalink / raw)
To: Benjamin Coddington; +Cc: linux-nfs, Chuck Lever III
On Wed, Apr 17, 2024 at 02:30:23PM -0400, Benjamin Coddington wrote:
> 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.
It looks good to me. I think it does make the code more obvious, but
again, I really try not to make static checker warnings a big burden for
people to deal with. These are a one time email, and since the code is
correct, if you want to leave it as-is that's also fine.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-04-17 18:52 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-15 8:08 [bug report] NFSv4: Fix free of uninitialized nfs4_label on referral lookup Dan Carpenter
2024-04-17 12:00 ` Benjamin Coddington
2024-04-17 12:40 ` Dan Carpenter
2024-04-17 13:51 ` Benjamin Coddington
2024-04-17 15:08 ` Dan Carpenter
2024-04-17 18:30 ` Benjamin Coddington
2024-04-17 18:52 ` Dan Carpenter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.