Re: [PATCH 15/29] ibmvfc: add NVMe/FC Query Target support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Low] The global array `proto_type` is missing the `static` keyword, causing namespace pollution.

Pre-existing issues:
- [High] Memory allocated from a mempool is freed directly via kfree(), permanently depleting the mempool reserve under memory pressure.
--

commit edf90dca8246218c80f44b69406be60422857d3c
Author: Tyrel Datwyler <tyreld@linux.ibm.com>

ibmvfc: add NVMe/FC Query Target support

Add protocol-specific query-target support for NVMe/FC targets. It uses the
NVMe query-target specific MAD when querying an NVMe target and updates the
associated debug and error logging to include the target protocol.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> index b45cd0183fb51..363bf75d62443 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> @@ -4849,7 +4849,7 @@ static void ibmvfc_tgt_query_target_done(struct ibmvfc_event *evt)
>  	ibmvfc_set_tgt_action(tgt, IBMVFC_TGT_ACTION_NONE);
>  	switch (status) {
>  	case IBMVFC_MAD_SUCCESS:
> -		tgt_dbg(tgt, "Query Target succeeded\n");
> +		tgt_dbg(tgt, "%s Query Target succeeded\n", proto_type[tgt->protocol]);

[Severity: Low]
Does proto_type need the static keyword? Since it is a generic identifier
defined at file scope without being in a header:

drivers/scsi/ibmvscsi/ibmvfc-core.c:
const char *proto_type[] = {
	"SCSI",
	"NVMe",
};

Could this cause symbol collisions in the global kernel namespace during
linking?

>  		if (be64_to_cpu(rsp->scsi_id) != tgt->scsi_id)
>  			ibmvfc_del_tgt(tgt);
>  		else

[ ... ]

> @@ -4913,7 +4913,10 @@ static void ibmvfc_tgt_query_target(struct ibmvfc_target *tgt)
>  	query_tgt = &evt->iu.query_tgt;
>  	memset(query_tgt, 0, sizeof(*query_tgt));
>  	query_tgt->common.version = cpu_to_be32(1);
> -	query_tgt->common.opcode = cpu_to_be32(IBMVFC_QUERY_TARGET);
> +	if (tgt->protocol == IBMVFC_PROTO_SCSI)
> +		query_tgt->common.opcode = cpu_to_be32(IBMVFC_QUERY_TARGET);
> +	else
> +		query_tgt->common.opcode = cpu_to_be32(IBMVFC_NVMF_QUERY_TARGET);
>  	query_tgt->common.length = cpu_to_be16(sizeof(*query_tgt));
>  	query_tgt->wwpn = cpu_to_be64(tgt->ids.port_name);
>  
> @@ -4923,7 +4926,7 @@ static void ibmvfc_tgt_query_target(struct ibmvfc_target *tgt)
>  		ibmvfc_set_tgt_action(tgt, IBMVFC_TGT_ACTION_NONE);
>  		kref_put(&tgt->kref, ibmvfc_release_tgt);

[Severity: High]
This is a pre-existing issue, but does ibmvfc_release_tgt() incorrectly free
the target?

The target is originally allocated using mempool_alloc():

drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_alloc_target() {
    ...
	tgt = mempool_alloc(vhost->tgt_pool, GFP_NOIO);
    ...
}

However, it is freed directly via kfree() instead of mempool_free():

drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_release_tgt() {
	struct ibmvfc_target *tgt = container_of(kref, struct ibmvfc_target, kref);
	kfree(tgt);
}

Under memory pressure, mempool_alloc() falls back to its preallocated
reserve. Will bypassing mempool_free() permanently leak the reserved
elements, eventually causing deadlocks when the pool is depleted and
mempool_alloc() blocks waiting for elements to be returned?

>  	} else
> -		tgt_dbg(tgt, "Sent Query Target\n");
> +		tgt_dbg(tgt, "Sent %s Query Target\n", proto_type[tgt->protocol]);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623013035.3436640-1-tyreld@linux.ibm.com?part=15