bpf: return proper error codes for lwt redirect

PATCH Superseded Yan Zhai 4 participants 10 messages Jul 25, 2023

Subsystems (7)

Networking Network drivers Kselftest Kernel janitors +3 more subsystems

lore

Share & tools

lore

Yan Zhai

author

2023-07-25 04:12 UTC #0 lore raw

v3 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.

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

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

 net/core/filter.c                             |  12 +-
 tools/testing/selftests/bpf/Makefile          |   1 +
 .../selftests/bpf/progs/test_lwt_redirect.c   |  67 +++++++
 .../selftests/bpf/test_lwt_redirect.sh        | 165 ++++++++++++++++++
 4 files changed, 244 insertions(+), 1 deletion(-)
 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-25 04:13 UTC #1 lore raw

PATCH v3 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. Such code are not handled at lwt xmit hook in function ip_finish_output2 and ip6_finish_output, which can cause unexpected problems. This change converts the positive status code to proper error code.

Reported-by Jordan Griege <[email protected]>

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

---
v3: converts also RX side return value in addition to TX values
v2: code style change suggested by Stanislav Fomichev

---
 net/core/filter.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

net/core/filter.c +11 -1

index 06ba0e56e369..3e232ce11ca0 100644
@@ -2095,7 +2095,12 @@ 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);
+
+	if (unlikely(ret > 0))
+		return -ENETDOWN;
+
+	return 0;
 }
 
 static inline int __bpf_rx_skb_no_mac(struct net_device *dev,

@@ -2106,6 +2111,8 @@ static inline int __bpf_rx_skb_no_mac(struct net_device *dev,
 	if (likely(!ret)) {
 		skb->dev = dev;
 		ret = netif_rx(skb);
+	} else if (ret > 0) {
+		return -ENETDOWN;
 	}
 
 	return ret;

@@ -2129,6 +2136,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-25 05:11 UTC #2 lore raw

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

Quoted 2 lines

… unexpected problems. This change
converts the positive status code to proper error code.

Please choose a corresponding imperative change suggestion.

Did you provide sufficient justification for a possible addition of the tag “Fixes”?

…

Quoted 3 lines

v2: code style change suggested by Stanislav Fomichev
---
 net/core/filter.c | 12 +++++++++++-

…

How do you think about to replace this marker by a line break?

Regards,
Markus

Yan Zhai

author

2023-07-25 14:41 UTC #3 lore raw

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

Quoted 19 lines · Markus Elfring

Quoted 3 lines
… unexpected problems. This change
converts the positive status code to proper error code.
Please choose a corresponding imperative change suggestion.
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
Did you provide sufficient justification for a possible addition of the tag “Fixes”?
…
Quoted 3 lines
v2: code style change suggested by Stanislav Fomichev
---
 net/core/filter.c | 12 +++++++++++-
…

Quoted 7 lines

How do you think about to replace this marker by a line break?
See also:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.5-rc3#n711
Regards,
Markus

Hi Markus,

Thanks for the suggestions, those are what I could use more help with. Will address these in the next version.

Yan

Jakub Sitnicki

2023-07-25 09:14 UTC #4 lore raw

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

In reply to Yan Zhai

Quoted 45 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. Such code are not handled at lwt xmit hook in function ip_finish_output2 and ip6_finish_output, which can cause unexpected problems. This change converts the positive status code to proper error code.
Suggested-by Stanislav Fomichev <[email protected]>
Reported-by Jordan Griege <[email protected]>
Signed-off-by Yan Zhai <[email protected]>
---
v3: converts also RX side return value in addition to TX values
v2: code style change suggested by Stanislav Fomichev
---
 net/core/filter.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)
net/core/filter.c +8 -1
index 06ba0e56e369..3e232ce11ca0 100644
@@ -2095,7 +2095,12 @@ 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);
+
+	if (unlikely(ret > 0))
+		return -ENETDOWN;
+
+	return 0;
 }
 
 static inline int __bpf_rx_skb_no_mac(struct net_device *dev,
@@ -2106,6 +2111,8 @@ static inline int __bpf_rx_skb_no_mac(struct net_device *dev,
 	if (likely(!ret)) {
 		skb->dev = dev;
 		ret = netif_rx(skb);
+	} else if (ret > 0) {
+		return -ENETDOWN;
 	}
 
 	return ret;

net/core/filter.c static inline int __bpf_tx_skb(struct net_device *dev, struct sk_buff *skb) · line 2143

Quoted 8 lines

net/core/filter.c +3 -0

@@ -2129,6 +2136,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;
 }

net_xmit_errno maps NET_XMIT_DROP to -ENOBUFS. It would make sense to me to map NET_RX_DROP to -ENOBUFS as well, instead of -ENETDOWN, to be consistent.

It looks like the Fixes tag for this should point to the change that introduced BPF for LWT:

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

Yan Zhai

author

2023-07-25 16:05 UTC #5 lore raw

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

Quoted 50 lines · Jakub Sitnicki

Quoted 48 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. Such code are not handled at lwt xmit hook in function ip_finish_output2 and ip6_finish_output, which can cause unexpected problems. This change converts the positive status code to proper error code.
Suggested-by Stanislav Fomichev <[email protected]>
Reported-by Jordan Griege <[email protected]>
Signed-off-by Yan Zhai <[email protected]>
---
v3: converts also RX side return value in addition to TX values
v2: code style change suggested by Stanislav Fomichev
---
 net/core/filter.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)
