Re: Add sparse "__iomem" infrastructure to check PCI address usage

Re: Add sparse "__iomem" infrastructure to check PCI address usage

[Jeff Garzik]

Linux Kernel Mailing List wrote:
> --- a/include/linux/compiler.h	2004-09-11 00:26:40 -07:00
> +++ b/include/linux/compiler.h	2004-09-11 00:26:40 -07:00
> @@ -6,13 +6,17 @@
>  # define __kernel	/* default address space */
>  # define __safe		__attribute__((safe))
>  # define __force	__attribute__((force))
> +# define __iomem	__attribute__((noderef, address_space(2)))

Dumb gcc attribute questions:

1) what does force do? it doesn't appear to be in gcc 3.3.3 docs.

2) is "volatile ... __force" redundant?

3) can we use 'malloc' attribute on kmalloc?

Re: Add sparse "__iomem" infrastructure to check PCI address usage

[David S. Miller]

On Sun, 12 Sep 2004 20:26:38 -0400
Jeff Garzik <jgarzik@pobox.com> wrote:

> Dumb gcc attribute questions:
> 
> 1) what does force do? it doesn't appear to be in gcc 3.3.3 docs.
> 
> 2) is "volatile ... __force" redundant?
> 
> 3) can we use 'malloc' attribute on kmalloc?

Jeff, this code you quoted is in the sparse ifdef block.

Re: Add sparse "__iomem" infrastructure to check PCI address usage

[Linus Torvalds]

On Sun, 12 Sep 2004, Jeff Garzik wrote:
>
> Linux Kernel Mailing List wrote:
> > --- a/include/linux/compiler.h	2004-09-11 00:26:40 -07:00
> > +++ b/include/linux/compiler.h	2004-09-11 00:26:40 -07:00
> > @@ -6,13 +6,17 @@
> >  # define __kernel	/* default address space */
> >  # define __safe		__attribute__((safe))
> >  # define __force	__attribute__((force))
> > +# define __iomem	__attribute__((noderef, address_space(2)))
> 
> Dumb gcc attribute questions:
> 
> 1) what does force do? it doesn't appear to be in gcc 3.3.3 docs.

It doesn't do anything for gcc. You're looking at the sparse-only code.

What "attribute((force))" does for sparse is to mark a type to be 
"forced", ie a cast to a forced type will not complain even if the cast 
otherwise would be invalid.

For example, "sparse" will warn about explicit casts that drop address 
space information:

	void __user *userptr;

	...
	memset((void *)userptr, 0, ...)

will cause a

	warning: cast removes address space of expression

complaint from sparse. But some _internal_ functions want to force the 
cast because they know it's safe. For example, you'll find

	#define __addr_ok(addr) ((unsigned long __force)(addr) < (current_thread_info()->addr_limit.seg))

because this internal x86 implementation detail knows that in that 
particular case it's safe to remove the address space information (it's 
just checking the user pointer against the address limit).

For gcc, none of this means anything, so all the #define's just become 
empty.

> 2) is "volatile ... __force" redundant?

No, although it's likely to be a strange combination. If you want to force 
a static address space conversion to a volatile pointer, you can do so. I 
don't see _why_ you'd want to do it ;)

> 3) can we use 'malloc' attribute on kmalloc?

Since we can't use the gcc alias analysis anyway (it's too broken until
very late gcc versions), the gcc 'malloc' attribute shouldn't make any
difference that I can tell.

But there wouldn't be anything _wrong_ in adding it to kmalloc(), if 
that's what you're asking.

		Linus

Re: Add sparse "__iomem" infrastructure to check PCI address usage

[Jeff Garzik]

Linus Torvalds wrote:
> On Sun, 12 Sep 2004, Jeff Garzik wrote:
>>1) what does force do? it doesn't appear to be in gcc 3.3.3 docs.

> It doesn't do anything for gcc. You're looking at the sparse-only code.

doh, and thanks for the info.


>>2) is "volatile ... __force" redundant?

> No, although it's likely to be a strange combination. If you want to force 
> a static address space conversion to a volatile pointer, you can do so. I 
> don't see _why_ you'd want to do it ;)

