Re: [PATCH 1/2] sunrpc: init gssp_lock before publishing proc entry

On Sat, 2026-05-30 at 16:21 -0400, Chuck Lever wrote:
> From: Chris Mason <[email protected]>
> 
> create_use_gss_proxy_proc_entry() publishes /proc/net/rpc/use-gss-proxy
> via proc_create_data() before init_gssp_clnt() runs mutex_init() on
> sn->gssp_lock.  Once the dentry is linked under proc_subdir_lock it is
> immediately reachable from userspace, so a write that lands in the
> window drives set_gssp_clnt() into mutex_lock() on a zero-initialized
> struct mutex.
> 
>     create_use_gss_proxy_proc_entry(net)
>       proc_create_data("use-gss-proxy", ...)   /* dentry live */
>       init_gssp_clnt(sn)
>         mutex_init(&sn->gssp_lock)             /* too late */
> 
>     write_gssp()
>       set_gssp_clnt(net)
>         mutex_lock(&sn->gssp_lock)             /* uninitialized */
>         gssp_rpc_create(...)
>         sn->gssp_clnt = clnt
>         mutex_unlock(&sn->gssp_lock)
> 
> The window spans only the two statements between proc_create_data()
> returning and init_gssp_clnt(), so a writer reaches it only if the
> registering thread is preempted there while another task is already
> opening the freshly published file.  register_pernet_subsys() runs in
> preemptible context under pernet_ops_rwsem, so that preemption is
> possible, and the window widens on auth_rpcgss module load, when the
> proc entry is created for every live net namespace whose tasks are
> already running.  A writer that wins the race locks a zero-filled
> struct mutex.  On CONFIG_DEBUG_MUTEXES the missing magic value trips a
> "lock used without init" splat; on a production kernel the fast path
> acquires the lock via CMPXCHG(owner, 0, current).  In the latter case
> a second writer that arrives before init_gssp_clnt() re-zeroes owner
> can enter set_gssp_clnt() concurrently, shut down the first writer's
> clnt while it is still in use, and leak the loser's clnt.
> 
> Fix by initializing sn->gssp_lock in sunrpc_init_net() so its lifetime
> matches the sunrpc_net it lives in.  sn->gssp_clnt is already NULL from
> the kzalloc that backs net_generic storage, so the lazy helper is no
> longer needed; drop init_gssp_clnt(), its prototype, and the call from
> create_use_gss_proxy_proc_entry().  sunrpc.ko is a build-time
> dependency of auth_rpcgss.ko, so sunrpc_init_net() has always run on
> every netns before any auth_gss pernet init can publish the proc
> entry.
> 
> Fixes: 030d794bf498 ("SUNRPC: Use gssproxy upcall for server RPCGSS authentication.")
> Assisted-by: kres:claude-opus-4-7
> Signed-off-by: Chris Mason <[email protected]>
> Signed-off-by: Chuck Lever <[email protected]>
> ---
>  net/sunrpc/auth_gss/gss_rpc_upcall.c | 6 ------
>  net/sunrpc/auth_gss/gss_rpc_upcall.h | 1 -
>  net/sunrpc/auth_gss/svcauth_gss.c    | 1 -
>  net/sunrpc/sunrpc_syms.c             | 1 +
>  4 files changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/net/sunrpc/auth_gss/gss_rpc_upcall.c b/net/sunrpc/auth_gss/gss_rpc_upcall.c
> index 0fa4778620d9..b7f70b1adb18 100644
> --- a/net/sunrpc/auth_gss/gss_rpc_upcall.c
> +++ b/net/sunrpc/auth_gss/gss_rpc_upcall.c
> @@ -121,12 +121,6 @@ static int gssp_rpc_create(struct net *net, struct rpc_clnt **_clnt)
>  	return result;
>  }
>  
> -void init_gssp_clnt(struct sunrpc_net *sn)
> -{
> -	mutex_init(&sn->gssp_lock);
> -	sn->gssp_clnt = NULL;
> -}
> -
>  int set_gssp_clnt(struct net *net)
>  {
>  	struct sunrpc_net *sn = net_generic(net, sunrpc_net_id);
> diff --git a/net/sunrpc/auth_gss/gss_rpc_upcall.h b/net/sunrpc/auth_gss/gss_rpc_upcall.h
> index 31e96344167e..b3c2b2b90798 100644
> --- a/net/sunrpc/auth_gss/gss_rpc_upcall.h
> +++ b/net/sunrpc/auth_gss/gss_rpc_upcall.h
> @@ -29,7 +29,6 @@ int gssp_accept_sec_context_upcall(struct net *net,
>  				struct gssp_upcall_data *data);
>  void gssp_free_upcall_data(struct gssp_upcall_data *data);
>  
> -void init_gssp_clnt(struct sunrpc_net *);
>  int set_gssp_clnt(struct net *);
>  void clear_gssp_clnt(struct sunrpc_net *);
>  
> diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
> index 8e8aceb31270..f112f5814f7e 100644
> --- a/net/sunrpc/auth_gss/svcauth_gss.c
> +++ b/net/sunrpc/auth_gss/svcauth_gss.c
> @@ -1468,7 +1468,6 @@ static int create_use_gss_proxy_proc_entry(struct net *net)
>  			      &use_gss_proxy_proc_ops, net);
>  	if (!*p)
>  		return -ENOMEM;
> -	init_gssp_clnt(sn);
>  	return 0;
>  }
>  
> diff --git a/net/sunrpc/sunrpc_syms.c b/net/sunrpc/sunrpc_syms.c
> index ab88ce46afb5..1a3884a0376a 100644
> --- a/net/sunrpc/sunrpc_syms.c
> +++ b/net/sunrpc/sunrpc_syms.c
> @@ -57,6 +57,7 @@ static __net_init int sunrpc_init_net(struct net *net)
>  	INIT_LIST_HEAD(&sn->all_clients);
>  	spin_lock_init(&sn->rpc_client_lock);
>  	spin_lock_init(&sn->rpcb_clnt_lock);
> +	mutex_init(&sn->gssp_lock);
>  	return 0;
>  
>  err_pipefs:

Reviewed-by: Jeff Layton <[email protected]>