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

[Jakub Kicinski <kuba@kernel.org>]

On Tue, 28 Nov 2023 17:05:48 +0100 Jiri Pirko wrote:
> >Not necessarily, you can have a helper which doesn't allocate, too.
> >What I'm saying is that the common case for ops will be to access
> >the state and allocate if it doesn't exist.
> >
> >How about genl_sk_family_priv() and genl_sk_has_family_priv() ?  
> 
> My point is, with what you suggest, it will look something like this:
> 
> 1) user does DEVLINK_CMD_NOTIFY_FILTER_SET
> 2) devlink calls into genetlink code for a sk_priv and inserts in xa_array
> 3) genetlink allocates sk_priv and returns back
> 4) devlink fills-up the sk_priv
> 
> 5) user does DEVLINK_CMD_NOTIFY_FILTER_SET, again
> 6) devlink calls into genetlink code for a sk_priv
> 7) genetlink returns already exising sk_priv found in xa_array
> 8) devlink fills-up the sk_priv
> 
> Now the notification thread, sees sk_priv zeroed between 3) and 4)
> and inconsistent during 4) and 8)
> 
> I originally solved that by rcu, DEVLINK_CMD_NOTIFY_FILTER_SET
> code always allocates and flips the rcu pointer. Notification thread
> always sees sk_priv consistent.
> 
> If you want to allocate sk_priv in genetlink code once, hard to use
> the rcu mechanism and have to protect the sk_priv memory by a lock.

No, you can do exact same thing, just instead of putting the string
directly into the xarray you put a struct which points to the string.

> What am I missing?

The fact that someone in the future may want to add another devlink
priv field, and if the state is basically a pointer to a string,
with complicated lifetime, they will have to suffer undoing that.

> >> If it is alloceted automatically, why is it needed?  
> >
> >Because priv may be a complex type which has member that need
> >individual fields to be destroyed (in fullness of time we also
> >need a constructor which can init things like list_head, but
> >we can defer that).
> >
> >I'm guessing in your case the priv will look like this:
> >
> >struct devlink_sk_priv {
> >	const char *nft_fltr_instance_name;
> >};
> >
> >static void devlink_sk_priv_free(void *ptr)
> >{
> >	struct devlink_sk_priv *priv = ptr;
> >
> >	kfree(priv->nft_fltr_instance_name);
> >}  
> 
> If genetlink code does the allocation, it should know how to free.
> Does not make sense to pass destructor to genetlink code to free memory
> it actually allocated :/
> 
> If devlink does the allocation, this callback makes sense. I was
> thinking about having it, but decided kfree is okay for now and
> destructor could be always introduced if needed.

Did you read the code snippet above?

Core still does the kfree of the container (struct devlink_sk_priv).
But what's inside the container struct (string pointer) has to be
handled by the destructor.

Feels like you focus on how to prove me wrong more than on
understanding what I'm saying :|