Re: [PATCH net-next v4 16/16] net: airoha: Introduce PPE debugfs support

[Simon Horman <horms@kernel.org>]

On Thu, Feb 13, 2025 at 04:34:35PM +0100, Lorenzo Bianconi wrote:
> Similar to PPE support for Mediatek devices, introduce PPE debugfs
> in order to dump binded and unbinded flows.
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>

...

> diff --git a/drivers/net/ethernet/airoha/airoha_ppe_debugfs.c b/drivers/net/ethernet/airoha/airoha_ppe_debugfs.c

...

> +static int airoha_ppe_debugfs_foe_show(struct seq_file *m, void *private,
> +				       bool bind)
> +{
> +	static const char *const ppe_type_str[] = {
> +		[PPE_PKT_TYPE_IPV4_HNAPT] = "IPv4 5T",
> +		[PPE_PKT_TYPE_IPV4_ROUTE] = "IPv4 3T",
> +		[PPE_PKT_TYPE_BRIDGE] = "L2B",
> +		[PPE_PKT_TYPE_IPV4_DSLITE] = "DS-LITE",
> +		[PPE_PKT_TYPE_IPV6_ROUTE_3T] = "IPv6 3T",
> +		[PPE_PKT_TYPE_IPV6_ROUTE_5T] = "IPv6 5T",
> +		[PPE_PKT_TYPE_IPV6_6RD] = "6RD",
> +	};
> +	static const char *const ppe_state_str[] = {
> +		[AIROHA_FOE_STATE_INVALID] = "INV",
> +		[AIROHA_FOE_STATE_UNBIND] = "UNB",
> +		[AIROHA_FOE_STATE_BIND] = "BND",
> +		[AIROHA_FOE_STATE_FIN] = "FIN",
> +	};
> +	struct airoha_ppe *ppe = m->private;
> +	int i;
> +
> +	for (i = 0; i < PPE_NUM_ENTRIES; i++) {
> +		const char *state_str, *type_str = "UNKNOWN";
> +		u16 *src_port = NULL, *dest_port = NULL;
> +		struct airoha_foe_mac_info_common *l2;
> +		unsigned char h_source[ETH_ALEN] = {};
> +		unsigned char h_dest[ETH_ALEN];
> +		struct airoha_foe_entry *hwe;
> +		u32 type, state, ib2, data;
> +		void *src_addr, *dest_addr;
> +		bool ipv6 = false;
> +
> +		hwe = airoha_ppe_foe_get_entry(ppe, i);
> +		if (!hwe)
> +			continue;
> +
> +		state = FIELD_GET(AIROHA_FOE_IB1_STATE, hwe->ib1);
> +		if (!state)
> +			continue;
> +
> +		if (bind && state != AIROHA_FOE_STATE_BIND)
> +			continue;
> +
> +		state_str = ppe_state_str[state % ARRAY_SIZE(ppe_state_str)];
> +		type = FIELD_GET(AIROHA_FOE_IB1_PACKET_TYPE, hwe->ib1);
> +		if (type < ARRAY_SIZE(ppe_type_str) && ppe_type_str[type])
> +			type_str = ppe_type_str[type];
> +
> +		seq_printf(m, "%05x %s %7s", i, state_str, type_str);
> +
> +		switch (type) {
> +		case PPE_PKT_TYPE_IPV4_HNAPT:
> +		case PPE_PKT_TYPE_IPV4_DSLITE:
> +			src_port = &hwe->ipv4.orig_tuple.src_port;
> +			dest_port = &hwe->ipv4.orig_tuple.dest_port;
> +			fallthrough;
> +		case PPE_PKT_TYPE_IPV4_ROUTE:
> +			src_addr = &hwe->ipv4.orig_tuple.src_ip;
> +			dest_addr = &hwe->ipv4.orig_tuple.dest_ip;
> +			break;
> +		case PPE_PKT_TYPE_IPV6_ROUTE_5T:
> +			src_port = &hwe->ipv6.src_port;
> +			dest_port = &hwe->ipv6.dest_port;
> +			fallthrough;
> +		case PPE_PKT_TYPE_IPV6_ROUTE_3T:
> +		case PPE_PKT_TYPE_IPV6_6RD:
> +			src_addr = &hwe->ipv6.src_ip;
> +			dest_addr = &hwe->ipv6.dest_ip;
> +			ipv6 = true;
> +			break;
> +		}

Hi Lorenzo,

Perhaps it can't happen, but if type is not one of the cases handled
by the switch statement above then src_addr and dest_addr will
be used while uninitialised by the call to airoha_debugfs_ppe_print_tuple()
below.

Flagged by Smatch.

> +
> +		seq_puts(m, " orig=");
> +		airoha_debugfs_ppe_print_tuple(m, src_addr, dest_addr,
> +					       src_port, dest_port, ipv6);
> +
> +		switch (type) {
> +		case PPE_PKT_TYPE_IPV4_HNAPT:
> +		case PPE_PKT_TYPE_IPV4_DSLITE:
> +			src_port = &hwe->ipv4.new_tuple.src_port;
> +			dest_port = &hwe->ipv4.new_tuple.dest_port;
> +			fallthrough;
> +		case PPE_PKT_TYPE_IPV4_ROUTE:
> +			src_addr = &hwe->ipv4.new_tuple.src_ip;
> +			dest_addr = &hwe->ipv4.new_tuple.dest_ip;
> +			seq_puts(m, " new=");
> +			airoha_debugfs_ppe_print_tuple(m, src_addr, dest_addr,
> +						       src_port, dest_port,
> +						       ipv6);
> +			break;
> +		}
> +
> +		if (type >= PPE_PKT_TYPE_IPV6_ROUTE_3T) {
> +			data = hwe->ipv6.data;
> +			ib2 = hwe->ipv6.ib2;
> +			l2 = &hwe->ipv6.l2;
> +		} else {
> +			data = hwe->ipv4.data;
> +			ib2 = hwe->ipv4.ib2;
> +			l2 = &hwe->ipv4.l2.common;
> +			*((__be16 *)&h_source[4]) =
> +				cpu_to_be16(hwe->ipv4.l2.src_mac_lo);
> +		}
> +
> +		*((__be32 *)h_dest) = cpu_to_be32(l2->dest_mac_hi);
> +		*((__be16 *)&h_dest[4]) = cpu_to_be16(l2->dest_mac_lo);
> +		*((__be32 *)h_source) = cpu_to_be32(l2->src_mac_hi);
> +
> +		seq_printf(m, " eth=%pM->%pM etype=%04x data=%08x"
> +			      " vlan=%d,%d ib1=%08x ib2=%08x\n",
> +			   h_source, h_dest, l2->etype, data,
> +			   l2->vlan1, l2->vlan2, hwe->ib1, ib2);
> +	}
> +
> +	return 0;
> +}

...