[PATCH] virtio_net: use correct accessors for scatterlists

[PATCH] virtio_net: use correct accessors for scatterlists

[Ira Snyder]

Without this fix, virtio_net makes incorrect usage of scatterlists. It sets
the end of the scatterlist chain after the first element, despite the fact
that more entries come after it.

If you try to run dma_map_sg() on one of the scatterlists given to you by
add_buf(), you will get a null pointer oops.

Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
---

I found this problem while working on implementing virtio-over-PCI as
suggested by Arnd Bergmann, David Miller, and others.

 drivers/net/virtio_net.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 43f6523..67ea2cf 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -286,7 +286,7 @@ static void try_fill_recv_maxbufs(struct virtnet_info *vi)
 		skb_put(skb, MAX_PACKET_LEN);
 
 		hdr = skb_vnet_hdr(skb);
-		sg_init_one(sg, hdr, sizeof(*hdr));
+		sg_set_buf(sg, hdr, sizeof(*hdr));
 
 		if (vi->big_packets) {
 			for (i = 0; i < MAX_SKB_FRAGS; i++) {
@@ -487,9 +487,9 @@ static int xmit_skb(struct virtnet_info *vi, struct sk_buff *skb)
 
 	/* Encode metadata header at front. */
 	if (vi->mergeable_rx_bufs)
-		sg_init_one(sg, mhdr, sizeof(*mhdr));
+		sg_set_buf(sg, mhdr, sizeof(*mhdr));
 	else
-		sg_init_one(sg, hdr, sizeof(*hdr));
+		sg_set_buf(sg, hdr, sizeof(*hdr));
 
 	num = skb_to_sgvec(skb, sg+1, 0, skb->len) + 1;
 
-- 
1.5.4.3

Re: [PATCH] virtio_net: use correct accessors for scatterlists

[Ira Snyder]

On Sun, Jan 25, 2009 at 12:10:10AM +1030, Rusty Russell wrote:
> On Saturday 24 January 2009 10:40:53 Ira Snyder wrote:
> > Without this fix, virtio_net makes incorrect usage of scatterlists. It sets
> > the end of the scatterlist chain after the first element, despite the fact
> > that more entries come after it.
> 
> Yes, but shouldn't you also be doing sg_mark_end() here somewhere if we're
> going to use this new sg mechanism?
> 

sg_init_table() and sg_init_one() have an sg_mark_end() inside them, so
the scatterlists are terminated at their last entry.

In the only case where the scatterlist has a single entry, we use
sg_init_one(), which is terminated correctly.

In all other cases, we use skb_to_sgvec(), which calls sg_mark_end().

In other words, they're all magically terminated at the right place. I
didn't even think about it until you mentioned it, but I just verified
that in every case, they get terminated correctly. So the patch is ok
as-is.

The only reason they can't be terminated early is that the for_each_sg()
loop in arch/x86/kernel/pci-nommu.c nommu_map_sg() gets a null pointer
for s, and then we oops. It doesn't check that the scatterlist ended
early. I'd argue that it is the definition of for_each_sg() that is
wrong, because it doesn't check for sg == NULL, which sg_next() returns
when you reach the end of the chain. I don't want to be changing core
kernel code, though.

> I despise the new scatterlist magic chaining no-typesafety crap too much to get around to doing the Right Thing, which is to change vq_ops->add_buf to take two (terminated) scatterlists instead of a sg array and two numbers.
> 

I didn't want to change the API. I'm comfortable with how virtio_net
works, but I haven't even tried to understand virtio_console or
virtio_blk yet.

The main problem I've run into is that the scatterlists are allocated on
the stack. Since I'm trying to do DMA transfers, I run dma_map_sg() on
them in add_buf(), and then I need to run dma_unmap_sg() on them in
get_buf(), after the DMA has completed. I've resorted to making copies
of the scatterlists.

Also, I have no idea how get()/set(), get_status()/set_status(), and
get_features()/finalize_features() are supposed to work. I'm going to
try and hook up two instances of virtio_net in memory, and see if I can
get them to communicate. Any pointers on that? :)

> But if you want to look at that... :)

