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

[Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>]

On Thu, Jun 11, 2015 at 03:56:52PM +0300, Yishai Hadas wrote:
> Enables the uverbs_remove_one to succeed despite the fact that there are
> running IB applications working with the given ib device.  This functionality
> enables a HW device to be unbind/reset despite the fact that there are running
> user space applications using it.

This looks much saner now, thanks. I only looked briefly today.

> -	struct ib_device		       *ib_dev;
> +	struct ib_device	__rcu	       *ib_dev;

Did you run a static checker to confirm the rcu annoation?

> @@ -332,9 +336,15 @@ static ssize_t ib_uverbs_event_read(struct file *filp, char __user *buf,
>  			return -EAGAIN;
>  
>  		if (wait_event_interruptible(file->poll_wait,
> -					     !list_empty(&file->event_list)))
> +					     (!list_empty(&file->event_list) ||
> +					     !file->uverbs_file->device->ib_dev)))

And it warned about this? Any place else?

The barriers built into wait_event_interruptible() and wake_up() make
this OK. That is worth a comment to explain why it is OK RCU protected
data is not using RCU here.

>  
> @@ -397,8 +407,11 @@ static int ib_uverbs_event_close(struct inode *inode, struct file *filp)
>  {
>  	struct ib_uverbs_event_file *file = filp->private_data;
>  	struct ib_uverbs_event *entry, *tmp;
> +	int closed_already = 0;
>  
> +	mutex_lock(&file->uverbs_file->device->lists_mutex);
>  	spin_lock_irq(&file->lock);
> +	closed_already = file->is_closed;
>  	file->is_closed = 1;

It doesn't loook like is_closed can be changed or read while
lists_mutex is held, so why this ordering and closed_already?

> +	struct ib_device *ib_dev = uverbs_dev->ib_dev;
> +	uverbs_dev->ib_dev = NULL;

These are the write side accesses - and we rely on the driver to
ensure there is only one call to ib_uverbs_free_hw_resources? A
comment would be good, it is unusual to see a RCU write side without
an explicit lock.

> +	/* Pending running commands to terminate */
> +	synchronize_srcu(&uverbs_dev->disassociate_srcu);
> +	event.event = IB_EVENT_DEVICE_FATAL;
> +	event.element.port_num = 0;
> +	event.device = ib_dev;
                     ^^^^^^^^^

How does this lifetime work? It doesn't look like
ib_uverbs_event_handler touches event.device? Is this a nop?

> +		/* We must release the mutex before going ahead and calling
> +		 * disassociate_ucontext. disassociate_ucontext might end up
> +		 * indirectly calling uverbs_close, for example due to freeing
> +		 * the resources (e.g mmput).
> +		 */

Can you provide a call trace for this deadlock possibility?

> +	}
> +
> +	mutex_unlock(&uverbs_dev->lists_mutex);
> +
> +	mutex_lock(&uverbs_dev->lists_mutex);

???

> +	while (!list_empty(&uverbs_dev->uverbs_events_file_list)) {
> +		event_file = list_first_entry(&uverbs_dev->
> +					      uverbs_events_file_list,
> +					      struct ib_uverbs_event_file,
> +					      list);
> +		event_file->is_closed = 1;
> +
> +		list_del(&event_file->list);
> +		if (event_file->is_async)
> +			ib_unregister_event_handler(&event_file->uverbs_file->
> +						    event_handler);

Why do we need to do this in ib_uverbs_free_hw_resources ? How does
the event handler hold a ref on the ib_dev?

> +	if (uverbs_dev->flags & UVERBS_FLAG_DISASSOCIATE) {

I wonder if the flag is needed, isn't having a non-null
disassociate_ucontext function enough proof the driver supports this?

Jason
--
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