diff mbox

virtio_net: use correct accessors for scatterlists

Message ID 20090124001053.GA20156@ovro.caltech.edu
State Superseded, archived
Delegated to: David Miller
Headers show

Commit Message

Ira Snyder Jan. 24, 2009, 12:10 a.m. UTC
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(-)


Ira Snyder Jan. 24, 2009, 6:41 p.m. UTC | #1
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

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


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