Message-ID: <1725952844.7580578-1-xuanzhuo@linux.alibaba.com>
Date: Tue, 10 Sep 2024 15:20:44 +0800
From: Xuan Zhuo <xuanzhuo@...ux.alibaba.com>
To: Jason Wang <jasowang@...hat.com>
Cc: "Michael S. Tsirkin" <mst@...hat.com>,
 netdev@...r.kernel.org,
 Eugenio Pérez <eperezma@...hat.com>,
 "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>,
 virtualization@...ts.linux.dev,
 "Si-Wei Liu" <si-wei.liu@...cle.com>,
 Darren Kenny <darren.kenny@...cle.com>
Subject: Re: [PATCH net] virtio-net: fix overflow inside virtnet_rq_alloc

On Tue, 10 Sep 2024 14:18:37 +0800, Jason Wang <jasowang@...hat.com> wrote:
> On Mon, Sep 9, 2024 at 4:50 PM Xuan Zhuo <xuanzhuo@...ux.alibaba.com> wrote:
> >
> > On Mon, 9 Sep 2024 16:38:16 +0800, Jason Wang <jasowang@...hat.com> wrote:
> > > On Fri, Sep 6, 2024 at 5:32 PM Xuan Zhuo <xuanzhuo@...ux.alibaba.com> wrote:
> > > >
> > > > On Fri, 6 Sep 2024 05:08:56 -0400, "Michael S. Tsirkin" <mst@...hat.com> wrote:
> > > > > On Fri, Sep 06, 2024 at 04:53:38PM +0800, Xuan Zhuo wrote:
> > > > > > On Fri, 6 Sep 2024 04:43:29 -0400, "Michael S. Tsirkin" <mst@...hat.com> wrote:
> > > > > > > On Tue, Aug 20, 2024 at 03:19:13PM +0800, Xuan Zhuo wrote:
> > > > > > > > leads to regression on VM with the sysctl value of:
> > > > > > > >
> > > > > > > > - net.core.high_order_alloc_disable=1
> > > > > > > >
> > > > > > > > which could see reliable crashes or scp failure (scp a file 100M in size
> > > > > > > > to VM):
> > > > > > > >
> > > > > > > > The issue is that the virtnet_rq_dma takes up 16 bytes at the beginning
> > > > > > > > of a new frag. When the frag size is larger than PAGE_SIZE,
> > > > > > > > everything is fine. However, if the frag is only one page and the
> > > > > > > > total size of the buffer and virtnet_rq_dma is larger than one page, an
> > > > > > > > overflow may occur. In this case, if an overflow is possible, I adjust
> > > > > > > > the buffer size. If net.core.high_order_alloc_disable=1, the maximum
> > > > > > > > buffer size is 4096 - 16. If net.core.high_order_alloc_disable=0, only
> > > > > > > > the first buffer of the frag is affected.
> > > > > > > >
> > > > > > > > Fixes: f9dac92ba908 ("virtio_ring: enable premapped mode whatever use_dma_api")
> > > > > > > > Reported-by: "Si-Wei Liu" <si-wei.liu@...cle.com>
> > > > > > > > Closes: http://lore.kernel.org/all/8b20cc28-45a9-4643-8e87-ba164a540c0a@oracle.com
> > > > > > > > Signed-off-by: Xuan Zhuo <xuanzhuo@...ux.alibaba.com>
> > > > > > >
> > > > > > >
> > > > > > > Guys where are we going with this? We have a crasher right now,
> > > > > > > if this is not fixed ASAP I'd have to revert a ton of
> > > > > > > work Xuan Zhuo just did.
> > > > > >
> > > > > > I think this patch can fix it and I tested it.
> > > > > > But Darren said this patch did not work.
> > > > > > I need more info about the crash that Darren encountered.
> > > > > >
> > > > > > Thanks.
> > > > >
> > > > > So what are we doing? Revert the whole pile for now?
> > > > > Seems to be a bit of a pity, but maybe that's the best we can do
> > > > > for this release.
> > > >
> > > > @Jason Could you review this?
> > >
> > > I think we probably need some tweaks for this patch.
> > >
> > > For example, the changelog is not easy to be understood especially
> > > consider it starts something like:
> > >
> > > "
> > >     leads to regression on VM with the sysctl value of:
> > >
> > >     - net.core.high_order_alloc_disable=1
> > >
> > >     which could see reliable crashes or scp failure (scp a file 100M in size
> > >     to VM):
> > > "
> > >
> > > Need some context and actually sysctl is not a must to reproduce the
> > > issue, it can also happen when memory is fragmented.
> >
> > OK.
> >
> >
> > >
> > > Another issue is that, if we move the skb_page_frag_refill() out of
> > > the virtnet_rq_alloc(). The function semantics turns out to be weird:
> > >
> > > skb_page_frag_refill(len, &rq->alloc_frag, gfp);
> > > ...
> > > virtnet_rq_alloc(rq, len, gfp);
> >
> > YES.
> >
> > >
> > > I wonder instead of subtracting the dma->len, how about simply count
> > > the dma->len in len if we call virtnet_rq_aloc() in
> > > add_recvbuf_small()?
> >
> > 1. For the small mode, it is safe. That just happens in the merge mode.
> > 2. In the merge mode, if we count the dma->len in len, we should know
> >    if the frag->offset is zero or not. We can not do that before
> >    skb_page_frag_refill(), because skb_page_frag_refill() may allocate
> >    new page, the frag->offset is zero. So the judgment must is after
> >    skb_page_frag_refill().
>
> I may miss something. I mean always reserve dma->len for each frag.

