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

[Ratheesh Kannoth]

On 2026-06-01 at 08:28:43, 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.

>>  static int npc_mcam_dstats_show(struct seq_file *s, void *unused)
>>  {
>>  	struct npc_priv_t *npc_priv;
>>  	int blkaddr, pf, mcam_idx;
>[ ... ]
>>  	npc_priv = npc_priv_get();
>>  	rvu = s->private;
>>  	blkaddr = rvu_get_blkaddr(rvu, BLKTYPE_NPC, 0);
>>  	if (blkaddr < 0)
>>  		return 0;
>>
>>  	mutex_lock(&stats_lock);
>>  	seq_puts(s, "idx\tpfunc\tstats\n");
>> -	for (int bank = npc_priv.num_banks - 1; bank >= 0; bank--) {
>> -		for (int idx = npc_priv.bank_depth - 1; idx >= 0; idx--) {
>> +	for (int bank = npc_priv->num_banks - 1; bank >= 0; bank--) {
>> +		for (int idx = npc_priv->bank_depth - 1; idx >= 0; idx--) {
>Could this unconditionally dereference a NULL pointer if debugfs is read
>after initialization fails? Since npc_priv is no longer a static structure,
>reading from it without checking for NULL will cause a crash here.

if memory allocation fails, debugfs entry wont be created in npc_cn20k_debugfs_init()

>  int npc_cn20k_ref_idx_alloc(struct rvu *rvu, int pcifunc, int key_type,
>>  			    int prio, u16 *mcam_idx, int ref, int limit,
>>  			    bool contig, int count, bool virt)
>>  {
>>  	bool defrag_candidate = false;
>>  	int i, eidx, rc, bd;
>>  	bool ref_valid;
>>  	u16 vidx;
>>
>> -	bd = npc_priv.bank_depth;
>> +	bd = npc_priv->bank_depth;
>Does this function also need a NULL check for npc_priv before dereferencing
>it? If the NPC block fails to initialize, this will crash immediately upon
>entry.
No. if npc_priv allocation fails, probe() itself will fail.

>>  int rvu_mbox_handler_npc_get_pfl_info(struct rvu *rvu, struct msg_req *req,
>>  				      struct npc_get_pfl_info_rsp *rsp)
>>  {
>>  	if (!is_cn20k(rvu->pdev)) {
>>  		dev_err(rvu->dev, "Mbox support is only for cn20k\n");
>>  		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;
>>  }
>If the NPC block is disabled or fails to initialize, npc_priv will remain
>NULL. Since this mailbox handler doesn't check for NULL before dereferencing,
>will this cause a kernel panic when triggered by a VF? Previously, reading
>from the static structure safely yielded zeroes.
>
If the NPC block is disabled or fails to initialize, rvu af probe() itself will fail.

>>  int rvu_mbox_handler_npc_get_pfl_info(struct rvu *rvu, struct msg_req *req,
>>  				      struct npc_get_pfl_info_rsp *rsp)
>>  {
>>  	if (!is_cn20k(rvu->pdev)) {
>>  		dev_err(rvu->dev, "Mbox support is only for cn20k\n");
>>  		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;
>>  }
>If the NPC block is disabled or fails to initialize, npc_priv will remain
>NULL. Since this mailbox handler doesn't check for NULL before dereferencing,
>will this cause a kernel panic when triggered by a VF? Previously, reading
>from the static structure safely yielded zeroes.
There is only one AF device per system, so this case won't happen