Re: [PATCH v15 net-next 9/9] octeontx2-af: npc: cn20k: Allocate npc_priv and dstats dynamically.

[Ratheesh Kannoth <rkannoth@marvell.com>]

On 2026-05-20 at 07:39:39, Ratheesh Kannoth (rkannoth@marvell.com) wrote:
> Replace the file-scope static npc_priv with a kcalloc'd struct filled
> from hardware bank/subbank geometry at init (num_banks is no longer a
> const compile-time constant; drop init_done and use a non-NULL
> npc_priv pointer for liveness). Thread npc_priv_get() / pointer access
> through the CN20K NPC code paths, extend teardown to kfree the root
> struct on failure and in npc_cn20k_deinit, and adjust MCAM section
> setup to use the discovered subbank count.
>

>> @@ -397,6 +398,10 @@ int npc_cn20k_debugfs_init(struct rvu *rvu)
>>  	debugfs_create_file("vidx2idx", 0444, rvu->rvu_dbg.npc,
>>  			    npc_priv, &npc_vidx2idx_map_fops);
>>
>> +	dstats = devm_kzalloc(rvu->dev, sizeof(*dstats), GFP_KERNEL);
>> +	if (!dstats)
>> +		return -ENOMEM;
>> +
>Since dstats is a file-scope global pointer, won't devm_kzalloc overwrite
>it unconditionally if a second device is probed? In a multi-device scenario,
>this would leak the memory allocated for the first device. Also, if a device
>is unbound, devm_kzalloc frees dstats, which leaves the global pointer
>dangling for any other active devices. Should this be stored in a per-device
>structure like struct rvu instead?

There is only one rvu AF device per system. so this condition wont happen.

>> @@ -3954,18 +3952,19 @@ int npc_cn20k_search_order_set(struct rvu *rvu,
>>  	npc_lock_all_subbank();
>>
>>  	for (sb_idx = 0; sb_idx < cnt; sb_idx++) {
>> -		sb = &npc_priv.sb[sb_idx];
>> +		sb = &npc_priv->sb[sb_idx];
>>  		save[sb->idx] = sb->arr_idx;
>>  	}
>>
>>  	for (prio = 0; prio < cnt; prio++) {
>>  		sb_idx = narr[prio];
>This is a pre-existing issue, but is there any bounds validation on the values
>in the narr array? If narr[prio] is greater than or equal to num_subbanks,
>sb_idx will be out of bounds, leading to memory corruption when accessing
>the npc_priv->sb array.

The function explicitly enforces a configuration guard
via the condition 'if (cnt != npc_priv.num_subbanks)'. This ensures that the
loop iteration space is perfectly identical to the active sub-bank count,
preventing mismatched boundary tracking.

The value of 'npc_priv.num_subbanks' is capped
strictly below or equal to MAX_NUM_SUB_BANKS. This invariant is validated
during the early Admin Function (AF) driver probe sequence; any violation
terminates the probe, ensuring that out-of-bounds array layout parameters
cannot manifest in an operational runtime state.

>> -		sb = &npc_priv.sb[sb_idx];
>>
>>  		if (sb->flags & NPC_SUBBANK_FLAG_USED)
>> -			xa = &npc_priv.xa_sb_used;
>> +			xa = &npc_priv->xa_sb_used;
>>  		else
>> -			xa = &npc_priv.xa_sb_free;
>> +			xa = &npc_priv->xa_sb_free;
>> +
>> +		sb = &npc_priv->sb[sb_idx];
>Does this reordering cause the code to evaluate the flags of the wrong
>subbank? Because sb is assigned after checking sb->flags, the condition is
>evaluating the flags of the subbank from the previous iteration (or the
>initialization loop) instead of the new sb_idx. This looks like it could
>route free subbanks into the used xarray or vice versa.
>[ ... ]
ACK.

>
>> -	rsp->kw_type = npc_priv.kw;
>> -	rsp->x4_slots = npc_priv.bank_depth;
>> +	rsp->kw_type = npc_priv->kw;
>> +	rsp->x4_slots = npc_priv->bank_depth;
>Since this is an externally reachable path (via mailbox), if npc_priv is NULL,
>could this be triggered by a VF to cause a NULL pointer dereference and kernel
>panic?
Mandatory Allocation Path: The 'npc_priv' structure allocation and
initialization are mandatory execution steps within the CN20K-specific chip
initialization pipeline. If this allocation fails due to memory exhaustion,
the core Admin Function (AF) driver probe sequence aborts immediately with
an error status.

Because a probe failure prevents the device from
registering or initializing its mailbox subsystem, no VF/PF can ever issue or
reach this mailbox handler unless the probe has completed successfully. Hence,
the allocation is guaranteed to be valid for the entire driver runtime.

>> @@ -4687,71 +4686,79 @@ static int npc_priv_init(struct rvu *rvu)
>>  		return -EINVAL;
>>  	}
>>
>> -	npc_priv.num_subbanks = num_subbanks;
>> -	npc_priv.bank_depth = bank_depth;
>> -	npc_priv.subbank_depth = subbank_depth;
>> +	npc_priv = kcalloc(1, sizeof(*npc_priv), GFP_KERNEL);
>> +	if (!npc_priv)
>> +		return -ENOMEM;
>> +
>Like dstats, npc_priv is a global pointer. If a second device is probed,
>won't this unconditionally overwrite npc_priv and leak the previously
>allocated memory? And similarly, if a device is unbound and npc_priv is
>freed, it will leave the global pointer dangling for other devices.

NO. as there is only one AF device per system.