| 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
| ||
|
Date: Wed, 28 Dec 2022 19:54:18 +0800
From: Jason Wang <jasowang@...hat.com>
To: Heng Qi <hengqi@...ux.alibaba.com>
Cc: netdev@...r.kernel.org, bpf@...r.kernel.org,
"Michael S . Tsirkin" <mst@...hat.com>,
Paolo Abeni <pabeni@...hat.com>,
Jakub Kicinski <kuba@...nel.org>,
John Fastabend <john.fastabend@...il.com>,
"David S . Miller" <davem@...emloft.net>,
Daniel Borkmann <daniel@...earbox.net>,
Alexei Starovoitov <ast@...nel.org>,
Eric Dumazet <edumazet@...gle.com>,
Xuan Zhuo <xuanzhuo@...ux.alibaba.com>
Subject: Re: [PATCH v2 5/9] virtio_net: construct multi-buffer xdp in mergeable
On Wed, Dec 28, 2022 at 4:23 PM Heng Qi <hengqi@...ux.alibaba.com> wrote:
>
> On Wed, Dec 28, 2022 at 02:24:22PM +0800, Jason Wang wrote:
> >
> > 在 2022/12/27 17:31, Heng Qi 写道:
> > >
> > >
> > >在 2022/12/27 下午3:01, Jason Wang 写道:
> > >>
> > >>在 2022/12/20 22:14, Heng Qi 写道:
> > >>>Build multi-buffer xdp using virtnet_build_xdp_buff_mrg().
> > >>>
> > >>>For the prefilled buffer before xdp is set, we will probably use
> > >>>vq reset in the future. At the same time, virtio net currently
> > >>>uses comp pages, and bpf_xdp_frags_increase_tail() needs to calculate
> > >>>the tailroom of the last frag, which will involve the offset of the
> > >>>corresponding page and cause a negative value, so we disable tail
> > >>>increase by not setting xdp_rxq->frag_size.
> > >>>
> > >>>Signed-off-by: Heng Qi <hengqi@...ux.alibaba.com>
> > >>>Reviewed-by: Xuan Zhuo <xuanzhuo@...ux.alibaba.com>
> > >>>---
> > >>> drivers/net/virtio_net.c | 60
> > >>>+++++++++++++++++++++++++++++-----------
> > >>> 1 file changed, 44 insertions(+), 16 deletions(-)
> > >>>
> > >>>diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > >>>index 8fc3b1841d92..40bc58fa57f5 100644
> > >>>--- a/drivers/net/virtio_net.c
> > >>>+++ b/drivers/net/virtio_net.c
> > >>>@@ -1018,6 +1018,7 @@ static struct sk_buff
> > >>>*receive_mergeable(struct net_device *dev,
> > >>> unsigned int *xdp_xmit,
> > >>> struct virtnet_rq_stats *stats)
> > >>> {
> > >>>+ unsigned int tailroom = SKB_DATA_ALIGN(sizeof(struct
> > >>>skb_shared_info));
> > >>> struct virtio_net_hdr_mrg_rxbuf *hdr = buf;
> > >>> u16 num_buf = virtio16_to_cpu(vi->vdev, hdr->num_buffers);
> > >>> struct page *page = virt_to_head_page(buf);
> > >>>@@ -1048,11 +1049,14 @@ static struct sk_buff
> > >>>*receive_mergeable(struct net_device *dev,
> > >>> rcu_read_lock();
> > >>> xdp_prog = rcu_dereference(rq->xdp_prog);
> > >>> if (xdp_prog) {
> > >>>+ unsigned int xdp_frags_truesz = 0;
> > >>>+ struct skb_shared_info *shinfo;
> > >>> struct xdp_frame *xdpf;
> > >>> struct page *xdp_page;
> > >>> struct xdp_buff xdp;
> > >>> void *data;
> > >>> u32 act;
> > >>>+ int i;
> > >>> /* Transient failure which in theory could occur if
> > >>> * in-flight packets from before XDP was enabled reach
> > >>>@@ -1061,19 +1065,23 @@ static struct sk_buff
> > >>>*receive_mergeable(struct net_device *dev,
> > >>> if (unlikely(hdr->hdr.gso_type))
> > >>> goto err_xdp;
> > >>> - /* Buffers with headroom use PAGE_SIZE as alloc size,
> > >>>- * see add_recvbuf_mergeable() + get_mergeable_buf_len()
> > >>>+ /* 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 = headroom ? PAGE_SIZE : truesize;
> > >>> - /* This happens when rx buffer size is underestimated
> > >>>- * or headroom is not enough because of the buffer
> > >>>- * was refilled before XDP is set. This should only
> > >>>- * happen for the first several packets, so we don't
> > >>>- * care much about its performance.
> > >>>+ /* 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 (unlikely(num_buf > 1 ||
> > >>>- headroom < virtnet_get_headroom(vi))) {
> > >>>+ 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,
> > >>>@@ -1084,17 +1092,26 @@ static struct sk_buff
> > >>>*receive_mergeable(struct net_device *dev,
> > >>> if (!xdp_page)
> > >>> goto err_xdp;
> > >>> offset = VIRTIO_XDP_HEADROOM;
> > >>>+ } else if (unlikely(headroom < virtnet_get_headroom(vi))) {
> > >>
> > >>
> > >>I believe we need to check xdp_prog->aux->xdp_has_frags at least
> > >>since this may not work if it needs more than one frags?
> > >
> > >Sorry Jason, I didn't understand you, I'll try to answer. For
> > >multi-buffer xdp programs, if the first buffer is a pre-filled
> > >buffer (no headroom),
> > >we need to copy it out and use the subsequent buffers of this
> > >packet as its frags (this is done in
> > >virtnet_build_xdp_buff_mrg()), therefore,
> > >it seems that there is no need to check
> > >'xdp_prog->aux->xdp_has_frags' to mark multi-buffer xdp (of course
> > >I can add it),
> > >
> > >+ } else if (unlikely(headroom < virtnet_get_headroom(vi))) {
> > >
> > >Because the linearization of single-buffer xdp has all been done
> > >before, the subsequent situation can only be applied to
> > >multi-buffer xdp:
> > >+ if (!xdp_prog->aux->xdp_has_frags &&
> > >+ (num_buf > 1 || headroom < virtnet_get_headroom(vi))) {
> >
> >
> > I basically meant what happens if
> >
> > !xdp_prog->aux->xdp_has_frags && num_buf > 2 && headroom <
> > virtnet_get_headroom(vi)
> >
> > In this case the current code seems to leave the second buffer in
> > the frags. This is the case of the buffer size underestimation that
> > is mentioned in the comment before (I'd like to keep that).
>
> If I'm not wrong, this case is still directly into the first 'if' loop.
My fault, you're right.
Thanks
>
> - if (unlikely(num_buf > 1 ||
> - headroom < virtnet_get_headroom(vi))) {
> + if (!xdp_prog->aux->xdp_has_frags &&
> + (num_buf > 1 || headroom < virtnet_get_headroom(vi))) {
>
> Thanks.
>
> >
> > (And that's why I'm asking to use linearizge_page())
> >
> > Thanks
> >
> >
> > >
> > >>
> > >>Btw, I don't see a reason why we can't reuse
> > >>xdp_linearize_page(), (we probably don't need error is the
> > >>buffer exceeds PAGE_SIZE).
> > >
> > >For multi-buffer xdp, we only need to copy out the pre-filled
> > >first buffer, and use the remaining buffers of this packet as
> > >frags in virtnet_build_xdp_buff_mrg().
> > >
> > >Thanks.
> > >
> > >>
> > >>Other looks good.
> > >>
> > >>Thanks
> > >>
> > >>
> > >>>+ if ((VIRTIO_XDP_HEADROOM + len + tailroom) > PAGE_SIZE)
> > >>>+ goto err_xdp;
> > >>>+
> > >>>+ xdp_page = alloc_page(GFP_ATOMIC);
> > >>>+ if (!xdp_page)
> > >>>+ goto err_xdp;
> > >>>+
> > >>>+ memcpy(page_address(xdp_page) + VIRTIO_XDP_HEADROOM,
> > >>>+ page_address(page) + offset, len);
> > >>>+ frame_sz = PAGE_SIZE;
> > >>>+ offset = VIRTIO_XDP_HEADROOM;
> > >>> } else {
> > >>> xdp_page = page;
> > >>> }
> > >>>-
> > >>>- /* Allow consuming headroom but reserve enough space to push
> > >>>- * the descriptor on if we get an XDP_TX return code.
> > >>>- */
> > >>> data = page_address(xdp_page) + offset;
> > >>>- xdp_init_buff(&xdp, frame_sz - vi->hdr_len, &rq->xdp_rxq);
> > >>>- xdp_prepare_buff(&xdp, data - VIRTIO_XDP_HEADROOM +
> > >>>vi->hdr_len,
> > >>>- VIRTIO_XDP_HEADROOM, len - vi->hdr_len, true);
> > >>>+ err = virtnet_build_xdp_buff_mrg(dev, vi, rq, &xdp,
> > >>>data, len, frame_sz,
> > >>>+ &num_buf, &xdp_frags_truesz, stats);
> > >>>+ if (unlikely(err))
> > >>>+ goto err_xdp_frags;
> > >>> act = bpf_prog_run_xdp(xdp_prog, &xdp);
> > >>> stats->xdp_packets++;
> > >>>@@ -1190,6 +1207,17 @@ static struct sk_buff
> > >>>*receive_mergeable(struct net_device *dev,
> > >>> __free_pages(xdp_page, 0);
> > >>> goto err_xdp;
> > >>> }
> > >>>+err_xdp_frags:
> > >>>+ shinfo = xdp_get_shared_info_from_buff(&xdp);
> > >>>+
> > >>>+ if (unlikely(xdp_page != page))
> > >>>+ __free_pages(xdp_page, 0);
> > >>>+
> > >>>+ for (i = 0; i < shinfo->nr_frags; i++) {
> > >>>+ xdp_page = skb_frag_page(&shinfo->frags[i]);
> > >>>+ put_page(xdp_page);
> > >>>+ }
> > >>>+ goto err_xdp;
> > >>> }
> > >>> rcu_read_unlock();
> > >
>
Powered by blists - more mailing lists