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

[netdev-bot+sashiko@kernel.org]

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 patch changes how ppm_get_cpu_entries() encodes the global
  ppod index but leaves the release path decoding it the old way.
- [Low] The commit fixes a user-visible PREEMPT_RT bug in
  ppm_get_cpu_entries()…

--

libcxgb: use raw_cpu_ptr() in ppm_get_cpu_entries()

The commit removes the get_cpu()/put_cpu() pair around spin_lock_bh() in
ppm_get_cpu_entries(), so the code no longer takes a sleeping lock with
preemption disabled on PREEMPT_RT. It reads the pool with raw_cpu_ptr().
It also builds the global ppod index from a new pool->base, set in
ppm_alloc_cpu_pool(), 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..7fb6c8a41e98f 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]
This isn't a bug, but should the patch have a Fixes: tag? The sequence
get_cpu(); per_cpu_ptr(); spin_lock_bh(); put_cpu(); dates back to the
original page pod manager, before the file moved to libcxgb:

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

>  
>  	i = ppm_find_unused_entries(pool->bmap, ppm->pool_index_max,
>  				    pool->next, count, 0);
> @@ -145,10 +142,10 @@ 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);
> +		 __func__, 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 +372,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]
Does the new encoding still match the release path?

ppm_alloc_cpu_pool() walks the pools with for_each_possible_cpu(), so
pool->base is the CPU's position in cpu_possible_mask times ppmax. It is
not cpu * ppmax. ppm_get_cpu_entries() now returns i + pool->base.

ppm_unmark_entries() is unchanged and still treats that value 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);
    ...
}

If cpu_possible_mask has no holes, position and CPU id are the same.
If it has holes, they differ.

With possible = {0,2,3}, CPU 3 is at position 2. So
cxgbi_ppm_ppod_release()->ppm_unmark_entries() clears CPU 3's entries in
CPU 2's pool:

- CPU 3's bits are never freed.
- CPU 2 can hand out an index that is still in use, so two live DDP tags
  could share one hardware page pod.

With possible = {0,2}, CPU 2's entries decode to cpu 1, which is not a
possible CPU. per_cpu_ptr(ppm->pool, 1) then points at a slot that was
never memset() or spin_lock_init()'d. The code takes a lock in it and
clears bits in it.

The new pr_debug() above also prints this position as "cpu".

The v1 changelog says "ppm_unmark_entries() derives the owning pool from
the index itself". After this patch, the index it decodes no longer
matches how ppm_get_cpu_entries() builds it.

Should ppm_unmark_entries() be updated as well? For example, it could
find the possible CPU whose [base, base + pool_index_max) range holds
the index, or the owning CPU could be recorded.

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