diff mbox series

net/sched: act_pedit: Parse L3 Header for L4 offset

Message ID 20230525164741.4188115-1-mtottenh@akamai.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series net/sched: act_pedit: Parse L3 Header for L4 offset | expand

Checks

Context Check Description
netdev/series_format warning Single patches do not need cover letters; Target tree name not specified in the subject
netdev/tree_selection success Guessed tree name to be net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit fail Errors and warnings before: 8 this patch: 14
netdev/cc_maintainers fail 2 blamed authors not CCed: ogerlitz@mellanox.com davem@davemloft.net; 5 maintainers not CCed: kuba@kernel.org ogerlitz@mellanox.com davem@davemloft.net pabeni@redhat.com edumazet@google.com
netdev/build_clang fail Errors and warnings before: 8 this patch: 16
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes fail Problems with Fixes tag: 2
netdev/build_allmodconfig_warn fail Errors and warnings before: 8 this patch: 14
netdev/checkpatch warning CHECK: Alignment should match open parenthesis WARNING: Please use correct Fixes: style 'Fixes: <12 chars of sha1> ("<title line>")' - ie: 'Fixes: a7762c419883 ("net/sched: act_pedit: Parse L3 Header for L4 offset")' WARNING: line length of 104 exceeds 80 columns WARNING: line length of 127 exceeds 80 columns WARNING: line length of 88 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Max Tottenham May 25, 2023, 4:47 p.m. UTC 
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>
---
 net/sched/act_pedit.c | 47 ++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 42 insertions(+), 5 deletions(-)

Comments

kernel test robot May 26, 2023, 7:52 a.m. UTC | #1 
Hi Max,

kernel test robot noticed the following build errors:

