Re: [RFC] adding into middle of RCU list

Re: [RFC] adding into middle of RCU list

[Paul E. McKenney]

On Fri, Aug 23, 2013 at 02:08:22PM -0700, Paul E. McKenney wrote:
> On Fri, Aug 23, 2013 at 01:16:53PM -0400, Mathieu Desnoyers wrote:
> > * Paul E. McKenney (paulmck@linux.vnet.ibm.com) wrote:
> > > On Thu, Aug 22, 2013 at 09:33:18PM -0700, Stephen Hemminger wrote:

[ . . . ]

> > > > +
> > > > +/**
> > > > + * Splice an RCU-protected list into an existing list.
> > > > + *
> > > > + * Note that this function blocks in synchronize_rcu()
> > > > + *
> > > > + * Important note: this function is not called concurrently
> > > > + *       with other updates to the list.
> > > > + */
> > > > +static inline void caa_list_splice_init_rcu(struct cds_list_head *list,
> > > > +					    struct cds_list_head *head)
> > > > +{
> > > > +	struct cds_list_head *first = list->next;
> > > > +	struct cds_list_head *last = list->prev;
> > > > +	struct cds_list_head *at = head->next;
> > > > +
> > > > +	if (cds_list_empty(list))
> > > > +		return;
> > > > +
> > > > +	/* "first" and "last" tracking list, so initialize it. */
> > > > +	CDS_INIT_LIST_HEAD(list);
> > > 
> > > This change is happening in the presence of readers on the list, right?
> > > For this to work reliably in the presence of mischievous compilers,
> > > wouldn't CDS_INIT_LIST_HEAD() need to use CMM_ACCESS_ONCE() for its
> > > pointer accesses?
> > 
> > Actually, we have rcu_assign_pointer()/rcu_set_pointer() exactly for
> > this. They even skip the memory barrier if they store a NULL pointer.
> > 
> > > Hmmm...  The kernel version seems to have the same issue...
> > 
> > The compiler memory model of the Linux kernel AFAIK does not require an
> > ACCESS_ONCE() for stores to word-aligned, word-sized integers/pointers,
> > even if those are expected to be read concurrently. For reference, see:
> > 
> > #define __rcu_assign_pointer(p, v, space) \
> >         do { \
> >                 smp_wmb(); \
> >                 (p) = (typeof(*v) __force space *)(v); \
> >         } while (0)
> 
> Or I need to fix this one as well.  ;-)

In that vein...  Is there anything like typeof() that also preserves
sparse's notion of address space?  Wrapping an ACCESS_ONCE() around
"p" in the assignment above results in sparse errors.

							Thanx, Paul

							Thanx, Paul

Re: [RFC] adding into middle of RCU list

[Josh Triplett]

On Thu, Aug 29, 2013 at 05:57:33PM -0700, Paul E. McKenney wrote:
> On Fri, Aug 23, 2013 at 02:08:22PM -0700, Paul E. McKenney wrote:
> > On Fri, Aug 23, 2013 at 01:16:53PM -0400, Mathieu Desnoyers wrote:
> > > #define __rcu_assign_pointer(p, v, space) \
> > >         do { \
> > >                 smp_wmb(); \
> > >                 (p) = (typeof(*v) __force space *)(v); \
> > >         } while (0)
> > 
> > Or I need to fix this one as well.  ;-)
> 
> In that vein...  Is there anything like typeof() that also preserves
> sparse's notion of address space?  Wrapping an ACCESS_ONCE() around
> "p" in the assignment above results in sparse errors.

typeof() will preserve sparse's notion of address space as long as you
do typeof(p), not typeof(*p):

$ cat test.c
#define as(n) __attribute__((address_space(n),noderef))
#define __force __attribute__((force))

int main(void)
{
    int target = 0;
    int as(1) *foo = (__force typeof(target) as(1) *) ⌖
    typeof(foo) bar = foo;
    return *bar;
}
$ sparse test.c
test.c:9:13: warning: dereference of noderef expression

Notice that sparse didn't warn on the assignment of foo to bar (because
typeof propagated the address space of 1), and warned on the dereference
of bar (because typeof propagated noderef).

- Josh Triplett

Re: [RFC] adding into middle of RCU list

[Paul E. McKenney]

