constness bug in include/linux/compiler.h

constness bug in include/linux/compiler.h

[Russ Cox]

Probably sparse just doesn't pay attention to const,
but shouldn't the definitions of __chk_user_ptr
and __chk_io_ptr be

    extern void __chk_user_ptr(const void __user *);
    extern void __chk_io_ptr(const void __iomem *);

instead of

    extern void __chk_user_ptr(void __user *);
    extern void __chk_io_ptr(void __iomem *);

?

Russ

Re: constness bug in include/linux/compiler.h

[Josh Triplett]

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

Russ Cox wrote:
> Probably sparse just doesn't pay attention to const,

Actually, it does:

$ cat /tmp/const.c
void f(int *p)
{
}

void g(void)
{
    int i = 0;
    const int *p = &i;
    f(p);
}
$ ./sparse /tmp/const.c
/tmp/const.c:9:7: warning: incorrect type in argument 1 (different modifiers)
/tmp/const.c:9:7:    expected int *p
/tmp/const.c:9:7:    got int const *p

> but shouldn't the definitions of __chk_user_ptr
> and __chk_io_ptr be
> 
>     extern void __chk_user_ptr(const void __user *);
>     extern void __chk_io_ptr(const void __iomem *);
> 
> instead of
> 
>     extern void __chk_user_ptr(void __user *);
>     extern void __chk_io_ptr(void __iomem *);
> 
> ?

Yes, that makes sense.  These functions just check for the annotation on their
pointer argument, and having the const annotation would allow them to check
const pointers without provoking a warning due to the lack of const.

- Josh Triplett


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

Re: constness bug in include/linux/compiler.h

[Russ Cox]

> Yes, that makes sense.  These functions just check for the annotation on their
> pointer argument, and having the const annotation would allow them to check
> const pointers without provoking a warning due to the lack of const.

They don't provoke a warning now.  Sparse is silent on this program:

	#define __user __attribute__((noderef, address_space(1)))

	extern void __chk_user_ptr(void __user *);

	void
	f(const void __user *p)
	{
		__chk_user_ptr(p);
	}

but arguably they should (and then be changed to const void __user*).

For an actual example, __copy_from_user_inatomic has a
const void __user* argument which it passes (via __get_user_size)
to __chk_user_ptr.

Russ

Re: constness bug in include/linux/compiler.h

[Josh Triplett]

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

Russ Cox wrote:
>> Yes, that makes sense.  These functions just check for the annotation on their
>> pointer argument, and having the const annotation would allow them to check
>> const pointers without provoking a warning due to the lack of const.
> 
> They don't provoke a warning now.  Sparse is silent on this program:
> 
> 	#define __user __attribute__((noderef, address_space(1)))
> 
> 	extern void __chk_user_ptr(void __user *);
> 
> 	void
> 	f(const void __user *p)
> 	{
> 		__chk_user_ptr(p);
> 	}

Hmmm, odd.  After a few iterations, I managed to discover that Sparse will
warn if you attempt to convert a const int * to an int *, but not if you
attempt to convert a const void * to a void *.  This seems like a bug to me.

> but arguably they should (and then be changed to const void __user*).

Yes.  Please do go ahead and propose the obvious const-adding patch to
include/linux/compiler.h, Cc linux-sparse, and feel free to add a
Signed-off-by from me.

- Josh Triplett


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

Re: constness bug in include/linux/compiler.h

[Josh Triplett]

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

Josh Triplett wrote:
> Russ Cox wrote:
>>> Yes, that makes sense.  These functions just check for the annotation on their
>>> pointer argument, and having the const annotation would allow them to check
>>> const pointers without provoking a warning due to the lack of const.
>> They don't provoke a warning now.  Sparse is silent on this program:
>>
>> 	#define __user __attribute__((noderef, address_space(1)))
>>
>> 	extern void __chk_user_ptr(void __user *);
>>
>> 	void
>> 	f(const void __user *p)
>> 	{
>> 		__chk_user_ptr(p);
>> 	}
> 
> Hmmm, odd.  After a few iterations, I managed to discover that Sparse will
> warn if you attempt to convert a const int * to an int *, but not if you
> attempt to convert a const void * to a void *.  This seems like a bug to me.

This bug appears to come from the following code in compatible_assignment_types:

                /* "void *" matches anything as long as the address space is OK */
                target_as = t->ctype.as | target->ctype.as;
                source_as = s->ctype.as | source->ctype.as;
                if (source_as == target_as && (s->type == SYM_PTR || s->type == SYM_ARRAY)) {
                        s = get_base_type(s);
                        t = get_base_type(t);
                        if (s == &void_ctype || t == &void_ctype)
                                goto Cast;
                }

This should almost certainly check more than just address spaces.

- Josh Triplett


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

Re: constness bug in include/linux/compiler.h

[Linus Torvalds]

On Fri, 16 Mar 2007, Josh Triplett wrote:
> 
> Hmmm, odd.  After a few iterations, I managed to discover that Sparse will
> warn if you attempt to convert a const int * to an int *, but not if you
> attempt to convert a const void * to a void *.  This seems like a bug to me.

Yeah, I think we allow *any* conversion to "void *". We probably should 
still check const/volatile.

		Linus