netdev - Re: [patch net-next v4 5/9] genetlink: introduce per-sock family private pointer 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: Wed, 29 Nov 2023 14:59:31 +0100
From: Jiri Pirko <jiri@...nulli.us>
To: Jakub Kicinski <kuba@...nel.org>
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
Subject: Re: [patch net-next v4 5/9] genetlink: introduce per-sock family
 private pointer storage

Tue, Nov 28, 2023 at 05:36:05PM CET, kuba@...nel.org wrote:
>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.

I'm lost. What "string" are you talking about exactly? I'm not putting
any string to xarray.

In the existing implementation, I have following struct:
struct devlink_obj_desc {
        struct rcu_head rcu;
        const char *bus_name;
        const char *dev_name;
        unsigned int port_index;
        bool port_index_valid;
        long data[];
};

This is the struct put pointer to into the xarray. Pointer to this
struct is dereferenced under rcu in notification code and the struct
is freed by kfree_rcu().


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

"the state is basically a pointer to a string". I don't follow what you
mean by this :/


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

Sure.


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

Not at all, I have no reason for it. I just want to get my job done
and I am having very hard time to understand what you want exactly.