Re: [PATCH v18 net-next 6/8] octeontx2-af: npc: Support for custom KPU profile from filesystem

[Ratheesh Kannoth]

On 2026-06-02 at 11:33:57, Ratheesh Kannoth (rkannoth@xxxxxxxxxxx) wrote:
> Flashing updated firmware on deployed devices is cumbersome. Provide a
> mechanism to load a custom KPU (Key Parse Unit) profile directly from
> the filesystem at module load time.
>
> When the rvu_af module is loaded with the kpu_profile parameter, the
> specified profile is read from /lib/firmware/kpu and programmed into
> the KPU registers. Add npc_kpu_profile_cam2 for the extended cam format
> used by filesystem-loaded profiles and support ptype/ptype_mask in
> npc_config_kpucam when profile->from_fs is set.
>
> Usage:
>   1. Copy the KPU profile file to /lib/firmware/kpu.
>   2. Build OCTEONTX2_AF as a module.
>   3. Load: insmod rvu_af.ko kpu_profile=<profile_name>
>
> Signed-off-by: Ratheesh Kannoth <rkannoth@xxxxxxxxxxx>

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

> +static int npc_alloc_kpu_cam2_n_action2(struct rvu *rvu, int kpu_num,
>> +					int num_entries)
>> +{
>> +	struct npc_kpu_profile_adapter *adapter = &rvu->kpu;
>> +	struct npc_kpu_profile *kpu;
>> +
>> +	kpu = &adapter->kpu[kpu_num];
>> +
>> +	kpu->cam2 = devm_kcalloc(rvu->dev, num_entries,
>> +				 sizeof(*kpu->cam2), GFP_KERNEL);
>Does this code introduce a use-after-free risk?
No.

>The adapter's kpu pointer (profile->kpu) is set to the global static array
>npc_kpu_profiles in npc_prepare_default_kpu(). When this function is called,
>it allocates device-managed memory (devm_kcalloc) and stores the pointer
>into that globally shared array.
>If the device unbinds, the devres subsystem will automatically free this
>memory, leaving dangling pointers in the global array. A subsequent probe or
>concurrent access by another device could result in a use-after-free.
There is only one AF device per system.
When device binds, the structure which holds these pointers (global array) are also freed.

> +	struct npc_kpu_fwdata *fw_kpu;
>> +	int entries, entry, kpu;
>>
>> -	if (is_cn20k(rvu->pdev))
>> -		return npc_cn20k_apply_custom_kpu(rvu, profile);
>> +	fw = rvu->kpu_fwdata;
>> +
>> +	for (kpu = 0; kpu < fw->kpus; kpu++) {
>> +		if (rvu->kpu_fwdata_sz < hdr_sz + offset) {
>This is a pre-existing issue, but does this boundary check allow an
>out-of-bounds read?
Pre-existing issue, will post a patch to net tree, once this series is merged.

> +		return -EINVAL;
>> +
>> +	/* The firmware layout does dependent on the internal size of
>> +	 * ikpu_action_entries.
>> +	 */
>> +	memcpy((void *)profile->ikpu2, action, sizeof(ikpu_action_entries));
>> +	offset += sizeof(ikpu_action_entries);
>Will this logic break backward compatibility with existing firmware binaries?
>The firmware parsing offset is incremented by sizeof(ikpu_action_entries).
>Because ikpu_action_entries is a static kernel array whose size is determined
>by the number of supported port kinds (PKINDs), any future addition of a
>PKIND to the kernel will increase the array's size.
>This would misalign the parser's offset calculation for all existing
>firmware binaries on disk. Could this be made independent of the internal
>kernel structure size?
No, Since there is no field in this structure to indicate number of ikpu_action_entries, we
will keep same number of entries always.

>> +	const struct npc_mcam_kex *mkex;
>>  	u64 nibble_ena, rx_kex, tx_kex;
>>  	u64 *keyx_cfg, reg;
>>  	u8 intf;
>>
>> +	mkex_extr = rvu->kpu.mcam_kex_prfl.mkex_extr;
>> +	mkex = rvu->kpu.mcam_kex_prfl.mkex;
>> +
>>  	if (is_cn20k(rvu->pdev)) {
>> -		keyx_cfg = mkex_extr->keyx_cfg;
>> +		keyx_cfg = (u64 *)mkex_extr->keyx_cfg;
>This is a pre-existing issue, but does this explicit cast mask a potential
>kernel panic?
>By casting the const pointer mkex_extr->keyx_cfg to a non-const u64 *,
>the code allows mutating the underlying data later in this function
>(e.g., keyx_cfg[NIX_INTF_TX] = tx_kex).
>If the profile was loaded via the firmware database (fwdb), this memory
>is an __iomem region. A direct assignment to __iomem on ARM64 without
>using the appropriate I/O accessors (like writeq) can cause a synchronous
>external abort.
This is firmware loaded in memory (request_firmware_direct()), not iomem.