On Tue, Jul 25, 2023 at 4:14 AM Jakub Sitnicki jakub@cloudflare.com wrote:
On Mon, Jul 24, 2023 at 09:13 PM -07, Yan Zhai wrote:
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 sdf@google.com Reported-by: Jordan Griege jgriege@cloudflare.com Signed-off-by: Yan Zhai yan@cloudflare.com
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(-)
diff --git a/net/core/filter.c b/net/core/filter.c index 06ba0e56e369..3e232ce11ca0 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -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;}
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.
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