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

["Chuck Lever" <cel@kernel.org>]

On Fri, Jan 23, 2026, at 4:30 PM, Jeff Layton wrote:
> 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?

I asked Claude specifically to look for races, and he agrees there
is a pre-existing synchronization issue in here. Certainly could
be addressed via a pre-requisite patch to 05/42.


-- 
Chuck Lever