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

[Jiri Pirko <jiri@resnulli.us>]

Tue, Nov 28, 2023 at 01:30:51PM CET, przemyslaw.kitszel@intel.com wrote:
>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 ;))

Okay, will fix. I just thought this is okay when scripts/kernel-doc is
happy.

>
>
>
>[snip]