Received-SPF: pass (google.com: domain of linux-nfs+bounces-2874-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Wed, 17 Apr 2024 15:40:50 +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: <7c4df27b-9698-4d49-a35f-9395b75348d3@moroto.mountain>
References: <ae03a217-e643-4127-bb4a-4993ad6a9d00@moroto.mountain>
 <13EE0F08-5567-48B8-A7C2-88A086FBDA89@redhat.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <13EE0F08-5567-48B8-A7C2-88A086FBDA89@redhat.com>

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