Linux Virtualization - Nov 2013 - [PATCH net] virtio-net: fix page refcnt leaking when fail to allocate frag skb

We need to drop the refcnt of page when we fail to allocate an skb for frag
list, otherwise it will be leaked. The bug was introduced by commit
2613af0ed18a11d5c566a81f9a6510b73180660a ("virtio_net: migrate mergeable rx
buffers to page frag allocators").

Cc: Michael Dalton <mwdalton at google.com>
Cc: Eric Dumazet <edumazet at google.com>
Cc: Rusty Russell <rusty at rustcorp.com.au>
Cc: Michael S. Tsirkin <mst at redhat.com>
Signed-off-by: Jason Wang <jasowang at redhat.com>
---
The patch was needed for 3.12 stable.
---
 drivers/net/virtio_net.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 69ad42b..3798517 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -322,9 +322,11 @@ static int receive_mergeable(struct receive_queue *rq,
struct sk_buff *head_skb)
 				 head_skb->dev->name);
 			len = MERGE_BUFFER_LEN;
 		}
+		page = virt_to_head_page(buf);
 		if (unlikely(num_skb_frags == MAX_SKB_FRAGS)) {
 			struct sk_buff *nskb = alloc_skb(0, GFP_ATOMIC);
 			if (unlikely(!nskb)) {
+				put_page(page);
 				head_skb->dev->stats.rx_dropped++;
 				return -ENOMEM;
 			}
@@ -341,7 +343,6 @@ static int receive_mergeable(struct receive_queue *rq,
struct sk_buff *head_skb)
 			head_skb->len += len;
 			head_skb->truesize += MERGE_BUFFER_LEN;
 		}
