| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 5 Jan 2023 22:33:58 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: davem@...emloft.net
Cc: netdev@...r.kernel.org, edumazet@...gle.com, pabeni@...hat.com,
jacob.e.keller@...el.com, jiri@...nulli.us,
Jakub Kicinski <kuba@...nel.org>
Subject: [PATCH net-next 5/9] 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@...el.com>
Signed-off-by: Jakub Kicinski <kuba@...nel.org>
---
include/net/devlink.h | 2 ++
net/devlink/core.c | 64 ++++++++++++++++---------------------
net/devlink/devl_internal.h | 2 --
3 files changed, 30 insertions(+), 38 deletions(-)
diff --git a/include/net/devlink.h b/include/net/devlink.h
index 6a2e4f21779f..425ecef431b7 100644
--- 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 c53c996edf1d..7cf0b3efbb2f 100644
--- 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 @@ struct devlink *devlinks_xa_find_get(struct net *net, unsigned long *indexp)
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 01a00df81d0e..5d2bbe295659 100644
--- 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);
--
2.38.1
Powered by blists - more mailing lists