Re: [PATCH 1/2] virtio-net: Verify page list size before fitting into skb

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Sep 26, 2011 at 08:41:08PM +0300, Sasha Levin wrote:
> This patch verifies that the length of a buffer stored in a linked list
> of pages is small enough to fit into a skb.
> 
> If the size is larger than a max size of a skb, it means that we shouldn't
> go ahead building skbs anyway since we won't be able to send the buffer as
> the user requested.
> 
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: virtualization@lists.linux-foundation.org
> Cc: netdev@vger.kernel.org
> Cc: kvm@vger.kernel.org
> Signed-off-by: Sasha Levin <levinsasha928@gmail.com>

Interesting.  This is a theoretical issue, correct?
Not a crash you actually see.

This crash would mean device is giving us packets
that are way too large. Avoiding crashes even in the face of
a misbehaved device is a good idea, but should
we print a diagnostic to a system log?
Maybe rate-limited or print once to avoid filling
up the disk. Other places in driver print with pr_debug
I'm not sure that's right but better than nothing.

> ---
>  drivers/net/virtio_net.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 0c7321c..64e0717 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -165,6 +165,9 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>  	unsigned int copy, hdr_len, offset;
>  	char *p;
>  
> +	if (len > MAX_SKB_FRAGS * PAGE_SIZE)

unlikely()?

Also, this seems too aggressive: at this point len includes the header
and the linear part. The right place for this
test is probably where we fill in the frags, just before
while (len)

The whole can only happen when mergeable buffers
are disabled, right?


> +		return NULL;
> +
>  	p = page_address(page);
>  
>  	/* copy small packet so we can reuse these pages for small data */
> -- 
> 1.7.6.1