On Thu, Aug 09, 2018 at 03:57:16PM +0000, Ruhl, Michael J wrote:
> >+struct uverbs_api_ioctl_method {
> >+ int (__rcu *handler)(struct ib_uverbs_file *ufile,
> >+ struct uverbs_attr_bundle *ctx);
> >+ DECLARE_BITMAP(attr_mandatory, UVERBS_API_ATTR_BKEY_LEN);
> >+ u8 driver_method:1;
>
> This is used as a bool in some places. Is there a reason for the 1 bit usage?
Yes, it is a 'bitfield as flags' pattern - the next patch adds more
flags to this memory location.
There isn't a particularly strong reason to over space optimize this,
but at the same time there isn't a strong reason not to..
> >@@ -1020,7 +1022,14 @@ static int ib_uverbs_create_uapi(struct ib_device
> >*device,
> > if (IS_ERR(specs_root))
> > return PTR_ERR(specs_root);
> >
> >+ uapi = uverbs_alloc_api(device->driver_specs, device->driver_id);
> >+ if (IS_ERR(uapi)) {
> >+ uverbs_free_spec_tree(specs_root);
> >+ return PTR_ERR(uapi);
> >+ }
> >+
>
> So this will completely replace the specs_root?
Yes.
> So what was the point of patch 1/10? Is it really necessary?
Yes, the point of patch 1/10 is to centralize the above in the core
code so it can be changed, and more critically, to allow the new api
code to tell the difference between driver provide specs and core
specs by splitting them up.
Prior to 1/10 the core code had no good way to tell what was driver
and what was core. Now it is clear, things referenced from
driver_specs are driver, everything else is core.
This part of making disassociation work as we need to know exactly
what specs came from the driver so to compute driver_method.
> >+static int uapi_merge_method(struct uverbs_api *uapi,
> >+ struct uverbs_api_object *obj_elm, u32 obj_key,
> >+ const struct uverbs_method_def *method,
> >+ bool is_driver)
> >+{
> >+ u32 method_key = obj_key | uapi_key_ioctl_method(method->id);
> >+ struct uverbs_api_ioctl_method *method_elm;
> >+ unsigned int i;
> >+
> >+ if (!method->attrs)
> >+ return 0;
> >+
> >+ method_elm = uapi_add_elm(uapi, method_key,
> >sizeof(*method_elm));
> >+ if (IS_ERR(method_elm)) {
> >+ if (method_elm != ERR_PTR(-EEXIST))
> >+ return PTR_ERR(method_elm);
> >+
> >+ /*
> >+ * This occurs when a driver uses
> >ADD_UVERBS_ATTRIBUTES_SIMPLE
> >+ */
> >+ if (WARN_ON(method->handler))
> >+ return -EINVAL;
> >+ method_elm = radix_tree_lookup(&uapi->radix,
> >method_key);
> >+ if (WARN_ON(!method_elm))
> >+ return -EINVAL;
> >+ } else {
> >+ WARN_ON(!method->handler);
> >+ method_elm->handler = method->handler;
> >+ if (method->handler != uverbs_destroy_def_handler)
> >+ method_elm->driver_method = is_driver;
> >+ }
> >+
> >+ for (i = 0; i != method->num_attrs; i++) {
> >+ const struct uverbs_attr_def *attr = (*method->attrs)[i];
> >+ struct uverbs_api_attr *attr_slot;
> >+
> >+ if (!attr)
> >+ continue;
> >+
> >+ /*
> >+ * ENUM_IN contains the 'ids' pointer to the driver's .rodata,
> >+ * so if it is specified by a driver then it always makes this
> >+ * into a driver method.
> >+ */
> >+ if (attr->attr.type == UVERBS_ATTR_TYPE_ENUM_IN)
> >+ method_elm->driver_method |= is_driver;
>
> Why do you | this?
driver_method is logically the OR of the 'driverness' of everything
that contributes to the method - the handler, and all the attribute
specs.
In this case we possibly could use assignment and still be correct,
but that would be a very obtuse way to write it..
'driverness' is the answer to the question: Does this method require
any data that is located in the driver module. ie a enum ids pointer
or a handler in the driver.
> >+static int uapi_merge_tree(struct uverbs_api *uapi,
> >+ const struct uverbs_object_tree_def *tree,
> >+ bool is_driver)
> >+{
> >+ unsigned int i, j;
> >+ int rc;
> >+
> >+ if (!tree->objects)
> >+ return 0;
> >+
> >+ for (i = 0; i != tree->num_objects; i++) {
> >+ const struct uverbs_object_def *obj = (*tree->objects)[i];
> >+ struct uverbs_api_object *obj_elm;
> >+ u32 obj_key;
> >+
> >+ if (!obj)
> >+ continue;
> >+
> >+
>
> Extra line?
Yep
> >+ obj_key = uapi_key_obj(obj->id);
> >+ obj_elm = uapi_add_elm(uapi, obj_key, sizeof(*obj_elm));
> >+ if (IS_ERR(obj_elm)) {
> >+ if (obj_elm != ERR_PTR(-EEXIST))
> >+ return PTR_ERR(obj_elm);
> >+
> >+ /* This occurs when a driver uses
> >ADD_UVERBS_METHODS */
> >+ if (WARN_ON(obj->type_attrs))
> >+ return -EINVAL;
> >+ obj_elm = radix_tree_lookup(&uapi->radix, obj_key);
> >+ if (WARN_ON(!obj_elm))
> >+ return -EINVAL;
> >+ } else {
> >+ obj_elm->type_attrs = obj->type_attrs;
> >+ if (obj->type_attrs) {
> >+ obj_elm->type_class =
> >+ obj->type_attrs->type_class;
> >+ /*
> >+ * Today drivers are only permitted to use
> >+ * idr_class types. They cannot use FD types
> >+ * because we currently have no way to
> >revoke
> >+ * the fops pointer after device
> >+ * disassociation.
> >+ */
> >+ if (WARN_ON(is_driver &&
> >+ obj->type_attrs->type_class !=
> >+ &uverbs_idr_class))
> >+ return -EINVAL;
> >+ }
>
> So you are saying that drivers cannot use this:
>
> UVERBS_TYPE_ALLOC_FD()
>
> type in their objects?
Yes.
> This seems like a new limitation. Is this going to change? Any thoughts on how
> to make it work?
It is not a new limitation, it was always true, we just didn't check
for it before..
The problem is how to handle the fops during disassociation, as the
fops pointer points to the driver, and the driver module can become
unloaded.
I guess it would be fine if the driver sets struct
file_operations->owner to THIS_MODULE - maybe we should just check for
that here.
It means disassocation via module unload doesn't work if the driver
creates an FD, which is probably OK.
This is a tricky area, I prefer to leave it blocked until we have an
example driver user to study to get the lifetime right.
> >+/*
> >+ * Called when a driver disassociates from the ib_uverbs_device. The
> >+ * assumption is that the driver module will unload after. Replace eveything
> >+ * related to the driver with NULL as a safety measure.
> >+ */
> >+void uverbs_disassociate_api(struct uverbs_api *uapi)
> >+{
> >+ struct radix_tree_iter iter;
> >+ void __rcu **slot;
> >+ bool driver_method = false;
> >+
> >+ radix_tree_for_each_slot (slot, &uapi->radix, &iter, 0) {
> >+ if (uapi_key_is_object(iter.index)) {
> >+ struct uverbs_api_object *object_elm =
> >+ rcu_dereference_protected(*slot, true);
> >+
> >+ /*
> >+ * Some type_attrs are in the driver, module we don't
> >+ * bother to keep track of which since there should be
> >+ * no use of this.
> >+ */
> >+ object_elm->type_attrs = NULL;
> >+ } else if (uapi_key_is_ioctl_method(iter.index)) {
> >+ struct uverbs_api_ioctl_method *method_elm =
> >+ rcu_dereference_protected(*slot, true);
> >+
> >+ driver_method = method_elm->driver_method;
>
> So what is the purpose of setting this?
> I don't see it being used anywhere else.
Indeed, an earlier revision was using this but I dropped that logic
off.
Thanks,
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
