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:
@@ -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?)
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 rest looked OK now..
> + /* 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
> + 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.
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
