[PATCH] net: core: set skb useful vars in __bpf_tx_skb

From: Tonghao Zhang <redacted>

We may use bpf_redirect to redirect the packets to other
netdevice (e.g. ifb) in ingress and egress path.

The target netdevice may check the *skb_iif, *redirected
and *from_ingress, for example, if skb_iif or redirected
is 0, ifb will drop the packets.

Fixes: a70b506efe89 ("bpf: enforce recursion limit on redirects")
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Tonghao Zhang <redacted>
---
 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 4bace37a6a44..2dbff0944768 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c

@@ -2107,9 +2107,19 @@ static inline int __bpf_tx_skb(struct net_device *dev, struct sk_buff *skb)
 		return -ENETDOWN;
 	}
 
-	skb->dev = dev;
+	/* The target netdevice (e.g. ifb) may use the:
+	 * - skb_iif
+	 * - redirected
+	 * - from_ingress
+	 */
+	skb->skb_iif = skb->dev->ifindex;
+#ifdef CONFIG_NET_CLS_ACT
+	skb_set_redirected(skb, skb->tc_at_ingress);
+#else
 	skb->tstamp = 0;
+#endif
 
+	skb->dev = dev;
 	dev_xmit_recursion_inc();
 	ret = dev_queue_xmit(skb);
 	dev_xmit_recursion_dec();

-- 
2.27.0

On 10/29/21 3:54 AM, xiangxia.m.yue@gmail.com wrote:
[...]

This doesn't look right to me to set it unconditionally in tx path, isn't ifb_ri_tasklet()
setting skb->skb_iif in this case (or __netif_receive_skb_core() in main rx path)?

Also, I would suggest to add a proper BPF selftest which outlines the issue you're solving
in here.

+#ifdef CONFIG_NET_CLS_ACT
+	skb_set_redirected(skb, skb->tc_at_ingress);
+#else
  	skb->tstamp = 0;
+#endif
  
+	skb->dev = dev;
  	dev_xmit_recursion_inc();
  	ret = dev_queue_xmit(skb);
  	dev_xmit_recursion_dec();

On Tue, Nov 2, 2021 at 5:46 AM Daniel Borkmann [off-list ref] wrote:

On 10/29/21 3:54 AM, xiangxia.m.yue@gmail.com wrote:
[...]

quoted

diff --git a/net/core/filter.c b/net/core/filter.c
index 4bace37a6a44..2dbff0944768 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c

@@ -2107,9 +2107,19 @@ static inline int __bpf_tx_skb(struct net_device *dev, struct sk_buff *skb)
              return -ENETDOWN;
      }

-     skb->dev = dev;
+     /* The target netdevice (e.g. ifb) may use the:
+      * - skb_iif
+      * - redirected
+      * - from_ingress
+      */
+     skb->skb_iif = skb->dev->ifindex;

This doesn't look right to me to set it unconditionally in tx path, isn't ifb_ri_tasklet()
setting skb->skb_iif in this case (or __netif_receive_skb_core() in main rx path)?

Hi
the act_mirred set the skb->skb_iif, redirected and from_ingress. and
__netif_receive_skb_core also set skb->skb_iif.
so we can use the act_mirred to ifb in ingress or egress path.
For ingress, when we use the bpf_redirct to ifb, we should set
redirected, and from_ingress.
For egress, when we use the bpf_redirct to ifb, we should skb_iif ,
set redirected, and from_ingress.

Also, I would suggest to add a proper BPF selftest which outlines the issue you're solving
in here.

Ok, thanks.

quoted

+#ifdef CONFIG_NET_CLS_ACT
+     skb_set_redirected(skb, skb->tc_at_ingress);
+#else
      skb->tstamp = 0;
+#endif

+     skb->dev = dev;
      dev_xmit_recursion_inc();
      ret = dev_queue_xmit(skb);
      dev_xmit_recursion_dec();

-- 
Best regards, Tonghao

On Tue, Nov 2, 2021 at 9:58 AM Tonghao Zhang [off-list ref] wrote:

On Tue, Nov 2, 2021 at 5:46 AM Daniel Borkmann [off-list ref] wrote:

quoted

On 10/29/21 3:54 AM, xiangxia.m.yue@gmail.com wrote:
[...]

quoted

diff --git a/net/core/filter.c b/net/core/filter.c
index 4bace37a6a44..2dbff0944768 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c

@@ -2107,9 +2107,19 @@ static inline int __bpf_tx_skb(struct net_device *dev, struct sk_buff *skb)
              return -ENETDOWN;
      }

-     skb->dev = dev;
+     /* The target netdevice (e.g. ifb) may use the:
+      * - skb_iif
+      * - redirected
+      * - from_ingress
+      */
+     skb->skb_iif = skb->dev->ifindex;

This doesn't look right to me to set it unconditionally in tx path, isn't ifb_ri_tasklet()
setting skb->skb_iif in this case (or __netif_receive_skb_core() in main rx path)?

Hi
the act_mirred set the skb->skb_iif, redirected and from_ingress. and
__netif_receive_skb_core also set skb->skb_iif.
so we can use the act_mirred to ifb in ingress or egress path.
For ingress, when we use the bpf_redirct to ifb, we should set
redirected, and from_ingress.
For egress, when we use the bpf_redirct to ifb, we should skb_iif ,
set redirected, and from_ingress.

Hi Daniel,
because we don't know bpf_redirct invoked in tx path, or rx path.
can we set the  skb->skb_iif unconditionally in bpf_redirct? any thoughts?

quoted

Also, I would suggest to add a proper BPF selftest which outlines the issue you're solving
in here.

Ok, thanks.

quoted

quoted

+#ifdef CONFIG_NET_CLS_ACT
+     skb_set_redirected(skb, skb->tc_at_ingress);
+#else
      skb->tstamp = 0;
+#endif

+     skb->dev = dev;
      dev_xmit_recursion_inc();
      ret = dev_queue_xmit(skb);
      dev_xmit_recursion_dec();

--
Best regards, Tonghao

-- 
Best regards, Tonghao