Maybe later. I need to get something working first.

Thanks for the input.
Ira

Re: [PATCH] virtio_net: use correct accessors for scatterlists

[Rusty Russell]

On Saturday 24 January 2009 10:40:53 Ira Snyder wrote:
> Without this fix, virtio_net makes incorrect usage of scatterlists. It sets
> the end of the scatterlist chain after the first element, despite the fact
> that more entries come after it.
> 
> If you try to run dma_map_sg() on one of the scatterlists given to you by
> add_buf(), you will get a null pointer oops.
> 
> Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>

Thanks, this should go in 2.6.29.  Dave?

Cheers,
Rusty.

Subject: virtio_net: use correct accessors for scatterlists
Date: Fri, 23 Jan 2009 16:10:53 -0800
From: Ira Snyder <iws@ovro.caltech.edu>

Without this fix, virtio_net makes incorrect usage of scatterlists. It sets
the end of the scatterlist chain after the first element, despite the fact
that more entries come after it.

If you try to run dma_map_sg() on one of the scatterlists given to you by
add_buf(), you will get a null pointer oops.

Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 drivers/net/virtio_net.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 43f6523..67ea2cf 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -286,7 +286,7 @@ static void try_fill_recv_maxbufs(struct virtnet_info *vi)
 		skb_put(skb, MAX_PACKET_LEN);
 
 		hdr = skb_vnet_hdr(skb);
-		sg_init_one(sg, hdr, sizeof(*hdr));
+		sg_set_buf(sg, hdr, sizeof(*hdr));
 
 		if (vi->big_packets) {
 			for (i = 0; i < MAX_SKB_FRAGS; i++) {
@@ -487,9 +487,9 @@ static int xmit_skb(struct virtnet_info *vi, struct sk_buff *skb)
 
 	/* Encode metadata header at front. */
 	if (vi->mergeable_rx_bufs)
-		sg_init_one(sg, mhdr, sizeof(*mhdr));
+		sg_set_buf(sg, mhdr, sizeof(*mhdr));
 	else
-		sg_init_one(sg, hdr, sizeof(*hdr));
+		sg_set_buf(sg, hdr, sizeof(*hdr));
 
 	num = skb_to_sgvec(skb, sg+1, 0, skb->len) + 1;
 

Re: [PATCH] virtio_net: use correct accessors for scatterlists

[David Miller]

From: Rusty Russell <rusty@rustcorp.com.au>
Date: Tue, 27 Jan 2009 15:21:19 +1030

> On Saturday 24 January 2009 10:40:53 Ira Snyder wrote:
> > Without this fix, virtio_net makes incorrect usage of scatterlists. It sets
> > the end of the scatterlist chain after the first element, despite the fact
> > that more entries come after it.
> > 
> > If you try to run dma_map_sg() on one of the scatterlists given to you by
> > add_buf(), you will get a null pointer oops.
> > 
> > Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
> 
> Thanks, this should go in 2.6.29.  Dave?

Applied, should I queue it up for -stable too Rusty?

Thanks.

Re: [PATCH] virtio_net: use correct accessors for scatterlists

[Rusty Russell]

On Tuesday 27 January 2009 15:30:55 David Miller wrote:
> From: Rusty Russell <rusty@rustcorp.com.au>
> Date: Tue, 27 Jan 2009 15:21:19 +1030
> 
> > On Saturday 24 January 2009 10:40:53 Ira Snyder wrote:
> > > Without this fix, virtio_net makes incorrect usage of scatterlists. It sets
> > > the end of the scatterlist chain after the first element, despite the fact
> > > that more entries come after it.
> > > 
> > > If you try to run dma_map_sg() on one of the scatterlists given to you by
> > > add_buf(), you will get a null pointer oops.
> > > 
> > > Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
> > 
> > Thanks, this should go in 2.6.29.  Dave?
> 
> Applied, should I queue it up for -stable too Rusty?

I don't think so.  It really is defined as an sg array, not an sg chain.  I'm happy to apply a little patch to make Ira's ongoing development easier, but no reason to panic about it.

Thanks,
Rusty.