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

[Ratheesh Kannoth]

On 2026-06-05 at 12:02:45, Ratheesh Kannoth (rkannoth@xxxxxxxxxxx) 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@xxxxxxxxxxx>

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

>> @@ -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;
>> +
>>  	debugfs_create_file("dstats", 0444, rvu->rvu_dbg.npc, rvu,
>>  			    &npc_mcam_dstats_fops);
>What happens when this devm_kzalloc() fails?
>The only caller is rvu_dbg_npc_init() in
>drivers/net/ethernet/marvell/octeontx2/af/rvu_debugfs.c, which invokes
>npc_cn20k_debugfs_init() as a void call and discards the return value.
>By the time the new -ENOMEM path is taken, mcam_layout, mcam_default and
>vidx2idx have already been registered via debugfs_create_file(), and
>dstats, mismatch, idx2vidx and defrag are then skipped. Three of the
>four skipped files (mismatch, idx2vidx, defrag) have no relationship to
>dstats and seem to be dropped only as a side effect of where the
>allocation was placed.
>Would it be better to either move the dstats allocation up to
>npc_priv_init() (or to the very start of npc_cn20k_debugfs_init() before
>any debugfs_create_file()), or alternatively gate just the dstats
>debugfs_create_file() on the allocation success while still creating the
>unrelated files? In either case, should the caller also be updated to
>honor the return value?

if "dtstats" debugfs is missing, it is still fine as this debugfs entry is used
only for live debugging.

>>  #include "cn20k/reg.h"
>>  #include "rvu_npc_fs.h"
>>
>> -static struct npc_priv_t npc_priv = {
>> -	.num_banks = MAX_NUM_BANKS,
>> -};
>> +static struct npc_priv_t *npc_priv;
>With the previous code, init_done was set to true only at the very end
>of npc_cn20k_init(), after npc_setup_mcam_section() succeeded, so
>'init_done == true' implied both the struct fields were populated and
>the hardware had been programmed.
>The new gate is 'npc_priv != NULL', but npc_priv is assigned by
>kcalloc() near the start of npc_priv_init(), well before bank_depth, kw,
>sb[], the xarrays, the mutex and the list head are populated. Does
>'npc_priv != NULL' still carry the same 'fully initialized' meaning that
>init_done used to?

npc_priv is initialized in npc_priv_init() during probe(). if any of the initialization
fails, npc_priv is freed and set to NULL, and probe() returns with error.

>> @@ -4251,8 +4249,8 @@ int rvu_mbox_handler_npc_get_pfl_info(struct rvu *rvu, struct msg_req *req,
>>  		return -EOPNOTSUPP;
>>  	}
>>
>> -	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;
>>  }
>Should this handler also gain an 'if (!npc_priv)' guard?
>The other public entry points were updated to check for NULL:
>  npc_cn20k_vidx2idx()
>  npc_cn20k_idx2vidx()
>  npc_cn20k_dft_rules_idx_get()
>  npc_cn20k_dft_rules_alloc()
>  npc_cn20k_dft_rules_free()
>but rvu_mbox_handler_npc_get_pfl_info() now dereferences npc_priv->kw
>and npc_priv->bank_depth without a NULL check. Is that intentional, or
>is the asymmetry just an oversight?

If npc_priv initialization fails, probe itself will fail. So these handlers are invoked much later by PF, which
will be active only after AF is fully up.

>>
>> -	xa_destroy(&npc_priv.xa_sb_used);
>[ ... ]
>> +	if (!npc_priv)
>> +		return;
>>
>> -	for (i = 0; i < npc_priv.pf_cnt; i++)
>> -		xa_destroy(&npc_priv.xa_pf2idx_map[i]);
>> +	xa_destroy(&npc_priv->xa_sb_used);
>[ ... ]
>> +	npc_defrag_list_clear();
>> +	kfree(npc_priv);
>> +	npc_priv = NULL;
>>  }
>Previously the storage was a file-scope static, so teardown only flipped
>init_done. Now the storage itself is freed and the pointer cleared
>without any synchronization (no synchronize_rcu(), no flush of the mbox
>workqueue, no lock).
>If a racing mbox handler such as rvu_mbox_handler_npc_get_pfl_info() (or
>any of the npc_cn20k_dft_rules_* helpers) has already passed its
>'if (!npc_priv) return;' check but has not yet read a field, can it now
>dereference freed memory?
>The probe ordering may make this hard to reach today, but does the new
>free-and-NULL pattern introduce a use-after-free window that the old
>init_done flag did not have?

rvu_remove() disables mbox first, so this case wont happen.