netdev - Re: [PATCH net-next 2/8] virtio_net: mergeable xdp: introduce mergeable_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:   Thu, 23 Mar 2023 12:45:41 +0800
From:   Yunsheng Lin <linyunsheng@...wei.com>
To:     Xuan Zhuo <xuanzhuo@...ux.alibaba.com>
CC:     "Michael S. Tsirkin" <mst@...hat.com>,
        Jason Wang <jasowang@...hat.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Jesper Dangaard Brouer <hawk@...nel.org>,
        John Fastabend <john.fastabend@...il.com>,
        <virtualization@...ts.linux-foundation.org>, <bpf@...r.kernel.org>,
        <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 2/8] virtio_net: mergeable xdp: introduce
 mergeable_xdp_prepare

On 2023/3/23 9:45, Xuan Zhuo wrote:
> On Wed, 22 Mar 2023 19:52:48 +0800, Yunsheng Lin <linyunsheng@...wei.com> wrote:
>> On 2023/3/22 11:03, Xuan Zhuo wrote:
>>> Separating the logic of preparation for xdp from receive_mergeable.
>>>
>>> The purpose of this is to simplify the logic of execution of XDP.
>>>
>>> The main logic here is that when headroom is insufficient, we need to
>>> allocate a new page and calculate offset. It should be noted that if
>>> there is new page, the variable page will refer to the new page.
>>>
>>> Signed-off-by: Xuan Zhuo <xuanzhuo@...ux.alibaba.com>
>>> ---
>>>  drivers/net/virtio_net.c | 135 ++++++++++++++++++++++-----------------
>>>  1 file changed, 77 insertions(+), 58 deletions(-)
>>>
>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>> index 4d2bf1ce0730..bb426958cdd4 100644
>>> --- a/drivers/net/virtio_net.c
>>> +++ b/drivers/net/virtio_net.c
>>> @@ -1162,6 +1162,79 @@ static int virtnet_build_xdp_buff_mrg(struct net_device *dev,
>>>  	return 0;
>>>  }
>>>
>>> +static void *mergeable_xdp_prepare(struct virtnet_info *vi,
>>> +				   struct receive_queue *rq,
>>> +				   struct bpf_prog *xdp_prog,
>>> +				   void *ctx,
>>> +				   unsigned int *frame_sz,
>>> +				   int *num_buf,
>>> +				   struct page **page,
>>> +				   int offset,
>>> +				   unsigned int *len,
>>> +				   struct virtio_net_hdr_mrg_rxbuf *hdr)
>>
>> The naming convention seems to be xdp_prepare_mergeable().
> 
> What convention?
> 
> 
>>
>>> +{
>>> +	unsigned int truesize = mergeable_ctx_to_truesize(ctx);
>>> +	unsigned int headroom = mergeable_ctx_to_headroom(ctx);
>>> +	struct page *xdp_page;
>>> +	unsigned int xdp_room;
>>> +
>>> +	/* Transient failure which in theory could occur if
>>> +	 * in-flight packets from before XDP was enabled reach
>>> +	 * the receive path after XDP is loaded.
>>> +	 */
>>> +	if (unlikely(hdr->hdr.gso_type))
>>> +		return NULL;
>>> +
>>> +	/* Now XDP core assumes frag size is PAGE_SIZE, but buffers
>>> +	 * with headroom may add hole in truesize, which
>>> +	 * make their length exceed PAGE_SIZE. So we disabled the
>>> +	 * hole mechanism for xdp. See add_recvbuf_mergeable().
>>> +	 */
>>> +	*frame_sz = truesize;
>>> +
>>> +	/* This happens when headroom is not enough because
>>> +	 * of the buffer was prefilled before XDP is set.
>>> +	 * This should only happen for the first several packets.
>>> +	 * In fact, vq reset can be used here to help us clean up
>>> +	 * the prefilled buffers, but many existing devices do not
>>> +	 * support it, and we don't want to bother users who are
>>> +	 * using xdp normally.
>>> +	 */
>>> +	if (!xdp_prog->aux->xdp_has_frags &&
>>> +	    (*num_buf > 1 || headroom < virtnet_get_headroom(vi))) {
>>> +		/* linearize data for XDP */
>>> +		xdp_page = xdp_linearize_page(rq, num_buf,
>>> +					      *page, offset,
>>> +					      VIRTIO_XDP_HEADROOM,
>>> +					      len);
>>> +
>>> +		if (!xdp_page)
>>> +			return NULL;
>>> +	} else if (unlikely(headroom < virtnet_get_headroom(vi))) {
>>> +		xdp_room = SKB_DATA_ALIGN(VIRTIO_XDP_HEADROOM +
>>> +					  sizeof(struct skb_shared_info));
>>> +		if (*len + xdp_room > PAGE_SIZE)
>>> +			return NULL;
>>> +
>>> +		xdp_page = alloc_page(GFP_ATOMIC);
>>> +		if (!xdp_page)
>>> +			return NULL;
>>> +
>>> +		memcpy(page_address(xdp_page) + VIRTIO_XDP_HEADROOM,
>>> +		       page_address(*page) + offset, *len);
>>
>> It seems the above 'else if' was not really tested even before this patch,
>> as there is no "--*num_buf" if xdp_linearize_page() is not called, which
>> may causes virtnet_build_xdp_buff_mrg() to comsume one more buffer than
>> expected?
> 
> Why do you think so?


In first 'if' block, there is a "--*num_buf" before gotoing 'err_xdp'
for virtqueue_get_buf() failure in xdp_linearize_page().

But here there is no "--*num_buf" before gotoing 'err_xdp' for
alloc_page() failure.

So one of them has to be wrong, right?

> 
>>
>> Also, it seems better to split the xdp_linearize_page() to two functions
>> as pskb_expand_head() and __skb_linearize() do, one to expand the headroom,
>> the other one to do the linearizing.
> 
> No skb here.

I means following the semantics of pskb_expand_head() and __skb_linearize(),
not to combine the headroom expanding and linearizing into one function as
xdp_linearize_page() does now if we want a better refoctor result.

> 
> 
>>