Message ID | 20230526095810.280474-1-mtottenh@akamai.com (mailing list archive) |
---|---|
State | Superseded |
Delegated to: | Netdev Maintainers |
Headers | show |
Series | [v2] net/sched: act_pedit: Parse L3 Header for L4 offset | expand |
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;
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; >
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; >
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
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.
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.
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;