diff mbox series

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

Message ID 20231216123001.1293639-6-jiri@resnulli.us (mailing list archive)
State Accepted
Commit a731132424adeda4d5383ef61afae2e804063fb7
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
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: 1167 this patch: 1167
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: 1194 this patch: 1194
netdev/checkpatch warning CHECK: No space is necessary after a cast
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. 16, 2023, 12:29 p.m. UTC
From: Jiri Pirko <jiri@nvidia.com>

Introduce an xarray for Generic netlink family to store per-socket
private. Initialize this xarray only if family uses per-socket privs.

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

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

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
v7->v8:
- changed __genl_sk_priv_get() to return ERR_PTR() encoded pointer
  in case of a family bug
v6->v7:
- converted family->sock_priv_list to family->sock_privs xarray
  and use it to store the per-socket privs, use sock pointer as
  an xarrar index. This made the code much simpler
- removed no longer needed struct genl_sock and related code as the priv
  is stored in family xarray only
- removed sk_priv wrapper struct as destroy() is available through
  family pointer during priv_free() call, store void *priv directly into
  xarray
- change the genl_release() to iterate over families to free privs
- changed xa_cmpxchg() error flow in genl_sk_priv_get()
- updated __genl_sk_priv_get() and genl_sk_priv_get() function comments
  accordingly
- swapped __genl_sk_priv_get() and genl_sk_priv_get() args to better fit
  the changed lookup scheme
- updated patch description
v5->v6:
- moved sock_priv* fields out of private section of struct genl_family
- introduced NETLINK_SOCK_PROTO_SIZE define and use that in
  NETLINK_SOCK_SIZE computation
- moved struct genl_sock into genetlink.c, added BUILD_BUG_ON check for
  its size
- added missing priv free when xa_cmpxchg() fails
- added per-family sock privs tracking in a list, init the list with
  lock on family register, free all related privs on family unregister
- moved code up in above family register/unregister code
- added documentation comment part for sock_priv* family struct fields
- added WARN_ON_ONCE priv size check in genl_sk_priv_alloc()
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
- moved genl_unlock() call a bit up in the unlikely case section
- remove "again" label and return directly
v3->v4:
- new patch
---
 include/net/genetlink.h |  11 +++
 net/netlink/genetlink.c | 144 +++++++++++++++++++++++++++++++++++++++-
 2 files changed, 154 insertions(+), 1 deletion(-)

Comments

Andy Shevchenko Dec. 18, 2023, 11:01 a.m. UTC | #1
On Sat, Dec 16, 2023 at 01:29:57PM +0100, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@nvidia.com>
> 
> Introduce an xarray for Generic netlink family to store per-socket
> private. Initialize this xarray only if family uses per-socket privs.
> 
> Introduce genl_sk_priv_get() to get the socket priv pointer for a family
> and initialize it in case it does not exist.
> Introduce __genl_sk_priv_get() to obtain socket priv pointer for a
> family under RCU read lock.
> 
> Allow family to specify the priv size, init() and destroy() callbacks.

...

> +	void			(*sock_priv_init)(void *priv);

Can in some cases init fail? Shouldn't we allow to propagate the error code
and fail the flow?

> +	void			(*sock_priv_destroy)(void *priv);

...

P.S> I'm fine with either, just consider above as a material to think about.
Jiri Pirko Dec. 18, 2023, 2:01 p.m. UTC | #2
Mon, Dec 18, 2023 at 12:01:09PM CET, andriy.shevchenko@linux.intel.com wrote:
>On Sat, Dec 16, 2023 at 01:29:57PM +0100, Jiri Pirko wrote:
>> From: Jiri Pirko <jiri@nvidia.com>
>> 
>> Introduce an xarray for Generic netlink family to store per-socket
>> private. Initialize this xarray only if family uses per-socket privs.
>> 
>> Introduce genl_sk_priv_get() to get the socket priv pointer for a family
>> and initialize it in case it does not exist.
>> Introduce __genl_sk_priv_get() to obtain socket priv pointer for a
>> family under RCU read lock.
>> 
>> Allow family to specify the priv size, init() and destroy() callbacks.
>
>...
>
>> +	void			(*sock_priv_init)(void *priv);
>
>Can in some cases init fail? Shouldn't we allow to propagate the error code
>and fail the flow?

