Received-SPF: pass (google.com: domain of linux-nfs+bounces-2880-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Wed, 17 Apr 2024 18:08:41 +0300
From: Dan Carpenter <dan.carpenter@linaro.org>
To: Benjamin Coddington <bcodding@redhat.com>
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.
Message-ID: <ae7bbc2e-49c6-46df-8876-06b11dd551e5@moroto.mountain>
References: <ae03a217-e643-4127-bb4a-4993ad6a9d00@moroto.mountain>
 <13EE0F08-5567-48B8-A7C2-88A086FBDA89@redhat.com>
 <7c4df27b-9698-4d49-a35f-9395b75348d3@moroto.mountain>
 <F0002E44-B2E9-4DE3-BF3B-771F814A8EE1@redhat.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <F0002E44-B2E9-4DE3-BF3B-771F814A8EE1@redhat.com>

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