This problem is a little complex.

We need to consider one point, if the frag reserves sizeof(dma), then
the len must minus that, otherwise we can not allocate from the
frag when frag is only one page and 'len' is PAGE_SIZE.

Thanks.


>
> But anyway, we need to tweak the function API, either explicitly pass
> the frag or use the rq->frag implicitly.
>
> Thanks
>
> >
> > Thanks.
> >
> >
> > >
> > > >
> > > > I think this problem is clear, though I do not know why it did not work
> > > > for Darren.
> > >
> > > I had a try. This issue could be reproduced easily and this patch
> > > seems to fix the issue with a KASAN enabled kernel.
> > >
> > > Thanks
> > >
> > > >
> > > > Thanks.
> > > >
> > > >
> > > > >
> > > > >
> > > > > > >
> > > > > > >
> > > > > > > > ---
> > > > > > > >  drivers/net/virtio_net.c | 12 +++++++++---
> > > > > > > >  1 file changed, 9 insertions(+), 3 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > > > > > index c6af18948092..e5286a6da863 100644
> > > > > > > > --- a/drivers/net/virtio_net.c
> > > > > > > > +++ b/drivers/net/virtio_net.c
> > > > > > > > @@ -918,9 +918,6 @@ static void *virtnet_rq_alloc(struct receive_queue *rq, u32 size, gfp_t gfp)
> > > > > > > >         void *buf, *head;
> > > > > > > >         dma_addr_t addr;
> > > > > > > >
> > > > > > > > -       if (unlikely(!skb_page_frag_refill(size, alloc_frag, gfp)))
> > > > > > > > -               return NULL;
> > > > > > > > -
> > > > > > > >         head = page_address(alloc_frag->page);
> > > > > > > >
> > > > > > > >         dma = head;
> > > > > > > > @@ -2421,6 +2418,9 @@ static int add_recvbuf_small(struct virtnet_info *vi, struct receive_queue *rq,
> > > > > > > >         len = SKB_DATA_ALIGN(len) +
> > > > > > > >               SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> > > > > > > >
> > > > > > > > +       if (unlikely(!skb_page_frag_refill(len, &rq->alloc_frag, gfp)))
> > > > > > > > +               return -ENOMEM;
> > > > > > > > +
> > > > > > > >         buf = virtnet_rq_alloc(rq, len, gfp);
> > > > > > > >         if (unlikely(!buf))
> > > > > > > >                 return -ENOMEM;
> > > > > > > > @@ -2521,6 +2521,12 @@ static int add_recvbuf_mergeable(struct virtnet_info *vi,
> > > > > > > >          */
> > > > > > > >         len = get_mergeable_buf_len(rq, &rq->mrg_avg_pkt_len, room);
> > > > > > > >
> > > > > > > > +       if (unlikely(!skb_page_frag_refill(len + room, alloc_frag, gfp)))
> > > > > > > > +               return -ENOMEM;
> > > > > > > > +
> > > > > > > > +       if (!alloc_frag->offset && len + room + sizeof(struct virtnet_rq_dma) > alloc_frag->size)
> > > > > > > > +               len -= sizeof(struct virtnet_rq_dma);
> > > > > > > > +
> > > > > > > >         buf = virtnet_rq_alloc(rq, len + room, gfp);
> > > > > > > >         if (unlikely(!buf))
> > > > > > > >                 return -ENOMEM;
> > > > > > > > --
> > > > > > > > 2.32.0.3.g01195cf9f
> > > > > > >
> > > > >
> > > >
> > >
> >
>

Powered by blists - more mailing lists