On Thu, Aug 29, 2013 at 07:16:37PM -0700, Josh Triplett wrote:
> On Thu, Aug 29, 2013 at 05:57:33PM -0700, Paul E. McKenney wrote:
> > On Fri, Aug 23, 2013 at 02:08:22PM -0700, Paul E. McKenney wrote:
> > > On Fri, Aug 23, 2013 at 01:16:53PM -0400, Mathieu Desnoyers wrote:
> > > > #define __rcu_assign_pointer(p, v, space) \
> > > >         do { \
> > > >                 smp_wmb(); \
> > > >                 (p) = (typeof(*v) __force space *)(v); \
> > > >         } while (0)
> > > 
> > > Or I need to fix this one as well.  ;-)
> > 
> > In that vein...  Is there anything like typeof() that also preserves
> > sparse's notion of address space?  Wrapping an ACCESS_ONCE() around
> > "p" in the assignment above results in sparse errors.
> 
> typeof() will preserve sparse's notion of address space as long as you
> do typeof(p), not typeof(*p):
> 
> $ cat test.c
> #define as(n) __attribute__((address_space(n),noderef))
> #define __force __attribute__((force))
> 
> int main(void)
> {
>     int target = 0;
>     int as(1) *foo = (__force typeof(target) as(1) *) ⌖
>     typeof(foo) bar = foo;
>     return *bar;
> }
> $ sparse test.c
> test.c:9:13: warning: dereference of noderef expression
> 
> Notice that sparse didn't warn on the assignment of foo to bar (because
> typeof propagated the address space of 1), and warned on the dereference
> of bar (because typeof propagated noderef).

Thank you for the info!

Suppose that I want to do something like this:

#define __rcu_assign_pointer(p, v, space) \
        do { \
                smp_wmb(); \
                ACCESS_ONCE(p) = (typeof(*v) __force space *)(v); \
        } while (0)

Now, this does typeof(*p), so as you noted above sparse complains about
address-space mismatches.  Thus far, I haven't been able to come up with
something that (1) does sparse address-space checking, (2) does C type
checking, and (3) forces the assignment to be volatile.

Any thoughts on how to do this?

							Thanx, Paul

Re: [RFC] adding into middle of RCU list

[Josh Triplett]

On Sat, Aug 31, 2013 at 02:32:28PM -0700, Paul E. McKenney wrote:
> On Thu, Aug 29, 2013 at 07:16:37PM -0700, Josh Triplett wrote:
> > On Thu, Aug 29, 2013 at 05:57:33PM -0700, Paul E. McKenney wrote:
> > > On Fri, Aug 23, 2013 at 02:08:22PM -0700, Paul E. McKenney wrote:
> > > > On Fri, Aug 23, 2013 at 01:16:53PM -0400, Mathieu Desnoyers wrote:
> > > > > #define __rcu_assign_pointer(p, v, space) \
> > > > >         do { \
> > > > >                 smp_wmb(); \
> > > > >                 (p) = (typeof(*v) __force space *)(v); \
> > > > >         } while (0)
> > > > 
> > > > Or I need to fix this one as well.  ;-)
> > > 
> > > In that vein...  Is there anything like typeof() that also preserves
> > > sparse's notion of address space?  Wrapping an ACCESS_ONCE() around
> > > "p" in the assignment above results in sparse errors.
> > 
> > typeof() will preserve sparse's notion of address space as long as you
> > do typeof(p), not typeof(*p):
> > 
> > $ cat test.c
> > #define as(n) __attribute__((address_space(n),noderef))
> > #define __force __attribute__((force))
> > 
> > int main(void)
> > {
> >     int target = 0;
> >     int as(1) *foo = (__force typeof(target) as(1) *) ⌖
> >     typeof(foo) bar = foo;
> >     return *bar;
> > }
> > $ sparse test.c
> > test.c:9:13: warning: dereference of noderef expression
> > 
> > Notice that sparse didn't warn on the assignment of foo to bar (because
> > typeof propagated the address space of 1), and warned on the dereference
> > of bar (because typeof propagated noderef).
> 
> Thank you for the info!
> 
> Suppose that I want to do something like this:
> 
> #define __rcu_assign_pointer(p, v, space) \
>         do { \
>                 smp_wmb(); \
>                 ACCESS_ONCE(p) = (typeof(*v) __force space *)(v); \
>         } while (0)
> 
> Now, this does typeof(*p), so as you noted above sparse complains about
> address-space mismatches.  Thus far, I haven't been able to come up with
> something that (1) does sparse address-space checking, (2) does C type
> checking, and (3) forces the assignment to be volatile.
> 
> Any thoughts on how to do this?

First of all, if p and v had compatible types *including* address
spaces, you wouldn't need the "space" argument; the following
self-contained test case passes both sparse and GCC typechecking:

#define as(n) __attribute__((address_space(n),noderef))
#define __force __attribute__((force))
#define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
extern void smp_wmb(void);

#define rcu_assign_pointer(p, v) \
    do { \
        smp_wmb(); \
        ACCESS_ONCE(p) = (v); \
    } while (0)

struct foo;

int main(void)
{
    struct foo as(1) *dest;
    struct foo as(1) *src = (void *)0;

    rcu_assign_pointer(dest, src);

    return 0;
}



But in this case, you want dest and src to have compatible types except
that dest must have the __rcu address space and src might not.  So,
let's change the types of dest and src, and add the appropriate cast.
The following also passes both GCC and sparse:

