netdev - Re: [PATCH v8 bpf-next 02/10] veth: Add driver XDP

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]

Date:   Wed, 8 Aug 2018 09:53:27 +0900
From:   Toshiaki Makita <makita.toshiaki@....ntt.co.jp>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     Alexei Starovoitov <ast@...nel.org>, netdev@...r.kernel.org,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        Jakub Kicinski <jakub.kicinski@...ronome.com>,
        John Fastabend <john.fastabend@...il.com>
Subject: Re: [PATCH v8 bpf-next 02/10] veth: Add driver XDP

Hi Daniel,

Thank you for taking a look!

On 2018/08/07 23:26, Daniel Borkmann wrote:
> On 08/03/2018 09:58 AM, Toshiaki Makita wrote:
> [...]
>> +
>> +static struct sk_buff *veth_xdp_rcv_skb(struct veth_priv *priv,
>> +					struct sk_buff *skb)
>> +{
>> +	u32 pktlen, headroom, act, metalen;
>> +	void *orig_data, *orig_data_end;
>> +	struct bpf_prog *xdp_prog;
>> +	int mac_len, delta, off;
>> +	struct xdp_buff xdp;
>> +
>> +	rcu_read_lock();
>> +	xdp_prog = rcu_dereference(priv->xdp_prog);
>> +	if (unlikely(!xdp_prog)) {
>> +		rcu_read_unlock();
>> +		goto out;
>> +	}
>> +
>> +	mac_len = skb->data - skb_mac_header(skb);
>> +	pktlen = skb->len + mac_len;
>> +	headroom = skb_headroom(skb) - mac_len;
>> +
>> +	if (skb_shared(skb) || skb_head_is_locked(skb) ||
>> +	    skb_is_nonlinear(skb) || headroom < XDP_PACKET_HEADROOM) {
> 
> Hmm, I think this is not fully correct. What happens if you have cloned skbs as
> e.g. the case with TCP? This would also need a full expensive unclone to make the
> data private as expected by XDP (this is basically a similar issue in generic
> XDP).

Well, cloned is checked in skb_head_is_locked() so TCP packets are
always uncloned here.

> It may potentially be worth to also share the code here with generic XDP
> implementation given it's quite similar?

For now I'm not sharing the code because of two reasons.

One is that as you say generic XDP skips cloned packets. I traced the
reason and it seems it is to skip packets redirected by act_mirred.
https://patchwork.ozlabs.org/patch/750127/
The assumption that no one provides cloned skbs other than mirred breaks
when generic XDP added support for virtual devices, but it is still
valid that we should skip packets redirected by mirred if we want to be
in line with driver XDP. So I'm thinking generic XDP needs something
more than just uncloning cloned skbs.

The other reason is performance for REDIRECT. We can make use of bulk
redirection in driver XDP, but it requires xdp_frames which requires
non-kmallocked skb head. This is different from generic XDP which allows
kmallocked skb head and uses kmalloc if head reallocation is needed.

-- 
Toshiaki Makita