Re: [patch net-next v4 5/9] genetlink: introduce per-sock family private pointer storage

Przemek Kitszel
2023-11-28T12:31:07Z

On 11/23/23 19:15, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@nvidia.com>
>
> Introduce a priv pointer into struct netlink_sock. Use it to store a per
> socket xarray that contains family->id indexed priv pointer storage.
> Note I used xarray instead of suggested linked list as it is more
> convenient, without need to have a container struct that would
> contain struct list_head item.
>
> Introduce genl_sk_priv_store() to store the priv pointer.
> Introduce genl_sk_priv_get() to obtain the priv pointer under RCU
> read lock.
>
> Assume that kfree() is good for free of privs for now, as the only user
> introduced by the follow-up patch (devlink) will use kzalloc() for the
> allocation of the memory of the stored pointer. If later on
> this needs to be made custom, a callback is going to be needed.
> Until then (if ever), do this in a simple way.
>
> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
> ---
> v3->v4:
> - new patch
> ---
> include/net/genetlink.h | 3 ++
> net/netlink/af_netlink.h | 1 +
> net/netlink/genetlink.c | 98 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 102 insertions(+)
>
> diff --git a/include/net/genetlink.h b/include/net/genetlink.h
> index e18a4c0d69ee..66c1e50415e0 100644
> --- a/include/net/genetlink.h
> +++ b/include/net/genetlink.h
> @@ -300,6 +300,9 @@ 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,
> 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_store(struct sock *sk, struct genl_family *family,
> + void *priv);
>
> 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.h b/net/netlink/af_netlink.h
> index 2145979b9986..5d96135a4cf3 100644
> --- a/net/netlink/af_netlink.h
> +++ b/net/netlink/af_netlink.h
> @@ -51,6 +51,7 @@ struct netlink_sock {
> struct rhash_head node;
> struct rcu_head rcu;
> struct work_struct work;
> + void __rcu *priv;
> };
>
> static inline struct netlink_sock *nlk_sk(struct sock *sk)
> diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
> index 92ef5ed2e7b0..aae5e63fa50b 100644
> --- a/net/netlink/genetlink.c
> +++ b/net/netlink/genetlink.c
> @@ -21,6 +21,7 @@
> #include <linux/idr.h>
> #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 +1700,109 @@ static int genl_bind(struct net *net, int group)
> return ret;
> }
>
> +struct genl_sk_ctx {
> + struct xarray family_privs;
> +};
> +
> +static struct genl_sk_ctx *genl_sk_ctx_alloc(void)
> +{
> + struct genl_sk_ctx *ctx;
> +
> + ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> + if (!ctx)
> + return NULL;
> + xa_init_flags(&ctx->family_privs, XA_FLAGS_ALLOC);
> + return ctx;
> +}
> +
> +static void genl_sk_ctx_free(struct genl_sk_ctx *ctx)
> +{
> + unsigned long family_id;
> + void *priv;
> +
> + xa_for_each(&ctx->family_privs, family_id, priv) {
> + xa_erase(&ctx->family_privs, family_id);
> + kfree(priv);
> + }
> + xa_destroy(&ctx->family_privs);
> + kfree(ctx);
> +}
> +
> +/**
> + * 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.
> + *
> + * Returns: valid pointer on success, otherwise NULL.

since you are going to post next revision,

kernel-doc requires "Return:" section (singular form)
https://docs.kernel.org/doc-guide/kernel-doc.html#function-documentation

for new code we should strive to fulfil the requirement
(or piss-off someone powerful enough to change the requirement ;))

[snip]