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

[Jiri Pirko <jiri@resnulli.us>]

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);
>
>👍️
>
>but I think it should be above the private fields (and have kdoc)
>The families are expected to make use the new fields, and are not
>supposed to touch anything private.

Oh, right, good point, I missed that.


>
>> --- 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)
>
>Would feel a little cleaner to me to add
>
>#define NETLINK_SOCK_PROTO_SIZE		8
>
>add that to the size, build time check that struct genl_sock's
>size is <= than sizeof(struct netlink_sock) + NETLINK_SOCK_PROTO_SIZE
>
>This way we don't have to fumble the layering by putting genl stuff
>in af_netlink.h

Yeah, I had it like that originally, I didn't like it :) Mainly because
if someone adds-in another field in the future, the build time check
may only fail on some archs. Also, wasting memory on archs there pointer
is 4 bytes :) But as you wish, I don't mind to switch it back.


>
>> +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;
>
>family->sock_priv_destroy may be in module memory.
>I think you need to wipe them when family goes :(
>
>> +	if (family->sock_priv_init)
>> +		family->sock_priv_init(priv->priv);
>> +	return priv;
>> +}
>
>> +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();
>
>nit: unlock can be moved up

Okay.


>
>> +		goto again;
>
>why not return READ_ONCE(gsk->family_privs); ?
>there's no need to loop

Right.

>
>One could also be tempted to:
>
>lock()
>if (likely(!gsk->family_privs)) {
>	WRITE
>} else {
>	destory()
>	free()
>	family_privs = READ
>}
>unlock()
>
>but it could be argued success path should be flat

Okay, I will think about it.



Thanks!


>
>> +	}
>> +	WRITE_ONCE(gsk->family_privs, family_privs);
>> +	genl_unlock();
>> +	return family_privs;
>> +}