[1/2] net: assert that tx packets have nonzero size

Commit Message

Alexander Bulekov July 19, 2019, 6:52 p.m. UTC

Virtual devices should not try to send zero-sized packets. The caller
should check the size prior to calling qemu_sendv_packet_async.

Signed-off-by: Alexander Oleinik <[e-mail]>
---
 net/net.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Stefan Hajnoczi July 22, 2019, 7:17 a.m. UTC | #1

On Fri, Jul 19, 2019 at 06:52:24PM +0000, Oleinik, Alexander wrote:
> Virtual devices should not try to send zero-sized packets. The caller
> should check the size prior to calling qemu_sendv_packet_async.
> 
> Signed-off-by: Alexander Oleinik <[e-mail]>
> ---
>  net/net.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/net/net.c b/net/net.c
> index 7d4098254f..fad20bc611 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -741,6 +741,9 @@ ssize_t qemu_sendv_packet_async(NetClientState *sender,
>      size_t size = iov_size(iov, iovcnt);
>      int ret;
>  
> +    /* 0-sized packets are unsupported. Check size in the caller */
> +    assert(size);

Please include the rationale:

  A return value of 0 means the packet has been queued and will be sent
  asynchronously.  Therefore this function has no way of reporting that
  a 0-sized packet has been sent successfully.  Forbid it for now and if
  someone needs this functionality then the API will require a change.

This way someone who hits this will understand why the assertion is
there.

Stefan

Jason Wang July 23, 2019, 3:38 a.m. UTC | #2

On 2019/7/20 上午2:52, Oleinik, Alexander wrote:
> Virtual devices should not try to send zero-sized packets. The caller
> should check the size prior to calling qemu_sendv_packet_async.
>
> Signed-off-by: Alexander Oleinik <[e-mail]>
> ---
>   net/net.c | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/net/net.c b/net/net.c
> index 7d4098254f..fad20bc611 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -741,6 +741,9 @@ ssize_t qemu_sendv_packet_async(NetClientState *sender,
>       size_t size = iov_size(iov, iovcnt);
>       int ret;
>   
> +    /* 0-sized packets are unsupported. Check size in the caller */
> +    assert(size);


Can this be triggered through a buggy device by guest? If yes, we need 
avoid using assert() here.

Thanks


> +
>       if (size > NET_BUFSIZE) {
>           return size;
>       }

Alexander Bulekov July 26, 2019, 2:42 p.m. UTC | #3

On Tue, 2019-07-23 at 11:38 +0800, Jason Wang wrote:
> On 2019/7/20 上午2:52, Oleinik, Alexander wrote:
> > Virtual devices should not try to send zero-sized packets. The
> > caller
> > should check the size prior to calling qemu_sendv_packet_async.
> > 
> > Signed-off-by: Alexander Oleinik <[e-mail]>
> > ---
> >   net/net.c | 3 +++
> >   1 file changed, 3 insertions(+)
> > 
> > diff --git a/net/net.c b/net/net.c
> > index 7d4098254f..fad20bc611 100644
> > --- a/net/net.c
> > +++ b/net/net.c
> > @@ -741,6 +741,9 @@ ssize_t qemu_sendv_packet_async(NetClientState
> > *sender,
> >       size_t size = iov_size(iov, iovcnt);
> >       int ret;
> >   
> > +    /* 0-sized packets are unsupported. Check size in the caller
> > */
> > +    assert(size);
> 
> Can this be triggered through a buggy device by guest? If yes, we
> need 
> avoid using assert() here.

Yes - for example, virtio-net could trigger trigger it because there is
no size check prior to qemu_sendv_packet_async, although PATCH 2/2
should fix this.
Instead of the assertion, should we return a negative value?  

> Thanks
> 
> 
> > +
> >       if (size > NET_BUFSIZE) {
> >           return size;
> >       }

Patch

diff --git a/net/net.c b/net/net.c
index 7d4098254f..fad20bc611 100644
--- a/net/net.c
+++ b/net/net.c
@@ -741,6 +741,9 @@  ssize_t qemu_sendv_packet_async(NetClientState *sender,
     size_t size = iov_size(iov, iovcnt);
     int ret;
 
+    /* 0-sized packets are unsupported. Check size in the caller */
+    assert(size);
+
     if (size > NET_BUFSIZE) {
         return size;
     }