devlink: remove the registration guarantee of references

The objective of exposing the devlink instance locks to
drivers was to let them use these locks to prevent user space
from accessing the device before it's fully initialized.
This is difficult because devlink_unregister() waits for all
references to be released, meaning that devlink_unregister()
can't itself be called under the instance lock.

To avoid this issue devlink_register() was moved after subobject
registration a while ago. Unfortunately the netdev paths get
a hold of the devlink instances _before_ they are registered.
Ideally netdev should wait for devlink init to finish (synchronizing
on the instance lock). This can't work because we don't know if the
instance will _ever_ be registered (in case of failures it may not).
The other option of returning an error until devlink_register()
is called is unappealing (user space would get a notification
netdev exist but would have to wait arbitrary amount of time
before accessing some of its attributes).

Weaken the guarantees of the devlink references.

Holding a reference will now only guarantee that the memory
of the object is around. Another way of looking at it is that
the reference now protects the object not its "registered" status.
Use devlink instance lock to synchronize unregistration.

This implies that releasing of the "main" reference of the devlink
instance moves from devlink_unregister() to devlink_free().

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/include/net/devlink.h b/include/net/devlink.h
index 6a2e4f21779fe547e5a0905c2e894c1f96c9da22..425ecef431b75ffaafe75a79e0572c5b5a3e5079 100644 (file)
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -1647,6 +1647,8 @@ static inline struct devlink *devlink_alloc(const struct devlink_ops *ops,
        return devlink_alloc_ns(ops, priv_size, &init_net, dev);
 }
 void devlink_set_features(struct devlink *devlink, u64 features);
+int devl_register(struct devlink *devlink);
+void devl_unregister(struct devlink *devlink);
 void devlink_register(struct devlink *devlink);
 void devlink_unregister(struct devlink *devlink);
 void devlink_free(struct devlink *devlink);

diff --git a/net/devlink/core.c b/net/devlink/core.c
index c53c996edf1da9805665ea058d736f4585d83944..7cf0b3efbb2f957b323cec2e001881fb388676b7 100644 (file)
--- a/net/devlink/core.c
+++ b/net/devlink/core.c
@@ -83,21 +83,10 @@ struct devlink *__must_check devlink_try_get(struct devlink *devlink)
        return NULL;
 }
 
-static void __devlink_put_rcu(struct rcu_head *head)
-{
-       struct devlink *devlink = container_of(head, struct devlink, rcu);
-
-       complete(&devlink->comp);
-}
-
 void devlink_put(struct devlink *devlink)
 {
        if (refcount_dec_and_test(&devlink->refcount))
-               /* Make sure unregister operation that may await the completion
-                * is unblocked only after all users are after the end of
-                * RCU grace period.
-                */
-               call_rcu(&devlink->rcu, __devlink_put_rcu);
+               kfree_rcu(devlink, rcu);
 }
 
 struct devlink *devlinks_xa_find_get(struct net *net, unsigned long *indexp)
@@ -110,13 +99,6 @@ retry:
        if (!devlink)
                goto unlock;
 
-       /* In case devlink_unregister() was already called and "unregistering"
-        * mark was set, do not allow to get a devlink reference here.
-        * This prevents live-lock of devlink_unregister() wait for completion.
-        */
-       if (xa_get_mark(&devlinks, *indexp, DEVLINK_UNREGISTERING))
-               goto next;
-
        if (!devlink_try_get(devlink))
                goto next;
        if (!net_eq(devlink_net(devlink), net)) {
@@ -152,37 +134,48 @@ void devlink_set_features(struct devlink *devlink, u64 features)
 EXPORT_SYMBOL_GPL(devlink_set_features);
 
 /**
- *     devlink_register - Register devlink instance
- *
- *     @devlink: devlink
+ * devl_register - Register devlink instance
+ * @devlink: devlink
  */
-void devlink_register(struct devlink *devlink)
+int devl_register(struct devlink *devlink)
 {
        ASSERT_DEVLINK_NOT_REGISTERED(devlink);
-       /* Make sure that we are in .probe() routine */
+       devl_assert_locked(devlink);
 
        xa_set_mark(&devlinks, devlink->index, DEVLINK_REGISTERED);
        devlink_notify_register(devlink);
+
+       return 0;
+}
+EXPORT_SYMBOL_GPL(devl_register);
+
+void devlink_register(struct devlink *devlink)
+{
+       devl_lock(devlink);
+       devl_register(devlink);
+       devl_unlock(devlink);
 }
 EXPORT_SYMBOL_GPL(devlink_register);
 
 /**
- *     devlink_unregister - Unregister devlink instance
- *
- *     @devlink: devlink
+ * devl_unregister - Unregister devlink instance
+ * @devlink: devlink
  */
-void devlink_unregister(struct devlink *devlink)
+void devl_unregister(struct devlink *devlink)
 {
        ASSERT_DEVLINK_REGISTERED(devlink);
-       /* Make sure that we are in .remove() routine */
-
-       xa_set_mark(&devlinks, devlink->index, DEVLINK_UNREGISTERING);
-       devlink_put(devlink);
-       wait_for_completion(&devlink->comp);
+       devl_assert_locked(devlink);
 
        devlink_notify_unregister(devlink);
        xa_clear_mark(&devlinks, devlink->index, DEVLINK_REGISTERED);
-       xa_clear_mark(&devlinks, devlink->index, DEVLINK_UNREGISTERING);
+}
+EXPORT_SYMBOL_GPL(devl_unregister);
+
+void devlink_unregister(struct devlink *devlink)
+{
+       devl_lock(devlink);
+       devl_unregister(devlink);
+       devl_unlock(devlink);
 }
 EXPORT_SYMBOL_GPL(devlink_unregister);
 
@@ -246,7 +239,6 @@ struct devlink *devlink_alloc_ns(const struct devlink_ops *ops,
        mutex_init(&devlink->reporters_lock);
        mutex_init(&devlink->linecards_lock);
        refcount_set(&devlink->refcount, 1);
-       init_completion(&devlink->comp);
 
        return devlink;
 
@@ -292,7 +284,7 @@ void devlink_free(struct devlink *devlink)
 
        xa_erase(&devlinks, devlink->index);
 
-       kfree(devlink);
+       devlink_put(devlink);
 }
 EXPORT_SYMBOL_GPL(devlink_free);
 

diff --git a/net/devlink/devl_internal.h b/net/devlink/devl_internal.h
index 01a00df81d0ecc9c7d4eaf1363eda2fec1a35f0a..5d2bbe295659664916623ddefc8809a3abf457e5 100644 (file)
--- a/net/devlink/devl_internal.h
+++ b/net/devlink/devl_internal.h
@@ -12,7 +12,6 @@
 #include <net/net_namespace.h>
 
 #define DEVLINK_REGISTERED XA_MARK_1
-#define DEVLINK_UNREGISTERING XA_MARK_2
 
 #define DEVLINK_RELOAD_STATS_ARRAY_SIZE \
        (__DEVLINK_RELOAD_LIMIT_MAX * __DEVLINK_RELOAD_ACTION_MAX)
@@ -52,7 +51,6 @@ struct devlink {
        struct lock_class_key lock_key;
        u8 reload_failed:1;
        refcount_t refcount;
-       struct completion comp;
        struct rcu_head rcu;
        struct notifier_block netdevice_nb;
        char priv[] __aligned(NETDEV_ALIGN);