Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Xiaomeng Tong <xiam0nd.tong@gmail.com>
To:     trondmy@hammerspace.com
Cc:     anna@kernel.org, linux-kernel@vger.kernel.org,
        linux-nfs@vger.kernel.org, stable@vger.kernel.org,
        xiam0nd.tong@gmail.com
Subject: Re: [PATCH] nfs: callback_proc: fix an incorrect NULL check on list iterator
Date:   Tue, 29 Mar 2022 12:00:02 +0800
Message-Id: <20220329040002.14484-1-xiam0nd.tong@gmail.com>
In-Reply-To: <c4af251c0b90180b187e8a328d4ce5b948db9fcd.camel@hammerspace.com>
References: <c4af251c0b90180b187e8a328d4ce5b948db9fcd.camel@hammerspace.com>
Precedence: bulk

On Mon, 28 Mar 2022 13:24:57 +0000, Trond Myklebust wrote:
> Let's just do the following.
> 
> 8<-----------------------------------------------
> From 7c9d845f0612e5bcd23456a2ec43be8ac43458f1 Mon Sep 17 00:00:00 2001
> From: Trond Myklebust <trond.myklebust@hammerspace.com>
> Date: Mon, 28 Mar 2022 08:36:34 -0400
> Subject: [PATCH] NFSv4/pNFS: Fix another issue with a list iterator pointing
>  to the head
> 
> In nfs4_callback_devicenotify(), if we don't find a matching entry for
> the deviceid, we're left with a pointer to 'struct nfs_server' that
> actually points to the list of super blocks associated with our struct
> nfs_client.
> Furthermore, even if we have a valid pointer, nothing pins the super
> block, and so the struct nfs_server could end up getting freed while
> we're using it.
> 
> Since all we want is a pointer to the struct pnfs_layoutdriver_type,
> let's skip all the iteration over super blocks, and just use APIs to
> find the layout driver directly.
> 
> Reported-by: Xiaomeng Tong <xiam0nd.tong@gmail.com>
> Fixes: 1be5683b03a7 ("pnfs: CB_NOTIFY_DEVICEID")
> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> ---
>  fs/nfs/callback_proc.c | 27 +++++++++------------------
>  fs/nfs/pnfs.c          | 11 +++++++++++
>  fs/nfs/pnfs.h          |  2 ++
>  3 files changed, 22 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/nfs/callback_proc.c b/fs/nfs/callback_proc.c
> index 39d1ec870d90..c8520284dda7 100644
> --- a/fs/nfs/callback_proc.c
> +++ b/fs/nfs/callback_proc.c
> @@ -358,12 +358,11 @@ __be32 nfs4_callback_devicenotify(void *argp, void *resp,
>  				  struct cb_process_state *cps)
>  {
>  	struct cb_devicenotifyargs *args = argp;
> +	const struct pnfs_layoutdriver_type *ld = NULL;
>  	uint32_t i;
>  	__be32 res = 0;
> -	struct nfs_client *clp = cps->clp;
> -	struct nfs_server *server = NULL;
>  
> -	if (!clp) {
> +	if (!cps->clp) {
>  		res = cpu_to_be32(NFS4ERR_OP_NOT_IN_SESSION);
>  		goto out;
>  	}
> @@ -371,23 +370,15 @@ __be32 nfs4_callback_devicenotify(void *argp, void *resp,
>  	for (i = 0; i < args->ndevs; i++) {
>  		struct cb_devicenotifyitem *dev = &args->devs[i];
>  
> -		if (!server ||
> -		    server->pnfs_curr_ld->id != dev->cbd_layout_type) {
> -			rcu_read_lock();
> -			list_for_each_entry_rcu(server, &clp->cl_superblocks, client_link)
> -				if (server->pnfs_curr_ld &&
> -				    server->pnfs_curr_ld->id == dev->cbd_layout_type) {
> -					rcu_read_unlock();
> -					goto found;
> -				}
> -			rcu_read_unlock();
> -			continue;
> +		if (!ld || ld->id != dev->cbd_layout_type) {
> +			pnfs_put_layoutdriver(ld);
> +			ld = pnfs_find_layoutdriver(dev->cbd_layout_type);
> +			if (!ld)
> +				continue;
>  		}
> -
> -	found:
> -		nfs4_delete_deviceid(server->pnfs_curr_ld, clp, &dev->cbd_dev_id);
> +		nfs4_delete_deviceid(ld, cps->clp, &dev->cbd_dev_id);
>  	}
> -
> +	pnfs_put_layoutdriver(ld);
>  out:
>  	kfree(args->devs);
>  	return res;
> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
> index de318bb5d349..856c962273c7 100644
> --- a/fs/nfs/pnfs.c
> +++ b/fs/nfs/pnfs.c
> @@ -92,6 +92,17 @@ find_pnfs_driver(u32 id)
>  	return local;
>  }
>  
> +const struct pnfs_layoutdriver_type *pnfs_find_layoutdriver(u32 id)
> +{
> +	return find_pnfs_driver(id);
> +}
> +
> +void pnfs_put_layoutdriver(const struct pnfs_layoutdriver_type *ld)
> +{
> +	if (ld)
> +		module_put(ld->owner);
> +}
> +
>  void
>  unset_pnfs_layoutdriver(struct nfs_server *nfss)
>  {
> diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h
> index f4d7548d67b2..07f11489e4e9 100644
> --- a/fs/nfs/pnfs.h
> +++ b/fs/nfs/pnfs.h
> @@ -234,6 +234,8 @@ struct pnfs_devicelist {
>  
>  extern int pnfs_register_layoutdriver(struct pnfs_layoutdriver_type *);
>  extern void pnfs_unregister_layoutdriver(struct pnfs_layoutdriver_type *);
> +extern const struct pnfs_layoutdriver_type *pnfs_find_layoutdriver(u32 id);
> +extern void pnfs_put_layoutdriver(const struct pnfs_layoutdriver_type *ld);
>  
>  /* nfs4proc.c */
>  extern size_t max_response_pages(struct nfs_server *server);
> -- 

Thank you, i have resend a PATCH v2 with fix as you suggested, and also with
some changes, please check it.

--
Xiaomeng Tong