Received: with ECARTIS (v1.0.0; list netdev); Sat, 29 May 2004 13:42:37 -0700 (PDT) Received: from www.linux.org.uk (IDENT:93@parcelfarce.linux.theplanet.co.uk [195.92.249.252]) by oss.sgi.com (8.12.10/8.12.9) with SMTP id i4TKgWgi019732 for ; Sat, 29 May 2004 13:42:32 -0700 Received: from viro by www.linux.org.uk with local (Exim 4.33) id 1BUAf0-0007Bh-Cb; Sat, 29 May 2004 21:42:30 +0100 Date: Sat, 29 May 2004 21:42:30 +0100 From: viro@parcelfarce.linux.theplanet.co.uk To: Linus Torvalds Cc: Jeff Garzik , Netdev , Linux Kernel , Andrew Morton , "David S. Miller" , Arjan van de Ven Subject: Re: [PATCH] remove net driver ugliness that sparse complains about Message-ID: <20040529204230.GG12308@parcelfarce.linux.theplanet.co.uk> References: <40B8D2F8.6090905@pobox.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.4.1i X-archive-position: 5465 X-ecartis-version: Ecartis v1.0.0 Sender: netdev-bounce@oss.sgi.com Errors-to: netdev-bounce@oss.sgi.com X-original-sender: viro@parcelfarce.linux.theplanet.co.uk Precedence: bulk X-list: netdev On Sat, May 29, 2004 at 11:23:56AM -0700, Linus Torvalds wrote: > Since the whole point of sparse is to have _static_ typechecking, such > code will never be sparse-clean, and either we have to ignore it, or we > should split up the use into two different kinds of structures (with the > same members apart from the address space) and explicitly convert between > the two. I'd obviously prefer that approach, but it might be a fair > amount of work (most of it should be really trivial, though, and I suspect > it would clarify pointer usage a lot to know when a "struct msghdr" points > to user space, and when it points to kernel space. Or whatever - maybe > that was a bad example). Right now there is only one serious false positive I know about. put_user(0, dirent->d_name) and its equivanlents in some places. That's __typeof__() handling bug. The rest is easy to spot - ## handling is broken in minimally tricky cases, [arg] is not recognized in asm arguments, some __attribute__() are not recognized and string constant length limits sometimes bite in asm bodies. The rest of patchset (~360Kb right now, and it will grow more) does include several splittings of structs, BTW. It removes pretty much all noise on my alpha / amd64 / x86 builds; the rest is real issues. Probably the worst annoyance is iovec - there is almost no intersection between the code that expects kernel pointers in it with code that expects userland ones (majority). I hadn't split that one, but that's worth considering. sync kiocb is a disaster waiting to happen. ->write() of tty_driver will take some research - we might want to try and keep copying from userland in generic code instead of just splitting the method, but that will require figuring out the locking issues. A bunch of set_fs() users in compat code is simply broken and should be using compat_alloc_user_space() instead. They end up with a mix of kernel and userland pointers, and set_fs(KERNEL_DS) is not enough to handle that. console code has some moderately minor annoyances; compared to the ugliness of the entire code in that area they are not too interesting. One thing I would _really_ hate to see is use of typecasts just to shut sparse up - the point is to find the potentially problematic places, not to hide them. We probably need a flag for sparse that would warn about explicit typecasts changing noderef and address_space inside the pointers; it obviously won't help the casts to unsigned long and back, but those are presumably used when people really mean it.