bpf: return proper error codes for lwt redirect

PATCH In review Yan Zhai 6 participants 26 messages Jul 26, 2023 – Aug 1, 2023 1 message not in the archive

Subsystems (8)

Networking Network drivers Kselftest Kernel janitors +4 more subsystems

lore

Share & tools

lore

Yan Zhai

author

2023-07-26 01:07 UTC #0 lore raw

v4 cover bpf: return proper error codes for lwt redirect

lwt xmit hook does not expect positive return values in function ip_finish_output2 and ip6_finish_output2. However, BPF redirect programs can return positive values such like NET_XMIT_DROP, NET_RX_DROP, and etc as errors. Such return values can panic the kernel unexpectedly:

https://gist.github.com/zhaiyan920/8fbac245b261fe316a7ef04c9b1eba48

This patch fixes the return values from BPF redirect, so the error handling would be consistent at xmit hook. It also adds a few test cases to prevent future regressions.

v3: https://lore.kernel.org/bpf/[email protected]/
v2: https://lore.kernel.org/netdev/[email protected]/
v1: https://lore.kernel.org/bpf/[email protected]/

changes since v3:
* minor change in commit message and changelogs
* tested by Jakub Sitnicki

changes since v2:
* subject name changed
* also covered redirect to ingress case
* added selftests

changes since v1:
* minor code style changes

Yan Zhai (2):
bpf: fix skb_do_redirect return values
bpf: selftests: add lwt redirect regression test cases

 include/linux/netdevice.h                     |   2 +
 net/core/filter.c                             |   9 +-
 tools/testing/selftests/bpf/Makefile          |   1 +
 .../selftests/bpf/progs/test_lwt_redirect.c   |  66 +++++++
 .../selftests/bpf/test_lwt_redirect.sh        | 174 ++++++++++++++++++
 5 files changed, 250 insertions(+), 2 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/test_lwt_redirect.c
 create mode 100755 tools/testing/selftests/bpf/test_lwt_redirect.sh

Signature · 1 line

2.30.2

Yan Zhai

author

2023-07-26 01:08 UTC #1 lore raw

PATCH v4 1/2 bpf: fix skb_do_redirect return values

skb_do_redirect returns various of values: error code (negative), 0 (success), and some positive status code, e.g. NET_XMIT_CN, NET_RX_DROP. Commit 3a0af8fd61f9 ("bpf: BPF for lightweight tunnel infrastructure") didn't check the return code correctly, so positive values are propagated back along call chain:

ip_finish_output2

    -> bpf_xmit
      -> run_lwt_bpf
        -> skb_do_redirect

Inside ip_finish_output2, redirected skb will continue to neighbor subsystem as if LWTUNNEL_XMIT_CONTINUE is returned, despite that this skb could have been freed. The bug can trigger use-after-free warning and crashes kernel afterwards:

https://gist.github.com/zhaiyan920/8fbac245b261fe316a7ef04c9b1eba48

Convert positive statuses from skb_do_redirect eliminates this issue.

Fixes 3a0af8fd61f9 ("bpf: BPF for lightweight tunnel infrastructure")

Tested-by Jakub Sitnicki <[email protected]>

Reported-by Jordan Griege <[email protected]>

Signed-off-by Yan Zhai <[email protected]>

---
 include/linux/netdevice.h | 2 ++
 net/core/filter.c         | 9 +++++++--
 2 files changed, 9 insertions(+), 2 deletions(-)

include/linux/netdevice.h +2 -0

index b828c7a75be2..520d808eec15 100644
@@ -87,6 +87,8 @@ void netdev_sw_irq_coalesce_default_on(struct net_device *dev);
 #define NET_RX_SUCCESS		0	/* keep 'em coming, baby */
 #define NET_RX_DROP		1	/* packet dropped */
 
+#define net_rx_errno(e)		((e) == NET_RX_DROP ? -ENOBUFS : (e))
+
 #define MAX_NEST_DEV 8
 
 /*

net/core/filter.c +7 -2

index 06ba0e56e369..564104543737 100644
@@ -2095,7 +2095,9 @@ static const struct bpf_func_proto bpf_csum_level_proto = {
 
 static inline int __bpf_rx_skb(struct net_device *dev, struct sk_buff *skb)
 {
-	return dev_forward_skb_nomtu(dev, skb);
+	int ret = dev_forward_skb_nomtu(dev, skb);
+
+	return net_rx_errno(ret);
 }
 
 static inline int __bpf_rx_skb_no_mac(struct net_device *dev,

@@ -2108,7 +2110,7 @@ static inline int __bpf_rx_skb_no_mac(struct net_device *dev,
 		ret = netif_rx(skb);
 	}
 
-	return ret;
+	return net_rx_errno(ret);
 }
 
 static inline int __bpf_tx_skb(struct net_device *dev, struct sk_buff *skb)

@@ -2129,6 +2131,9 @@ static inline int __bpf_tx_skb(struct net_device *dev, struct sk_buff *skb)
 	ret = dev_queue_xmit(skb);
 	dev_xmit_recursion_dec();
 
+	if (unlikely(ret > 0))
+		ret = net_xmit_errno(ret);
+
 	return ret;
 }

Signature · 1 line

2.30.2

Markus Elfring

2023-07-26 07:43 UTC #2 lore raw

Re: [PATCH v4 bpf 1/2] bpf: fix skb_do_redirect return values

Quoted 2 lines

skb_do_redirect returns various of values: error code (negative), 0 (success), and some positive status code, e.g. NET_XMIT_CN,

How do you think about to use a wording variant (like the following)?

skb_do_redirect() returns different value kinds so far:

Can it be nicer to use a multi-line enumeration here?

Quoted 1 line

NET_RX_DROP. Commit 3a0af8fd61f9 ("bpf: BPF for lightweight tunnel

I suggest to start this sentence on a separate line. Can the commit specification fit into the same line then?

…

Quoted 1 line

https://gist.github.com/zhaiyan920/8fbac245b261fe316a7ef04c9b1eba48

Can it help to mention here that you would like to refer to a complete KASAN report (which contains 89 text lines then)?

Quoted 1 line

Convert positive statuses from skb_do_redirect eliminates this issue.

Would you like to avoid another wording weakness in this sentence? How do you think about to use the following wording variant?

Thus convert positive status values from the execution of the function “skb_do_redirect” into special error codes.

…

Quoted 1 line

Suggested-by Markus Elfring

…

I dare to point review concerns out for various software components. But I did not get the impression that I suggested the patch idea.

Thus I find this tag inappropriate here.
Would any information become more relevant for related version descriptions?

Regards,
Markus

Markus Elfring

2023-07-26 11:50 UTC #3 lore raw

Re: [PATCH v4 bpf 1/2] bpf: fix skb_do_redirect return values

In reply to Yan Zhai

…

include/linux/netdevice.h void netdev_sw_irq_coalesce_default_on(struct net_device *dev); · line 90

Quoted 6 lines

+++ b/include/linux/netdevice.h

include/linux/netdevice.h +1 -0

@@ -87,6 +87,8 @@ void netdev_sw_irq_coalesce_default_on(struct net_device *dev);
 #define NET_RX_SUCCESS		0	/* keep 'em coming, baby */
 #define NET_RX_DROP		1	/* packet dropped */
 
