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

On Fri, 31 Oct 2014, Thomas Gleixner wrote:

quoted

+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()'

quoted

+     /* 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