On Mon, Jan 28, 2019 at 11:38:15AM -0700, Jason Gunthorpe wrote:
> On Mon, Jan 28, 2019 at 12:11:15PM +0200, Shamir Rabinovitch wrote:
> > Add ib_ucontext to the uverbs_attr_bundle sent down the iocl and cmd flows
> > as soon as the flow has ib_uobject to make sure the function
> > rdma_get_ucontext will never fail in those flows.
> >
> > Signed-off-by: Shamir Rabinovitch <shamir.rabinovitch@xxxxxxxxxx>
> > drivers/infiniband/core/uverbs_cmd.c | 2 +-
> > drivers/infiniband/core/uverbs_ioctl.c | 3 ++
> > drivers/infiniband/core/uverbs_main.c | 12 ++-----
> > drivers/infiniband/core/uverbs_std_types.c | 37 ++++++++++++++++++++++
> > include/rdma/uverbs_ioctl.h | 1 +
> > include/rdma/uverbs_std_types.h | 16 ++++++----
> > 6 files changed, 54 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
> > index d4f1a2ef5015..bd8bcab01e86 100644
> > +++ b/drivers/infiniband/core/uverbs_cmd.c
> > @@ -2620,7 +2620,7 @@ void flow_resources_add(struct ib_uflow_resources *uflow_res,
> > }
> > EXPORT_SYMBOL(flow_resources_add);
> >
> > -static int kern_spec_to_ib_spec_action(const struct uverbs_attr_bundle *attrs,
> > +static int kern_spec_to_ib_spec_action(struct uverbs_attr_bundle *attrs,
> > struct ib_uverbs_flow_spec *kern_spec,
> > union ib_flow_spec *ib_spec,
> > struct ib_uflow_resources *uflow_res)
> > diff --git a/drivers/infiniband/core/uverbs_ioctl.c b/drivers/infiniband/core/uverbs_ioctl.c
> > index 8c81ff698052..5d3251a51850 100644
> > +++ b/drivers/infiniband/core/uverbs_ioctl.c
> > @@ -198,6 +198,7 @@ static int uverbs_process_idrs_array(struct bundle_priv *pbundle,
> > ret = PTR_ERR(attr->uobjects[i]);
> > break;
> > }
> > + pbundle->bundle.context = attr->uobjects[i]->context;
> > }
> >
> > attr->len = i;
> > @@ -315,6 +316,7 @@ static int uverbs_process_attr(struct bundle_priv *pbundle,
> > uattr->data_s64);
> > if (IS_ERR(o_attr->uobject))
> > return PTR_ERR(o_attr->uobject);
> > + pbundle->bundle.context = o_attr->uobject->context;
> > __set_bit(attr_bkey, pbundle->uobj_finalize);
> >
> > if (spec->u.obj.access == UVERBS_ACCESS_NEW) {
> > @@ -564,6 +566,7 @@ static int ib_uverbs_cmd_verbs(struct ib_uverbs_file *ufile,
> > pbundle->method_elm = method_elm;
> > pbundle->method_key = attrs_iter.index;
> > pbundle->bundle.ufile = ufile;
> > + pbundle->bundle.context = NULL; /* only valid if bundle has uobject */
> > pbundle->radix = &uapi->radix;
> > pbundle->radix_slots = slot;
> > pbundle->radix_slots_len = radix_tree_chunk_size(&attrs_iter);
> > diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
> > index 996f167d1436..fb2f6f98204e 100644
> > +++ b/drivers/infiniband/core/uverbs_main.c
> > @@ -113,15 +113,8 @@ struct ib_ucontext *rdma_get_ucontext(struct ib_udata *udata)
> > if (!udata)
> > return ERR_PTR(-EIO);
> >
> > - /*
> > - * FIXME: Really all cases that get here with a udata will have
> > - * already called ib_uverbs_get_ucontext_file, or located a uobject
> > - * that points to a ucontext. We could store that result in the udata
> > - * so this function can't fail.
> > - */
> > - return ib_uverbs_get_ucontext_file(
> > - container_of(udata, struct uverbs_attr_bundle, driver_udata)
> > - ->ufile);
> > + return (container_of(udata, struct uverbs_attr_bundle, driver_udata)
> > + ->context) ? : ERR_PTR(-EIO);
>
> I have a feeling this should fall back to
> ib_uverbs_get_ucontext_file() if the context is zero?
You replied in another mail that this request is not needed as we have
100% guarantee that for user objects this can't fail so I'd skip this
with your permission.
>
> > diff --git a/drivers/infiniband/core/uverbs_std_types.c b/drivers/infiniband/core/uverbs_std_types.c
> > index cbc72312eb41..e1113bc1928b 100644
> > +++ b/drivers/infiniband/core/uverbs_std_types.c
> > @@ -39,6 +39,43 @@
> > #include "rdma_core.h"
> > #include "uverbs.h"
> >
> > +struct ib_uobject *uobj_get_read(enum uverbs_default_objects type,
> > + u32 object_id,
> > + struct uverbs_attr_bundle *attrs)
> > +{
> > + struct ib_uobject *uobj;
> > +
> > + uobj = rdma_lookup_get_uobject(uobj_get_type(attrs, type),
> > + attrs->ufile,
> > + _uobj_check_id(object_id),
> > + UVERBS_LOOKUP_READ);
> > + if (IS_ERR(uobj))
> > + return uobj;
> > +
> > + attrs->context = uobj->context;
> > +
> > + return uobj;
> > +}
> > +
> > +struct ib_uobject *uobj_get_write(enum uverbs_default_objects type,
> > + u32 object_id,
> > + struct uverbs_attr_bundle *attrs)
> > +{
> > + struct ib_uobject *uobj;
> > +
> > + uobj = rdma_lookup_get_uobject(uobj_get_type(attrs, type),
> > + attrs->ufile,
> > + _uobj_check_id(object_id),
> > + UVERBS_LOOKUP_WRITE);
> > +
> > + if (IS_ERR(uobj))
> > + return uobj;
> > +
> > + attrs->context = uobj->context;
> > +
> > + return uobj;
> > +}
>
> Please put these in rdma_core.c
Done.
>
> > static int uverbs_free_ah(struct ib_uobject *uobject,
> > enum rdma_remove_reason why)
> > {
> > diff --git a/include/rdma/uverbs_ioctl.h b/include/rdma/uverbs_ioctl.h
> > index 27da906beea7..b14a9ee786e9 100644
> > +++ b/include/rdma/uverbs_ioctl.h
> > @@ -652,6 +652,7 @@ struct uverbs_attr_bundle {
> > struct ib_udata driver_udata;
> > struct ib_udata ucore;
> > struct ib_uverbs_file *ufile;
> > + struct ib_ucontext *context;
> > DECLARE_BITMAP(attr_present, UVERBS_API_ATTR_BKEY_LEN);
> > struct uverbs_attr attrs[];
> > };
> > diff --git a/include/rdma/uverbs_std_types.h b/include/rdma/uverbs_std_types.h
> > index 883abcf6d36e..e19d0eec16fa 100644
> > +++ b/include/rdma/uverbs_std_types.h
> > @@ -48,9 +48,9 @@
> > #define uobj_get_type(_attrs, _object) \
> > uapi_get_object((_attrs)->ufile->device->uapi, _object)
> >
> > -#define uobj_get_read(_type, _id, _attrs) \
> > - rdma_lookup_get_uobject(uobj_get_type(_attrs, _type), (_attrs)->ufile, \
> > - _uobj_check_id(_id), UVERBS_LOOKUP_READ)
> > +struct ib_uobject *uobj_get_read(enum uverbs_default_objects type,
> > + u32 object_id,
> > + struct uverbs_attr_bundle *attrs);
>
> This looses the _uobj_check_id safety typecheck.. I'd prefer not to...
>
> > #define ufd_get_read(_type, _fdnum, _attrs) \
> > rdma_lookup_get_uobject(uobj_get_type(_attrs, _type), (_attrs)->ufile, \
> > @@ -67,9 +67,9 @@ static inline void *_uobj_get_obj_read(struct ib_uobject *uobj)
> > ((struct ib_##_object *)_uobj_get_obj_read( \
> > uobj_get_read(_type, _id, _attrs)))
> >
> > -#define uobj_get_write(_type, _id, _attrs) \
> > - rdma_lookup_get_uobject(uobj_get_type(_attrs, _type), (_attrs)->ufile, \
> > - _uobj_check_id(_id), UVERBS_LOOKUP_WRITE)
> > +struct ib_uobject *uobj_get_write(enum uverbs_default_objects type,
> > + u32 object_id,
> > + struct uverbs_attr_bundle *attrs);
> >
> > int __uobj_perform_destroy(const struct uverbs_api_object *obj, u32 id,
> > const struct uverbs_attr_bundle *attrs);
> > @@ -123,8 +123,10 @@ __uobj_alloc(const struct uverbs_api_object *obj,
> > {
> > struct ib_uobject *uobj = rdma_alloc_begin_uobject(obj, attrs->ufile);
> >
> > - if (!IS_ERR(uobj))
> > + if (!IS_ERR(uobj)) {
> > *ib_dev = uobj->context->device;
> > + attrs->context = uobj->context;
> > + }
> > return uobj;
> > }
>
> Should probably de-inline this as well.
I do not see any compile issues. Am I missing something?
>
> Jason
>
