lkml.org 
[lkml]   [2014]   [Oct]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: kdbus: add driver skeleton, ioctl entry points and utility functions
    On Fri, 31 Oct 2014, Thomas Gleixner wrote:

    > > +static long kdbus_handle_ioctl(struct file *file, unsigned int cmd,
    > > + unsigned long arg)
    > > +{
    > > + struct kdbus_handle *handle = file->private_data;
    > > + void __user *argp = (void __user *)arg;
    > > + enum kdbus_handle_type type = handle->type;
    > > +
    > > + /* make sure all handle fields are set if handle->type is */
    > > + smp_rmb();
    >
    > Sure. You really need this kind of serialization because your design
    > choice of allowing opaque handles in the first place.
    >
    > I'm really interested why you need this rmb() at all. Just because you
    > have several threads in user space which might race with the type
    > assignment when they call the ioctl?
    >
    > We have a strict requirement to document memory barriers. The
    > following comment definitely does not fulfil this requirement as it
    > just documents that someone observed a race of unknown provenance and
    > got it 'fixed' with a 'smp_rmb()'
    >
    > > + /* make sure all handle fields are set if handle->type is */
    >
    > That's really hillarious, The user space side knows excatly upfront
    > which type of 'handle' it wants to open. Making it an opaque handle in
    > the first place and let the kernel deal with the actual type
    > assignment is beyond silly. Especially if that involves undocumented
    > memory barriers.

    I have been staring at exactly this for rather a long time today.

    Apparently this barrier pairs with smp_wmb() in kdbus_handle_transform()
    and tries to make sure that whenever handle->type is seen as updated,
    handle->ptr is as well.

    But it's still difficult for me to understand all the memory ordering
    rules and consequences of this strict ordering (my current understanding
    is that the barrier is not needed, but I will have to think about it a
    little bit more), so a nice and explanatory comment precisely describing
    the race this is protecting against would be very welcome.

    Thanks,

    --
    Jiri Kosina
    SUSE Labs



    \
     
     \ /
      Last update: 2014-10-31 02:01    [W:3.077 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site