+#define net_rx_errno(e)		((e) == NET_RX_DROP ? -ENOBUFS : (e))

…

Can it become safer to use an inline function
instead of the proposed macro addition?

Regards,
Markus

Jakub Sitnicki

Reviewed-by on v4 1/2

2023-07-26 12:26 UTC #4 lore raw

Re: [PATCH v4 bpf 1/2] bpf: fix skb_do_redirect return values

In reply to Yan Zhai

Quoted 20 lines · Yan Zhai

skb_do_redirect returns various of values: error code (negative), 0 (success), and some positive status code, e.g. NET_XMIT_CN, NET_RX_DROP. Commit 3a0af8fd61f9 ("bpf: BPF for lightweight tunnel infrastructure") didn't check the return code correctly, so positive values are propagated back along call chain:
ip_finish_output2
    -> bpf_xmit
      -> run_lwt_bpf
        -> skb_do_redirect
Inside ip_finish_output2, redirected skb will continue to neighbor subsystem as if LWTUNNEL_XMIT_CONTINUE is returned, despite that this skb could have been freed. The bug can trigger use-after-free warning and crashes kernel afterwards:
https://gist.github.com/zhaiyan920/8fbac245b261fe316a7ef04c9b1eba48
Convert positive statuses from skb_do_redirect eliminates this issue.

Quoted 7 lines

Fixes 3a0af8fd61f9 ("bpf: BPF for lightweight tunnel infrastructure")
Tested-by Jakub Sitnicki <[email protected]>
Suggested-by Markus Elfring <[email protected]>
Suggested-by Stanislav Fomichev <[email protected]>
Reported-by Jordan Griege <[email protected]>
Signed-off-by Yan Zhai <[email protected]>
---

Reviewed-by Jakub Sitnicki <[email protected]>

Dan Carpenter

2023-07-26 13:39 UTC #5 lore raw

Re: [PATCH v4 bpf 1/2] bpf: fix skb_do_redirect return values

In reply to Yan Zhai

I'm not positive I understand the code in ip_finish_output2(). I think instead of looking for LWTUNNEL_XMIT_DONE it should instead look for != LWTUNNEL_XMIT_CONTINUE. It's unfortunate that NET_XMIT_DROP and LWTUNNEL_XMIT_CONTINUE are the both 0x1. Why don't we just change that instead?

Also there seems to be a leak in lwtunnel_xmit(). Should that return LWTUNNEL_XMIT_CONTINUE or should it call kfree_skb() before returning?

Something like the following?

include/linux/netdevice.h +3 -0

index 11652e464f5d..375790b672bc 100644
@@ -112,6 +112,9 @@ void netdev_sw_irq_coalesce_default_on(struct net_device *dev);
 #define NET_XMIT_CN		0x02	/* congestion notification	*/
 #define NET_XMIT_MASK		0x0f	/* qdisc flags in net/sch_generic.h */
 
+#define LWTUNNEL_XMIT_DONE NET_XMIT_SUCCESS
+#define LWTUNNEL_XMIT_CONTINUE 0x3
+
 /* NET_XMIT_CN is special. It does not guarantee that this packet is lost. It
  * indicates that the device will soon be dropping packets, or already drops
  * some packets of the same priority; prompting us to send less aggressively. */

include/net/lwtunnel.h +0 -6

index 6f15e6fa154e..8ab032ee04d0 100644
@@ -16,12 +16,6 @@
 #define LWTUNNEL_STATE_INPUT_REDIRECT	BIT(1)
 #define LWTUNNEL_STATE_XMIT_REDIRECT	BIT(2)
 