Currently not needed. Easy to add when (if ever) this is going to be
needed.

>
>> +	void			(*sock_priv_destroy)(void *priv);
>
>...
>
>P.S> I'm fine with either, just consider above as a material to think about.

I always do consider these :)


>
>-- 
>With Best Regards,
>Andy Shevchenko
>
>
diff mbox series

Patch

diff --git a/include/net/genetlink.h b/include/net/genetlink.h
index c53244f20437..6bc37f392a9a 100644
--- a/include/net/genetlink.h
+++ b/include/net/genetlink.h
@@ -51,6 +51,9 @@  struct genl_info;
  * @split_ops: the split do/dump form of operation definition
  * @n_split_ops: number of entries in @split_ops, not that with split do/dump
  *	ops the number of entries is not the same as number of commands
+ * @sock_priv_size: the size of per-socket private memory
+ * @sock_priv_init: the per-socket private memory initializer
+ * @sock_priv_destroy: the per-socket private memory destructor
  *
  * Attribute policies (the combination of @policy and @maxattr fields)
  * can be attached at the family level or at the operation level.
@@ -84,11 +87,17 @@  struct genl_family {
 	const struct genl_multicast_group *mcgrps;
 	struct module		*module;
 
+	size_t			sock_priv_size;
+	void			(*sock_priv_init)(void *priv);
+	void			(*sock_priv_destroy)(void *priv);
+
 /* private: internal use only */
 	/* protocol family identifier */
 	int			id;
 	/* starting number of multicast group IDs in this family */
 	unsigned int		mcgrp_offset;
+	/* list of per-socket privs */
+	struct xarray		*sock_privs;
 };
 
 /**
@@ -298,6 +307,8 @@  static inline bool genl_info_is_ntf(const struct genl_info *info)
 	return !info->nlhdr;
 }
 
+void *__genl_sk_priv_get(struct genl_family *family, struct sock *sk);
+void *genl_sk_priv_get(struct genl_family *family, struct sock *sk);
 int genl_register_family(struct genl_family *family);
 int genl_unregister_family(const struct genl_family *family);
 void genl_notify(const struct genl_family *family, struct sk_buff *skb,
diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 9c7ffd10df2a..c0d15470a10b 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -631,6 +631,138 @@  static int genl_validate_ops(const struct genl_family *family)
 	return 0;
 }
 
+static void *genl_sk_priv_alloc(struct genl_family *family)
+{
+	void *priv;
+
+	priv = kzalloc(family->sock_priv_size, GFP_KERNEL);
+	if (!priv)
+		return ERR_PTR(-ENOMEM);
+
+	if (family->sock_priv_init)
+		family->sock_priv_init(priv);
+
+	return priv;
+}
+
+static void genl_sk_priv_free(const struct genl_family *family, void *priv)
+{
+	if (family->sock_priv_destroy)
+		family->sock_priv_destroy(priv);
+	kfree(priv);
+}
+
+static int genl_sk_privs_alloc(struct genl_family *family)
+{
+	if (!family->sock_priv_size)
+		return 0;
+
+	family->sock_privs = kzalloc(sizeof(*family->sock_privs), GFP_KERNEL);
+	if (!family->sock_privs)
+		return -ENOMEM;
+	xa_init(family->sock_privs);
+	return 0;
+}
+
+static void genl_sk_privs_free(const struct genl_family *family)
+{
+	unsigned long id;
+	void *priv;
+
+	if (!family->sock_priv_size)
+		return;
+
+	xa_for_each(family->sock_privs, id, priv)
+		genl_sk_priv_free(family, priv);
+
+	xa_destroy(family->sock_privs);
+	kfree(family->sock_privs);
+}
+
+static void genl_sk_priv_free_by_sock(struct genl_family *family,
+				      struct sock *sk)
+{
+	void *priv;
+
+	if (!family->sock_priv_size)
+		return;
+	priv = xa_erase(family->sock_privs, (unsigned long) sk);
+	if (!priv)
+		return;
+	genl_sk_priv_free(family, priv);
+}
+
+static void genl_release(struct sock *sk, unsigned long *groups)
+{
+	struct genl_family *family;
+	unsigned int id;
+
+	down_read(&cb_lock);
+
+	idr_for_each_entry(&genl_fam_idr, family, id)
+		genl_sk_priv_free_by_sock(family, sk);
+
+	up_read(&cb_lock);
+}
+
+/**
+ * __genl_sk_priv_get - Get family private pointer for socket, if exists
+ *
+ * @family: family
+ * @sk: socket
+ *
+ * Lookup a private memory for a Generic netlink family and specified socket.
+ *
+ * Caller should make sure this is called in RCU read locked section.
+ *
+ * Return: valid pointer on success, otherwise negative error value
+ * encoded by ERR_PTR(), NULL in case priv does not exist.
+ */
+void *__genl_sk_priv_get(struct genl_family *family, struct sock *sk)
+{
+	if (WARN_ON_ONCE(!family->sock_privs))
+		return ERR_PTR(-EINVAL);
+	return xa_load(family->sock_privs, (unsigned long) sk);
+}
+
+/**
+ * genl_sk_priv_get - Get family private pointer for socket
+ *
+ * @family: family
+ * @sk: socket
+ *
+ * Lookup a private memory for a Generic netlink family and specified socket.
+ * Allocate the private memory in case it was not already done.
+ *
+ * Return: valid pointer on success, otherwise negative error value
+ * encoded by ERR_PTR().
+ */
+void *genl_sk_priv_get(struct genl_family *family, struct sock *sk)
+{
+	void *priv, *old_priv;
+
+	priv = __genl_sk_priv_get(family, sk);
+	if (priv)
+		return 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->sock_privs, (unsigned long) sk, NULL,
+			      priv, GFP_KERNEL);
+	if (old_priv) {
+		genl_sk_priv_free(family, priv);
+		if (xa_is_err(old_priv))
+			return ERR_PTR(xa_err(old_priv));
+		/* Race happened, priv for the socket was already inserted. */
+		return old_priv;
+	}
+	return priv;
+}
+
 /**
  * genl_register_family - register a generic netlink family
  * @family: generic netlink family
@@ -659,6 +791,10 @@  int genl_register_family(struct genl_family *family)
 		goto errout_locked;
 	}
 
+	err = genl_sk_privs_alloc(family);
+	if (err)
+		goto errout_locked;
+
 	/*
 	 * Sadly, a few cases need to be special-cased
 	 * due to them having previously abused the API
@@ -679,7 +815,7 @@  int genl_register_family(struct genl_family *family)
 				      start, end + 1, GFP_KERNEL);
 	if (family->id < 0) {
 		err = family->id;
-		goto errout_locked;
+		goto errout_sk_privs_free;
 	}
 
 	err = genl_validate_assign_mc_groups(family);
@@ -698,6 +834,8 @@  int genl_register_family(struct genl_family *family)
 
 errout_remove:
 	idr_remove(&genl_fam_idr, family->id);
+errout_sk_privs_free:
+	genl_sk_privs_free(family);
 errout_locked:
 	genl_unlock_all();
 	return err;
@@ -728,6 +866,9 @@  int genl_unregister_family(const struct genl_family *family)
 	up_write(&cb_lock);
 	wait_event(genl_sk_destructing_waitq,
 		   atomic_read(&genl_sk_destructing_cnt) == 0);
+
+	genl_sk_privs_free(family);
+
 	genl_unlock();
 
 	genl_ctrl_event(CTRL_CMD_DELFAMILY, family, NULL, 0);
@@ -1708,6 +1849,7 @@  static int __net_init genl_pernet_init(struct net *net)
 		.input		= genl_rcv,
 		.flags		= NL_CFG_F_NONROOT_RECV,
 		.bind		= genl_bind,
+		.release	= genl_release,
 	};
 
 	/* we'll bump the group number right afterwards */