Re: [PATCH] libcxgb: use raw_cpu_ptr() in ppm_get_cpu_entries()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] libcxgb: CPU migration between raw_smp_processor_id() and raw_cpu_ptr()
--

commit 45e692c0f47c6b069f27d74df45dc3a4a6360a3d
Author: Haoran Wang <haoranwangsec@gmail.com>

libcxgb: use raw_cpu_ptr() in ppm_get_cpu_entries()

This commit replaces get_cpu(), per_cpu_ptr(), and put_cpu() in
ppm_get_cpu_entries() with raw_smp_processor_id() and raw_cpu_ptr() to avoid
acquiring spin_lock_bh() with preemption disabled on PREEMPT_RT kernels.

> diff --git a/drivers/net/ethernet/chelsio/libcxgb/libcxgb_ppm.c b/drivers/net/ethernet/chelsio/libcxgb/libcxgb_ppm.c
> index 2e3973a32d9d1..5f68c5d5766e5 100644
> --- a/drivers/net/ethernet/chelsio/libcxgb/libcxgb_ppm.c
> +++ b/drivers/net/ethernet/chelsio/libcxgb/libcxgb_ppm.c
> @@ -125,10 +125,9 @@ static int ppm_get_cpu_entries(struct cxgbi_ppm *ppm, unsigned int count,
>  	if (!ppm->pool)
>  		return -EINVAL;
>  
> -	cpu = get_cpu();
> -	pool = per_cpu_ptr(ppm->pool, cpu);
> +	cpu = raw_smp_processor_id();
> +	pool = raw_cpu_ptr(ppm->pool);
>  	spin_lock_bh(&pool->lock);
> -	put_cpu();
>  
>  	i = ppm_find_unused_entries(pool->bmap, ppm->pool_index_max,
>  				    pool->next, count, 0);

[Severity: Critical]
Can the task be preempted and migrated to another CPU between
raw_smp_processor_id() and raw_cpu_ptr(ppm->pool)?

Because preemption is enabled here, if a task in a preemptible iSCSI offload
path (such as cxgbit_ddp_reserve() -> cxgbi_ppm_ppods_reserve() ->
ppm_get_cpu_entries()) is migrated from CPU A to CPU B between the two
calls, cpu will hold CPU A while pool will point to CPU B's per-CPU pool.

ppm_get_cpu_entries() will then allocate bits in CPU B's pool->bmap, but
compute the global pagepod index using CPU A's ID:

drivers/net/ethernet/chelsio/libcxgb/libcxgb_ppm.c:ppm_get_cpu_entries() {
	...
	i += cpu * ppm->pool_index_max;
	ppm_mark_entries(ppm, i, count, caller_data);

	return i;
}

Because those bits were never marked in CPU A's pool->bmap, a subsequent
allocation on CPU A can hand out the same global index while it is still
active, overwriting ppm->ppod_data[i] and corrupting in-flight iSCSI DDP
DMA transfers.

Later, when cxgbi_ppm_ppod_release() -> ppm_unmark_entries() releases the
entries, it derives the pool from the global index:

drivers/net/ethernet/chelsio/libcxgb/libcxgb_ppm.c:ppm_unmark_entries() {
	...
	if (i < ppm->pool_rsvd) {
		unsigned int cpu;
		struct cxgbi_ppm_pool *pool;

		cpu = i / ppm->pool_index_max;
		i %= ppm->pool_index_max;

		pool = per_cpu_ptr(ppm->pool, cpu);
		spin_lock_bh(&pool->lock);
		bitmap_clear(pool->bmap, i, count);
	...
}

This clears the bits in CPU A's pool->bmap while permanently leaking the
bits that were set in CPU B's pool->bmap.

Would keeping per_cpu_ptr(ppm->pool, cpu) after raw_smp_processor_id() avoid
this mismatch by ensuring cpu and pool always refer to the same per-CPU pool
even if migration occurs?

([
---
])

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/179241614797.5916.13928836073297916808@whr?part=1