On Wed, Feb 15, 2017 at 8:54 PM, Jason Gunthorpe
<jgunthorpe@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Wed, Feb 15, 2017 at 03:48:40PM +0200, Matan Barak wrote:
>> >> +static struct ib_uobject *lookup_get_fd_uobject(const struct uverbs_obj_type *type,
>> >> + struct ib_ucontext *ucontext,
>> >> + int id, bool write)
>> >> +{
>> >> + struct file *f;
>> >> + struct ib_uobject *uobject;
>> >> + const struct uverbs_obj_fd_type *fd_type =
>> >> + container_of(type, struct uverbs_obj_fd_type, type);
>> >> +
>> >> + if (write)
>> >> + return ERR_PTR(-EOPNOTSUPP);
>> >> +
>> >> + f = fget(id);
>> >> + if (!f)
>> >> + return ERR_PTR(-EBADF);
>> >> +
>> >> + uobject = f->private_data;
>> >> + if (f->f_op != fd_type->fops ||
>> >> + !uobject->context) {
>> >> + fput(f);
>> >> + return ERR_PTR(-EBADF);
>> >> + }
>> >
>> > The read of uobject->context needs locking
>> >
>>
>> All these callbacks are assumed to run when the context is alive.
>
> 'uboject->context' is touched outside the SRCU, eg in
> uverbs_close_fd() - so it absolutely needs locking.
>
uobject->context isn't protected by SRCU as intended.
lookup_get_fd_uobject can't be
executed concurrently with uverbs_close_fd, as uverbs_close_fd is
called when the
last reference of the fd is put and in that case fget called by
lookup_get_fd_uobject
would simply fail. Since fput and fget are atomically safe, this should be safe.
>> The only call we allow the user to execute while this is in process
>> is uverbs_close_fd and that's why this is the only call which is
>> protected by the cleanup_mutex.
>
> So cleanup_mutex would need to be held during lookup
>
You can't run lookup_get and uverbs_close_fd concurrently as stated above.
Regarding the locking, assuming uverbs_close_fd is called from the
release file operation and SRCU
protects us (as currently implemented), we don't see locking issues here.
Yishai and Matan.
--
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
