Re: kdbus: add driver skeleton, ioctl entry points and utility functions

[Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>]

On Fri, 31 Oct 2014, Jiri Kosina wrote:
> 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.

Right. But it does not make any sense at all. 

The underlying problem is the design of the whole character device
interface as an opaque type. Just look at the absurd workarounds in
the userspace implementation of this,

Now we copy it to kernel space 1:1 and find other absurd workarounds
for it instead of designing it new.

Welcome to the world of bug compability...

Thanks,

	tglx