net/core/filter.c +8 -1
index 06ba0e56e369..3e232ce11ca0 100644
@@ -2095,7 +2095,12 @@ 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);
+
+     if (unlikely(ret > 0))
+             return -ENETDOWN;
+
+     return 0;
 }
 
 static inline int __bpf_rx_skb_no_mac(struct net_device *dev,
@@ -2106,6 +2111,8 @@ static inline int __bpf_rx_skb_no_mac(struct net_device *dev,
      if (likely(!ret)) {
              skb->dev = dev;
              ret = netif_rx(skb);
+     } else if (ret > 0) {
+             return -ENETDOWN;
      }
 
      return ret;
@@ -2129,6 +2136,9 @@ static inline int __bpf_tx_skb(struct net_device *dev, struct sk_buff *skb)
      ret = dev_queue_xmit(skb);
      dev_xmit_recursion_dec();
 

Quoted 10 lines

net/core/filter.c static inline int __bpf_tx_skb(struct net_device *dev, struct sk_buff *skb) · line 2143
Quoted 5 lines
net/core/filter.c +3 -0
+     if (unlikely(ret > 0))
+             ret = net_xmit_errno(ret);
+
      return ret;
 }
net_xmit_errno maps NET_XMIT_DROP to -ENOBUFS. It would make sense to me to map NET_RX_DROP to -ENOBUFS as well, instead of -ENETDOWN, to be consistent.

In fact I looked at all those errno, but found none actually covers this situation completely. For the redirect ingress case, there are three reasons to fail: backlog full, dev down, and MTU issue. This won't be a problem for typical RX paths, since the error code is usually discarded by call sites of deliver_skb. But redirect ingress opens a call chain that would propagate this error to local sendmsg, which may be very confusing to troubleshoot in a complex environment (especially when backlog fills).

That said I agree ENOBUF covers the most likely reason to fail (backlog). Let me change to that one in the next version if there are no new suggestions.

Quoted 5 lines

It looks like the Fixes tag for this should point to the change that introduced BPF for LWT:
Fixes 3a0af8fd61f9 ("bpf: BPF for lightweight tunnel infrastructure")

Thanks for finding the tag. I was debating if it should be LWT commit or bpf_redirect commit: the error is not handled at LWT, but it seems actually innocent. The actual fix is the return value from the bpf redirect code. Let me incorporate both in the next one to justify better.

--
Yan

Stanislav Fomichev

2023-07-25 17:07 UTC #6 lore raw

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

Quoted 70 lines · Yan Zhai

Quoted 50 lines · Jakub Sitnicki
Quoted 48 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. Such code are not handled at lwt xmit hook in function ip_finish_output2 and ip6_finish_output, which can cause unexpected problems. This change converts the positive status code to proper error code.
Suggested-by Stanislav Fomichev <[email protected]>
Reported-by Jordan Griege <[email protected]>
Signed-off-by Yan Zhai <[email protected]>
---
v3: converts also RX side return value in addition to TX values
v2: code style change suggested by Stanislav Fomichev
---
 net/core/filter.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)
