Re: [PATCH v2 05/42] NFS: Use nlmclnt_rpc_clnt() helper to retrieve nlm_host's rpc_clnt

[Jeff Layton <jlayton@kernel.org>]

On Fri, 2026-01-23 at 15:44 -0500, Chuck Lever wrote:
> On 1/23/26 3:23 PM, Jeff Layton wrote:
> > On Fri, 2026-01-23 at 13:52 -0500, Chuck Lever wrote:
> > > From: Chuck Lever <chuck.lever@oracle.com>
> > > 
> > > The external API definitions for lockd reside in linux/lockd/bind.h.
> > > Because "struct nlm_host" is an internal lockd structure, bind.h
> > > does not include a definition of it. Dereferencing that structure
> > > outside of lockd violates the layering boundary between NFS and
> > > lockd.
> > > 
> > > The proper approach is to use the nlmclnt_rpc_clnt() helper function
> > > already provided in lockd/bind.h, which retrieves the NLM host's
> > > struct rpc_clnt without exposing internal lockd structures. This
> > > maintains clean separation between the NFS client and lockd
> > > internals.
> > > 
> > > Note that the nlm_host's h_rpcclnt field can be NULL during
> > > initialization (host.c:141) or after cleanup (host.c:629). Add a
> > > NULL check before calling shutdown_client() to prevent a potential
> > > NULL pointer dereference in the sysfs shutdown path.
> > > 
> > > Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> > > ---
> > >  fs/nfs/sysfs.c | 10 +++++++---
> > >  1 file changed, 7 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/fs/nfs/sysfs.c b/fs/nfs/sysfs.c
> > > index ea6e6168092b..186b29de0129 100644
> > > --- a/fs/nfs/sysfs.c
> > > +++ b/fs/nfs/sysfs.c
> > > @@ -12,7 +12,7 @@
> > >  #include <linux/string.h>
> > >  #include <linux/nfs_fs.h>
> > >  #include <linux/rcupdate.h>
> > > -#include <linux/lockd/lockd.h>
> > > +#include <linux/lockd/bind.h>
> > >  
> > >  #include "internal.h"
> > >  #include "nfs4_fs.h"
> > > @@ -284,8 +284,12 @@ shutdown_store(struct kobject *kobj, struct kobj_attribute *attr,
> > >  	if (!IS_ERR(server->client_acl))
> > >  		shutdown_client(server->client_acl);
> > >  
> > > -	if (server->nlm_host)
> > > -		shutdown_client(server->nlm_host->h_rpcclnt);
> > > +	if (server->nlm_host) {
> > > +		struct rpc_clnt *nlm_clnt = nlmclnt_rpc_clnt(server->nlm_host);
> > > +
> > > +		if (nlm_clnt)
> > > +			shutdown_client(nlm_clnt);
> > 
> > I don't see any locking here. Soon after this thing goes NULL, the
> > nlm_clnt can be freed. ISTM that this ought to take a reference to
> > nlm_clnt and put it afterward.
> 
> So there is no locking here before the patch is applied. The patch does
> not change that. Do you mean that the patch should add the additional
> reference count bump (and document that fix in the commit message) ?
> 
> Mason's prompts did not call this out, so I assumed there wasn't an
> obvious UAF possible in this path.
> 

(Adding Ben since he wrote this originally...)

Sorry, I didn't make it clear. This is (possibly) an existing bug and
not something that is changed by your patches.

If that value can go NULL and be freed (and it looks like it can in
nlm_shutdown_hosts_net()) then I think that could race with someone
writing to the "shutdown" file. OTOH, maybe that can't happen because
the sysfs file gets removed before lockd_down() runs? I'm not sure.

The safest thing might be to take and hold the (global) nlm_host_mutex
around the NLM parts of shutdown_store(). Maybe we could add a helper
to the nlm public interface that does that so we don't need to expose
that mutex outside of NLM?

> 
> > 
> > > +	}
> > >  out:
> > >  	shutdown_nfs_client(server->nfs_client);
> > >  	return count;
> > 
> 

-- 
Jeff Layton <jlayton@kernel.org>