#define __rcu __attribute__((address_space(4),noderef))
#define __force __attribute__((force))
#define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
extern void smp_wmb(void);

#define rcu_assign_pointer(p, v) \
    do { \
        smp_wmb(); \
        ACCESS_ONCE(p) = (typeof(*(v)) __rcu __force *)(v); \
    } while (0)

struct foo { int x; };

int main(void)
{
    struct foo __rcu *dest;
    struct foo *src = (void *)0;

    rcu_assign_pointer(dest, src);

    return 0;
}


However, that cast forces the source to have the __rcu address space
without checking what address space it started out with.  If you want to
verify that the source has the kernel address space, you can cast to
that address space first, *without* __force, which will warn if the
source doesn't start out with that address space:

#define __kernel __attribute__((address_space(0)))
#define __user __attribute__((address_space(1),noderef))
#define __rcu __attribute__((address_space(4),noderef))
#define __force __attribute__((force))
#define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
extern void smp_wmb(void);

#define rcu_assign_pointer(p, v) \
    do { \
        smp_wmb(); \
        ACCESS_ONCE(p) = (typeof(*(v)) __rcu __force *)(typeof(*(v)) __kernel *)(v); \
    } while (0)

struct foo { int x; };

int main(void)
{
    struct foo __rcu *dest;
    struct foo *src = (void *)0;
    struct foo __user *badsrc = (void *)0;

    rcu_assign_pointer(dest, src);
    rcu_assign_pointer(dest, badsrc);

    return 0;
}


This produces a warning on the line using badsrc:

test.c:23:5: warning: cast removes address space of expression

However, that doesn't seem like the most obvious warning, since
rcu_assign_pointer doesn't look like a cast, and since it doesn't print
the full types involved like most address space warnings do.  So,
instead, let's add and use a __chk_kernel_ptr function, similar to
__chk_user_ptr in compiler.h:

#define __kernel __attribute__((address_space(0)))
#define __user __attribute__((address_space(1),noderef))
#define __rcu __attribute__((address_space(4),noderef))
#define __force __attribute__((force))
#define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
extern void __chk_kernel_ptr(const volatile void *);
extern void smp_wmb(void);

#define rcu_assign_pointer(p, v) \
    do { \
        smp_wmb(); \
        __chk_kernel_ptr(v); \
        ACCESS_ONCE(p) = (typeof(*(v)) __rcu __force *)(v); \
    } while (0)

struct foo { int x; };

int main(void)
{
    struct foo __rcu *dest;
    struct foo *src = (void *)0;
    struct foo __user *badsrc = (void *)0;

    rcu_assign_pointer(dest, src);
    rcu_assign_pointer(dest, badsrc);

    return 0;
}


This produces a somewhat better warning:

test.c:25:5: warning: incorrect type in argument 1 (different address spaces)
test.c:25:5:    expected void const volatile *<noident>
test.c:25:5:    got struct foo [noderef] <asn:1>*badsrc

That at least shows the full type of badsrc, but it still seems
suboptimal for two reasons: it says it expects "void const volatile *"
rather than the actual type it wants, and it says "in argument 1" (of
__chk_kernel_ptr), which seems unnecessarily confusing when the type
error actually applies to argument 2 of rcu_assign_pointer.  We can do
better by declaring a fake local function for checking, instead:

#define __kernel __attribute__((address_space(0)))
#define __user __attribute__((address_space(1),noderef))
#define __rcu __attribute__((address_space(4),noderef))
#define __force __attribute__((force))
#define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
extern void smp_wmb(void);

#define rcu_assign_pointer(p, v) \
    do { \
        smp_wmb(); \
        extern void __rcu_assign_pointer_typecheck(int, typeof(*(v)) __kernel *); \
        __rcu_assign_pointer_typecheck(0, v); \
        ACCESS_ONCE(p) = (typeof(*(v)) __rcu __force *)(v); \
    } while (0)

struct foo { int x; };

int main(void)
{
    struct foo __rcu *dest;
    struct foo *src = (void *)0;
    struct foo __user *badsrc = (void *)0;

    rcu_assign_pointer(dest, src);
    rcu_assign_pointer(dest, badsrc);

    return 0;
}


This last approach produces a very clear warning:

test.c:25:5: warning: incorrect type in argument 2 (different address spaces)
test.c:25:5:    expected struct foo *<noident>
test.c:25:5:    got struct foo [noderef] <asn:1>*badsrc

If you want, you can even add an argument name for the second argument
of __rcu_assign_pointer_typecheck, and it'll replace the <noident> in
the second line of the warning.

So, that last approach meets all the criteria you mentioned:
> something that (1) does sparse address-space checking, (2) does C type
> checking, and (3) forces the assignment to be volatile.

Will that work for all the use cases you have in mind?  If so, I'll
submit a patch changing rcu_assign_pointer to use that approach.

- Josh Triplett

Re: [RFC] adding into middle of RCU list

