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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Thu, 7 Dec 2023 18:55:26 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: Jiri Pirko <jiri@...nulli.us>
Cc: netdev@...r.kernel.org, pabeni@...hat.com, davem@...emloft.net,
 edumazet@...gle.com, jacob.e.keller@...el.com, jhs@...atatu.com,
 johannes@...solutions.net, andriy.shevchenko@...ux.intel.com,
 amritha.nambiar@...el.com, sdf@...gle.com, horms@...nel.org,
 przemyslaw.kitszel@...el.com
Subject: Re: [patch net-next v5 5/9] genetlink: introduce per-sock family
 private storage

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.

> --- 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

> +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

> +		goto again;

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

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

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