netdev - Re: [PATCH v6 bpf-next 4/9] veth: Handle 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:   Tue, 31 Jul 2018 12:26:03 +0200
From:   Jesper Dangaard Brouer <brouer@...hat.com>
To:     Toshiaki Makita <makita.toshiaki@....ntt.co.jp>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>, netdev@...r.kernel.org,
        Jakub Kicinski <jakub.kicinski@...ronome.com>,
        John Fastabend <john.fastabend@...il.com>, brouer@...hat.com,
        "Karlsson, Magnus" <magnus.karlsson@...el.com>,
        Björn Töpel <bjorn.topel@...el.com>
Subject: Re: [PATCH v6 bpf-next 4/9] veth: Handle xdp_frames in xdp napi
 ring


Context needed from: [PATCH v6 bpf-next 2/9] veth: Add driver XDP

On Mon, 30 Jul 2018 19:43:44 +0900
Toshiaki Makita <makita.toshiaki@....ntt.co.jp> wrote:

> +static struct sk_buff *veth_build_skb(void *head, int headroom, int len,
> +				      int buflen)
> +{
> +	struct sk_buff *skb;
> +
> +	if (!buflen) {
> +		buflen = SKB_DATA_ALIGN(headroom + len) +
> +			 SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> +	}
> +	skb = build_skb(head, buflen);
> +	if (!skb)
> +		return NULL;
> +
> +	skb_reserve(skb, headroom);
> +	skb_put(skb, len);
> +
> +	return skb;
> +}


On Mon, 30 Jul 2018 19:43:46 +0900
Toshiaki Makita <makita.toshiaki@....ntt.co.jp> wrote:

> +static struct sk_buff *veth_xdp_rcv_one(struct veth_priv *priv,
> +					struct xdp_frame *frame)
> +{
> +	int len = frame->len, delta = 0;
> +	struct bpf_prog *xdp_prog;
> +	unsigned int headroom;
> +	struct sk_buff *skb;
> +
> +	rcu_read_lock();
> +	xdp_prog = rcu_dereference(priv->xdp_prog);
> +	if (likely(xdp_prog)) {
> +		struct xdp_buff xdp;
> +		u32 act;
> +
> +		xdp.data_hard_start = frame->data - frame->headroom;
> +		xdp.data = frame->data;
> +		xdp.data_end = frame->data + frame->len;
> +		xdp.data_meta = frame->data - frame->metasize;
> +		xdp.rxq = &priv->xdp_rxq;
> +
> +		act = bpf_prog_run_xdp(xdp_prog, &xdp);
> +
> +		switch (act) {
> +		case XDP_PASS:
> +			delta = frame->data - xdp.data;
> +			len = xdp.data_end - xdp.data;
> +			break;
> +		default:
> +			bpf_warn_invalid_xdp_action(act);
> +		case XDP_ABORTED:
> +			trace_xdp_exception(priv->dev, xdp_prog, act);
> +		case XDP_DROP:
> +			goto err_xdp;
> +		}
> +	}
> +	rcu_read_unlock();
> +
> +	headroom = frame->data - delta - (void *)frame;
> +	skb = veth_build_skb(frame, headroom, len, 0);

Here you are adding an assumption that struct xdp_frame is always
located in-the-top of the packet-data area.  I tried hard not to add
such a dependency!  You can calculate the beginning of the frame from
the xdp_frame->data pointer.

Why not add such a dependency?  Because for AF_XDP zero-copy, we cannot
make such an assumption.  

Currently, when an RX-queue is in AF-XDP-ZC mode (MEM_TYPE_ZERO_COPY)
the packet will get dropped when calling convert_to_xdp_frame(), but as
the TODO comment indicated in convert_to_xdp_frame() this is not the
end-goal. 

The comment in convert_to_xdp_frame(), indicate we need a full
alloc+copy, but that is actually not necessary, if we can just use
another memory area for struct xdp_frame, and a pointer to data.  Thus,
allowing devmap-redir to work-ZC and allow cpumap-redir to do the copy
on the remote CPU.


> +	if (!skb) {
> +		xdp_return_frame(frame);
> +		goto err;
> +	}
> +
> +	memset(frame, 0, sizeof(*frame));
> +	skb->protocol = eth_type_trans(skb, priv->dev);
> +err:
> +	return skb;
> +err_xdp:
> +	rcu_read_unlock();
> +	xdp_return_frame(frame);
> +
> +	return NULL;
> +}


-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer