netdev - Re: [PATCH] virtio_net: Fix queue full check

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: <20101104164544.GB1038@redhat.com>
Date:	Thu, 4 Nov 2010 18:45:44 +0200
From:	"Michael S. Tsirkin" <mst@...hat.com>
To:	Krishna Kumar2 <krkumar2@...ibm.com>
Cc:	davem@...emloft.net, netdev@...r.kernel.org,
	Rusty Russell <rusty@...tcorp.com.au>, yvugenfi@...hat.com
Subject: Re: [PATCH] virtio_net: Fix queue full check

On Thu, Nov 04, 2010 at 09:47:04PM +0530, Krishna Kumar2 wrote:
> "Michael S. Tsirkin" <mst@...hat.com> wrote on 11/04/2010 05:54:24 PM:
> 
> > > Signed-off-by: Michael S. Tsirkin <mst@...hat.com>
> >
> > I thought about this some more.  I think the original
> > code is actually correct in returning ENOSPC: indirect
> > buffers are nice, but it's a mistake
> > to rely on them as a memory allocation might fail.
> >
> > And if you look at virtio-net, it is dropping packets
> > under memory pressure which is not really a happy outcome:
> > the packet will get freed, reallocated and we get another one,
> > adding pressure on the allocator instead of releasing it
> > until we free up some buffers.
> >
> > So I now think we should calculate the capacity
> > assuming non-indirect entries, and if we manage to
> > use indirect, all the better.
> >
> > So below is what I propose now - as a replacement for
> > my original patch.  Krishna Kumar, Rusty, what do you think?
> >
> > Separately I'm also considering moving the
> >    if (vq->num_free < out + in)
> > check earlier in the function to keep all users honest,
> > but need to check what the implications are for e.g. block.
> > Thoughts on this?
> 
> This looks like the right thing to do.  Besides this, I
> think virtio-net still needs to remove check for ENOMEM?

Yes, the only valid reason for failure would be a unexpected error.
No need to special-case ENOMEM anymore.

> I will test this patch tomorrow.
> 
> Another question about add_recvbuf_small and
> add_recvbuf_big - both call virtqueue_add_buf_gfp with
> in+out > 1, and that can fail with -ENOSPC.  So try_fill_recv
> gets -ENOSPC.  When that happens, oom is not set to true,
> I thought it should have got set.  Is this a bug?
> 
> Thanks,
> 
> - KK

I don't see a bug: on ENOSPC we don't need to (and can't) add any more
buffers, we know we will make progress since there must be some buffers
in the ring already, ENOMEM makes us try again later with more buffers
(and possibly more aggressive GFP flag).  What's wrong?

-- 
MST
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html