-		page = virt_to_head_page(buf);
 		offset = buf - (char *)page_address(page);
 		if (skb_can_coalesce(curr_skb, num_skb_frags, page, offset)) {
 			put_page(page);
-- 
1.8.3.2

On Tue, 2013-11-19 at 16:05 +0800, Jason Wang wrote:
> We need to drop the refcnt of page when we fail to allocate an skb for frag
> list, otherwise it will be leaked. The bug was introduced by commit
> 2613af0ed18a11d5c566a81f9a6510b73180660a ("virtio_net: migrate
mergeable rx
> buffers to page frag allocators").
> 
> Cc: Michael Dalton <mwdalton at google.com>
> Cc: Eric Dumazet <edumazet at google.com>
> Cc: Rusty Russell <rusty at rustcorp.com.au>
> Cc: Michael S. Tsirkin <mst at redhat.com>
> Signed-off-by: Jason Wang <jasowang at redhat.com>
> ---
> The patch was needed for 3.12 stable.
Good catch, but if we return from receive_mergeable() in the 'middle'
of the frags we would need for the current skb, who will
call the virtqueue_get_buf() to flush the remaining frags ?

Don't we also need to call virtqueue_get_buf() like 

while (--num_buf) {
    buf = virtqueue_get_buf(rq->vq, &len);
    if (!buf)
        break;
    put_page(virt_to_head_page(buf));
}

?

On Tue, Nov 19, 2013 at 06:03:48AM -0800, Eric Dumazet
wrote:
> On Tue, 2013-11-19 at 16:05 +0800, Jason Wang wrote:
> > We need to drop the refcnt of page when we fail to allocate an skb for
frag
> > list, otherwise it will be leaked. The bug was introduced by commit
> > 2613af0ed18a11d5c566a81f9a6510b73180660a ("virtio_net: migrate
mergeable rx
> > buffers to page frag allocators").
> > 
> > Cc: Michael Dalton <mwdalton at google.com>
> > Cc: Eric Dumazet <edumazet at google.com>
> > Cc: Rusty Russell <rusty at rustcorp.com.au>
> > Cc: Michael S. Tsirkin <mst at redhat.com>
> > Signed-off-by: Jason Wang <jasowang at redhat.com>
> > ---
> > The patch was needed for 3.12 stable.
> 
> Good catch, but if we return from receive_mergeable() in the
'middle'
> of the frags we would need for the current skb, who will
> call the virtqueue_get_buf() to flush the remaining frags ?
> 
> Don't we also need to call virtqueue_get_buf() like 
> 
> while (--num_buf) {
>     buf = virtqueue_get_buf(rq->vq, &len);
>     if (!buf)
>         break;
>     put_page(virt_to_head_page(buf));
> }
> 
> ?
> 
> 
> 

virtqueue_get_buf only gives you back a buffer that has been DMA-ed
to by hardware. ATM there's no way to get back a buffer once you
gave it to hardware without doing a NIC reset.

On Tue, Nov 19, 2013 at 06:03:48AM -0800, Eric Dumazet
wrote:
> On Tue, 2013-11-19 at 16:05 +0800, Jason Wang wrote:
> > We need to drop the refcnt of page when we fail to allocate an skb for
frag
> > list, otherwise it will be leaked. The bug was introduced by commit
> > 2613af0ed18a11d5c566a81f9a6510b73180660a ("virtio_net: migrate
mergeable rx
> > buffers to page frag allocators").
> > 
> > Cc: Michael Dalton <mwdalton at google.com>
> > Cc: Eric Dumazet <edumazet at google.com>
> > Cc: Rusty Russell <rusty at rustcorp.com.au>
> > Cc: Michael S. Tsirkin <mst at redhat.com>
> > Signed-off-by: Jason Wang <jasowang at redhat.com>
> > ---
> > The patch was needed for 3.12 stable.
> 
> Good catch, but if we return from receive_mergeable() in the
'middle'
> of the frags we would need for the current skb, who will
> call the virtqueue_get_buf() to flush the remaining frags ?
> 
> Don't we also need to call virtqueue_get_buf() like 
> 
> while (--num_buf) {
>     buf = virtqueue_get_buf(rq->vq, &len);
>     if (!buf)
>         break;
>     put_page(virt_to_head_page(buf));
> }
> 
> ?
> 
> 

Let me explain what worries me in your suggestion:

                        struct sk_buff *nskb = alloc_skb(0, GFP_ATOMIC);
                        if (unlikely(!nskb)) {
                                head_skb->dev->stats.rx_dropped++;
                                return -ENOMEM;
                        }

is this the failure case we are talking about?

I think this is a symprom of a larger problem
introduced by 2613af0ed18a11d5c566a81f9a6510b73180660a,
namely that we now need to allocate memory in the
middle of processing a packet.


I think discarding a completely valid and well-formed
packet from the receive queue because we are unable
to allocate new memory with GFP_ATOMIC
for future packets is not a good idea.

It certainly violates the principle of least surprize:
when one sees host pass packet to guest, one expects
the packet to get into the networking stack, not get
dropped by the driver internally.
Guest stack can do with the packet what it sees fit.

We actually wake up a thread if we can't fill up the queue,
that will fill it up in GFP_KERNEL context.

So I think we should find a way to pre-allocate if necessary and avoid
error paths where allocating new memory is a required to avoid drops.

-- 
MST

On 11/19/2013 10:03 PM, Eric Dumazet wrote:
> On Tue, 2013-11-19 at 16:05 +0800, Jason Wang wrote:
>> > We need to drop the refcnt of page when we fail to allocate an skb
for frag
>> > list, otherwise it will be leaked. The bug was introduced by
commit
>> > 2613af0ed18a11d5c566a81f9a6510b73180660a ("virtio_net:
migrate mergeable rx
>> > buffers to page frag allocators").
>> > 
>> > Cc: Michael Dalton <mwdalton at google.com>
>> > Cc: Eric Dumazet <edumazet at google.com>
>> > Cc: Rusty Russell <rusty at rustcorp.com.au>
>> > Cc: Michael S. Tsirkin <mst at redhat.com>
>> > Signed-off-by: Jason Wang <jasowang at redhat.com>
>> > ---
>> > The patch was needed for 3.12 stable.
> Good catch, but if we return from receive_mergeable() in the
'middle'
> of the frags we would need for the current skb, who will
> call the virtqueue_get_buf() to flush the remaining frags ?
>
> Don't we also need to call virtqueue_get_buf() like 
>
> while (--num_buf) {
>     buf = virtqueue_get_buf(rq->vq, &len);
>     if (!buf)
>         break;
>     put_page(virt_to_head_page(buf));
> }
>
> ?
Yes we need this, will send V2.

Thanks