[PATCH 1/1] nvmet: fix freeing unallocated p2pmem

[Logan Gunthorpe]

On 2021-06-01 10:22 a.m., Max Gurtovoy wrote:
> In case p2p device was found but the p2p pool is empty, the nvme target
> is still trying to free the sgl from the p2p pool instead of the
> regular sgl pool and causing a crash (BUG() is called). Instead, assign
> the p2p_dev for the request only if it was allocated from p2p pool.
> 
> This is the crash that was caused:
> 
> [Sun May 30 19:13:53 2021] ------------[ cut here ]------------
> [Sun May 30 19:13:53 2021] kernel BUG at lib/genalloc.c:518!
> [Sun May 30 19:13:53 2021] invalid opcode: 0000 [#1] SMP PTI
> ...
> [Sun May 30 19:13:53 2021] kernel BUG at lib/genalloc.c:518!
> ...
> [Sun May 30 19:13:53 2021] RIP: 0010:gen_pool_free_owner+0xa8/0xb0
> ...
> [Sun May 30 19:13:53 2021] Call Trace:
> [Sun May 30 19:13:53 2021] ------------[ cut here ]------------
> [Sun May 30 19:13:53 2021]  pci_free_p2pmem+0x2b/0x70
> [Sun May 30 19:13:53 2021]  pci_p2pmem_free_sgl+0x4f/0x80
> [Sun May 30 19:13:53 2021]  nvmet_req_free_sgls+0x1e/0x80 [nvmet]
> [Sun May 30 19:13:53 2021] kernel BUG at lib/genalloc.c:518!
> [Sun May 30 19:13:53 2021]  nvmet_rdma_release_rsp+0x4e/0x1f0 [nvmet_rdma]
> [Sun May 30 19:13:53 2021]  nvmet_rdma_send_done+0x1c/0x60 [nvmet_rdma]
> 
> Fixes: c6e3f1339812 ("nvmet: add metadata support for block devices")
> Reviewed-by: Israel Rukshin <israelr at nvidia.com>
> Signed-off-by: Max Gurtovoy <mgurtovoy at nvidia.com>

This makes sense to me save one small nit below. Thanks!

Reviewed-by: Logan Gunthorpe <logang at deltatee.com>



> -static bool nvmet_req_find_p2p_dev(struct nvmet_req *req)
> +static struct pci_dev *nvmet_req_find_p2p_dev(struct nvmet_req *req)
>  {
> -	if (!IS_ENABLED(CONFIG_PCI_P2PDMA))
> -		return false;
> +	struct pci_dev *p2p_dev = NULL;
>  
> -	if (req->sq->ctrl && req->sq->qid && req->ns) {
> -		req->p2p_dev = radix_tree_lookup(&req->sq->ctrl->p2p_ns_map,
> -						 req->ns->nsid);
> -		if (req->p2p_dev)
> -			return true;
> -	}
> +	if (!IS_ENABLED(CONFIG_PCI_P2PDMA))
> +		goto out;

Seems like we could just return NULL here and save the label and goto.

> -	req->p2p_dev = NULL;
> -	return false;
> +	if (req->sq->ctrl && req->sq->qid && req->ns)
> +		p2p_dev = radix_tree_lookup(&req->sq->ctrl->p2p_ns_map,
> +					    req->ns->nsid);
> +out:
> +	return p2p_dev;
>  }
>