[Paul E. McKenney]

On Sun, Sep 01, 2013 at 01:42:10PM -0700, Josh Triplett wrote:
> On Sat, Aug 31, 2013 at 02:32:28PM -0700, Paul E. McKenney wrote:
> > On Thu, Aug 29, 2013 at 07:16:37PM -0700, Josh Triplett wrote:
> > > On Thu, Aug 29, 2013 at 05:57:33PM -0700, Paul E. McKenney wrote:
> > > > On Fri, Aug 23, 2013 at 02:08:22PM -0700, Paul E. McKenney wrote:
> > > > > On Fri, Aug 23, 2013 at 01:16:53PM -0400, Mathieu Desnoyers wrote:
> > > > > > #define __rcu_assign_pointer(p, v, space) \
> > > > > >         do { \
> > > > > >                 smp_wmb(); \
> > > > > >                 (p) = (typeof(*v) __force space *)(v); \
> > > > > >         } while (0)
> > > > > 
> > > > > Or I need to fix this one as well.  ;-)
> > > > 
> > > > In that vein...  Is there anything like typeof() that also preserves
> > > > sparse's notion of address space?  Wrapping an ACCESS_ONCE() around
> > > > "p" in the assignment above results in sparse errors.
> > > 
> > > typeof() will preserve sparse's notion of address space as long as you
> > > do typeof(p), not typeof(*p):
> > > 
> > > $ cat test.c
> > > #define as(n) __attribute__((address_space(n),noderef))
> > > #define __force __attribute__((force))
> > > 
> > > int main(void)
> > > {
> > >     int target = 0;
> > >     int as(1) *foo = (__force typeof(target) as(1) *) ⌖
> > >     typeof(foo) bar = foo;
> > >     return *bar;
> > > }
> > > $ sparse test.c
> > > test.c:9:13: warning: dereference of noderef expression
> > > 
> > > Notice that sparse didn't warn on the assignment of foo to bar (because
> > > typeof propagated the address space of 1), and warned on the dereference
> > > of bar (because typeof propagated noderef).
> > 
> > Thank you for the info!
> > 
> > Suppose that I want to do something like this:
> > 
> > #define __rcu_assign_pointer(p, v, space) \
> >         do { \
> >                 smp_wmb(); \
> >                 ACCESS_ONCE(p) = (typeof(*v) __force space *)(v); \
> >         } while (0)
> > 
> > Now, this does typeof(*p), so as you noted above sparse complains about
> > address-space mismatches.  Thus far, I haven't been able to come up with
> > something that (1) does sparse address-space checking, (2) does C type
> > checking, and (3) forces the assignment to be volatile.
> > 
> > Any thoughts on how to do this?
> 
> First of all, if p and v had compatible types *including* address
> spaces, you wouldn't need the "space" argument; the following
> self-contained test case passes both sparse and GCC typechecking:
> 
> #define as(n) __attribute__((address_space(n),noderef))
> #define __force __attribute__((force))
> #define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
> extern void smp_wmb(void);
> 
> #define rcu_assign_pointer(p, v) \
>     do { \
>         smp_wmb(); \
>         ACCESS_ONCE(p) = (v); \
>     } while (0)
> 
> struct foo;
> 
> int main(void)
> {
>     struct foo as(1) *dest;
>     struct foo as(1) *src = (void *)0;
> 
>     rcu_assign_pointer(dest, src);
> 
>     return 0;
> }
> 
> 
> 
> But in this case, you want dest and src to have compatible types except
> that dest must have the __rcu address space and src might not.  So,
> let's change the types of dest and src, and add the appropriate cast.
> The following also passes both GCC and sparse:
> 
> #define __rcu __attribute__((address_space(4),noderef))
> #define __force __attribute__((force))
> #define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
> extern void smp_wmb(void);
> 
> #define rcu_assign_pointer(p, v) \
>     do { \
>         smp_wmb(); \
>         ACCESS_ONCE(p) = (typeof(*(v)) __rcu __force *)(v); \
>     } while (0)
> 
> struct foo { int x; };
> 
> int main(void)
> {
>     struct foo __rcu *dest;
>     struct foo *src = (void *)0;
> 
>     rcu_assign_pointer(dest, src);
> 
>     return 0;
> }
> 
> 
> However, that cast forces the source to have the __rcu address space
> without checking what address space it started out with.  If you want to
> verify that the source has the kernel address space, you can cast to
> that address space first, *without* __force, which will warn if the
> source doesn't start out with that address space:
> 
> #define __kernel __attribute__((address_space(0)))
> #define __user __attribute__((address_space(1),noderef))
> #define __rcu __attribute__((address_space(4),noderef))
> #define __force __attribute__((force))
> #define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
> extern void smp_wmb(void);
> 
> #define rcu_assign_pointer(p, v) \
>     do { \
>         smp_wmb(); \
>         ACCESS_ONCE(p) = (typeof(*(v)) __rcu __force *)(typeof(*(v)) __kernel *)(v); \
>     } while (0)
> 
> struct foo { int x; };
> 
> int main(void)
> {
>     struct foo __rcu *dest;
>     struct foo *src = (void *)0;
>     struct foo __user *badsrc = (void *)0;
> 
>     rcu_assign_pointer(dest, src);
>     rcu_assign_pointer(dest, badsrc);
> 
>     return 0;
> }
> 
> 
> This produces a warning on the line using badsrc:
> 
> test.c:23:5: warning: cast removes address space of expression
> 
> However, that doesn't seem like the most obvious warning, since
> rcu_assign_pointer doesn't look like a cast, and since it doesn't print
> the full types involved like most address space warnings do.  So,
> instead, let's add and use a __chk_kernel_ptr function, similar to
> __chk_user_ptr in compiler.h:
> 
> #define __kernel __attribute__((address_space(0)))
> #define __user __attribute__((address_space(1),noderef))
> #define __rcu __attribute__((address_space(4),noderef))
> #define __force __attribute__((force))
> #define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
> extern void __chk_kernel_ptr(const volatile void *);
> extern void smp_wmb(void);
> 
> #define rcu_assign_pointer(p, v) \
>     do { \
>         smp_wmb(); \
>         __chk_kernel_ptr(v); \
>         ACCESS_ONCE(p) = (typeof(*(v)) __rcu __force *)(v); \
>     } while (0)
> 
> struct foo { int x; };
> 
> int main(void)
> {
>     struct foo __rcu *dest;
>     struct foo *src = (void *)0;
>     struct foo __user *badsrc = (void *)0;
> 
>     rcu_assign_pointer(dest, src);
>     rcu_assign_pointer(dest, badsrc);
> 
>     return 0;
> }
> 
> 
> This produces a somewhat better warning:
> 
> test.c:25:5: warning: incorrect type in argument 1 (different address spaces)
> test.c:25:5:    expected void const volatile *<noident>
> test.c:25:5:    got struct foo [noderef] <asn:1>*badsrc
> 
> That at least shows the full type of badsrc, but it still seems
> suboptimal for two reasons: it says it expects "void const volatile *"
> rather than the actual type it wants, and it says "in argument 1" (of
> __chk_kernel_ptr), which seems unnecessarily confusing when the type
> error actually applies to argument 2 of rcu_assign_pointer.  We can do
> better by declaring a fake local function for checking, instead:
> 
> #define __kernel __attribute__((address_space(0)))
> #define __user __attribute__((address_space(1),noderef))
> #define __rcu __attribute__((address_space(4),noderef))
> #define __force __attribute__((force))
> #define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
> extern void smp_wmb(void);
> 
> #define rcu_assign_pointer(p, v) \
>     do { \
>         smp_wmb(); \
>         extern void __rcu_assign_pointer_typecheck(int, typeof(*(v)) __kernel *); \
>         __rcu_assign_pointer_typecheck(0, v); \
>         ACCESS_ONCE(p) = (typeof(*(v)) __rcu __force *)(v); \
>     } while (0)
> 
> struct foo { int x; };
> 
> int main(void)
> {
>     struct foo __rcu *dest;
>     struct foo *src = (void *)0;
>     struct foo __user *badsrc = (void *)0;
> 
>     rcu_assign_pointer(dest, src);
>     rcu_assign_pointer(dest, badsrc);
> 
>     return 0;
> }
> 
> 
> This last approach produces a very clear warning:
> 
> test.c:25:5: warning: incorrect type in argument 2 (different address spaces)
> test.c:25:5:    expected struct foo *<noident>
> test.c:25:5:    got struct foo [noderef] <asn:1>*badsrc
> 
> If you want, you can even add an argument name for the second argument
> of __rcu_assign_pointer_typecheck, and it'll replace the <noident> in
> the second line of the warning.
> 
> So, that last approach meets all the criteria you mentioned:
> > something that (1) does sparse address-space checking, (2) does C type
> > checking, and (3) forces the assignment to be volatile.
> 
> Will that work for all the use cases you have in mind?  If so, I'll
> submit a patch changing rcu_assign_pointer to use that approach.

