diff mbox series

[net-next,v5,5/9] genetlink: introduce per-sock family private storage

Message ID 20231206182120.957225-6-jiri@resnulli.us (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series devlink: introduce notifications filtering | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next, async
netdev/ynl success Generated files up to date; no warnings/errors; GEN HAS DIFF 2 files changed, 90 insertions(+);
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1170 this patch: 1170
netdev/cc_maintainers success CCed 5 of 5 maintainers
netdev/build_clang success Errors and warnings before: 1143 this patch: 1143
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1197 this patch: 1197
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 211 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Jiri Pirko Dec. 6, 2023, 6:21 p.m. UTC
From: Jiri Pirko <jiri@nvidia.com>

Introduce a wrapper sock struct for Generic netlink and store
a pointer to family privs xarray. This per socket xarray contains
family->id indexed priv storage.

Note I used xarray instead of suggested linked list as it is more
convenient.

Introduce genl_sk_priv_get() to get the family priv pointer and
initialize it in case it does not exist.
Introduce __genl_sk_priv_get() to obtain family the priv pointer
under RCU read lock.

Allow family to specify the priv size, init() and destroy() callbacks.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
v4->v5:
- s/Returns/Return/ in function comments
- introduced wrapper genl sock struct and store xarray there
- changed family helpers to genl_sk_priv_get() and __genl_sk_priv_get()
- introduced sock_priv_size for family and use this to allocate the priv
  in generic netlink code
- introduced init/destroy callbacks for family privs
v3->v4:
- new patch
---
 include/net/genetlink.h  |   6 ++
 net/netlink/af_netlink.c |   2 +-
 net/netlink/af_netlink.h |  15 ++++
 net/netlink/genetlink.c  | 146 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 168 insertions(+), 1 deletion(-)

Comments

Jakub Kicinski Dec. 8, 2023, 2:55 a.m. UTC | #1
On Wed,  6 Dec 2023 19:21:16 +0100 Jiri Pirko wrote:
> diff --git a/include/net/genetlink.h b/include/net/genetlink.h
> index e18a4c0d69ee..dbf11464e96a 100644
> --- a/include/net/genetlink.h
> +++ b/include/net/genetlink.h
> @@ -87,6 +87,9 @@ struct genl_family {
>  	int			id;
>  	/* starting number of multicast group IDs in this family */
>  	unsigned int		mcgrp_offset;
> +	size_t			sock_priv_size;
> +	void			(*sock_priv_init)(void *priv);
> +	void			(*sock_priv_destroy)(void *priv);


Jiri Pirko Dec. 8, 2023, 10:07 a.m. UTC | #2
Fri, Dec 08, 2023 at 03:55:26AM CET, kuba@kernel.org wrote:
>On Wed,  6 Dec 2023 19:21:16 +0100 Jiri Pirko wrote:
>> diff --git a/include/net/genetlink.h b/include/net/genetlink.h
>> index e18a4c0d69ee..dbf11464e96a 100644
>> --- a/include/net/genetlink.h
>> +++ b/include/net/genetlink.h
>> @@ -87,6 +87,9 @@ struct genl_family {
>>  	int			id;
>>  	/* starting number of multicast group IDs in this family */
>>  	unsigned int		mcgrp_offset;
>> +	size_t			sock_priv_size;
>> +	void			(*sock_priv_init)(void *priv);
>> +	void			(*sock_priv_destroy)(void *priv);
>
>
Jiri Pirko Dec. 8, 2023, 2:21 p.m. UTC | #3
Fri, Dec 08, 2023 at 03:55:26AM CET, kuba@kernel.org wrote:
>On Wed,  6 Dec 2023 19:21:16 +0100 Jiri Pirko wrote:

[...]

>> +static struct genl_sk_priv *genl_sk_priv_alloc(struct genl_family *family)
>> +{
>> +	struct genl_sk_priv *priv;
>> +
>> +	priv = kzalloc(size_add(sizeof(*priv), family->sock_priv_size),
>> +		       GFP_KERNEL);
>> +	if (!priv)
>> +		return ERR_PTR(-ENOMEM);
>> +	priv->destructor = family->sock_priv_destroy;
>
>family->sock_priv_destroy may be in module memory.
>I think you need to wipe them when family goes :(

Crap. That's a bit problematic. Family can unregister and register
again, with user having the same sock sill opened with legitimate
expectation of filter being applied. Don't see now how to handle this
other then no-destroy and just kfree here in genetlink.c :/ Going back
to v4?


>
>> +	if (family->sock_priv_init)
>> +		family->sock_priv_init(priv->priv);
>> +	return priv;
>> +}

[...]
Jakub Kicinski Dec. 8, 2023, 4:11 p.m. UTC | #4
On Fri, 8 Dec 2023 15:21:52 +0100 Jiri Pirko wrote:
> >> +static struct genl_sk_priv *genl_sk_priv_alloc(struct genl_family *family)
> >> +{
> >> +	struct genl_sk_priv *priv;
> >> +
> >> +	priv = kzalloc(size_add(sizeof(*priv), family->sock_priv_size),
> >> +		       GFP_KERNEL);
> >> +	if (!priv)
> >> +		return ERR_PTR(-ENOMEM);
> >> +	priv->destructor = family->sock_priv_destroy;  
> >
> >family->sock_priv_destroy may be in module memory.
> >I think you need to wipe them when family goes :(  
> 
> Crap. That's a bit problematic. Family can unregister and register
> again, with user having the same sock sill opened with legitimate
> expectation of filter being applied. Don't see now how to handle this
> other then no-destroy and just kfree here in genetlink.c :/ Going back
> to v4?

When family gets removed all subs must be cleared. So the user
sock will have to resolve the mcast ID again, and re-subscribe
again to get any notification. Having to re-sub implies having
to re-add filters in my mind.
Jiri Pirko Dec. 9, 2023, 10:36 a.m. UTC | #5
Fri, Dec 08, 2023 at 05:11:23PM CET, kuba@kernel.org wrote:
>On Fri, 8 Dec 2023 15:21:52 +0100 Jiri Pirko wrote:
>> >> +static struct genl_sk_priv *genl_sk_priv_alloc(struct genl_family *family)
>> >> +{
>> >> +	struct genl_sk_priv *priv;
>> >> +
>> >> +	priv = kzalloc(size_add(sizeof(*priv), family->sock_priv_size),
>> >> +		       GFP_KERNEL);
>> >> +	if (!priv)
>> >> +		return ERR_PTR(-ENOMEM);
>> >> +	priv->destructor = family->sock_priv_destroy;  
>> >
>> >family->sock_priv_destroy may be in module memory.
>> >I think you need to wipe them when family goes :(  
>> 
>> Crap. That's a bit problematic. Family can unregister and register
>> again, with user having the same sock sill opened with legitimate
>> expectation of filter being applied. Don't see now how to handle this
>> other then no-destroy and just kfree here in genetlink.c :/ Going back
>> to v4?
>
>When family gets removed all subs must be cleared. So the user
>sock will have to resolve the mcast ID again, and re-subscribe
>again to get any notification. Having to re-sub implies having
>to re-add filters in my mind.

Okay, that sounds fine. Thanks!
diff mbox series

Patch

diff --git a/include/net/genetlink.h b/include/net/genetlink.h
index e18a4c0d69ee..dbf11464e96a 100644
--- a/include/net/genetlink.h
+++ b/include/net/genetlink.h
@@ -87,6 +87,9 @@  struct genl_family {
 	int			id;
 	/* starting number of multicast group IDs in this family */
 	unsigned int		mcgrp_offset;
+	size_t			sock_priv_size;
+	void			(*sock_priv_init)(void *priv);
+	void			(*sock_priv_destroy)(void *priv);
 };
 
 /**
@@ -301,6 +304,9 @@  int genl_unregister_family(const struct genl_family *family);
 void genl_notify(const struct genl_family *family, struct sk_buff *skb,
 		 struct genl_info *info, u32 group, gfp_t flags);
 
+void *__genl_sk_priv_get(struct sock *sk, struct genl_family *family);
+void *genl_sk_priv_get(struct sock *sk, struct genl_family *family);
+
 void *genlmsg_put(struct sk_buff *skb, u32 portid, u32 seq,
 		  const struct genl_family *family, int flags, u8 cmd);
 
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 177126fb0484..5683b0ca23b1 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -632,7 +632,7 @@  static void netlink_remove(struct sock *sk)
 static struct proto netlink_proto = {
 	.name	  = "NETLINK",
 	.owner	  = THIS_MODULE,
-	.obj_size = sizeof(struct netlink_sock),
+	.obj_size = NETLINK_SOCK_SIZE,
 };
 
 static int __netlink_create(struct net *net, struct socket *sock,
diff --git a/net/netlink/af_netlink.h b/net/netlink/af_netlink.h
index 2145979b9986..1b3ed8919574 100644
--- a/net/netlink/af_netlink.h
+++ b/net/netlink/af_netlink.h
@@ -60,6 +60,21 @@  static inline struct netlink_sock *nlk_sk(struct sock *sk)
 
 #define nlk_test_bit(nr, sk) test_bit(NETLINK_F_##nr, &nlk_sk(sk)->flags)
 
+struct genl_sock {
+	struct netlink_sock nlk_sk;
+	struct xarray *family_privs;
+};
+
+static inline struct genl_sock *genl_sk(struct sock *sk)
+{
+	return container_of(nlk_sk(sk), struct genl_sock, nlk_sk);
+}
+
+/* Size of netlink sock is size of the biggest user with priv,
+ * which is currently just Generic Netlink.
+ */
+#define NETLINK_SOCK_SIZE sizeof(struct genl_sock)
+
 struct netlink_table {
 	struct rhashtable	hash;
 	struct hlist_head	mc_list;
diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 92ef5ed2e7b0..51720c2c6bda 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -22,6 +22,8 @@ 
 #include <net/sock.h>
 #include <net/genetlink.h>
 
+#include "af_netlink.h"
+
 static DEFINE_MUTEX(genl_mutex); /* serialization of message processing */
 static DECLARE_RWSEM(cb_lock);
 
@@ -1699,12 +1701,156 @@  static int genl_bind(struct net *net, int group)
 	return ret;
 }
 
+struct genl_sk_priv {
+	void (*destructor)(void *priv);
+	long priv[];
+};
+
+static struct genl_sk_priv *genl_sk_priv_alloc(struct genl_family *family)
+{
+	struct genl_sk_priv *priv;
+
+	priv = kzalloc(size_add(sizeof(*priv), family->sock_priv_size),
+		       GFP_KERNEL);
+	if (!priv)
+		return ERR_PTR(-ENOMEM);
+	priv->destructor = family->sock_priv_destroy;
+	if (family->sock_priv_init)
+		family->sock_priv_init(priv->priv);
+	return priv;
+}
+
+static void genl_sk_priv_free(struct genl_sk_priv *priv)
+{
+	if (priv->destructor)
+		priv->destructor(priv->priv);
+	kfree(priv);
+}
+
+static void genl_release(struct sock *sk, unsigned long *groups)
+{
+	struct genl_sock *gsk = genl_sk(sk);
+	struct genl_sk_priv *priv;
+	unsigned long family_id;
+
+	if (!gsk->family_privs)
+		return;
+	xa_for_each(gsk->family_privs, family_id, priv) {
+		xa_erase(gsk->family_privs, family_id);
+		genl_sk_priv_free(priv);
+	}
+	xa_destroy(gsk->family_privs);
+	kfree(gsk->family_privs);
+}
+
+static struct xarray *genl_family_privs_get(struct genl_sock *gsk)
+{
+	struct xarray *family_privs;
+
+again:
+	family_privs = READ_ONCE(gsk->family_privs);
+	if (family_privs)
+		return family_privs;
+
+	family_privs = kzalloc(sizeof(*family_privs), GFP_KERNEL);
+	if (!family_privs)
+		return ERR_PTR(-ENOMEM);
+	xa_init_flags(family_privs, XA_FLAGS_ALLOC);
+
+	/* Use genl lock to protect family_privs to be
+	 * initialized in parallel by different CPU.
+	 */
+	genl_lock();
+	if (unlikely(gsk->family_privs)) {
+		xa_destroy(family_privs);
+		kfree(family_privs);
+		genl_unlock();
+		goto again;
+	}
+	WRITE_ONCE(gsk->family_privs, family_privs);
+	genl_unlock();
+	return family_privs;
+}
+
+/**
+ * __genl_sk_priv_get - Get per-socket private pointer for family
+ *
+ * @sk: socket
+ * @family: family
+ *
+ * Lookup a private pointer stored per-socket by a specified
+ * Generic netlink family.
+ *
+ * Caller should make sure this is called in RCU read locked section.
+ *
+ * Return: valid pointer on success, otherwise NULL.
+ */
+void *__genl_sk_priv_get(struct sock *sk, struct genl_family *family)
+{
+	struct genl_sock *gsk = genl_sk(sk);
+	struct genl_sk_priv *priv;
+	struct xarray *family_privs;
+
+	family_privs = READ_ONCE(gsk->family_privs);
+	if (!family_privs)
+		return NULL;
+	priv = xa_load(family_privs, family->id);
+	return priv ? priv->priv : NULL;
+}
+
+/**
+ * genl_sk_priv_get - Get per-socket private pointer for family
+ *
+ * @sk: socket
+ * @family: family
+ *
+ * Store a private pointer per-socket for a specified
+ * Generic netlink family.
+ *
+ * Caller has to make sure this is not called in parallel multiple times
+ * for the same sock and also in parallel to genl_release() for the same sock.
+ *
+ * Return: previously stored private pointer for the family (could be NULL)
+ * on success, otherwise negative error value encoded by ERR_PTR().
+ */
+void *genl_sk_priv_get(struct sock *sk, struct genl_family *family)
+{
+	struct genl_sk_priv *priv, *old_priv;
+	struct genl_sock *gsk = genl_sk(sk);
+	struct xarray *family_privs;
+
+	family_privs = genl_family_privs_get(gsk);
+	if (IS_ERR(family_privs))
+		return ERR_CAST(family_privs);
+
+	priv = xa_load(family_privs, family->id);
+	if (priv)
+		return priv->priv;
+
+	/* priv for the family does not exist so far, create it. */
+
+	priv = genl_sk_priv_alloc(family);
+	if (IS_ERR(priv))
+		return ERR_CAST(priv);
+
+	old_priv = xa_cmpxchg(family_privs, family->id, NULL, priv, GFP_KERNEL);
+	if (xa_is_err(old_priv))
+		return ERR_PTR(xa_err(old_priv));
+	else if (!old_priv)
+		return priv->priv;
+
+	/* Race happened, priv was already inserted. */
+	genl_sk_priv_free(priv);
+	return old_priv->priv;
+}
+
 static int __net_init genl_pernet_init(struct net *net)
 {
 	struct netlink_kernel_cfg cfg = {
 		.input		= genl_rcv,
 		.flags		= NL_CFG_F_NONROOT_RECV,
 		.bind		= genl_bind,
+		.release	= genl_release,
 	};
 
 	/* we'll bump the group number right afterwards */