Well the reason I ask....

static inline void writeb(unsigned char b, volatile void __iomem *addr)
{
         *(volatile unsigned char __force *) addr = b;
}
static inline void writew(unsigned short b, volatile void __iomem *addr)
{
         *(volatile unsigned short __force *) addr = b;
}
static inline void writel(unsigned int b, volatile void __iomem *addr)
{
         *(volatile unsigned int __force *) addr = b;
}


>>3) can we use 'malloc' attribute on kmalloc?

> Since we can't use the gcc alias analysis anyway (it's too broken until
> very late gcc versions), the gcc 'malloc' attribute shouldn't make any
> difference that I can tell.
> 
> But there wouldn't be anything _wrong_ in adding it to kmalloc(), if 
> that's what you're asking.

That's what I'm asking.

Thanks,

	Jeff

Re: Add sparse "__iomem" infrastructure to check PCI address usage

[Linus Torvalds]

On Sun, 12 Sep 2004, Jeff Garzik wrote:
> 
> > No, although it's likely to be a strange combination. If you want to force 
> > a static address space conversion to a volatile pointer, you can do so. I 
> > don't see _why_ you'd want to do it ;)
> 
> Well the reason I ask....
> 
> static inline void writeb(unsigned char b, volatile void __iomem *addr)
> {
>          *(volatile unsigned char __force *) addr = b;
> }

Right. Let's look a bit closer (more of an explanation than you need, but 
hey, maybe somebody else is also wondering):

 - for gcc, none of this matters one whit. We're just passing in a 
   "volatile void *", and we're storing the value "b" to the byte pointed
   by it. Which is correct on x86, since memory-mapped PCI-space just 
   looks like memory on x86.

   This is important to remember: for gcc, the sparse annotations are 
   meaningless. They can still be useful just to tell the _programmer_ 
   that "hey, that pointer you got wasn't a normal pointer" in a fairly 
   readable manner, but in the end, unless you use sparse, they don't 
   actually _do_ anything.

HOWEVER. When you _do_ use parse, it is another matter entirely. For
"sparse", that "__iomem"  has lots of meaning:

	# define __iomem       __attribute__((noderef, address_space(2)))