Looks like it does the right thing, thank you!

Would it also be possible for the call to __rcu_assign_pointer_typecheck()
to be only present when building under sparse?

							Thanx, Paul

Re: [RFC] adding into middle of RCU list

[Josh Triplett]

On Sun, Sep 01, 2013 at 03:26:19PM -0700, Paul E. McKenney wrote:
> On Sun, Sep 01, 2013 at 01:42:10PM -0700, Josh Triplett wrote:
> > On Sat, Aug 31, 2013 at 02:32:28PM -0700, Paul E. McKenney wrote:
> > > On Thu, Aug 29, 2013 at 07:16:37PM -0700, Josh Triplett wrote:
> > > > On Thu, Aug 29, 2013 at 05:57:33PM -0700, Paul E. McKenney wrote:
> > > > > On Fri, Aug 23, 2013 at 02:08:22PM -0700, Paul E. McKenney wrote:
> > > > > > On Fri, Aug 23, 2013 at 01:16:53PM -0400, Mathieu Desnoyers wrote:
> > > > > > > #define __rcu_assign_pointer(p, v, space) \
> > > > > > >         do { \
> > > > > > >                 smp_wmb(); \
> > > > > > >                 (p) = (typeof(*v) __force space *)(v); \
> > > > > > >         } while (0)
> > > > > > 
> > > > > > Or I need to fix this one as well.  ;-)
> > > > > 
> > > > > In that vein...  Is there anything like typeof() that also preserves
> > > > > sparse's notion of address space?  Wrapping an ACCESS_ONCE() around
> > > > > "p" in the assignment above results in sparse errors.
> > > > 
> > > > typeof() will preserve sparse's notion of address space as long as you
> > > > do typeof(p), not typeof(*p):
> > > > 
> > > > $ cat test.c
> > > > #define as(n) __attribute__((address_space(n),noderef))
> > > > #define __force __attribute__((force))
> > > > 
> > > > int main(void)
> > > > {
> > > >     int target = 0;
> > > >     int as(1) *foo = (__force typeof(target) as(1) *) ⌖
> > > >     typeof(foo) bar = foo;
> > > >     return *bar;
> > > > }
> > > > $ sparse test.c
> > > > test.c:9:13: warning: dereference of noderef expression
> > > > 
> > > > Notice that sparse didn't warn on the assignment of foo to bar (because
> > > > typeof propagated the address space of 1), and warned on the dereference
> > > > of bar (because typeof propagated noderef).
> > > 
> > > Thank you for the info!
> > > 
> > > Suppose that I want to do something like this:
> > > 
> > > #define __rcu_assign_pointer(p, v, space) \
> > >         do { \
> > >                 smp_wmb(); \
> > >                 ACCESS_ONCE(p) = (typeof(*v) __force space *)(v); \
> > >         } while (0)
> > > 
> > > Now, this does typeof(*p), so as you noted above sparse complains about
> > > address-space mismatches.  Thus far, I haven't been able to come up with
> > > something that (1) does sparse address-space checking, (2) does C type
> > > checking, and (3) forces the assignment to be volatile.
> > > 
> > > Any thoughts on how to do this?
> > 
> > First of all, if p and v had compatible types *including* address
> > spaces, you wouldn't need the "space" argument; the following
> > self-contained test case passes both sparse and GCC typechecking:
> > 
> > #define as(n) __attribute__((address_space(n),noderef))
> > #define __force __attribute__((force))
> > #define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
> > extern void smp_wmb(void);
> > 
> > #define rcu_assign_pointer(p, v) \
> >     do { \
> >         smp_wmb(); \
> >         ACCESS_ONCE(p) = (v); \
> >     } while (0)
> > 
> > struct foo;
> > 
> > int main(void)
> > {
> >     struct foo as(1) *dest;
> >     struct foo as(1) *src = (void *)0;
> > 
> >     rcu_assign_pointer(dest, src);
> > 
> >     return 0;
> > }
> > 
> > 
> > 
> > But in this case, you want dest and src to have compatible types except
> > that dest must have the __rcu address space and src might not.  So,
> > let's change the types of dest and src, and add the appropriate cast.
> > The following also passes both GCC and sparse:
> > 
> > #define __rcu __attribute__((address_space(4),noderef))
> > #define __force __attribute__((force))
> > #define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
> > extern void smp_wmb(void);
> > 
> > #define rcu_assign_pointer(p, v) \
> >     do { \
> >         smp_wmb(); \
> >         ACCESS_ONCE(p) = (typeof(*(v)) __rcu __force *)(v); \
> >     } while (0)
> > 
> > struct foo { int x; };
> > 
> > int main(void)
> > {
> >     struct foo __rcu *dest;
> >     struct foo *src = (void *)0;
> > 
> >     rcu_assign_pointer(dest, src);
> > 
> >     return 0;
> > }
> > 
> > 
> > However, that cast forces the source to have the __rcu address space
> > without checking what address space it started out with.  If you want to
> > verify that the source has the kernel address space, you can cast to
> > that address space first, *without* __force, which will warn if the
> > source doesn't start out with that address space:
> > 
> > #define __kernel __attribute__((address_space(0)))
> > #define __user __attribute__((address_space(1),noderef))
> > #define __rcu __attribute__((address_space(4),noderef))
> > #define __force __attribute__((force))
> > #define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
> > extern void smp_wmb(void);
> > 
> > #define rcu_assign_pointer(p, v) \
> >     do { \
> >         smp_wmb(); \
> >         ACCESS_ONCE(p) = (typeof(*(v)) __rcu __force *)(typeof(*(v)) __kernel *)(v); \
> >     } while (0)
> > 
> > struct foo { int x; };
> > 
> > int main(void)
> > {
> >     struct foo __rcu *dest;
> >     struct foo *src = (void *)0;
> >     struct foo __user *badsrc = (void *)0;
> > 
> >     rcu_assign_pointer(dest, src);
> >     rcu_assign_pointer(dest, badsrc);
> > 
> >     return 0;
> > }
> > 
> > 
> > This produces a warning on the line using badsrc:
> > 
> > test.c:23:5: warning: cast removes address space of expression
> > 
> > However, that doesn't seem like the most obvious warning, since
> > rcu_assign_pointer doesn't look like a cast, and since it doesn't print
> > the full types involved like most address space warnings do.  So,
> > instead, let's add and use a __chk_kernel_ptr function, similar to
> > __chk_user_ptr in compiler.h:
> > 
> > #define __kernel __attribute__((address_space(0)))
> > #define __user __attribute__((address_space(1),noderef))
> > #define __rcu __attribute__((address_space(4),noderef))
> > #define __force __attribute__((force))
> > #define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
> > extern void __chk_kernel_ptr(const volatile void *);
> > extern void smp_wmb(void);
> > 
> > #define rcu_assign_pointer(p, v) \
> >     do { \
> >         smp_wmb(); \
> >         __chk_kernel_ptr(v); \
> >         ACCESS_ONCE(p) = (typeof(*(v)) __rcu __force *)(v); \
> >     } while (0)
> > 
> > struct foo { int x; };
> > 
> > int main(void)
> > {
> >     struct foo __rcu *dest;
> >     struct foo *src = (void *)0;
> >     struct foo __user *badsrc = (void *)0;
> > 
> >     rcu_assign_pointer(dest, src);
> >     rcu_assign_pointer(dest, badsrc);
> > 
> >     return 0;
> > }
> > 
> > 
> > This produces a somewhat better warning:
> > 
> > test.c:25:5: warning: incorrect type in argument 1 (different address spaces)
> > test.c:25:5:    expected void const volatile *<noident>
> > test.c:25:5:    got struct foo [noderef] <asn:1>*badsrc
> > 
> > That at least shows the full type of badsrc, but it still seems
> > suboptimal for two reasons: it says it expects "void const volatile *"
> > rather than the actual type it wants, and it says "in argument 1" (of
> > __chk_kernel_ptr), which seems unnecessarily confusing when the type
> > error actually applies to argument 2 of rcu_assign_pointer.  We can do
> > better by declaring a fake local function for checking, instead:
> > 
> > #define __kernel __attribute__((address_space(0)))
> > #define __user __attribute__((address_space(1),noderef))
> > #define __rcu __attribute__((address_space(4),noderef))
> > #define __force __attribute__((force))
> > #define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
> > extern void smp_wmb(void);
> > 
> > #define rcu_assign_pointer(p, v) \
> >     do { \
> >         smp_wmb(); \
> >         extern void __rcu_assign_pointer_typecheck(int, typeof(*(v)) __kernel *); \
> >         __rcu_assign_pointer_typecheck(0, v); \
> >         ACCESS_ONCE(p) = (typeof(*(v)) __rcu __force *)(v); \
> >     } while (0)
> > 
> > struct foo { int x; };
> > 
> > int main(void)
> > {
> >     struct foo __rcu *dest;
> >     struct foo *src = (void *)0;
> >     struct foo __user *badsrc = (void *)0;
> > 
> >     rcu_assign_pointer(dest, src);
> >     rcu_assign_pointer(dest, badsrc);
> > 
> >     return 0;
> > }
> > 
> > 
> > This last approach produces a very clear warning:
> > 
> > test.c:25:5: warning: incorrect type in argument 2 (different address spaces)
> > test.c:25:5:    expected struct foo *<noident>
> > test.c:25:5:    got struct foo [noderef] <asn:1>*badsrc
> > 
> > If you want, you can even add an argument name for the second argument
> > of __rcu_assign_pointer_typecheck, and it'll replace the <noident> in
> > the second line of the warning.
> > 
> > So, that last approach meets all the criteria you mentioned:
> > > something that (1) does sparse address-space checking, (2) does C type
> > > checking, and (3) forces the assignment to be volatile.
> > 
> > Will that work for all the use cases you have in mind?  If so, I'll
> > submit a patch changing rcu_assign_pointer to use that approach.
> 
> Looks like it does the right thing, thank you!
> 
> Would it also be possible for the call to __rcu_assign_pointer_typecheck()
> to be only present when building under sparse?