net/core/filter.c +8 -1
index 06ba0e56e369..3e232ce11ca0 100644
@@ -2095,7 +2095,12 @@ 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);
+
+     if (unlikely(ret > 0))
+             return -ENETDOWN;
+
+     return 0;
 }
 
 static inline int __bpf_rx_skb_no_mac(struct net_device *dev,
@@ -2106,6 +2111,8 @@ static inline int __bpf_rx_skb_no_mac(struct net_device *dev,
      if (likely(!ret)) {
              skb->dev = dev;
              ret = netif_rx(skb);
+     } else if (ret > 0) {
+             return -ENETDOWN;
      }
 
      return ret;
@@ -2129,6 +2136,9 @@ static inline int __bpf_tx_skb(struct net_device *dev, struct sk_buff *skb)
      ret = dev_queue_xmit(skb);
      dev_xmit_recursion_dec();
 
Quoted 10 lines
net/core/filter.c static inline int __bpf_tx_skb(struct net_device *dev, struct sk_buff *skb) · line 2143
Quoted 5 lines
net/core/filter.c +3 -0
+     if (unlikely(ret > 0))
+             ret = net_xmit_errno(ret);
+
      return ret;
 }
net_xmit_errno maps NET_XMIT_DROP to -ENOBUFS. It would make sense to me to map NET_RX_DROP to -ENOBUFS as well, instead of -ENETDOWN, to be consistent.
In fact I looked at all those errno, but found none actually covers this situation completely. For the redirect ingress case, there are three reasons to fail: backlog full, dev down, and MTU issue. This won't be a problem for typical RX paths, since the error code is usually discarded by call sites of deliver_skb. But redirect ingress opens a call chain that would propagate this error to local sendmsg, which may be very confusing to troubleshoot in a complex environment (especially when backlog fills).

Quoted 3 lines

That said I agree ENOBUF covers the most likely reason to fail (backlog). Let me change to that one in the next version if there are no new suggestions.

nit: also maybe wrap these rx paths into some new net_rx_errno ? To mirror the tx side.

Quoted 13 lines

Quoted 5 lines
It looks like the Fixes tag for this should point to the change that introduced BPF for LWT:
Fixes 3a0af8fd61f9 ("bpf: BPF for lightweight tunnel infrastructure")
Thanks for finding the tag. I was debating if it should be LWT commit or bpf_redirect commit: the error is not handled at LWT, but it seems actually innocent. The actual fix is the return value from the bpf redirect code. Let me incorporate both in the next one to justify better.
--
Yan

Yan Zhai

author

2023-07-25 04:14 UTC #7 lore raw

PATCH v3 2/2 selftests/bpf: test lwt redirect error handling

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.

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

---
 tools/testing/selftests/bpf/Makefile          |   1 +
 .../selftests/bpf/progs/test_lwt_redirect.c   |  67 +++++++
 .../selftests/bpf/test_lwt_redirect.sh        | 165 ++++++++++++++++++
 3 files changed, 233 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 +67 -0

67 lines not on this page · raw message

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

165 lines not on this page · raw message

Signature · 1 line

2.30.2

Jakub Sitnicki

2023-07-25 13:20 UTC #8 lore raw

Re: [PATCH v3 bpf 2/2] selftests/bpf: test lwt redirect error handling

Quoted 31 lines · Yan Zhai

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

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

---
 tools/testing/selftests/bpf/Makefile          |   1 +
 .../selftests/bpf/progs/test_lwt_redirect.c   |  67 +++++++
 .../selftests/bpf/test_lwt_redirect.sh        | 165 ++++++++++++++++++
 3 files changed, 233 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 +0 -0

new file mode 100644
index 000000000000..622c6b1e7128

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

Quoted 5 lines

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

@@ -0,0 +1,67 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+
+#define ETH_LEN 14

Nit: We have ETH_HLEN in bpf_tracing_net.h defined.

[...]

Jakub Sitnicki

Tested-by on the v3 series

2023-07-25 13:21 UTC #9 lore raw

In reply to Yan Zhai

Quoted 26 lines · Yan Zhai

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.
v2: https://lore.kernel.org/netdev/[email protected]/
v1: https://lore.kernel.org/bpf/[email protected]/
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

Quoted 7 lines

 net/core/filter.c                             |  12 +-
 tools/testing/selftests/bpf/Makefile          |   1 +
 .../selftests/bpf/progs/test_lwt_redirect.c   |  67 +++++++
 .../selftests/bpf/test_lwt_redirect.sh        | 165 ++++++++++++++++++
 4 files changed, 244 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/bpf/progs/test_lwt_redirect.c
 create mode 100755 tools/testing/selftests/bpf/test_lwt_redirect.sh

For the series:

Tested-by Jakub Sitnicki <[email protected]>