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
