| Message ID | 1317220855-9352-1-git-send-email-levinsasha928@gmail.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Sasha Levin <levinsasha928@gmail.com> Date: Wed, 28 Sep 2011 17:40:54 +0300 > 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> Michael, tell me what's happening with these two virtio-net bug fixes. Thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 03, 2011 at 02:19:51PM -0400, David Miller wrote: > From: Sasha Levin <levinsasha928@gmail.com> > Date: Wed, 28 Sep 2011 17:40:54 +0300 > > > 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> > > Michael, tell me what's happening with these two virtio-net bug fixes. > > Thanks. Thanks for the reminder. They are more enhancements than bugfixes, in that both help debug buggy hypervisors. So net-next material. Patch 1 is ok patch 2 would benefit from a slightly clearer comment.
On Wed, Sep 28, 2011 at 05:40:54PM +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> > --- > drivers/net/virtio_net.c | 13 +++++++++++++ > 1 files changed, 13 insertions(+), 0 deletions(-) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index 0c7321c..bde0dec 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -195,6 +195,19 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi, > len -= copy; > offset += copy; > > + /* > + * Verify that we can indeed put this data into a skb. > + * This is here to handle cases when the device erroneously > + * tries to receive more than is possible. This is usually > + * the case of a broken device. > + */ > + if (unlikely(len > MAX_SKB_FRAGS * PAGE_SIZE)) { > + if (net_ratelimit()) > + pr_debug("%s: too much data\n", skb->dev->name); > + dev_kfree_skb(skb); > + return NULL; > + } > + BTW, receive_mergeable does pr_debug("%s: packet too long\n", skb->dev->name); skb->dev->stats.rx_length_errors++; which makes sense. > while (len) { > set_skb_frag(skb, page, offset, &len); > page = (struct page *)page->private; > -- > 1.7.6.1 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2011-10-03 at 21:04 +0200, Michael S. Tsirkin wrote: > On Wed, Sep 28, 2011 at 05:40:54PM +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> > > --- > > drivers/net/virtio_net.c | 13 +++++++++++++ > > 1 files changed, 13 insertions(+), 0 deletions(-) > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > index 0c7321c..bde0dec 100644 > > --- a/drivers/net/virtio_net.c > > +++ b/drivers/net/virtio_net.c > > @@ -195,6 +195,19 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi, > > len -= copy; > > offset += copy; > > > > + /* > > + * Verify that we can indeed put this data into a skb. > > + * This is here to handle cases when the device erroneously > > + * tries to receive more than is possible. This is usually > > + * the case of a broken device. > > + */ > > + if (unlikely(len > MAX_SKB_FRAGS * PAGE_SIZE)) { > > + if (net_ratelimit()) > > + pr_debug("%s: too much data\n", skb->dev->name); > > + dev_kfree_skb(skb); > > + return NULL; > > + } > > + > > BTW, receive_mergeable does > pr_debug("%s: packet too long\n", skb->dev->name); > skb->dev->stats.rx_length_errors++; > > which makes sense. Do you think we should increase rx_length_errors here as well?
On Wed, Oct 05, 2011 at 03:50:54PM +0200, Sasha Levin wrote: > On Mon, 2011-10-03 at 21:04 +0200, Michael S. Tsirkin wrote: > > On Wed, Sep 28, 2011 at 05:40:54PM +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> > > > --- > > > drivers/net/virtio_net.c | 13 +++++++++++++ > > > 1 files changed, 13 insertions(+), 0 deletions(-) > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > > index 0c7321c..bde0dec 100644 > > > --- a/drivers/net/virtio_net.c > > > +++ b/drivers/net/virtio_net.c > > > @@ -195,6 +195,19 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi, > > > len -= copy; > > > offset += copy; > > > > > > + /* > > > + * Verify that we can indeed put this data into a skb. > > > + * This is here to handle cases when the device erroneously > > > + * tries to receive more than is possible. This is usually > > > + * the case of a broken device. > > > + */ > > > + if (unlikely(len > MAX_SKB_FRAGS * PAGE_SIZE)) { > > > + if (net_ratelimit()) > > > + pr_debug("%s: too much data\n", skb->dev->name); > > > + dev_kfree_skb(skb); > > > + return NULL; > > > + } > > > + > > > > BTW, receive_mergeable does > > pr_debug("%s: packet too long\n", skb->dev->name); > > skb->dev->stats.rx_length_errors++; > > > > which makes sense. > > Do you think we should increase rx_length_errors here as well? this is all debugging tool for devices/drivers, right? so maybe not worth the noise. > -- > > Sasha. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Sasha Levin <levinsasha928@gmail.com> Date: Wed, 28 Sep 2011 17:40:54 +0300 > 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> Applied to net-next -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index 0c7321c..bde0dec 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -195,6 +195,19 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi, len -= copy; offset += copy; + /* + * Verify that we can indeed put this data into a skb. + * This is here to handle cases when the device erroneously + * tries to receive more than is possible. This is usually + * the case of a broken device. + */ + if (unlikely(len > MAX_SKB_FRAGS * PAGE_SIZE)) { + if (net_ratelimit()) + pr_debug("%s: too much data\n", skb->dev->name); + dev_kfree_skb(skb); + return NULL; + } + while (len) { set_skb_frag(skb, page, offset, &len); page = (struct page *)page->private;
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> --- drivers/net/virtio_net.c | 13 +++++++++++++ 1 files changed, 13 insertions(+), 0 deletions(-)