netdev - Re: [PATCH v2] Add Mergeable RX buffer feature to vhost

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, 1 Apr 2010 11:22:37 -0700
From:	David Stevens <dlstevens@...ibm.com>
To:	"Michael S. Tsirkin" <mst@...hat.com>
Cc:	kvm@...r.kernel.org, kvm-owner@...r.kernel.org,
	netdev@...r.kernel.org, rusty@...tcorp.com.au,
	virtualization@...ts.osdl.org
Subject: Re: [PATCH v2] Add Mergeable RX buffer feature to vhost_net

kvm-owner@...r.kernel.org wrote on 04/01/2010 03:54:15 AM:

> On Wed, Mar 31, 2010 at 03:04:43PM -0700, David Stevens wrote:

> > 
> > > > +               head.iov_base = (void 
*)vhost_get_vq_desc(&net->dev, 
> > vq,
> > > > +                       vq->iov, ARRAY_SIZE(vq->iov), &out, &in, 
NULL, 
> > 
> > > > NULL);
> > > 
> > > I this casting confusing.
> > > Is it really expensive to add an array of heads so that
> > > we do not need to cast?
> > 
> >         It needs the heads and the lengths, which looks a lot
> > like an iovec. I was trying to resist adding a new
> > struct XXX { unsigned head; unsigned len; } just for this,
> > but I could make these parallel arrays, one with head index and
> > the other with length.

        Michael, on this one, if I add vq->heads as an argument to
vhost_get_heads (aka vhost_get_desc_n), I'd need the length too.
Would you rather this 1) remain an iovec (and a single arg added) but
cast still there, 2) 2 arrays (head and length) and 2 args added, or
3) a new struct type of {unsigned,int} to carry for the heads+len
instead of iovec?
        My preference would be 1). I agree the casts are ugly, but
it is essentially an iovec the way we use it; it's just that the
base isn't a pointer but a descriptor index instead.

> > 
> >         EAGAIN is not possible after the change, because we don't
> > even enter the loop unless we have an skb on the read queue; the
> > other cases bomb out, so I figured the comment for future work is
> > now done. :-)
> 
> Guest could be buggy so we'll get EFAULT.
> If skb is taken off the rx queue (as below), we might get EAGAIN.

        We break on any error. If we get EAGAIN because someone read
on the socket, this code would break the loop, but EAGAIN is a more
serious problem if it changed since we peeked (because it means
someone else is reading the socket).
        But I don't understand -- are you suggesting that the error
handling be different than that, or that the comment is still
relevant? My intention here is to do the "TODO" from the comment
so that it can be removed, by handling all error cases. I think
because of the peek, EAGAIN isn't something to be ignored anymore,
but the effect is the same whether we break out of the loop or
not, since we retry the packet next time around. Essentially, we
ignore every error since we will redo it with the same packet the
next time around. Maybe we should print something here, but since
we'll be retrying the packet that's still on the socket, a permanent
error would spew continuously. Maybe we should shut down entirely
if we get any negative return value here (including EAGAIN, since
that tells us someone messed with the socket when we don't want them
to).
        If you want the comment still there, ok, but I do think EAGAIN
isn't a special case per the comment anymore, and is handled as all
other errors are: by exiting the loop and retrying next time.

                                                                +-DLS

--
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