netdev - Re: [PATCH 0/2] fix generic netlink locking issue(s)

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 22 Aug 2013 08:51:23 +0200
From:	Johannes Berg <johannes@...solutions.net>
To:	Pravin Shelar <pshelar@...ira.com>
Cc:	Oliver Hartkopp <socketcan@...tkopp.net>,
	netdev <netdev@...r.kernel.org>, Thomas Graf <tgraf@...g.ch>
Subject: Re: [PATCH 0/2] fix generic netlink locking issue(s)

On Wed, 2013-08-21 at 15:53 -0700, Pravin Shelar wrote:
> This looks like more general problem and not specific to genl_ctrl
> family. I have couple of fixes for issues you are seeing, I will send
> out those patches soon.

No, it's not a general problem.

You don't _want_ dump generally locked, it's not really useful. Netlink
guarantees with the cb_mutex that it can't go away, so locking dump at
the generic netlink level doesn't buy the average generic netlink user
at all.

I think that the problem is that generic netlink is "inside itself" in a
manner of speaking.

OTOH, I'm not sure I fully understand what would happen if a family is
deleted while something inside of it is dumping. Even the module unload
wouldn't be prevented since netlink_dump_start() uses THIS_MODULE (which
is really NULL since generic netlink is built into the kernel), so maybe
you're right about the locking, but then your patch still wouldn't be
correct because the family might still go away even if you lock the
genl_lock() every round.

I think to fix that additional problem we need something like the
(untested!) patch below (which is missing documentation updates) in
addition to fix that problem.

johannes

diff --git a/include/net/genetlink.h b/include/net/genetlink.h
index 93024a4..0bc3a53 100644
--- a/include/net/genetlink.h
+++ b/include/net/genetlink.h
@@ -42,6 +42,7 @@ struct genl_info;
  * @ops_list: list of all assigned operations
  * @family_list: family list
  * @mcast_groups: multicast groups list
+ * @module: the module containing this family
  */
 struct genl_family {
 	unsigned int		id;
@@ -61,6 +62,7 @@ struct genl_family {
 	struct list_head	ops_list;	/* private */
 	struct list_head	family_list;	/* private */
 	struct list_head	mcast_groups;	/* private */
+	struct module *		module;		/* private */
 };
 
 /**
@@ -121,9 +123,24 @@ struct genl_ops {
 	struct list_head	ops_list;
 };
 
-extern int genl_register_family(struct genl_family *family);
-extern int genl_register_family_with_ops(struct genl_family *family,
+extern int __genl_register_family(struct genl_family *family);
+extern int __genl_register_family_with_ops(struct genl_family *family,
 	struct genl_ops *ops, size_t n_ops);
+
+static inline int genl_register_family(struct genl_family *family)
+{
+	family->module = THIS_MODULE;
+	return __genl_register_family(family);
+}
+
+static inline int genl_register_family_with_ops(struct genl_family *family,
+						struct genl_ops *ops,
+						size_t n_ops)
+{
+	family->module = THIS_MODULE;
+	return __genl_register_family_with_ops(family, ops, n_ops);
+}
+
 extern int genl_unregister_family(struct genl_family *family);
 extern int genl_register_ops(struct genl_family *, struct genl_ops *ops);
 extern int genl_unregister_ops(struct genl_family *, struct genl_ops *ops);
diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 2fd6dbe..8bab493 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -374,7 +374,7 @@ EXPORT_SYMBOL(genl_unregister_ops);
  *
  * Return 0 on success or a negative error code.
  */
-int genl_register_family(struct genl_family *family)
+int __genl_register_family(struct genl_family *family)
 {
 	int err = -EINVAL;
 
@@ -430,7 +430,7 @@ errout_locked:
 errout:
 	return err;
 }
-EXPORT_SYMBOL(genl_register_family);
+EXPORT_SYMBOL(__genl_register_family);
 
 /**
  * genl_register_family_with_ops - register a generic netlink family
@@ -457,12 +457,12 @@ EXPORT_SYMBOL(genl_register_family);
  *
  * Return 0 on success or a negative error code.
  */
-int genl_register_family_with_ops(struct genl_family *family,
+int __genl_register_family_with_ops(struct genl_family *family,
 	struct genl_ops *ops, size_t n_ops)
 {
 	int err, i;
 
-	err = genl_register_family(family);
+	err = __genl_register_family(family);
 	if (err)
 		return err;
 
@@ -476,7 +476,7 @@ err_out:
 	genl_unregister_family(family);
 	return err;
 }
-EXPORT_SYMBOL(genl_register_family_with_ops);
+EXPORT_SYMBOL(__genl_register_family_with_ops);
 
 /**
  * genl_unregister_family - unregister generic netlink family


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html