Re: [PATCH v2] net/sched: act_pedit: Parse L3 Header for L4 offset

[Pedro Tammela <pctammela@mojatatu.com>]

On 26/05/2023 06:58, Max Tottenham wrote:
> Instead of relying on skb->transport_header being set correctly, opt
> instead to parse the L3 header length out of the L3 headers for both
> IPv4/IPv6 when the Extended Layer Op for tcp/udp is used. This fixes a
> bug if GRO is disabled, when GRO is disabled skb->transport_header is
> set by __netif_receive_skb_core() to point to the L3 header, it's later
> fixed by the upper protocol layers, but act_pedit will receive the SKB
> before the fixups are completed. The existing behavior causes the
> following to edit the L3 header if GRO is disabled instead of the UDP
> header:
> 
>      tc filter add dev eth0 ingress protocol ip flower ip_proto udp \
>   dst_ip 192.168.1.3 action pedit ex munge udp set dport 18053
> 
> Also re-introduce a rate-limited warning if we were unable to extract
> the header offset when using the 'ex' interface.
> 
> Fixes: 71d0ed7079df ("net/act_pedit: Support using offset relative to
> the conventional network headers")

Just a FYI: the automatic back port will probably fail because of a 
recent cleanup in this code

> Signed-off-by: Max Tottenham <mtottenh@akamai.com>
> Reviewed-by: Josh Hunt <johunt@akamai.com>
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202305261541.N165u9TZ-lkp@intel.com/
> ---
> V1 -> V2:
>    * Fix minor bug reported by kernel test bot.
> 
> ---
>   net/sched/act_pedit.c | 48 ++++++++++++++++++++++++++++++++++++++-----
>   1 file changed, 43 insertions(+), 5 deletions(-)
> 
> diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
> index fc945c7e4123..d28335519459 100644
> --- a/net/sched/act_pedit.c
> +++ b/net/sched/act_pedit.c
> @@ -13,7 +13,10 @@
>   #include <linux/rtnetlink.h>
>   #include <linux/module.h>
>   #include <linux/init.h>
> +#include <linux/ip.h>
> +#include <linux/ipv6.h>
>   #include <linux/slab.h>
> +#include <net/ipv6.h>
>   #include <net/netlink.h>
>   #include <net/pkt_sched.h>
>   #include <linux/tc_act/tc_pedit.h>
> @@ -327,28 +330,58 @@ static bool offset_valid(struct sk_buff *skb, int offset)
>   	return true;
>   }
>   
> -static void pedit_skb_hdr_offset(struct sk_buff *skb,
> +static int pedit_l4_skb_offset(struct sk_buff *skb, int *hoffset, const int header_type)
> +{
> +	int noff = skb_network_offset(skb);
> +	struct iphdr *iph = NULL;
> +	int ret = -EINVAL;

nit: Should be in reverse Christmas tree

> +
> +	switch (skb->protocol) {
> +	case htons(ETH_P_IP):
> +		if (!pskb_may_pull(skb, sizeof(*iph) + noff))
> +			goto out;

I might have missed something but is this really needed?
https://elixir.bootlin.com/linux/latest/source/net/ipv4/ip_input.c#L456

> +		iph = ip_hdr(skb);
> +		*hoffset = noff + iph->ihl *  > +		ret = 0;
> +		break;
> +	case htons(ETH_P_IPV6):
> +		*hoffset = 0;
nit: Not needed

> +		ret = ipv6_find_hdr(skb, hoffset, header_type, NULL, NULL) == header_type ? 0 : -EINVAL;
> +		break;
> +	}
> +out:
> +	return ret;
> +}
> +
> +static int pedit_skb_hdr_offset(struct sk_buff *skb,
>   				 enum pedit_header_type htype, int *hoffset)
>   {
> +	int ret = -EINVAL;
>   	/* 'htype' is validated in the netlink parsing */
>   	switch (htype) {
>   	case TCA_PEDIT_KEY_EX_HDR_TYPE_ETH:
> -		if (skb_mac_header_was_set(skb))
> +		if (skb_mac_header_was_set(skb)) {
>   			*hoffset = skb_mac_offset(skb);
> +			ret = 0;
> +		}
>   		break;
>   	case TCA_PEDIT_KEY_EX_HDR_TYPE_NETWORK:
>   	case TCA_PEDIT_KEY_EX_HDR_TYPE_IP4:
>   	case TCA_PEDIT_KEY_EX_HDR_TYPE_IP6:
>   		*hoffset = skb_network_offset(skb);
> +		ret = 0;
>   		break;
>   	case TCA_PEDIT_KEY_EX_HDR_TYPE_TCP:
> +		ret = pedit_l4_skb_offset(skb, hoffset, IPPROTO_TCP);
> +		break;
>   	case TCA_PEDIT_KEY_EX_HDR_TYPE_UDP:
> -		if (skb_transport_header_was_set(skb))
> -			*hoffset = skb_transport_offset(skb);
> +		ret = pedit_l4_skb_offset(skb, hoffset, IPPROTO_UDP);
>   		break;
>   	default:
> +		ret = -EINVAL;

nit: Not needed

>   		break;
>   	}
> +	return ret;
>   }
>   
>   TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb,
> @@ -384,6 +417,7 @@ TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb,
>   		int hoffset = 0;
>   		u32 *ptr, hdata;
>   		u32 val;
> +		int rc;
>   
>   		if (tkey_ex) {
>   			htype = tkey_ex->htype;
> @@ -392,7 +426,11 @@ TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb,
>   			tkey_ex++;
>   		}
>   
> -		pedit_skb_hdr_offset(skb, htype, &hoffset);
> +		rc = pedit_skb_hdr_offset(skb, htype, &hoffset);
> +		if (rc) {
> +			pr_info_ratelimited("tc action pedit unable to extract header offset for header type (0x%x)\n", htype);
> +			goto bad;
> +		}
>   
>   		if (tkey->offmask) {
>   			u8 *d, _d;