-enum {
-	LWTUNNEL_XMIT_DONE,
-	LWTUNNEL_XMIT_CONTINUE,
-};
-
-
 struct lwtunnel_state {
 	__u16		type;
 	__u16		flags;

net/core/lwtunnel.c +1 -1

index 711cd3b4347a..732415d1287d 100644
@@ -371,7 +371,7 @@ int lwtunnel_xmit(struct sk_buff *skb)
 
 	if (lwtstate->type == LWTUNNEL_ENCAP_NONE ||
 	    lwtstate->type > LWTUNNEL_ENCAP_MAX)
-		return 0;
+		return LWTUNNEL_XMIT_CONTINUE;
 
 	ret = -EOPNOTSUPP;
 	rcu_read_lock();

net/ipv4/ip_output.c +1 -1

index 6e70839257f7..4be50a211b14 100644
@@ -216,7 +216,7 @@ static int ip_finish_output2(struct net *net, struct sock *sk, struct sk_buff *s
 	if (lwtunnel_xmit_redirect(dst->lwtstate)) {
 		int res = lwtunnel_xmit(skb);
 
-		if (res < 0 || res == LWTUNNEL_XMIT_DONE)
+		if (res != LWTUNNEL_XMIT_CONTINUE)
 			return res;
 	}

net/ipv6/ip6_output.c +1 -1

index 1e8c90e97608..016b0a513259 100644
@@ -113,7 +113,7 @@ static int ip6_finish_output2(struct net *net, struct sock *sk, struct sk_buff *
 	if (lwtunnel_xmit_redirect(dst->lwtstate)) {
 		int res = lwtunnel_xmit(skb);
 
-		if (res < 0 || res == LWTUNNEL_XMIT_DONE)
+		if (res != LWTUNNEL_XMIT_CONTINUE)
 			return res;
 	}

Yan Zhai

author

2023-07-26 14:15 UTC #6 lore raw

Re: [PATCH v4 bpf 1/2] bpf: fix skb_do_redirect return values

Quoted 6 lines · Dan Carpenter

I'm not positive I understand the code in ip_finish_output2(). I think instead of looking for LWTUNNEL_XMIT_DONE it should instead look for != LWTUNNEL_XMIT_CONTINUE. It's unfortunate that NET_XMIT_DROP and LWTUNNEL_XMIT_CONTINUE are the both 0x1. Why don't we just change that instead?

I considered about changing lwt side logic. But it would bring larger impact since there are multiple types of encaps on this hook, not just bpf redirect. Changing bpf return values is a minimum change on the other hand. In addition, returning value of NET_RX_DROP and NET_XMIT_CN are the same, so if we don't do something in bpf redirect, there is no way to distinguish them later: the former is considered as an error, while "CN" is considered as non-error.

Quoted 41 lines

Also there seems to be a leak in lwtunnel_xmit(). Should that return LWTUNNEL_XMIT_CONTINUE or should it call kfree_skb() before returning?

Something like the following?

include/linux/netdevice.h +3 -0

index 11652e464f5d..375790b672bc 100644
@@ -112,6 +112,9 @@ void netdev_sw_irq_coalesce_default_on(struct net_device *dev);
 #define NET_XMIT_CN		0x02	/* congestion notification	*/
 #define NET_XMIT_MASK		0x0f	/* qdisc flags in net/sch_generic.h */
 
+#define LWTUNNEL_XMIT_DONE NET_XMIT_SUCCESS
+#define LWTUNNEL_XMIT_CONTINUE 0x3
+
 /* NET_XMIT_CN is special. It does not guarantee that this packet is lost. It
  * indicates that the device will soon be dropping packets, or already drops
  * some packets of the same priority; prompting us to send less aggressively. */

include/net/lwtunnel.h +0 -6

index 6f15e6fa154e..8ab032ee04d0 100644
@@ -16,12 +16,6 @@
 #define LWTUNNEL_STATE_INPUT_REDIRECT	BIT(1)
 #define LWTUNNEL_STATE_XMIT_REDIRECT	BIT(2)
 
-enum {
-	LWTUNNEL_XMIT_DONE,
-	LWTUNNEL_XMIT_CONTINUE,
-};
-
-
 struct lwtunnel_state {
 	__u16		type;
 	__u16		flags;

net/core/lwtunnel.c +0 -0

index 711cd3b4347a..732415d1287d 100644

net/core/lwtunnel.c int lwtunnel_xmit(struct sk_buff *skb) · line 374

Quoted 5 lines

net/core/lwtunnel.c +1 -1

@@ -371,7 +371,7 @@ int lwtunnel_xmit(struct sk_buff *skb)
 
 	if (lwtstate->type == LWTUNNEL_ENCAP_NONE ||
 	    lwtstate->type > LWTUNNEL_ENCAP_MAX)
-		return 0;
+		return LWTUNNEL_XMIT_CONTINUE;

You are correct this path would leak skb. Return continue (or drop) would avoid the leak. Personally I'd prefer drop instead to signal the error setup. Since this is a separate issue, do you want to send a separate patch on this? Or I am happy to do it if you prefer.

Quoted 8 lines

net/core/lwtunnel.c +0 -0
 	ret = -EOPNOTSUPP;
 	rcu_read_lock();
net/ipv4/ip_output.c +0 -0
index 6e70839257f7..4be50a211b14 100644

net/ipv4/ip_output.c static int ip_finish_output2(struct net *net, struct sock *sk, struct sk_buff *s · line 220

Quoted 6 lines

net/ipv4/ip_output.c +1 -1

@@ -216,7 +216,7 @@ static int ip_finish_output2(struct net *net, struct sock *sk, struct sk_buff *s
 	if (lwtunnel_xmit_redirect(dst->lwtstate)) {
 		int res = lwtunnel_xmit(skb);
 
-		if (res < 0 || res == LWTUNNEL_XMIT_DONE)
+		if (res != LWTUNNEL_XMIT_CONTINUE)
 			return res;

Unfortunately we cannot return res directly here when res > 0. This is the final reason why I didn't patch here. Return values here can be propagated back to sendmsg syscall, so returning a positive value would break the syscall convention.

best,
Yan

Quoted 15 lines

net/ipv4/ip_output.c +0 -0

net/ipv6/ip6_output.c +1 -1

index 1e8c90e97608..016b0a513259 100644
@@ -113,7 +113,7 @@ static int ip6_finish_output2(struct net *net, struct sock *sk, struct sk_buff *
 	if (lwtunnel_xmit_redirect(dst->lwtstate)) {
 		int res = lwtunnel_xmit(skb);
 
-		if (res < 0 || res == LWTUNNEL_XMIT_DONE)
+		if (res != LWTUNNEL_XMIT_CONTINUE)
 			return res;
 	}

Dan Carpenter

2023-07-26 15:01 UTC #7 lore raw

Re: [PATCH v4 bpf 1/2] bpf: fix skb_do_redirect return values

Quoted 7 lines · Yan Zhai

Quoted 6 lines · Dan Carpenter
I'm not positive I understand the code in ip_finish_output2(). I think instead of looking for LWTUNNEL_XMIT_DONE it should instead look for != LWTUNNEL_XMIT_CONTINUE. It's unfortunate that NET_XMIT_DROP and LWTUNNEL_XMIT_CONTINUE are the both 0x1. Why don't we just change that instead?

Quoted 7 lines

I considered about changing lwt side logic. But it would bring larger impact since there are multiple types of encaps on this hook, not just bpf redirect. Changing bpf return values is a minimum change on the other hand. In addition, returning value of NET_RX_DROP and NET_XMIT_CN are the same, so if we don't do something in bpf redirect, there is no way to distinguish them later: the former is considered as an error, while "CN" is considered as non-error.

Uh, NET_RX/XMIT_DROP values are 1. NET_XMIT_CN is 2.

I'm not an expert but I think what happens is that we treat NET_XMIT_CN as success so that it takes a while for the resend to happen. Eventually the TCP layer will detect it as a dropped packet.

Quoted 43 lines

Also there seems to be a leak in lwtunnel_xmit(). Should that return LWTUNNEL_XMIT_CONTINUE or should it call kfree_skb() before returning?

Something like the following?

include/linux/netdevice.h +3 -0

index 11652e464f5d..375790b672bc 100644
@@ -112,6 +112,9 @@ void netdev_sw_irq_coalesce_default_on(struct net_device *dev);
 #define NET_XMIT_CN		0x02	/* congestion notification	*/
 #define NET_XMIT_MASK		0x0f	/* qdisc flags in net/sch_generic.h */
 
+#define LWTUNNEL_XMIT_DONE NET_XMIT_SUCCESS
+#define LWTUNNEL_XMIT_CONTINUE 0x3
+
 /* NET_XMIT_CN is special. It does not guarantee that this packet is lost. It
  * indicates that the device will soon be dropping packets, or already drops
  * some packets of the same priority; prompting us to send less aggressively. */

include/net/lwtunnel.h +0 -6

index 6f15e6fa154e..8ab032ee04d0 100644
@@ -16,12 +16,6 @@
 #define LWTUNNEL_STATE_INPUT_REDIRECT	BIT(1)
 #define LWTUNNEL_STATE_XMIT_REDIRECT	BIT(2)
 
-enum {
-	LWTUNNEL_XMIT_DONE,
-	LWTUNNEL_XMIT_CONTINUE,
-};
-
-
 struct lwtunnel_state {
 	__u16		type;
 	__u16		flags;

net/core/lwtunnel.c +0 -0

index 711cd3b4347a..732415d1287d 100644
@@ -371,7 +371,7 @@ int lwtunnel_xmit(struct sk_buff *skb)

Quoted 10 lines

net/core/lwtunnel.c int lwtunnel_xmit(struct sk_buff *skb) · line 374
Quoted 4 lines
net/core/lwtunnel.c +1 -1
 	if (lwtstate->type == LWTUNNEL_ENCAP_NONE ||
 	    lwtstate->type > LWTUNNEL_ENCAP_MAX)
-		return 0;
+		return LWTUNNEL_XMIT_CONTINUE;
You are correct this path would leak skb. Return continue (or drop) would avoid the leak. Personally I'd prefer drop instead to signal the error setup. Since this is a separate issue, do you want to send a separate patch on this? Or I am happy to do it if you prefer.

I don't know which makes sense so I'll leave that up to you.

Quoted 11 lines

net/core/lwtunnel.c +0 -0

 	ret = -EOPNOTSUPP;
 	rcu_read_lock();

net/ipv4/ip_output.c +0 -0

index 6e70839257f7..4be50a211b14 100644
@@ -216,7 +216,7 @@ static int ip_finish_output2(struct net *net, struct sock *sk, struct sk_buff *s
 	if (lwtunnel_xmit_redirect(dst->lwtstate)) {
 		int res = lwtunnel_xmit(skb);

Quoted 8 lines

net/ipv4/ip_output.c static int ip_finish_output2(struct net *net, struct sock *sk, struct sk_buff *s · line 220
Quoted 3 lines
net/ipv4/ip_output.c +1 -1
-		if (res < 0 || res == LWTUNNEL_XMIT_DONE)
+		if (res != LWTUNNEL_XMIT_CONTINUE)
 			return res;
Unfortunately we cannot return res directly here when res > 0. This is the final reason why I didn't patch here. Return values here can be propagated back to sendmsg syscall, so returning a positive value would break the syscall convention.

The neigh_output() function is going to return NET_XMIT_DROP so this already happens. Is that not what we want to happen?

I guess my concern is that eventually people will eventually new introduce bugs. Fixing incorrect error codes is something that I do several times per week. :P

regards,
dan carpenter

Yan Zhai

author

2023-07-26 16:10 UTC #8 lore raw

Re: [PATCH v4 bpf 1/2] bpf: fix skb_do_redirect return values

Quoted 16 lines · Dan Carpenter

Quoted 7 lines · Yan Zhai
Quoted 6 lines · Dan Carpenter
I'm not positive I understand the code in ip_finish_output2(). I think instead of looking for LWTUNNEL_XMIT_DONE it should instead look for != LWTUNNEL_XMIT_CONTINUE. It's unfortunate that NET_XMIT_DROP and LWTUNNEL_XMIT_CONTINUE are the both 0x1. Why don't we just change that instead?
Quoted 7 lines
I considered about changing lwt side logic. But it would bring larger impact since there are multiple types of encaps on this hook, not just bpf redirect. Changing bpf return values is a minimum change on the other hand. In addition, returning value of NET_RX_DROP and NET_XMIT_CN are the same, so if we don't do something in bpf redirect, there is no way to distinguish them later: the former is considered as an error, while "CN" is considered as non-error.

Quoted 6 lines

Uh, NET_RX/XMIT_DROP values are 1. NET_XMIT_CN is 2.
I'm not an expert but I think what happens is that we treat NET_XMIT_CN as success so that it takes a while for the resend to happen. Eventually the TCP layer will detect it as a dropped packet.

My eyes slipped lines. CN is 2. But the fact RX return value can be returned on a TX path still makes me feel unclean. Odds are low that we will have new statuses in future, it is a risk. I'd hope to contain these values only inside BPF redirect code as they are the reason why such rx values can show up there. Meanwhile, your argument do make good sense to me that the same problem may occur for other stuff. It is true. In fact, I just re-examined BPF-REROUTE path, it has the exact same issue by directly sending dst_output value back.

So I would propose to do two things:
1. still convert BPF redirect ingress code to contain the propagation of mixed return. Return only TX side value instead, which is also what majority of those local senders are expecting. (I was wrong about positive values returned to sendmsg below btw, they are not).

2. change LWTUNNEL_XMIT_CONTINUE and check for this at xmit hook.

Quoted 54 lines

Quoted 43 lines
Quoted 43 lines
Also there seems to be a leak in lwtunnel_xmit(). Should that return LWTUNNEL_XMIT_CONTINUE or should it call kfree_skb() before returning?
Something like the following?
include/linux/netdevice.h +3 -0
index 11652e464f5d..375790b672bc 100644
@@ -112,6 +112,9 @@ void netdev_sw_irq_coalesce_default_on(struct net_device *dev);
 #define NET_XMIT_CN		0x02	/* congestion notification	*/
 #define NET_XMIT_MASK		0x0f	/* qdisc flags in net/sch_generic.h */
 
+#define LWTUNNEL_XMIT_DONE NET_XMIT_SUCCESS
+#define LWTUNNEL_XMIT_CONTINUE 0x3
+
 /* NET_XMIT_CN is special. It does not guarantee that this packet is lost. It
  * indicates that the device will soon be dropping packets, or already drops
  * some packets of the same priority; prompting us to send less aggressively. */
include/net/lwtunnel.h +0 -6
index 6f15e6fa154e..8ab032ee04d0 100644
@@ -16,12 +16,6 @@
 #define LWTUNNEL_STATE_INPUT_REDIRECT	BIT(1)
 #define LWTUNNEL_STATE_XMIT_REDIRECT	BIT(2)
 
-enum {
-	LWTUNNEL_XMIT_DONE,
-	LWTUNNEL_XMIT_CONTINUE,
-};
-
-
 struct lwtunnel_state {
 	__u16		type;
 	__u16		flags;
net/core/lwtunnel.c +0 -0
index 711cd3b4347a..732415d1287d 100644
@@ -371,7 +371,7 @@ int lwtunnel_xmit(struct sk_buff *skb)
 
Quoted 10 lines
net/core/lwtunnel.c int lwtunnel_xmit(struct sk_buff *skb) · line 374
Quoted 4 lines
net/core/lwtunnel.c +1 -1
 	if (lwtstate->type == LWTUNNEL_ENCAP_NONE ||
 	    lwtstate->type > LWTUNNEL_ENCAP_MAX)
-		return 0;
+		return LWTUNNEL_XMIT_CONTINUE;
You are correct this path would leak skb. Return continue (or drop) would avoid the leak. Personally I'd prefer drop instead to signal the error setup. Since this is a separate issue, do you want to send a separate patch on this? Or I am happy to do it if you prefer.

Quoted 2 lines

I don't know which makes sense so I'll leave that up to you.

This conversation is juicy, I think we discovered two potential new problem sites (the leak here and the reroute path) :)

Quoted 20 lines

Quoted 11 lines
Quoted 11 lines
net/core/lwtunnel.c +0 -0
 	ret = -EOPNOTSUPP;
 	rcu_read_lock();
net/ipv4/ip_output.c +0 -0
index 6e70839257f7..4be50a211b14 100644
@@ -216,7 +216,7 @@ static int ip_finish_output2(struct net *net, struct sock *sk, struct sk_buff *s
 	if (lwtunnel_xmit_redirect(dst->lwtstate)) {
 		int res = lwtunnel_xmit(skb);
 
Quoted 8 lines
net/ipv4/ip_output.c static int ip_finish_output2(struct net *net, struct sock *sk, struct sk_buff *s · line 220
Quoted 3 lines
net/ipv4/ip_output.c +1 -1
-		if (res < 0 || res == LWTUNNEL_XMIT_DONE)
+		if (res != LWTUNNEL_XMIT_CONTINUE)
 			return res;
Unfortunately we cannot return res directly here when res > 0. This is the final reason why I didn't patch here. Return values here can be propagated back to sendmsg syscall, so returning a positive value would break the syscall convention.

Quoted 3 lines

The neigh_output() function is going to return NET_XMIT_DROP so this already happens. Is that not what we want to happen?

My bad, those return values are processed at ip_send_skb etc, while I was staring only at ip_local_out and beneath with my sleepy eyes.

Quoted 8 lines

I guess my concern is that eventually people will eventually new introduce bugs. Fixing incorrect error codes is something that I do several times per week. :P
regards,
dan carpenter

Dan Carpenter

2023-07-26 16:53 UTC #9 lore raw

Re: [PATCH v4 bpf 1/2] bpf: fix skb_do_redirect return values

Quoted 32 lines · Yan Zhai

Quoted 16 lines · Dan Carpenter
Quoted 7 lines · Yan Zhai
Quoted 6 lines · Dan Carpenter
I'm not positive I understand the code in ip_finish_output2(). I think instead of looking for LWTUNNEL_XMIT_DONE it should instead look for != LWTUNNEL_XMIT_CONTINUE. It's unfortunate that NET_XMIT_DROP and LWTUNNEL_XMIT_CONTINUE are the both 0x1. Why don't we just change that instead?
Quoted 7 lines
I considered about changing lwt side logic. But it would bring larger impact since there are multiple types of encaps on this hook, not just bpf redirect. Changing bpf return values is a minimum change on the other hand. In addition, returning value of NET_RX_DROP and NET_XMIT_CN are the same, so if we don't do something in bpf redirect, there is no way to distinguish them later: the former is considered as an error, while "CN" is considered as non-error.
Quoted 6 lines
Uh, NET_RX/XMIT_DROP values are 1. NET_XMIT_CN is 2.
I'm not an expert but I think what happens is that we treat NET_XMIT_CN as success so that it takes a while for the resend to happen. Eventually the TCP layer will detect it as a dropped packet.
My eyes slipped lines. CN is 2. But the fact RX return value can be returned on a TX path still makes me feel unclean. Odds are low that we will have new statuses in future, it is a risk. I'd hope to contain these values only inside BPF redirect code as they are the reason why such rx values can show up there. Meanwhile, your argument do make good sense to me that the same problem may occur for other stuff. It is true. In fact, I just re-examined BPF-REROUTE path, it has the exact same issue by directly sending dst_output value back.

Quoted 8 lines

So I would propose to do two things:
1. still convert BPF redirect ingress code to contain the propagation of mixed return. Return only TX side value instead, which is also what majority of those local senders are expecting. (I was wrong about positive values returned to sendmsg below btw, they are not).
2. change LWTUNNEL_XMIT_CONTINUE and check for this at xmit hook.

Sounds good!

regards,
dan carpenter

Dan Carpenter

2023-07-31 14:26 UTC #10 lore raw

Re: [PATCH v4 bpf 1/2] bpf: fix skb_do_redirect return values

I'm not a networking person, but I was looking at some use after free static checker warnings.

Apparently the rule with xmit functions is that if they return a value

Quoted 1 line

15 then that means the skb was not freed. Otherwise it's supposed to

be freed. So like NETDEV_TX_BUSY is 0x10 so it's not freed.

This is checked with using the dev_xmit_complete() function. So I feel like it would make sense for LWTUNNEL_XMIT_CONTINUE to return higher than 15.

Because that's the bug right? The original code was assuming that everything besides LWTUNNEL_XMIT_DONE was freed.

regards,
dan carpenter

Yan Zhai

author

2023-08-01 22:18 UTC #11 lore raw

Re: [PATCH v4 bpf 1/2] bpf: fix skb_do_redirect return values

Quoted 3 lines · Dan Carpenter

I'm not a networking person, but I was looking at some use after free static checker warnings.

Did you refer to the gist I posted or something new?

Quoted 8 lines

Apparently the rule with xmit functions is that if they return a value
Quoted 1 line
15 then that means the skb was not freed. Otherwise it's supposed to
be freed. So like NETDEV_TX_BUSY is 0x10 so it's not freed.
This is checked with using the dev_xmit_complete() function. So I feel like it would make sense for LWTUNNEL_XMIT_CONTINUE to return higher than 15.

Yes I am adopting your suggestion in v5. Dealing with NETDEV_TX_BUSY would be left as another item (potentially more suited for netdev rather than bpf). Would be great to find a reproduction of memleak.

Quoted 7 lines

Because that's the bug right? The original code was assuming that everything besides LWTUNNEL_XMIT_DONE was freed.
regards,
dan carpenter

Signature · 1 line

Yan

Martin KaFai Lau

reviewer

2023-07-28 22:02 UTC #12 lore raw

Re: [PATCH v4 bpf 1/2] bpf: fix skb_do_redirect return values

In reply to Yan Zhai

Quoted 10 lines · Yan Zhai

skb_do_redirect returns various of values: error code (negative), 0 (success), and some positive status code, e.g. NET_XMIT_CN, NET_RX_DROP. Commit 3a0af8fd61f9 ("bpf: BPF for lightweight tunnel infrastructure") didn't check the return code correctly, so positive values are propagated back along call chain:
   ip_finish_output2
     -> bpf_xmit
       -> run_lwt_bpf
         -> skb_do_redirect

From looking at skb_do_redirect, the skb_do_redirect should have consumed the skb except for the -EAGAIN return value. afaik, -EAGAIN could only happen by using the bpf_redirect_peer helper. lwt does not have the bpf_redirect_peer helper available, so there is no -EAGAIN case in lwt. iow, skb_do_redirect should have always consumed the skb in lwt. or did I miss something?

If that is the case, it feels like the fix should be in run_lwt_bpf() and the "if (ret == 0)" test in run_lwt_bpf() is unnecessary?

			ret = skb_do_redirect(skb);
			if (ret == 0)
				ret = BPF_REDIRECT;

Quoted 7 lines

Inside ip_finish_output2, redirected skb will continue to neighbor subsystem as if LWTUNNEL_XMIT_CONTINUE is returned, despite that this skb could have been freed. The bug can trigger use-after-free warning and crashes kernel afterwards:
https://gist.github.com/zhaiyan920/8fbac245b261fe316a7ef04c9b1eba48

Yan Zhai

author

2023-07-31 21:35 UTC #13 lore raw

Re: [PATCH v4 bpf 1/2] bpf: fix skb_do_redirect return values

Quoted 19 lines · Martin KaFai Lau

Quoted 10 lines · Yan Zhai
skb_do_redirect returns various of values: error code (negative), 0 (success), and some positive status code, e.g. NET_XMIT_CN, NET_RX_DROP. Commit 3a0af8fd61f9 ("bpf: BPF for lightweight tunnel infrastructure") didn't check the return code correctly, so positive values are propagated back along call chain:
   ip_finish_output2
     -> bpf_xmit
       -> run_lwt_bpf
         -> skb_do_redirect
From looking at skb_do_redirect, the skb_do_redirect should have consumed the
skb except for the -EAGAIN return value. afaik, -EAGAIN could only happen by using the bpf_redirect_peer helper. lwt does not have the bpf_redirect_peer helper available, so there is no -EAGAIN case in lwt. iow, skb_do_redirect should have always consumed the skb in lwt. or did I miss something?

Quoted 8 lines

If that is the case, it feels like the fix should be in run_lwt_bpf() and the "if (ret == 0)" test in run_lwt_bpf() is unnecessary?
                        ret = skb_do_redirect(skb);
                        if (ret == 0)
                                ret = BPF_REDIRECT;

Just fixing skb redirect return code won't be sufficient. I realized there are other return paths that need to be treated, e.g. bpf reroute path also directly returns dev_queue_xmit status. I plan to check for LWTUNNEL_XMIT_CONTINUE (and change it to a value that does not conflict with NET_RX_DROP and NET_XMIT_DROP) in the next revision. On the other hand, the return value of NETDEV_TX_BUSY is another hassle. As Dan suggested, packets might not have been freed when this is returned from drivers. The caller of dev_queue_xmit might need to free skb when this happens.

Yan

Martin KaFai Lau

reviewer

2023-07-31 22:11 UTC #14 lore raw

Re: [PATCH v4 bpf 1/2] bpf: fix skb_do_redirect return values

Quoted 28 lines · Yan Zhai

Quoted 19 lines · Martin KaFai Lau
Quoted 10 lines · Yan Zhai
skb_do_redirect returns various of values: error code (negative), 0 (success), and some positive status code, e.g. NET_XMIT_CN, NET_RX_DROP. Commit 3a0af8fd61f9 ("bpf: BPF for lightweight tunnel infrastructure") didn't check the return code correctly, so positive values are propagated back along call chain:
    ip_finish_output2
      -> bpf_xmit
        -> run_lwt_bpf
          -> skb_do_redirect
From looking at skb_do_redirect, the skb_do_redirect should have consumed the
skb except for the -EAGAIN return value. afaik, -EAGAIN could only happen by using the bpf_redirect_peer helper. lwt does not have the bpf_redirect_peer helper available, so there is no -EAGAIN case in lwt. iow, skb_do_redirect should have always consumed the skb in lwt. or did I miss something?
Quoted 8 lines
If that is the case, it feels like the fix should be in run_lwt_bpf() and the "if (ret == 0)" test in run_lwt_bpf() is unnecessary?
                         ret = skb_do_redirect(skb);
                         if (ret == 0)
                                 ret = BPF_REDIRECT;

Quoted 6 lines

Just fixing skb redirect return code won't be sufficient. I realized there are other return paths that need to be treated, e.g. bpf reroute path also directly returns dev_queue_xmit status. I plan to check for LWTUNNEL_XMIT_CONTINUE (and change it to a value that does not conflict with NET_RX_DROP and NET_XMIT_DROP) in the next revision. On the other hand, the return value of NETDEV_TX_BUSY is another hassle.

I suspect we are talking about different things or I am still missing something.

I was thinking skb_do_redirect() should have always consumed the skb and bpf_xmit should always return LWTUNNEL_XMIT_DONE also (instead of LWTUNNEL_XMIT_CONTINUE described in the this patch commit message). It is what sch_handle_egress() is doing also. Could you explain how is it different from the skb_do_redirect usage in sch_handle_egress() or you are suggesting the current sch_handle_egress() has the issue too also?

Quoted 5 lines

As Dan suggested, packets might not have been freed when this is returned from drivers. The caller of dev_queue_xmit might need to free skb when this happens.
Yan

Yan Zhai

author

2023-07-31 23:02 UTC #15 lore raw

Re: [PATCH v4 bpf 1/2] bpf: fix skb_do_redirect return values

Quoted 37 lines · Martin KaFai Lau

Quoted 28 lines · Yan Zhai
Quoted 19 lines · Martin KaFai Lau
Quoted 10 lines · Yan Zhai
skb_do_redirect returns various of values: error code (negative), 0 (success), and some positive status code, e.g. NET_XMIT_CN, NET_RX_DROP. Commit 3a0af8fd61f9 ("bpf: BPF for lightweight tunnel infrastructure") didn't check the return code correctly, so positive values are propagated back along call chain:
    ip_finish_output2
      -> bpf_xmit
        -> run_lwt_bpf
          -> skb_do_redirect
From looking at skb_do_redirect, the skb_do_redirect should have consumed the
skb except for the -EAGAIN return value. afaik, -EAGAIN could only happen by using the bpf_redirect_peer helper. lwt does not have the bpf_redirect_peer helper available, so there is no -EAGAIN case in lwt. iow, skb_do_redirect should have always consumed the skb in lwt. or did I miss something?
Quoted 8 lines
If that is the case, it feels like the fix should be in run_lwt_bpf() and the "if (ret == 0)" test in run_lwt_bpf() is unnecessary?
                         ret = skb_do_redirect(skb);
                         if (ret == 0)
                                 ret = BPF_REDIRECT;
Quoted 6 lines
Just fixing skb redirect return code won't be sufficient. I realized there are other return paths that need to be treated, e.g. bpf reroute path also directly returns dev_queue_xmit status. I plan to check for LWTUNNEL_XMIT_CONTINUE (and change it to a value that does not conflict with NET_RX_DROP and NET_XMIT_DROP) in the next revision. On the other hand, the return value of NETDEV_TX_BUSY is another hassle.

Quoted 9 lines

I suspect we are talking about different things or I am still missing something.
I was thinking skb_do_redirect() should have always consumed the skb and bpf_xmit should always return LWTUNNEL_XMIT_DONE also (instead of LWTUNNEL_XMIT_CONTINUE described in the this patch commit message). It is what sch_handle_egress() is doing also. Could you explain how is it different from the skb_do_redirect usage in sch_handle_egress() or you are suggesting the current sch_handle_egress() has the issue too also?

I think we were not on the same page. You are absolutely right that skb_do_redirect should consume the packet anyway. The difference between your proposal and this patch is that this patch returns errno or LWTUNNEL_XMIT_DONE, and yours does not even return errno. Both approaches fix the issue of "redirect to down device crashes the kernel".

What I commented was an exact same issue at different location: BPF reroute may trigger the crash as well, since it also returns dev_queue_xmit status in bpf_xmit. Need to fix this, or instead fixing LWTUNNEL_XMIT_CONTINUE value and correct the behavior at lwtunnel_xmit rather than bpf_xmit.

Yan

Quoted 7 lines

Quoted 6 lines
As Dan suggested, packets might not have been freed when this is returned from drivers. The caller of dev_queue_xmit might need to free skb when this happens.
Yan

Martin KaFai Lau

reviewer

2023-07-31 23:52 UTC #16 lore raw

Re: [PATCH v4 bpf 1/2] bpf: fix skb_do_redirect return values

Quoted 5 lines · Yan Zhai

What I commented was an exact same issue at different location: BPF reroute may trigger the crash as well, since it also returns dev_queue_xmit status in bpf_xmit. Need to fix this, or instead fixing LWTUNNEL_XMIT_CONTINUE value and correct the behavior at lwtunnel_xmit rather than bpf_xmit.

Ah. I think I got it. You meant the bpf_lwt_xmit_reroute() / BPF_LWT_REROUTE case? It would be clearer if some of these names were quoted instead. "reroute" could mean many things.

Please put details comment in v5. Thanks.

Quoted 10 lines

Yan
Quoted 7 lines
Quoted 6 lines
As Dan suggested, packets might not have been freed when this is returned from drivers. The caller of dev_queue_xmit might need to free skb when this happens.
Yan

Yan Zhai

author

2023-07-26 01:09 UTC #17 lore raw

PATCH v4 2/2 bpf: selftests: add lwt redirect regression test cases

In reply to Yan Zhai

Tests BPF redirect at the lwt xmit hook to ensure error handling are safe, i.e. won't panic the kernel.

Tested-by Jakub Sitnicki <[email protected]>

Signed-off-by Yan Zhai <[email protected]>

---
 tools/testing/selftests/bpf/Makefile          |   1 +
 .../selftests/bpf/progs/test_lwt_redirect.c   |  66 +++++++
 .../selftests/bpf/test_lwt_redirect.sh        | 174 ++++++++++++++++++
 3 files changed, 241 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/test_lwt_redirect.c
 create mode 100755 tools/testing/selftests/bpf/test_lwt_redirect.sh

tools/testing/selftests/bpf/Makefile +1 -0

index 538df8fb8c42..e3a24d053793 100644
@@ -66,6 +66,7 @@ TEST_PROGS := test_kmod.sh \
 	test_xdp_vlan_mode_generic.sh \
 	test_xdp_vlan_mode_native.sh \
 	test_lwt_ip_encap.sh \
+	test_lwt_redirect.sh \
 	test_tcp_check_syncookie.sh \
 	test_tc_tunnel.sh \
 	test_tc_edt.sh \

tools/testing/selftests/bpf/progs/test_lwt_redirect.c new file +66 -0

66 lines not on this page · raw message

tools/testing/selftests/bpf/test_lwt_redirect.sh new file +174 -0

174 lines not on this page · raw message

Signature · 1 line

2.30.2

Markus Elfring

2023-07-26 08:10 UTC #18 lore raw

Re: [PATCH v4 bpf 2/2] bpf: selftests: add lwt redirect regression test cases

Quoted 2 lines

Tests BPF redirect at the lwt xmit hook to ensure error handling are safe, i.e. won't panic the kernel.

Are imperative change descriptions still preferred?

Can remaining wording weaknesses be adjusted accordingly?

Regards,
Markus

A message that is not in the archive (sent off-list or not yet received): CAO3-PbraNcfQnqHUG_992vssuA795RxtexYsMdEo=k9zp-XHog@mail.gmail.com

Yan Zhai

author

2023-07-26 10:30 UTC #20 lore raw

Re: [PATCH v4 bpf 2/2] bpf: selftests: add lwt redirect regression test cases

Apologize for sending previous mail from a wrong app (not text mode). Resending to keep the mailing list thread consistent.

Quoted 5 lines · Markus Elfring

Quoted 3 lines
Tests BPF redirect at the lwt xmit hook to ensure error handling are safe, i.e. won't panic the kernel.
Are imperative change descriptions still preferred?

Hi Markus,

I think you linked this to me yesterday that it should be described
imperatively:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.5-rc3#n155

Quoted 5 lines

See also:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.5-rc3#n94

I don’t follow the purpose of this reference. This points to user impact but this is a selftest, so I don’t see any user impact here. Or is there anything I missed?

Quoted 2 lines

Can remaining wording weaknesses be adjusted accordingly?

I am not following this question . Can you be more specific or provide an example?

Yan

Quoted 4 lines

Regards,
Markus

Dan Carpenter

2023-07-26 13:22 UTC #21 lore raw

Re: [PATCH v4 bpf 2/2] bpf: selftests: add lwt redirect regression test cases

Was Markus unbanned from vger? How are we recieving these emails?

regards,
dan carpenter

Markus Elfring

2023-07-26 11:05 UTC #22 lore raw

Re: [v4 bpf 2/2] bpf: selftests: add lwt redirect regression test cases

In reply to a message not in the archive

Quoted 4 lines

See also:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.5-rc3#n94
I don’t follow the purpose of this reference.

You picked a corresponding advice up for the first update step of this patch series.

Quoted 1 line

This points to user impact but this is a selftest,

The linked document contains descriptions for general Linux development requirements, doesn't it?

Quoted 1 line

so I don’t see any user impact here.

Please reconsider such a view.

Quoted 1 line

Or is there anything I missed?

Yes.

Can you adhere to published guidelines at any more places?

Quoted 3 lines

Can remaining wording weaknesses be adjusted accordingly?
I am not following this question . Can you be more specific or provide an example?

I interpret word combinations (like the following) also as update candidates.

* lwt redirect regression

* handling are

Regards,
Markus

Jakub Sitnicki

Reviewed-by on v4 2/2

2023-07-26 12:27 UTC #23 lore raw

Re: [PATCH v4 bpf 2/2] bpf: selftests: add lwt redirect regression test cases

In reply to Yan Zhai

Quoted 7 lines · Yan Zhai

Tests BPF redirect at the lwt xmit hook to ensure error handling are safe, i.e. won't panic the kernel.
Tested-by Jakub Sitnicki <[email protected]>
Suggested-by Alexei Starovoitov <[email protected]>
Signed-off-by Yan Zhai <[email protected]>
---

Reviewed-by Jakub Sitnicki <[email protected]>

Martin KaFai Lau

reviewer

2023-07-28 22:47 UTC #24 lore raw

Re: [PATCH v4 bpf 2/2] bpf: selftests: add lwt redirect regression test cases

In reply to Yan Zhai

Quoted 7 lines · Yan Zhai

tools/testing/selftests/bpf/progs/test_lwt_redirect.c new file +1 -0
new file mode 100644
index 000000000000..3674e101f68f
@@ -0,0 +1,66 @@
+// SPDX-License-Identifier: GPL-2.0

tools/testing/selftests/bpf/progs/test_lwt_redirect.c line 11

Quoted 10 lines

tools/testing/selftests/bpf/progs/test_lwt_redirect.c new file +10 -0

+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+#include "bpf_tracing_net.h"
+
+/* We don't care about whether the packet can be received by network stack.
+ * Just care if the packet is sent to the correct device at correct direction
+ * and not panic the kernel.
+ */
+static __always_inline int prepend_dummy_mac(struct __sk_buff *skb)
+{

__always_inline is no longer a must for a long time.

tools/testing/selftests/bpf/progs/test_lwt_redirect.c line 16

Quoted 5 lines

tools/testing/selftests/bpf/progs/test_lwt_redirect.c new file +5 -0

+	char mac[] = {0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0xf,
+		      0xe, 0xd, 0xc, 0xb, 0xa, 0x08, 0x00};
+
+	if (bpf_skb_change_head(skb, ETH_HLEN, 0)) {
+		bpf_printk("%s: fail to change head", __func__);

Avoid using bpf_printk(). The bpf CI runs other tests also.

tools/testing/selftests/bpf/progs/test_lwt_redirect.c line 28

Quoted 12 lines

tools/testing/selftests/bpf/progs/test_lwt_redirect.c new file +12 -0

+		return -1;
+	}
+
+	if (bpf_skb_store_bytes(skb, 0, mac, sizeof(mac), 0)) {
+		bpf_printk("%s: fail to update mac", __func__);
+		return -1;
+	}
+
+	return 0;
+}
+
+SEC("redir_ingress")

Use SEC("lwt_xmit"). Then the libbpf will figure out the prog type.

tools/testing/selftests/bpf/test_lwt_redirect.sh

Quoted 43 lines

tools/testing/selftests/bpf/progs/test_lwt_redirect.c new file +38 -0

+int test_lwt_redirect_in(struct __sk_buff *skb)
+{
+	if (prepend_dummy_mac(skb))
+		return BPF_DROP;
+
+	bpf_printk("Redirect skb to link %d ingress", skb->mark);
+	return bpf_redirect(skb->mark, BPF_F_INGRESS);
+}
+
+SEC("redir_egress")
+int test_lwt_redirect_out(struct __sk_buff *skb)
+{
+	if (prepend_dummy_mac(skb))
+		return BPF_DROP;
+
+	bpf_printk("Redirect skb to link %d egress", skb->mark);
+	return bpf_redirect(skb->mark, 0);
+}
+
+SEC("redir_egress_nomac")
+int test_lwt_redirect_out_nomac(struct __sk_buff *skb)
+{
+	int ret = bpf_redirect(skb->mark, 0);
+
+	bpf_printk("Redirect skb to link %d egress nomac: %d", skb->mark, ret);
+	return ret;
+}
+
+SEC("redir_ingress_nomac")
+int test_lwt_redirect_in_nomac(struct __sk_buff *skb)
+{
+	int ret = bpf_redirect(skb->mark, BPF_F_INGRESS);
+
+	bpf_printk("Redirect skb to link %d ingress nomac: %d", skb->mark, ret);
+	return ret;
+}
+
+char _license[] SEC("license") = "GPL";

tools/testing/selftests/bpf/test_lwt_redirect.sh new file +0 -0

new file mode 100755
index 000000000000..1b7b78b48174

This has to be written in the test_progs infrastructure in C. Only test_progs is run by the BPF CI. Take a look at other tests in prog_tests/. For example, tc_redirect.c and xdp_metadata.c which are having setup in netns/link/...etc. It currently has helpers to add tc qdisc and filter but not adding route yet which could be a useful addition.

--
pw-bot: cr

Jakub Sitnicki

2023-07-31 09:53 UTC #25 lore raw

Re: [PATCH v4 bpf 2/2] bpf: selftests: add lwt redirect regression test cases

Quoted 1 line · Martin KaFai Lau

On 7/25/23 6:09 PM, Yan Zhai wrote:

[...]

Quoted 4 lines

Quoted 4 lines
tools/testing/selftests/bpf/test_lwt_redirect.sh new file +0 -0
new file mode 100755
index 000000000000..1b7b78b48174

Quoted 8 lines

tools/testing/selftests/bpf/test_lwt_redirect.sh
Quoted 2 lines
tools/testing/selftests/bpf/test_lwt_redirect.sh new file +0 -0
This has to be written in the test_progs infrastructure in C. Only test_progs is run by the BPF CI. Take a look at other tests in prog_tests/. For example, tc_redirect.c and xdp_metadata.c which are having setup in netns/link/...etc. It currently has helpers to add tc qdisc and filter but not adding route yet which could be a useful addition.

Can we help make the BPF CI better so that it also runs other tests in addition test_progs?

We have bpf selftests written in shell and even Python. These are sometimes the right tools for the job and make adding tests easier, IMHO. Network setup from C is verbose and tedious. Not to mention, hard to read through.

# ./run_kselftest.sh --list
bpf:test_verifier
bpf:test_tag
bpf:test_maps
bpf:test_lru_map
bpf:test_lpm_map
bpf:test_progs
bpf:test_dev_cgroup
bpf:test_sock
bpf:test_sockmap
bpf:get_cgroup_id_user
bpf:test_cgroup_storage
bpf:test_tcpnotify_user
bpf:test_sysctl
bpf:test_progs-no_alu32
bpf:test_kmod.sh
bpf:test_xdp_redirect.sh
bpf:test_xdp_redirect_multi.sh
bpf:test_xdp_meta.sh
bpf:test_xdp_veth.sh
bpf:test_offload.py
bpf:test_sock_addr.sh
bpf:test_tunnel.sh
bpf:test_lwt_seg6local.sh
bpf:test_lirc_mode2.sh
bpf:test_skb_cgroup_id.sh
bpf:test_flow_dissector.sh
bpf:test_xdp_vlan_mode_generic.sh
bpf:test_xdp_vlan_mode_native.sh
bpf:test_lwt_ip_encap.sh
bpf:test_tcp_check_syncookie.sh
bpf:test_tc_tunnel.sh
bpf:test_tc_edt.sh
bpf:test_xdping.sh
bpf:test_bpftool_build.sh
bpf:test_bpftool.sh
bpf:test_bpftool_metadata.sh
bpf:test_doc_build.sh
bpf:test_xsk.sh
bpf:test_xdp_features.sh

Alexei Starovoitov

2023-07-31 18:47 UTC #26 lore raw

Re: [PATCH v4 bpf 2/2] bpf: selftests: add lwt redirect regression test cases

Quoted 19 lines · Jakub Sitnicki

Quoted 1 line · Martin KaFai Lau
On 7/25/23 6:09 PM, Yan Zhai wrote:
[...]
Quoted 4 lines
Quoted 4 lines
tools/testing/selftests/bpf/test_lwt_redirect.sh new file +0 -0
new file mode 100755
index 000000000000..1b7b78b48174
Quoted 8 lines
tools/testing/selftests/bpf/test_lwt_redirect.sh
Quoted 2 lines
tools/testing/selftests/bpf/test_lwt_redirect.sh new file +0 -0
This has to be written in the test_progs infrastructure in C. Only test_progs is run by the BPF CI. Take a look at other tests in prog_tests/. For example, tc_redirect.c and xdp_metadata.c which are having setup in netns/link/...etc. It currently has helpers to add tc qdisc and filter but not adding route yet which could be a useful addition.

Quoted 2 lines

Can we help make the BPF CI better so that it also runs other tests in addition test_progs?

Not really.
CI is not just running the test. It needs to understand the output, pass it to UI, run in parallel, etc.
All the shell scripts are not suitable for long term CI exposure.

So I completely agree with Martin. No new shell scripts. All selftests must be in test_progs.

Quoted 4 lines

We have bpf selftests written in shell and even Python. These are sometimes the right tools for the job and make adding tests easier, IMHO. Network setup from C is verbose and tedious. Not to mention, hard to read through.

For comparison take a look at BPF CI code base and what it takes to run the tests and process the output. There is plenty of work for CI ahead.