Sure; it just needs to go in a separate macro that only gets a non-empty
definition ifdef __CHECKER__.

Patch momentarily.

- Josh Triplett

[PATCH] rcu: Make rcu_assign_pointer's assignment volatile and type-safe

[Josh Triplett]

rcu_assign_pointer needs to use ACCESS_ONCE to make the assignment to
the destination pointer volatile, to protect against compilers too
clever for their own good.

In addition, since rcu_assign_pointer force-casts the source pointer to
add the __rcu address space (overriding any existing address space), add
an explicit check that the source pointer has the __kernel address space
to start with.

This new check produces warnings like this, when attempting to assign
from a __user pointer:

test.c:25:9: warning: incorrect type in argument 2 (different address spaces)
test.c:25:9:    expected struct foo *<noident>
test.c:25:9:    got struct foo [noderef] <asn:1>*badsrc

Signed-off-by: Josh Triplett <josh@joshtriplett.org>
---
 include/linux/rcupdate.h | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 4b14bdc..3f62def 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -510,8 +510,17 @@ static inline void rcu_preempt_sleep_check(void)
 #ifdef __CHECKER__
 #define rcu_dereference_sparse(p, space) \
 	((void)(((typeof(*p) space *)p) == p))
+/* The dummy first argument in __rcu_assign_pointer_typecheck makes the
+ * typechecked pointer the second argument, matching rcu_assign_pointer itself;
+ * this avoids confusion about argument numbers in warning messages. */
+#define __rcu_assign_pointer_check_kernel(v) \
+	do { \
+		extern void __rcu_assign_pointer_typecheck(int, typeof(*(v)) __kernel *); \
+		__rcu_assign_pointer_typecheck(0, v); \
+	} while (0)
 #else /* #ifdef __CHECKER__ */
 #define rcu_dereference_sparse(p, space)
