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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <20090124184154.GA18513@ovro.caltech.edu>
Date:	Sat, 24 Jan 2009 10:41:54 -0800
From:	Ira Snyder <iws@...o.caltech.edu>
To:	Rusty Russell <rusty@...tcorp.com.au>
Cc:	linux-kernel@...r.kernel.org, Mark McLoughlin <markmc@...hat.com>,
	netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH] virtio_net: use correct accessors for scatterlists

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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html