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 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.

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

-- 
Jeff Layton <jlayton@kernel.org>