netdev
[Top] [All Lists]

Re: [PATCH] remove net driver ugliness that sparse complains about

To: Linus Torvalds <torvalds@xxxxxxxx>
Subject: Re: [PATCH] remove net driver ugliness that sparse complains about
From: viro@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
Date: Sat, 29 May 2004 21:42:30 +0100
Cc: Jeff Garzik <jgarzik@xxxxxxxxx>, Netdev <netdev@xxxxxxxxxxx>, Linux Kernel <linux-kernel@xxxxxxxxxxxxxxx>, Andrew Morton <akpm@xxxxxxxx>, "David S. Miller" <davem@xxxxxxxxxx>, Arjan van de Ven <arjanv@xxxxxxxxxx>
In-reply-to: <Pine.LNX.4.58.0405291117511.1648@xxxxxxxxxxxxxxx>
References: <40B8D2F8.6090905@xxxxxxxxx> <Pine.LNX.4.58.0405291117511.1648@xxxxxxxxxxxxxxx>
Sender: netdev-bounce@xxxxxxxxxxx
User-agent: Mutt/1.4.1i
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.

<Prev in Thread] Current Thread [Next in Thread>