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