Re: [PATCH v3 2/3] net: stmmac: fix l3l4 filter rejecting unsupported offload requests

[Nazle Asmade, Muhammad Nazim Amirul]

On 30/6/2026 7:56 pm, Nazle Asmade, Muhammad Nazim Amirul wrote:

Hi,

do we have any update on this patch status to go in?

BR,
Nazim
> From: Nazim Amirul <muhammad.nazim.amirul.nazle.asmade@xxxxxxxxxx>
> 
> The basic flow parser in tc_add_basic_flow() does not validate match
> keys before proceeding. Unsupported offload configurations such as
> partial protocol masks, non-IPv4 network proto, or non-TCP/UDP transport
> proto are silently accepted instead of returning -EOPNOTSUPP.
> 
> Add validation to return -EOPNOTSUPP early for:
> - No network or transport proto present in the key
> - Partial protocol mask (only full mask supported)
> - Network proto is not IPv4
> - Transport proto is not TCP or UDP
> 
> Each rejection includes an extack message so the user knows which part
> of the match is unsupported.
> 
> Also propagate -EOPNOTSUPP from tc_add_basic_flow() in tc_add_flow()
> by returning it directly rather than using break. The break was silently
> discarding the error for FLOW_CLS_REPLACE operations where entry->in_use
> is already true, causing tc_add_flow() to return 0 (success) for
> unsupported replace requests.
> 
> Fixes: 425eabddaf0f ("net: stmmac: Implement L3/L4 Filters using TC Flower")
> Signed-off-by: Rohan G Thomas <rohan.g.thomas@xxxxxxxxxx>
> Signed-off-by: Nazim Amirul <muhammad.nazim.amirul.nazle.asmade@xxxxxxxxxx>
> ---
> Changes in v3:
> - Add extack messages to each -EOPNOTSUPP return so users know which
>    part of the match is unsupported (Jakub Kicinski)
> - Return -EOPNOTSUPP directly instead of break to avoid silently
>    reporting success on unsupported FLOW_CLS_REPLACE (Sashiko review)
> - Patches 1/3 and 3/3 are unchanged from v2
> 
> Changes in v2:
> - No changes
> 
> ---
>   .../net/ethernet/stmicro/stmmac/stmmac_tc.c   | 34 +++++++++++++++++++
>   1 file changed, 34 insertions(+)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> index d78652718599..14cabe76e53e 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> @@ -446,6 +446,7 @@ static int tc_parse_flow_actions(struct stmmac_priv *priv,
>   }
>   
>   #define ETHER_TYPE_FULL_MASK	cpu_to_be16(~0)
> +#define IP_PROTO_FULL_MASK	0xFF
>   
>   static int tc_add_basic_flow(struct stmmac_priv *priv,
>   			     struct flow_cls_offload *cls,
> @@ -461,6 +462,33 @@ static int tc_add_basic_flow(struct stmmac_priv *priv,
>   
>   	flow_rule_match_basic(rule, &match);
>   
> +	/* Both network proto and transport proto not present in the key */
> +	if (!match.mask || !(match.mask->n_proto || match.mask->ip_proto)) {
> +		NL_SET_ERR_MSG_MOD(cls->common.extack,
> +				   "filter must specify network or transport protocol");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	/* If the proto is present in the key and is not full mask */
> +	if ((match.mask->n_proto && match.mask->n_proto != ETHER_TYPE_FULL_MASK) ||
> +	    (match.mask->ip_proto && match.mask->ip_proto != IP_PROTO_FULL_MASK)) {
> +		NL_SET_ERR_MSG_MOD(cls->common.extack,
> +				   "only full protocol mask is supported");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	/* Network proto is present in the key and is not IPv4 */
> +	if (match.mask->n_proto && match.key->n_proto != cpu_to_be16(ETH_P_IP)) {
> +		NL_SET_ERR_MSG_MOD(cls->common.extack,
> +				   "only IPv4 network protocol is supported");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	/* Transport proto is present in the key and is not TCP or UDP */
> +	if (match.mask->ip_proto &&
> +	    match.key->ip_proto != IPPROTO_TCP &&
> +	    match.key->ip_proto != IPPROTO_UDP) {
> +		NL_SET_ERR_MSG_MOD(cls->common.extack,
> +				   "only TCP and UDP transport protocols are supported");
> +		return -EOPNOTSUPP;
> +	}
> +
>   	entry->ip_proto = match.key->ip_proto;
>   	return 0;
>   }
> @@ -598,11 +626,7 @@ static int tc_add_flow(struct stmmac_priv *priv,
>   		ret = tc_flow_parsers[i].fn(priv, cls, entry);
>   		if (!ret)
>   			entry->in_use = true;
> -		else if (ret == -EOPNOTSUPP)
> -			/* The basic flow parser will return EOPNOTSUPP, if a
> -			 * requested offload not fully supported by the hw. And
> -			 * in that case fail early.
> -			 */
> -			break;
> +		else if (ret == -EOPNOTSUPP)
> +			return ret;
>   	}
>   
>   	if (!entry->in_use)