+#define __rcu_assign_pointer_check_kernel(v) do { } while (0)
 #endif /* #else #ifdef __CHECKER__ */
 
 #define __rcu_access_pointer(p, space) \
@@ -555,7 +564,8 @@ static inline void rcu_preempt_sleep_check(void)
 #define __rcu_assign_pointer(p, v, space) \
 	do { \
 		smp_wmb(); \
-		(p) = (typeof(*v) __force space *)(v); \
+		__rcu_assign_pointer_check_kernel(v); \
+		ACCESS_ONCE(p) = (typeof(*(v)) __force space *)(v); \
 	} while (0)
 
 

Re: [PATCH] rcu: Make rcu_assign_pointer's assignment volatile and type-safe

[Paul E. McKenney]

On Sun, Sep 01, 2013 at 04:42:52PM -0700, Josh Triplett wrote:
> rcu_assign_pointer needs to use ACCESS_ONCE to make the assignment to
> the destination pointer volatile, to protect against compilers too
> clever for their own good.
> 
> In addition, since rcu_assign_pointer force-casts the source pointer to
> add the __rcu address space (overriding any existing address space), add
> an explicit check that the source pointer has the __kernel address space
> to start with.
> 
> This new check produces warnings like this, when attempting to assign
> from a __user pointer:
> 
> test.c:25:9: warning: incorrect type in argument 2 (different address spaces)
> test.c:25:9:    expected struct foo *<noident>
> test.c:25:9:    got struct foo [noderef] <asn:1>*badsrc
> 
> Signed-off-by: Josh Triplett <josh@joshtriplett.org>

