netdev - Re: [net-next PATCH v4 5/6] virtio_net: add 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]

Message-ID: <58423791.4020802@gmail.com>
Date:   Fri, 2 Dec 2016 19:10:09 -0800
From:   John Fastabend <john.fastabend@...il.com>
To:     daniel@...earbox.net, mst@...hat.com, shm@...ulusnetworks.com,
        davem@...emloft.net, tgraf@...g.ch, alexei.starovoitov@...il.com
Cc:     john.r.fastabend@...el.com, netdev@...r.kernel.org,
        bblanco@...mgrid.com, brouer@...hat.com
Subject: Re: [net-next PATCH v4 5/6] virtio_net: add XDP_TX support

On 16-12-02 12:51 PM, John Fastabend wrote:
> This adds support for the XDP_TX action to virtio_net. When an XDP
> program is run and returns the XDP_TX action the virtio_net XDP
> implementation will transmit the packet on a TX queue that aligns
> with the current CPU that the XDP packet was processed on.
> 
> Before sending the packet the header is zeroed.  Also XDP is expected
> to handle checksum correctly so no checksum offload  support is
> provided.
> 
> Signed-off-by: John Fastabend <john.r.fastabend@...el.com>
> ---
>  drivers/net/virtio_net.c |   63 ++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 60 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index b67203e..137caba 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -330,12 +330,43 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>  	return skb;
>  }
>  
> +static void virtnet_xdp_xmit(struct virtnet_info *vi,
> +			     unsigned int qnum, struct xdp_buff *xdp)
> +{
> +	struct send_queue *sq = &vi->sq[qnum];
> +	struct virtio_net_hdr_mrg_rxbuf *hdr;
> +	unsigned int num_sg, len;
> +	void *xdp_sent;
> +	int err;
> +
> +	/* Free up any pending old buffers before queueing new ones. */
> +	while ((xdp_sent = virtqueue_get_buf(sq->vq, &len)) != NULL) {
> +		struct page *page = virt_to_head_page(xdp_sent);
> +
> +		put_page(page);
> +	}
> +
> +	/* Zero header and leave csum up to XDP layers */
> +	hdr = xdp->data;
> +	memset(hdr, 0, vi->hdr_len);
> +
> +	num_sg = 1;
> +	sg_init_one(sq->sg, xdp->data, xdp->data_end - xdp->data);
> +	err = virtqueue_add_outbuf(sq->vq, sq->sg, num_sg,
> +				   xdp->data, GFP_ATOMIC);
> +	if (unlikely(err))
> +		put_page(virt_to_head_page(xdp->data));
> +	else
> +		virtqueue_kick(sq->vq);
> +}
> +

Hi Michael,

Any idea why the above pattern

> +	err = virtqueue_add_outbuf(sq->vq, sq->sg, num_sg,
> +				   xdp->data, GFP_ATOMIC);
> +	if (unlikely(err))
> +		put_page(virt_to_head_page(xdp->data));
> +	else
> +		virtqueue_kick(sq->vq);
> +}

would cause a hang but if I call the virtqueue_kick as below
even in the error case everything seems to be fine.

	err = virtqueue_add_outbuf(sq->vq, sq->sg, num_sg,
				   xdp->data, GFP_ATOMIC);
	if (unlikely(err))
		put_page(virt_to_head_page(xdp->data));

	virtqueue_kick(sq->vq);


I'll take a look through the virtio code but thought I might ask in
case you know off-hand or it could be something else entirely.

I noticed virtio_input.c uses the second pattern and virtio_net.c
uses the above pattern but I'm guessing it never gets exercised due
to stack backoff.

Thanks,
John