| Message ID | 20230607162353.3631199-1-mtottenh@akamai.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 6c02568fd1ae53099b4ab86365c5be1ff15f586b |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v3] net/sched: act_pedit: Parse L3 Header for L4 offset | expand |
On 07/06/2023 13:23, 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") > 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/ LGTM, Reviewed-by: Pedro Tammela <pctammela@mojatatu.com> > --- > V2 -> V3: > * Address review feedback. > 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..c819b812a899 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) > +{ > + const int noff = skb_network_offset(skb); > + int ret = -EINVAL; > + struct iphdr _iph; > + > + switch (skb->protocol) { > + case htons(ETH_P_IP): { > + const struct iphdr *iph = skb_header_pointer(skb, noff, sizeof(_iph), &_iph); > + > + if (!iph) > + goto out; > + *hoffset = noff + iph->ihl * 4; > + ret = 0; > + break; > + } > + case htons(ETH_P_IPV6): > + 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: > 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;
Hello: This patch was applied to netdev/net.git (main) by David S. Miller <davem@davemloft.net>: On Wed, 7 Jun 2023 12:23:54 -0400 you 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: > > [...] Here is the summary with links: - [v3] net/sched: act_pedit: Parse L3 Header for L4 offset https://git.kernel.org/netdev/net/c/6c02568fd1ae You are awesome, thank you!
diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c index fc945c7e4123..c819b812a899 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) +{ + const int noff = skb_network_offset(skb); + int ret = -EINVAL; + struct iphdr _iph; + + switch (skb->protocol) { + case htons(ETH_P_IP): { + const struct iphdr *iph = skb_header_pointer(skb, noff, sizeof(_iph), &_iph); + + if (!iph) + goto out; + *hoffset = noff + iph->ihl * 4; + ret = 0; + break; + } + case htons(ETH_P_IPV6): + 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: 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;
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/ --- V2 -> V3: * Address review feedback. V1 -> V2: * Fix minor bug reported by kernel test bot. --- net/sched/act_pedit.c | 48 ++++++++++++++++++++++++++++++++++++++----- 1 file changed, 43 insertions(+), 5 deletions(-)