Re: [PATCH v12 net-next 7/9] octeontx2-af: npc: Support for custom KPU profile from filesystem

[Ratheesh Kannoth <rkannoth@marvell.com>]

On 2026-05-08 at 09:19:10, Ratheesh Kannoth (rkannoth@marvell.com) 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

pw-bot: changes-requested

>>  } __packed;
>>
>> +struct npc_kpu_profile_cam2 {
>> +	u8 state;
>> +	u8 state_mask;
>> +	u16 dp0;
>> +	u16 dp0_mask;
>> +	u16 dp1;
>> +	u16 dp1_mask;
>> +	u16 dp2;
>> +	u16 dp2_mask;
>Since the firmware image is little-endian, should these fields be defined as
>__le16?
>When they are accessed as host-order integers without le16_to_cpu() conversion,
>will this lead to incorrect packet parsing rules and silent data corruption on
>big-endian architectures?

The current KPU profile loading infrastructure is architecturally
coupled to a little-endian firmware format. While I acknowledge that
defining these members as u16 instead of __le16 introduces a
potential risk of silent data corruption on big-endian hosts, the
existing OcteonTX2 driver stack is currently for
little-endian operation to match the native hardware and firmware
ecosystem.

Implementing full endian-neutrality for the KPU profile involves a
deep refactoring of the firmware parser and associated hardware
abstraction layers. Given that current deployments are exclusively
little-endian, I propose addressing host-endianness portability as a
comprehensive enhancement in a future net-next series. This ensures
that endian safety is addressed systematically across the entire
driver rather than being localized to this

>> -static int npc_apply_custom_kpu(struct rvu *rvu,
>> -				struct npc_kpu_profile_adapter *profile)
>> +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);
>> +	if (!kpu->cam2)
>> +		return -ENOMEM;
>> +
>> +	kpu->action2 = devm_kcalloc(rvu->dev, num_entries,
>> +				    sizeof(*kpu->action2), GFP_KERNEL);
>Will this cause data corruption and a use-after-free across devices?
>In npc_prepare_default_kpu(), the device's profile->kpu pointer is initialized
>to point to the statically defined npc_kpu_profiles global array.
>Here, we are assigning device-scoped devm_kcalloc memory directly into this
>globally shared array. If multiple RVU devices probe concurrently, they might
>data-race on this shared state. Additionally, upon device unbind, devres will
>free the memory, leaving dangling pointers in the global array.

No. There is only one AF device and one LF for NPC. So mutluple proble of RVU af device is not
possible.

> +	fw = rvu->kpu_fwdata;
>> +
>> +	for (kpu = 0; kpu < fw->kpus; kpu++) {
>> +		if (rvu->kpu_fwdata_sz < hdr_sz + offset) {
>> +			dev_warn(rvu->dev,
>> +				 "Profile size mismatch on KPU%i parsing\n",
>> +				 kpu + 1);
>> +			return -EINVAL;
>> +		}
>> +
>> +		fw_kpu = (struct npc_kpu_fwdata *)(fw->data + offset);
>> +		if (fw_kpu->entries > KPU_MAX_CST_ENT)
>> +			dev_warn(rvu->dev,
>> +				 "Too many custom entries on KPU%d: %d > %d\n",
>> +				 kpu, fw_kpu->entries, KPU_MAX_CST_ENT);
>> +		entries = min_t(int, fw_kpu->entries, KPU_MAX_CST_ENT);
>> +		cam = (struct npc_kpu_profile_cam *)fw_kpu->data;
>> +		offset += sizeof(*fw_kpu) + fw_kpu->entries * sizeof(*cam);
>Is there a missing bounds check here for negative values?
>Since fw_kpu->entries is read as a signed int, a malicious firmware blob
>providing a negative value (e.g., -1) could cause the offset calculation to
>underflow the unsigned size_t offset.
>This would wrap the offset backwards, allowing the boundary check
>(rvu->kpu_fwdata_sz < hdr_sz + offset) to incorrectly pass in the next
>iteration, resulting in an out-of-bounds access.
ACK. Added a check to check if fw_kpu->entries < 0 and return error.

>> +	/* Binary blob contains ikpu actions entries at start of data[0] */
>> +	profile->ikpu2 = devm_kcalloc(rvu->dev, 1,
>> +				      sizeof(ikpu_action_entries),
>> +				      GFP_KERNEL);
>> +	if (!profile->ikpu2)
>> +		return -ENOMEM;
>> +
>> +	action = (struct npc_kpu_profile_action *)(fw->data + offset);
>> +
>> +	if (rvu->kpu_fwdata_sz < hdr_sz + sizeof(ikpu_action_entries))
>> +		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 create a fragile firmware ABI coupled to the internal kernel
>structure size?
>Because ikpu_action_entries is a kernel-internal static array, its size
>depends on the number of port kinds (NPC_MAX_PKIND) defined in the specific
>kernel version compiling it.
>If future kernel versions add elements to this array, the parser will expect a
>larger initial block than the firmware file actually provides. This would
>misalign all subsequent read offsets, causing the driver to interpret garbage
>memory.
I agree that number of ikpu entries is not part of the structure. We dont want to
add a field and break legacy ABI. Here assumption is that ikpu section entries match
number of hardware ikpu entries.

>	strcat(path, kpu_profile);
>> +
>> +	if (request_firmware_direct(&fw, path, rvu->dev))
>> +		return -ENOENT;
>> +
>> +	dev_info(rvu->dev, "Loading KPU profile from filesystem: %s\n",
>> +		 path);
>> +
>> +	rvu->kpu_fwdata = fw->data;
>> +	rvu->kpu_fwdata_sz = fw->size;
>> +
>> +	ret = npc_apply_custom_kpu(rvu, profile, true, &fw_kpus);
>> +	release_firmware(fw);
>> +	rvu->kpu_fwdata = NULL;
>> +
>> +	if (ret) {
>> +		rvu->kpu_fwdata_sz = 0;
>> +		dev_err(rvu->dev,
>> +			"Loading KPU profile from filesystem failed\n");
>> +		return ret;
>> +	}
>> +
>> +	rvu->kpu.kpus = fw_kpus;
>> +	profile->kpus = fw_kpus;
>Does this truncate the hardware programming loop in npc_parser_profile_init()?
>By explicitly updating profile->kpus to fw_kpus, the hardware programming loop
>will only configure up to fw_kpus. Since all KPUs are disabled at the beginning
>of npc_parser_profile_init(), the remaining hardware KPUs will be left
>completely disabled, breaking network parsing for traffic reliant on default
>KPU logic.
BInary blob contains all entries fow all kpu hw entries. There is no partial loading as in
fw binary loading.

> npc_config_kpucam when profile->from_fs is set.