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

[Ratheesh Kannoth <rkannoth@marvell.com>]

On 2026-06-05 at 12:02:45, 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.
>
> Allocate MCAM debugfs dstats via devm_kzalloc instead of a static matrix,
> and use the allocated backing store consistently when computing deltas
> (including the counter rollover compare).
>
> Signed-off-by: Ratheesh Kannoth <rkannoth@marvell.com>

https://sashiko.dev/#/patchset/20260605063245.3553861-1-rkannoth%40marvell.com says

>>   */
>> -static u64 dstats[MAX_NUM_BANKS][MAX_SUBBANK_DEPTH * MAX_NUM_SUB_BANKS] = {};
>> +static u64 (*dstats)[MAX_NUM_BANKS][MAX_SUBBANK_DEPTH * MAX_NUM_SUB_BANKS];
>> +
>>  static int npc_mcam_dstats_show(struct seq_file *s, void *unused)
>>  {
>What happens if npc_priv is NULL when this function is called?
npc_priv is allocated during probe() much before debugfs entries are created.

>>  	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;
>> +
>Does allocating dstats per-device using devm_kzalloc overwrite the global
>dstats pointer in multi-device systems?
Only one AF device per system, first patch in the series enforce the same.

>>  	int bank_off, sb_id;
>>
>>  	/* mcam_idx should be less than (2 * bank depth) */
>> -	if (mcam_idx >= npc_priv.bank_depth * 2) {
>> +	if (mcam_idx >= npc_priv->bank_depth * 2) {
>Is it possible for unprivileged Virtual Functions to trigger a denial of
>service here?
>Mailbox handlers invoke npc_mcam_idx_2_key_type(). If a VF sends a mailbox
>message early during host probe, or after an NPC initialization failure,
>npc_priv might be NULL. Would this crash the host kernel without a
>NULL pointer check?
Not possibe; as VF/PF can be probed only after AF is initialized properly.

>> -	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;
>>  	return 0;
>>  }
>Could this also crash if npc_priv is NULL?
>Since mailbox handlers process messages from Virtual Functions, checking
>npc_priv for NULL before dereferencing seems necessary to prevent a crash.
Not possibe; as VF/PF can be probed only after AF is initialized properly.

> -	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);
>Will probing a second RVU device overwrite the npc_priv pointer allocated
>by the first device?
>Since npc_priv remains a global static pointer, it appears this should be
>stored in a per-device structure like struct rvu rather than a global
>variable to support multi-device setups safely.
Only one AF device per system, first patch in the series enforce the same.

>
>> +	kfree(npc_priv);
>> +	npc_priv = NULL;
>>  }
>If any single RVU device unbinds or fails probing, will this free the
>global npc_priv and set it to NULL?
>If so, this seems like it would corrupt the shared state and cause
>use-after-free or NULL pointer crashes for other active RVU devices on
>the system.
Ubinding and failuer handling in AF driver is not complete; as is not in
the scope of this patch. Will work on a proper error handlling (hardening
patch series) once this series is merged.