Queued for 3.13, thank you very much!

							Thanx, Paul

> ---
>  include/linux/rcupdate.h | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> index 4b14bdc..3f62def 100644
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -510,8 +510,17 @@ static inline void rcu_preempt_sleep_check(void)
>  #ifdef __CHECKER__
>  #define rcu_dereference_sparse(p, space) \
>  	((void)(((typeof(*p) space *)p) == p))
> +/* The dummy first argument in __rcu_assign_pointer_typecheck makes the
> + * typechecked pointer the second argument, matching rcu_assign_pointer itself;
> + * this avoids confusion about argument numbers in warning messages. */
> +#define __rcu_assign_pointer_check_kernel(v) \
> +	do { \
> +		extern void __rcu_assign_pointer_typecheck(int, typeof(*(v)) __kernel *); \
> +		__rcu_assign_pointer_typecheck(0, v); \
> +	} while (0)
>  #else /* #ifdef __CHECKER__ */
>  #define rcu_dereference_sparse(p, space)
> +#define __rcu_assign_pointer_check_kernel(v) do { } while (0)
>  #endif /* #else #ifdef __CHECKER__ */
> 
>  #define __rcu_access_pointer(p, space) \
> @@ -555,7 +564,8 @@ static inline void rcu_preempt_sleep_check(void)
>  #define __rcu_assign_pointer(p, v, space) \
>  	do { \
>  		smp_wmb(); \
> -		(p) = (typeof(*v) __force space *)(v); \
> +		__rcu_assign_pointer_check_kernel(v); \
> +		ACCESS_ONCE(p) = (typeof(*(v)) __force space *)(v); \
>  	} while (0)
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>