Re: [PATCH for-next V5 3/5] IB/uverbs: Enable device removal when there are active user space applications

[Yishai Hadas <yishaih-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>]

On 6/24/2015 9:25 PM, Jason Gunthorpe wrote:
> On Mon, Jun 22, 2015 at 05:47:16PM +0300, Yishai Hadas wrote:
>> +++ b/drivers/infiniband/core/uverbs_main.c
>> @@ -137,7 +137,12 @@ static void ib_uverbs_release_dev(struct kref *ref)
>>   	struct ib_uverbs_device *dev =
>>   		container_of(ref, struct ib_uverbs_device, ref);
>>
>> -	complete(&dev->comp);
>> +	if (!dev->ib_dev) {
>> +		cleanup_srcu_struct(&dev->disassociate_srcu);
>> +		kfree(dev);
>> +	} else {
>> +		complete(&dev->comp);
>> +	}
>
> Oy.. It is so gross to see a kref now being simultaneously used for
> actual memory accounting and also for general reference counting.
>
> It is also locked wrong, for instance:
Not correct, see below.

>
> @@ -792,13 +889,18 @@  static int ib_uverbs_open(struct inode *inode, struct file *filp)
>   err:
> +	mutex_unlock(&dev->lists_mutex);
> +	srcu_read_unlock(&dev->disassociate_srcu, srcu_key);
>   	kref_put(&dev->ref, ib_uverbs_release_dev);
>
> Is not holding the RCU lock while ib_uverbs_release_dev is reading
> ib_dev. The barriers in kref are not strong enough to guarentee the RCU
> protected data will be visible. (remember when I asked if you checked
> all of these?)
>

This is not a locking problem, the only option that here the reference 
count becomes 0 is if ib_uverbs_remove_one was previously called and 
decreased the reference count that was taken upon load. However, it was 
done after that rcu_assign_pointer(uverbs_dev->ib_dev, NULL) was called 
so the check whether if (!dev->ib_dev) is fully protected and can't race 
with HW removal flow.


> Obviously you can't hold the disassociate_srcu and call kref_put, so
> maybe grabbing it in release_dev would work. I didn't look that
> closely.
>
> But really, don't make a kref do two kinds of things, it just doesn't
> make any sense. You should split it into a proper memory ownership
> tracking kref that always does kfree and a counter used to cause
> complete().

The kref is used to manage the uverbs_dev allocation, the internal code 
in ib_uverbs_release_dev depends on the state. Usually the natural place 
to free the memory is as part of the release function as done in other 
kernel places. In case that ib_device was previously removed it can be 
safely freed here as it's called when the last client disconnected, this 
logic is introduced by this patch. In case there is a need to wait 
clients as the driver doesn't support HW device removal the free can't 
be done internally but must be done externally in ib_uverbs_remove_one 
when last client disconnected and complete should be used instead. As of 
I believe that we can stay with only one kref that manage the uverbs_dev 
which is safe as I pointed above.

>
> The rest looked OK now..
Thanks

>
>> +                       /* The barriers built into wait_event_interruptible()
>> +                        * and wake_up() make this ib_dev check RCU protected
>> +                        */
>
> No..
>
>   The barriers built into wait_event_interruptible() and wake_up()
>   guarentee this will see the null set without using RCU
>
Will fix the comment

>> +	if (device->disassociate_ucontext) {
>> +		/* We disassociate HW resources and immediately returning, not
>> +		 * pending to active userspace clients. Upon returning ib_device
>> +		 * may be freed internally and is not valid any more.
>> +		 * uverbs_device is still available, when all clients close
>> +		 * their files, the uverbs device ref count will be zero and its
>> +		 * resources will be freed.
>> +		 * Note: At that step no more files can be opened on that cdev
>> +		 * as it was deleted, however active clients can still issue
>> +		 * commands and close their open files.
>> +		 */
>
> Clean up the grammer..
>
> We disassociate HW resources and immediately return. Userspace will
> see a EIO errno for all future access. Upon returning, ib_device may be
> freed internally and is not valid any more.  uverbs_device is still
> available until all clients close their files, then the uverbs device
> ref count will be zero and its resources will be freed.  Note: At this
> point no more files can be opened since the cdev was deleted, however
> active clients can still issue commands and close their open files.

OK
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html