ie "iomem" means two separate things: it means that sparse should complain 
if the pointer is ever dereferenced (it's a "noderef" pointer) directly, 
and it's in "address space 2" as opposed to the normal address space (0).

Now, that means that _sparse_ will complain if such a pointer is ever
passed into a function that wants a regular pointer (because it is _not_ a
normal pointer, and you obviously shouldn't do things like "strcmp()" etc 
on it), and sparse will also complain if you try to cast it to another 
pointer in another address space.

So if you compile and install sparse, and build with "make C=1", you'll 
get warnings like

	drivers/video/aty/radeon_base.c:1725:42: warning: incorrect type in argument 2 (different address spaces)
	drivers/video/aty/radeon_base.c:1725:42:    expected void const *from
	drivers/video/aty/radeon_base.c:1725:42:    got void [noderef] *[assigned] base_addr<asn:2>

which is just another way sparse tells you that there is a bug in the 
source code (in this case, we try to copy from PCI memory-mapped space 
directly to user space using "copy_to_user()", which is a _bad_ idea).

So not only can you not dereference it by mistake, you can't even _turn_
it into a pointer that you could dereference by mistake. Sparse will
complain. Sparse will complain even if you use an explicit cast to make it 
a normal "(void *)".

And that's good, because on some other architectures, if you try to 
dereference the pointer, the machine just oopses. You need to do all the 
special magic to actually read from memory-mapped PCI space.

HOWEVER. On x86, it just so happens that dereferencing the pointer _is_ 
actually the right thing to do, as long as you only do it with the proper 
interfaces (ie readb/writeb and friends). And so that sparse won't be 
upset, we use the "__force" directive - we're telling sparse that "I know 
what I'm doing". And so sparse will quietly allow us to dereference that 
pointer that was originally not dereferencable.

Generally, you shouldn't ever use __force in a driver or anything like 
that. It's usually a valid thing to do only in the code that is defined 
for that particular type. By definition, a driver isn't an entity that 
should understand how architecture-specific data structures work, so it 
shouldn't try to force things.

So a good use of "__force" is exactly the usage you quote: the 
arch-specific code that actually really _knows_ what the magic address 
space means, and knows what to do about it.

		Linus

Re: Add sparse "__iomem" infrastructure to check PCI address usage

[Geert Uytterhoeven]

While resuming adding __user annotations to the m68k-specific parts of the
code, I stumbled on

    struct task_struct {
	...
	unsigned long sas_ss_sp;
	...
    }

If I'm not mistaken, sas_ss_sp is always a pointer to user stack space.
Shouldn't it be changed to `void __user *sas_ss_sp', or is an
unsigned long/void * change in generic code a too controversial change for
making sparse happy?

And I guess I can find a few more of these...

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: Add sparse "__iomem" infrastructure to check PCI address usage

[Linus Torvalds]

On Mon, 13 Sep 2004, Geert Uytterhoeven wrote:
> 
> While resuming adding __user annotations to the m68k-specific parts of the
> code, I stumbled on
> 
>     struct task_struct {
> 	...
> 	unsigned long sas_ss_sp;
> 	...
>     }
> 
> If I'm not mistaken, sas_ss_sp is always a pointer to user stack space.
> Shouldn't it be changed to `void __user *sas_ss_sp', or is an
> unsigned long/void * change in generic code a too controversial change for
> making sparse happy?

I don't think it's too controversial per se, and it would certainly remove 
at least two casts from kernel/signal.c. Are you ready to fix up some 
other architectures from the fall-out (x86 at the least..)

		Linus

Re: Add sparse "__iomem" infrastructure to check PCI address usage

[Tonnerre]

[-- Attachment #1: Type: text/plain, Size: 362 bytes --]

Salut,

On Sun, Sep 12, 2004 at 08:00:48PM -0700, Linus Torvalds wrote:
> Generally, you shouldn't ever use __force in a driver or anything like 
> that.

Why don't we send the __force attribute into some #ifdef that is never
defined unless  you're in  arch specific code?  This way  we'd prevent
stupid people from doing stupid things.

				Tonnerre

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: Add sparse "__iomem" infrastructure to check PCI address usage

[Linus Torvalds]

On Mon, 13 Sep 2004, Tonnerre wrote:
> 
> On Sun, Sep 12, 2004 at 08:00:48PM -0700, Linus Torvalds wrote:
> > Generally, you shouldn't ever use __force in a driver or anything like 
> > that.
> 
> Why don't we send the __force attribute into some #ifdef that is never
> defined unless  you're in  arch specific code?  This way  we'd prevent
> stupid people from doing stupid things.

Hey, the thing is, that may well prevent smart people from doing smart 
things too. Give 'em rope, within reason.

The point behind __force is not so much that you should never use it, but 
that you should never use it by _mistake_. It's very easy (and often 
_required_) to have a regular typecast in C, and it can often hide bugs 
when you cast something in a way that you didn't really think through.

For example, we often have generic "void *" or "unsigned long" things that
are used for passing opaque data around, and they need casts when working
with them. It is quite conceivable that you need an address space cast
too, and that you need to use "__force" to do so. It might be ok, even in
a driver. But hopefully it's something where the action of having to force 
the cast makes people think about it more. And the fact that there 
probably never will be very _man_ casts like that means that they'll stand 
out.

If people start using "__force" to hide their own bugs, we'll just have to 
start stringing them up. Hang'em high, I say. But maybe somebody has a 
valid reason at times.

		Linus

Re: Add sparse "__iomem" infrastructure to check PCI address usage

[viro]

On Mon, Sep 13, 2004 at 08:31:26PM +0200, Tonnerre wrote:
> Salut,
> 
> On Sun, Sep 12, 2004 at 08:00:48PM -0700, Linus Torvalds wrote:
> > Generally, you shouldn't ever use __force in a driver or anything like 
> > that.
> 
> Why don't we send the __force attribute into some #ifdef that is never
> defined unless  you're in  arch specific code?  This way  we'd prevent
> stupid people from doing stupid things.

man grep