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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <cb5dca49-1c73-47c4-d21a-819da8a75b1c@iogearbox.net>
Date:   Thu, 3 Dec 2020 01:06:41 +0100
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Jesper Dangaard Brouer <brouer@...hat.com>, bpf@...r.kernel.org
Cc:     netdev@...r.kernel.org, Daniel Borkmann <borkmann@...earbox.net>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        maze@...gle.com, lmb@...udflare.com, shaun@...era.io,
        Lorenzo Bianconi <lorenzo@...nel.org>, marek@...udflare.com,
        John Fastabend <john.fastabend@...il.com>,
        Jakub Kicinski <kuba@...nel.org>, eyal.birger@...il.com,
        colrack@...il.com
Subject: Re: [PATCH bpf-next V8 6/8] bpf: make it possible to identify BPF
 redirected SKBs

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@...hat.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,
> 
>