On Tue, 2007-02-13 at 02:55 +1100, Herbert Xu wrote:

On Mon, Feb 12, 2007 at 02:52:01PM +1100, Rusty Russell wrote:

quoted

+static void skb_to_dma(const struct sk_buff *skb, unsigned int len,
+		       struct lguest_dma *dma)
+{
+	unsigned int i, seg;
+
+	for (i = seg = 0; i < len; seg++, i += rest_of_page(skb->data + i)) {

The length field from the skb covers the fragmented parts as well.
So you don't want to use it as the boundary for the loop over the
skb header data.  As it is, if the skb has fragments, the first
loop will try to read them off the header.

Good spotting!  This function needs to be passed skb_headlen(skb),
rather than skb->len.  Patch is below (I renamed the parameter as well,
for clarity).

It's fascinating why this "worked".  Firstly, for inter-guest
communication, I am not calculating checksums, nor checking them.
Secondly, for guest->host, I turn off hw checksumming so the kernel
disables SG and this code is never executed.  Thirdly, for virtbench's
inter-guest sendfile test does not check the data received is actually
correct.  Finally, while we end up producing a packet which is larger
than skb->len of 1514, the DMA receive buffer for the other guest is
only 1514 bytes, so the result of the transfer is 1514 (==skb->len), so
no error reported by the driver.

==
lguest network driver fix: handle scatter-gather packets correctly

Thanks to Herbert Xu for noticing the bug: "len" here is skb_headlen(),
not skb->len.  Renamed the function to clarify, too.

Signed-off-by: Rusty Russell <redacted>

diff -r ed6484d145a4 drivers/net/lguest_net.c

--- a/drivers/net/lguest_net.c	Tue Feb 13 11:30:39 2007 +1100
+++ b/drivers/net/lguest_net.c	Tue Feb 13 12:07:15 2007 +1100

@@ -59,14 +59,14 @@ static unsigned long peer_addr(struct lg
 	return info->peer_phys + 4 * peernum;
 }
 
-static void skb_to_dma(const struct sk_buff *skb, unsigned int len,
+static void skb_to_dma(const struct sk_buff *skb, unsigned int headlen,
 		       struct lguest_dma *dma)
 {
 	unsigned int i, seg;
 
-	for (i = seg = 0; i < len; seg++, i += rest_of_page(skb->data + i)) {
+	for (i = seg = 0; i < headlen; seg++, i += rest_of_page(skb->data+i)) {
 		dma->addr[seg] = virt_to_phys(skb->data + i);
-		dma->len[seg] = min((unsigned)(len - i),
+		dma->len[seg] = min((unsigned)(headlen - i),
 				    rest_of_page(skb->data + i));
 	}
 	for (i = 0; i < skb_shinfo(skb)->nr_frags; i++, seg++) {

@@ -90,7 +90,7 @@ static void transfer_packet(struct net_d
 	struct lguestnet_info *info = dev->priv;
 	struct lguest_dma dma;
 
-	skb_to_dma(skb, skb->len, &dma);
+	skb_to_dma(skb, skb_headlen(skb), &dma);
 	pr_debug("xfer length %04x (%u)\n", htons(skb->len), skb->len);
 
 	hcall(LHCALL_SEND_DMA, peer_addr(info,peernum), __pa(&dma),0);