Re: [PATCH net-next] net: ethernet: mtk_eth_soc: ppe: prevent ppe update for non-mtk devices

[Sven Eckelmann <sven@narfation.org>]

[-- Attachment #1: Type: text/plain, Size: 2877 bytes --]

On Sunday, 23 June 2024 19:51:09 CET Elad Yifee wrote:
> Introduce an additional validation to ensure that the PPE index
> is modified exclusively for mtk_eth ingress devices.
> This primarily addresses the issue related
> to WED operation with multiple PPEs.
> 
> Fixes: dee4dd10c79a ("net: ethernet: mtk_eth_soc: ppe: add support for multiple PPEs")
> Signed-off-by: Elad Yifee <eladwf@gmail.com>
> ---
>  drivers/net/ethernet/mediatek/mtk_ppe_offload.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/mediatek/mtk_ppe_offload.c b/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
> index f80af73d0a1b..f20bb390df3a 100644
> --- a/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
> +++ b/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
> @@ -266,7 +266,7 @@ mtk_flow_offload_replace(struct mtk_eth *eth, struct flow_cls_offload *f,
>  		flow_rule_match_meta(rule, &match);
>  		if (mtk_is_netsys_v2_or_greater(eth)) {
>  			idev = __dev_get_by_index(&init_net, match.key->ingress_ifindex);
> -			if (idev) {
> +			if (idev && idev->netdev_ops == eth->netdev[0]->netdev_ops) {
>  				struct mtk_mac *mac = netdev_priv(idev);
>  
>  				if (WARN_ON(mac->ppe_idx >= eth->soc->ppe_num))
> 

This will immediately cause a NULL-deref when gmac0 is disabled but gmac1 (or 
gmac2) is enabled - and you try to offload a flow.

You have the problem that `eth->netdev[0]` is NULL and then you try to deref 
it to get to its netdev_ops.

Btw. can you describe what netdev[0] makes it so special that you need to 
check for it and not netdev[1] or netdev[2]? With something like:

diff --git i/drivers/net/ethernet/mediatek/mtk_ppe_offload.c w/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
index cb30108f2bf6..5f88ad7eb27a 100644
--- i/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
+++ w/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
@@ -244,6 +244,21 @@ mtk_flow_set_output_device(struct mtk_eth *eth, struct mtk_foe_entry *foe,
 	return 0;
 }
 
+static bool mtk_is_valid_mtk_netdev(const struct mtk_eth *eth, const struct net_device *idev)
+{
+	size_t i;
+
+	for (i = 0; i < ARRAY_SIZE(eth->netdev); i++) {
+		if (!eth->netdev[i])
+			continue;
+
+		if (idev->netdev_ops == eth->netdev[i]->netdev_ops)
+			return true;
+	}
+
+	return false;
+}
+
 static int
 mtk_flow_offload_replace(struct mtk_eth *eth, struct flow_cls_offload *f,
 			 int ppe_index)
@@ -270,7 +285,7 @@ mtk_flow_offload_replace(struct mtk_eth *eth, struct flow_cls_offload *f,
 		flow_rule_match_meta(rule, &match);
 		if (mtk_is_netsys_v2_or_greater(eth)) {
 			idev = __dev_get_by_index(&init_net, match.key->ingress_ifindex);
-			if (idev && idev->netdev_ops == eth->netdev[0]->netdev_ops) {
+			if (idev && mtk_is_valid_mtk_netdev(eth, idev)) {
 				struct mtk_mac *mac = netdev_priv(idev);
 
 				if (WARN_ON(mac->ppe_idx >= eth->soc->ppe_num))


Regards,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 228 bytes --]