diff mbox series

[bpf-next,V8,6/8] bpf: make it possible to identify BPF redirected SKBs

Message ID 160650040800.2890576.9811290366501747109.stgit@firesoul
State New
Headers show
Series [bpf-next,V8,1/8] bpf: Remove MTU check in __bpf_skb_max_len | expand

Commit Message

Jesper Dangaard Brouer Nov. 27, 2020, 6:06 p.m. UTC
This change makes it possible to identify SKBs that have been redirected
by TC-BPF (cls_act). This is needed for a number of cases.

(1) For collaborating with driver ifb net_devices.
(2) For avoiding starting generic-XDP prog on TC ingress redirect.

It is most important to fix XDP case(2), because this can break userspace
when a driver gets support for native-XDP. Imagine userspace loads XDP
prog on eth0, which fallback to generic-XDP, and it process TC-redirected
packets. When kernel is updated with native-XDP support for eth0, then the
program no-longer see the TC-redirected packets. Therefore it is important
to keep the order intact; that XDP runs before TC-BPF.

Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
 net/core/dev.c    |    2 ++
 net/sched/Kconfig |    1 +
 2 files changed, 3 insertions(+)

Comments

Daniel Borkmann Dec. 3, 2020, 12:06 a.m. UTC | #1
On 11/27/20 7:06 PM, Jesper Dangaard Brouer wrote:
> This change makes it possible to identify SKBs that have been redirected

> by TC-BPF (cls_act). This is needed for a number of cases.

> 

> (1) For collaborating with driver ifb net_devices.

> (2) For avoiding starting generic-XDP prog on TC ingress redirect.

> 

> It is most important to fix XDP case(2), because this can break userspace

> when a driver gets support for native-XDP. Imagine userspace loads XDP

> prog on eth0, which fallback to generic-XDP, and it process TC-redirected

> packets. When kernel is updated with native-XDP support for eth0, then the

> program no-longer see the TC-redirected packets. Therefore it is important

> to keep the order intact; that XDP runs before TC-BPF.


I don't follow the statement in the very last sentence here.. the order is
still intact just that these rediected packets are invoked for generic XDP
as well, so from an ingress path perspective it's still generic XDP and then
tc ingress. What this aims to achieve is to bypass the generic XDP instead.

> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>

> ---

>   net/core/dev.c    |    2 ++

>   net/sched/Kconfig |    1 +

>   2 files changed, 3 insertions(+)

> 

> diff --git a/net/core/dev.c b/net/core/dev.c

> index 6ceb6412ee97..26b40f8005ae 100644

> --- a/net/core/dev.c

> +++ b/net/core/dev.c

> @@ -3872,6 +3872,7 @@ sch_handle_egress(struct sk_buff *skb, int *ret, struct net_device *dev)

>   		return NULL;

>   	case TC_ACT_REDIRECT:

>   		/* No need to push/pop skb's mac_header here on egress! */

> +		skb_set_redirected(skb, false);

>   		skb_do_redirect(skb);

>   		*ret = NET_XMIT_SUCCESS;

>   		return NULL;

> @@ -4963,6 +4964,7 @@ sch_handle_ingress(struct sk_buff *skb, struct packet_type **pt_prev, int *ret,

>   		 * redirecting to another netdev

>   		 */

>   		__skb_push(skb, skb->mac_len);

> +		skb_set_redirected(skb, true);

>   		if (skb_do_redirect(skb) == -EAGAIN) {

>   			__skb_pull(skb, skb->mac_len);

>   			*another = true;


I'm not sure I follow the logic in the two cases here.. why mark it in one but
not the other? Both are used in practice to recircle back to ingress. Also, this
is not BPF specific.. same goes for other subsystems that recircle from TX into
RX where they will still go into generic XDP (think of action attached to actual
qdisc as an example).

> diff --git a/net/sched/Kconfig b/net/sched/Kconfig

> index a3b37d88800e..a1bbaa8fd054 100644

> --- a/net/sched/Kconfig

> +++ b/net/sched/Kconfig

> @@ -384,6 +384,7 @@ config NET_SCH_INGRESS

>   	depends on NET_CLS_ACT

>   	select NET_INGRESS

>   	select NET_EGRESS

> +	select NET_REDIRECT

>   	help

>   	  Say Y here if you want to use classifiers for incoming and/or outgoing

>   	  packets. This qdisc doesn't do anything else besides running classifiers,

> 

>
diff mbox series

Patch

diff --git a/net/core/dev.c b/net/core/dev.c
index 6ceb6412ee97..26b40f8005ae 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3872,6 +3872,7 @@  sch_handle_egress(struct sk_buff *skb, int *ret, struct net_device *dev)
 		return NULL;
 	case TC_ACT_REDIRECT:
 		/* No need to push/pop skb's mac_header here on egress! */
+		skb_set_redirected(skb, false);
 		skb_do_redirect(skb);
 		*ret = NET_XMIT_SUCCESS;
 		return NULL;
@@ -4963,6 +4964,7 @@  sch_handle_ingress(struct sk_buff *skb, struct packet_type **pt_prev, int *ret,
 		 * redirecting to another netdev
 		 */
 		__skb_push(skb, skb->mac_len);
+		skb_set_redirected(skb, true);
 		if (skb_do_redirect(skb) == -EAGAIN) {
 			__skb_pull(skb, skb->mac_len);
 			*another = true;
diff --git a/net/sched/Kconfig b/net/sched/Kconfig
index a3b37d88800e..a1bbaa8fd054 100644
--- a/net/sched/Kconfig
+++ b/net/sched/Kconfig
@@ -384,6 +384,7 @@  config NET_SCH_INGRESS
 	depends on NET_CLS_ACT
 	select NET_INGRESS
 	select NET_EGRESS
+	select NET_REDIRECT
 	help
 	  Say Y here if you want to use classifiers for incoming and/or outgoing
 	  packets. This qdisc doesn't do anything else besides running classifiers,