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

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 0 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The allocation and free paths no longer agree on how a per-CPU
  pool index is encoded.
- [Low] The commit fixes a user-visible PREEMPT_RT bug: `spin_lock_bh()`,
  which is a sleeping rt_mutex-based lock on RT, is taken inside a…

--

libcxgb: use raw_cpu_ptr() in ppm_get_cpu_entries()

This patch replaces the get_cpu()/per_cpu_ptr()/put_cpu() sequence in
ppm_get_cpu_entries() with raw_cpu_ptr(). As a result, spin_lock_bh() is
no longer taken with preemption disabled on PREEMPT_RT. It also stores a
base index in each per-CPU pool in ppm_alloc_cpu_pool(), and builds the
global ppod index from pool->base instead of cpu * pool_index_max.

> diff --git a/drivers/net/ethernet/chelsio/libcxgb/libcxgb_ppm.c b/drivers/net/ethernet/chelsio/libcxgb/libcxgb_ppm.c
> index 2e3973a32d9d1..708013623c2f7 100644
> --- a/drivers/net/ethernet/chelsio/libcxgb/libcxgb_ppm.c
> +++ b/drivers/net/ethernet/chelsio/libcxgb/libcxgb_ppm.c
> @@ -119,16 +119,13 @@ static int ppm_get_cpu_entries(struct cxgbi_ppm *ppm, unsigned int count,
>  			       unsigned long caller_data)
>  {
>  	struct cxgbi_ppm_pool *pool;
> -	unsigned int cpu;
>  	int i;
>  
>  	if (!ppm->pool)
>  		return -EINVAL;
>  
> -	cpu = get_cpu();
> -	pool = per_cpu_ptr(ppm->pool, cpu);
> +	pool = raw_cpu_ptr(ppm->pool);
>  	spin_lock_bh(&pool->lock);
> -	put_cpu();

[Severity: Low]
Should this patch have a Fixes: tag? The get_cpu(); per_cpu_ptr();
spin_lock_bh(); put_cpu(); sequence appears to come from:

Fixes: 47da7e62b391 ("cxgb4: add iSCSI DDP page pod manager")

Commit a248384e6420 later moved it into libcxgb_ppm.c. Without the tag,
it is harder for stable maintainers to tell which trees need this fix.

[ ... ]

> @@ -144,11 +141,12 @@ static int ppm_get_cpu_entries(struct cxgbi_ppm *ppm, unsigned int count,
>  
>  	spin_unlock_bh(&pool->lock);
>  
> -	pr_debug("%s: cpu %u, idx %d + %d (%d), next %u.\n",
> -		 __func__, cpu, i, count, i + cpu * ppm->pool_index_max,
> -		pool->next);
> +	pr_debug("%s: cpu %u, pool cpu %u, idx %d + %d (%d), next %u.\n",
> +		 __func__, raw_smp_processor_id(),
> +		 pool->base / ppm->pool_index_max, i, count, i + pool->base,
> +		 pool->next);
>  
> -	i += cpu * ppm->pool_index_max;
> +	i += pool->base;
>  	ppm_mark_entries(ppm, i, count, caller_data);
>  
>  	return i;
> @@ -375,6 +373,7 @@ ppm_alloc_cpu_pool(unsigned int *total, unsigned int *pcpu_ppmax)
>  
>  		memset(ppool, 0, alloc_sz);
>  		spin_lock_init(&ppool->lock);
> +		ppool->base = count;
>  		count += ppmax;
>  	}

[Severity: High]
Do the alloc and free paths still agree on the index encoding when
cpu_possible_mask has holes?

for_each_possible_cpu() sets base from the pool's position in the
possible mask (0, ppmax, 2 * ppmax, ...), not from cpu * ppmax.
ppm_get_cpu_entries() now returns i + pool->base. ppm_unmark_entries()
was not changed, and it still decodes the index as a cpu id:

drivers/net/ethernet/chelsio/libcxgb/libcxgb_ppm.c:ppm_unmark_entries() {
    ...
		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);
    ...
}

Take a possible mask of {0,2}. CPU 2's pool gets base = 1 * ppmax, and
the release path through cxgbi_ppm_ppod_release()->ppm_unmark_entries()
computes cpu = 1. CPU 1 is not possible. setup_per_cpu_areas() in
mm/percpu.c only fills __per_cpu_offset[] for possible CPUs, so
per_cpu_ptr(ppm->pool, 1) does not point into this allocation.

Can spin_lock_bh(), bitmap_clear() and the pool->next store then write to
unrelated kernel memory?

Now take a possible mask of {0,2,3}. Entries from CPU 3's pool
(position 2) would be released into CPU 2's real pool.

Could that clear bits that CPU 2 has already handed out? If so, the same
page-pod index could be given to a second DDP tag while the first is
still in use, and CPU 3's bits would never be cleared.

The commit message says:

  the free path ppm_unmark_entries() derives the owning pool from the
  index instead of from the running CPU, so any thread can service any
  shard.

That only holds while pool->base / pool_index_max equals the cpu id. The
new "pool cpu" label in pr_debug() makes the same assumption. The 2-CPU
stress test runs with a contiguous mask, so it would not reach this case.

The old cpu * ppmax encoding was also wrong on sparse masks, because it
could run past pool_rsvd into the general pool. Would it work to either
update ppm_unmark_entries() to map the index back to the pool in the
same way, or keep base = cpu * ppmax and size pool_rsvd from nr_cpu_ids?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110165742.1204-1-haoranwangsec%40gmail.com