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

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

[Max Tottenham]

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")
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;
+
+	switch (skb->protocol) {
+	case htons(ETH_P_IP):
+		if (!pskb_may_pull(skb, sizeof(*iph) + noff))
+			goto out;
+		iph = ip_hdr(skb);
+		*hoffset = noff + iph->ihl * 4;
+		ret = 0;
+		break;
+	case htons(ETH_P_IPV6):
+		*hoffset = 0;
+		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;
 		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;
-- 
2.25.1

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

[Pedro Tammela]

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;

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

[Pedro Tammela]

On 26/05/2023 10:47, Pedro Tammela wrote:
> 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
> 

Sorry, I forgot to delete this comment. It's clearly 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;
> 

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

[Pedro Tammela]

On 26/05/2023 10:47, Pedro Tammela wrote:
> 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

Yes this obviously happens before the mentioned function.
Now I'm wondering if it's not better to use skb_header_pointer() instead...

> 
>> +        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;
> 

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

[Josh Hunt]

On 5/26/23 7:03 AM, Pedro Tammela wrote:
> On 26/05/2023 10:47, Pedro Tammela wrote:
>>
>>> +
>>> +    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://urldefense.com/v3/__https://elixir.bootlin.com/linux/latest/source/net/ipv4/ip_input.c*L456__;Iw!!GjvTz_vk!TyuEOA10ZxgU7TBKFX6HAZ359qEMEuo3H0jNMIF1EP75tQbrs8uiSNQSpaaW4N34AH1sCdf5vHcUrV0qsw$ 
> 
> Yes this obviously happens before the mentioned function.
> Now I'm wondering if it's not better to use skb_header_pointer() instead...

Can you elaborate on why you think it would be better?

Thanks
Josh

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

[Pedro Tammela]

On 26/05/2023 18:54, Josh Hunt wrote:
> On 5/26/23 7:03 AM, Pedro Tammela wrote:
>> On 26/05/2023 10:47, Pedro Tammela wrote:
>>>
>>>> +
>>>> +    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://urldefense.com/v3/__https://elixir.bootlin.com/linux/latest/source/net/ipv4/ip_input.c*L456__;Iw!!GjvTz_vk!TyuEOA10ZxgU7TBKFX6HAZ359qEMEuo3H0jNMIF1EP75tQbrs8uiSNQSpaaW4N34AH1sCdf5vHcUrV0qsw$ 
>>
>> Yes this obviously happens before the mentioned function.
>> Now I'm wondering if it's not better to use skb_header_pointer() 
>> instead...
> 
> Can you elaborate on why you think it would be better?
> 

I don't have a strong argument for one over the other and I believe it's 
fine as is.
It just looks like 'skb_header_pointer()' is a more conservative 
approach as ithas a smaller margin for errorwhen compared to 
'pskb_may_pull()'.
But I shall admit that the errors conditions for 'pskb_may_pull()' are 
extreme.

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

[Max Tottenham]

On 05/27, Pedro Tammela wrote:
> On 26/05/2023 18:54, Josh Hunt wrote:
> > On 5/26/23 7:03 AM, Pedro Tammela wrote:
> >> On 26/05/2023 10:47, Pedro Tammela wrote:
> >>>
> >>>> +
> >>>> +    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 
> >>
> >> Yes this obviously happens before the mentioned function.
> >> Now I'm wondering if it's not better to use skb_header_pointer() 
> >> instead...
> > 
> > Can you elaborate on why you think it would be better?
> > 
> 
> I don't have a strong argument for one over the other and I believe it's 
> fine as is.
> It just looks like 'skb_header_pointer()' is a more conservative 
> approach as ithas a smaller margin for errorwhen compared to 
> 'pskb_may_pull()'.
> But I shall admit that the errors conditions for 'pskb_may_pull()' are 
> extreme.
> 
Okay, I'm happy to re-spin a v3 addressing the above, and the other nits
you identified.

-- 
Max Tottenham | Senior Software Engineer
/(* Akamai Technologies