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

[Jakub Kicinski <kuba@kernel.org>]

On Tue, 12 Dec 2023 11:17:32 +0100 Jiri Pirko wrote:
> +static void genl_sk_priv_free(struct genl_sk_priv *priv)
> +{
> +	spin_lock(&priv->family->sock_priv_list->lock);
> +	list_del(&priv->list);
> +	spin_unlock(&priv->family->sock_priv_list->lock);
> +	if (priv->destructor)
> +		priv->destructor(priv->priv);
> +	kfree(priv);

> +static void genl_sk_priv_list_free(const struct genl_family *family)
> +{
> +	struct genl_sk_priv *priv, *tmp;
> +
> +	if (!family->sock_priv_size)
> +		return;
> +
> +	list_for_each_entry_safe(priv, tmp, &family->sock_priv_list->list, list)
> +		genl_sk_priv_free(priv);
> +	kfree(family->sock_priv_list);

Is this not racy for socket close vs family unregister?
Once family starts to unregister no new privs can be installed
(on the basis that such family's callbacks can no longer be called).
But a socket may get closed in the meantime, and we'll end up entering
genl_sk_priv_free() both from genl_release() and genl_sk_priv_list_free().

Also I'm afraid there is a race still between removing the entry from
the list and calling destroy.