[auto build test ERROR on net/main]
[also build test ERROR on net-next/main linus/master v6.4-rc3 next-20230525]
[cannot apply to horms-ipvs/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Max-Tottenham/net-sched-act_pedit-Parse-L3-Header-for-L4-offset/20230526-020135
base:   net/main
patch link:    https://lore.kernel.org/r/20230525164741.4188115-1-mtottenh%40akamai.com
patch subject: [PATCH] net/sched: act_pedit: Parse L3 Header for L4 offset
config: mips-allyesconfig (https://download.01.org/0day-ci/archive/20230526/202305261541.N165u9TZ-lkp@intel.com/config)
compiler: mips-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        mkdir -p ~/bin
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/dc60ecbe9b4432221ba2109d4ca1956f47d5c5d6
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Max-Tottenham/net-sched-act_pedit-Parse-L3-Header-for-L4-offset/20230526-020135
        git checkout dc60ecbe9b4432221ba2109d4ca1956f47d5c5d6
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 ~/bin/make.cross W=1 O=build_dir ARCH=mips olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 ~/bin/make.cross W=1 O=build_dir ARCH=mips SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202305261541.N165u9TZ-lkp@intel.com/

All errors (new ones prefixed by >>):

   net/sched/act_pedit.c: In function 'tcf_pedit_act':
>> net/sched/act_pedit.c:428:17: error: 'rc' undeclared (first use in this function); did you mean 'rq'?
     428 |                 rc = pedit_skb_hdr_offset(skb, htype, &hoffset);
         |                 ^~
         |                 rq
   net/sched/act_pedit.c:428:17: note: each undeclared identifier is reported only once for each function it appears in


vim +428 net/sched/act_pedit.c

   386	
   387	TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb,
   388					    const struct tc_action *a,
   389					    struct tcf_result *res)
   390	{
   391		enum pedit_header_type htype = TCA_PEDIT_KEY_EX_HDR_TYPE_NETWORK;
   392		enum pedit_cmd cmd = TCA_PEDIT_KEY_EX_CMD_SET;
   393		struct tcf_pedit *p = to_pedit(a);
   394		struct tcf_pedit_key_ex *tkey_ex;
   395		struct tcf_pedit_parms *parms;
   396		struct tc_pedit_key *tkey;
   397		u32 max_offset;
   398		int i;
   399	
   400		parms = rcu_dereference_bh(p->parms);
   401	
   402		max_offset = (skb_transport_header_was_set(skb) ?
   403			      skb_transport_offset(skb) :
   404			      skb_network_offset(skb)) +
   405			     parms->tcfp_off_max_hint;
   406		if (skb_ensure_writable(skb, min(skb->len, max_offset)))
   407			goto done;
   408	
   409		tcf_lastuse_update(&p->tcf_tm);
   410		tcf_action_update_bstats(&p->common, skb);
   411	
   412		tkey = parms->tcfp_keys;
   413		tkey_ex = parms->tcfp_keys_ex;
   414	
   415		for (i = parms->tcfp_nkeys; i > 0; i--, tkey++) {
   416			int offset = tkey->off;
   417			int hoffset = 0;
   418			u32 *ptr, hdata;
   419			u32 val;
   420	
   421			if (tkey_ex) {
   422				htype = tkey_ex->htype;
   423				cmd = tkey_ex->cmd;
   424	
   425				tkey_ex++;
   426			}
   427	
 > 428			rc = pedit_skb_hdr_offset(skb, htype, &hoffset);
   429			if (rc) {
   430				pr_info_ratelimited("tc action pedit unable to extract header offset for header type (0x%x)\n", htype);
   431				goto bad;
   432			}
   433	
   434			if (tkey->offmask) {
   435				u8 *d, _d;
   436	
   437				if (!offset_valid(skb, hoffset + tkey->at)) {
   438					pr_info_ratelimited("tc action pedit 'at' offset %d out of bounds\n",
   439							    hoffset + tkey->at);
   440					goto bad;
   441				}
   442				d = skb_header_pointer(skb, hoffset + tkey->at,
   443						       sizeof(_d), &_d);
   444				if (!d)
   445					goto bad;
   446	
   447				offset += (*d & tkey->offmask) >> tkey->shift;
   448				if (offset % 4) {
   449					pr_info_ratelimited("tc action pedit offset must be on 32 bit boundaries\n");
   450					goto bad;
   451				}
   452			}
   453	
   454			if (!offset_valid(skb, hoffset + offset)) {
   455				pr_info_ratelimited("tc action pedit offset %d out of bounds\n", hoffset + offset);
   456				goto bad;
   457			}
   458	
   459			ptr = skb_header_pointer(skb, hoffset + offset,
   460						 sizeof(hdata), &hdata);
   461			if (!ptr)
   462				goto bad;
   463			/* just do it, baby */
   464			switch (cmd) {
   465			case TCA_PEDIT_KEY_EX_CMD_SET:
   466				val = tkey->val;
   467				break;
   468			case TCA_PEDIT_KEY_EX_CMD_ADD:
   469				val = (*ptr + tkey->val) & ~tkey->mask;
   470				break;
   471			default:
   472				pr_info_ratelimited("tc action pedit bad command (%d)\n", cmd);
   473				goto bad;
   474			}
   475	
   476			*ptr = ((*ptr & tkey->mask) ^ val);
   477			if (ptr == &hdata)
   478				skb_store_bits(skb, hoffset + offset, ptr, 4);
   479		}
   480	
   481		goto done;
   482	
   483	bad:
   484		tcf_action_inc_overlimit_qstats(&p->common);
   485	done:
   486		return p->tcf_action;
   487	}
   488
Max Tottenham May 26, 2023, 9:22 a.m. UTC | #2 
On 05/26, kernel test robot wrote:
> Hi Max,
> 
> kernel test robot noticed the following build errors:
> 
> [auto build test ERROR on net/main]
> [also build test ERROR on net-next/main linus/master v6.4-rc3 next-20230525]
> [cannot apply to horms-ipvs/master]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information ]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Max-Tottenham/net-sched-act_pedit-Parse-L3-Header-for-L4-offset/20230526-020135 
> base:   net/main
> patch link:    https://lore.kernel.org/r/20230525164741.4188115-1-mtottenh%40akamai.com 
> patch subject: [PATCH] net/sched: act_pedit: Parse L3 Header for L4 offset
> config: mips-allyesconfig https://download.01.org/0day-ci/archive/20230526/202305261541.N165u9TZ-lkp@intel.com/config )
> compiler: mips-linux-gcc (GCC) 12.1.0
> reproduce (this is a W=1 build):
>         mkdir -p ~/bin
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross  -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # https://github.com/intel-lab-lkp/linux/commit/dc60ecbe9b4432221ba2109d4ca1956f47d5c5d6 
>         git remote add linux-review https://github.com/intel-lab-lkp/linux 
>         git fetch --no-tags linux-review Max-Tottenham/net-sched-act_pedit-Parse-L3-Header-for-L4-offset/20230526-020135
>         git checkout dc60ecbe9b4432221ba2109d4ca1956f47d5c5d6
>         # save the config file
>         mkdir build_dir && cp config build_dir/.config
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 ~/bin/make.cross W=1 O=build_dir ARCH=mips olddefconfig
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 ~/bin/make.cross W=1 O=build_dir ARCH=mips SHELL=/bin/bash
> 
> If you fix the issue, kindly add following tag where applicable
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202305261541.N165u9TZ-lkp@intel.com/ 
> 
> All errors (new ones prefixed by >>):
> 
>    net/sched/act_pedit.c: In function 'tcf_pedit_act':
> >> net/sched/act_pedit.c:428:17: error: 'rc' undeclared (first use in this function); did you mean 'rq'?
>      428 |                 rc = pedit_skb_hdr_offset(skb, htype, &hoffset);
>          |                 ^~
>          |                 rq
>    net/sched/act_pedit.c:428:17: note: each undeclared identifier is reported only once for each function it appears in
> 
> 
> vim +428 net/sched/act_pedit.c
> 
>    386	
>    387	TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb,
>    388					    const struct tc_action *a,
>    389					    struct tcf_result *res)
>    390	{
>    391		enum pedit_header_type htype = TCA_PEDIT_KEY_EX_HDR_TYPE_NETWORK;
>    392		enum pedit_cmd cmd = TCA_PEDIT_KEY_EX_CMD_SET;
>    393		struct tcf_pedit *p = to_pedit(a);
>    394		struct tcf_pedit_key_ex *tkey_ex;
>    395		struct tcf_pedit_parms *parms;
>    396		struct tc_pedit_key *tkey;
>    397		u32 max_offset;
>    398		int i;
>    399	
>    400		parms = rcu_dereference_bh(p->parms);
>    401	
>    402		max_offset = (skb_transport_header_was_set(skb) ?
>    403			      skb_transport_offset(skb) :
>    404			      skb_network_offset(skb)) +
>    405			     parms->tcfp_off_max_hint;
>    406		if (skb_ensure_writable(skb, min(skb->len, max_offset)))
>    407			goto done;
>    408	
>    409		tcf_lastuse_update(&p->tcf_tm);
>    410		tcf_action_update_bstats(&p->common, skb);
>    411	
>    412		tkey = parms->tcfp_keys;
>    413		tkey_ex = parms->tcfp_keys_ex;
>    414	
>    415		for (i = parms->tcfp_nkeys; i > 0; i--, tkey++) {
>    416			int offset = tkey->off;
>    417			int hoffset = 0;
>    418			u32 *ptr, hdata;
>    419			u32 val;
>    420	
>    421			if (tkey_ex) {
>    422				htype = tkey_ex->htype;
>    423				cmd = tkey_ex->cmd;
>    424	
>    425				tkey_ex++;
>    426			}
>    427	
>  > 428			rc = pedit_skb_hdr_offset(skb, htype, &hoffset);

Ah yep looks like I made an omission while bringing back the rate limited
error message. Will respin a V2 with the fix.

>    429			if (rc) {
>    430				pr_info_ratelimited("tc action pedit unable to extract header offset for header type (0x%x)\n", htype);
>    431				goto bad;
>    432			}
>    433	
>    434			if (tkey->offmask) {
>    435				u8 *d, _d;
>    436	
>    437				if (!offset_valid(skb, hoffset + tkey->at)) {
>    438					pr_info_ratelimited("tc action pedit 'at' offset %d out of bounds\n",
>    439							    hoffset + tkey->at);
>    440					goto bad;
>    441				}
>    442				d = skb_header_pointer(skb, hoffset + tkey->at,
>    443						       sizeof(_d), &_d);
>    444				if (!d)
>    445					goto bad;
>    446	
>    447				offset += (*d & tkey->offmask) >> tkey->shift;
>    448				if (offset % 4) {
>    449					pr_info_ratelimited("tc action pedit offset must be on 32 bit boundaries\n");
>    450					goto bad;
>    451				}
>    452			}
>    453	
>    454			if (!offset_valid(skb, hoffset + offset)) {
>    455				pr_info_ratelimited("tc action pedit offset %d out of bounds\n", hoffset + offset);
>    456				goto bad;
>    457			}
>    458	
>    459			ptr = skb_header_pointer(skb, hoffset + offset,
>    460						 sizeof(hdata), &hdata);
>    461			if (!ptr)
>    462				goto bad;
>    463			/* just do it, baby */
>    464			switch (cmd) {
>    465			case TCA_PEDIT_KEY_EX_CMD_SET:
>    466				val = tkey->val;
>    467				break;
>    468			case TCA_PEDIT_KEY_EX_CMD_ADD:
>    469				val = (*ptr + tkey->val) & ~tkey->mask;
>    470				break;
>    471			default:
>    472				pr_info_ratelimited("tc action pedit bad command (%d)\n", cmd);
>    473				goto bad;
>    474			}
>    475	
>    476			*ptr = ((*ptr & tkey->mask) ^ val);
>    477			if (ptr == &hdata)
>    478				skb_store_bits(skb, hoffset + offset, ptr, 4);
>    479		}
>    480	
>    481		goto done;
>    482	
>    483	bad:
>    484		tcf_action_inc_overlimit_qstats(&p->common);
>    485	done:
>    486		return p->tcf_action;
>    487	}
>    488	
> 
> -- 
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki
diff mbox series

Patch

diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index fc945c7e4123..6362aa51e00b 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,
@@ -392,7 +425,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;