Re: [PATCH net] net: zero-initialize skb extensions on allocation

[Vlad Buslov <vladbu@nvidia.com>]

On Mon 24 May 2021 at 13:01, Florian Westphal <fw@strlen.de> wrote:
> Vlad Buslov <vladbu@nvidia.com> wrote:
>> Function skb_ext_add() doesn't initialize created skb extension with any
>> value and leaves it up to the user.
>
> That was intentional.
>
> Its unlikely that all extensions are active at the same time on same skb.
>
> This is also the reason why the extension struct uses offset addressing
> to get the extension data rather than the simpler
>
> skb_ext {
> 	struct sec_path sp;
> 	struct nf_bridge_info nfbr;
> 	...
> }
>
> So adding e.g. mptcp extension will only touch 1 cacheline instead of 3
> (or more if more extensions get added in the future).
>
> IOW, i would prefer if tc would add tc_skb_add_ext() or similar and
> zero whats needed there.

Got it. Thanks for the explanation!

>
>> Fix the issue by changing __skb_ext_alloc() function to request
>> zero-initialized memory from kmem cache. Note that skb extension allocation
>> in skb_ext_maybe_cow() is not changed because newly allocated memory is
>> immediately overwritten with content of old skb extension so there is no
>> need to pre-initialize it.
>>
>> Multiple users of skb extension API have already been manually setting
>> newly allocated skb extension memory to zero. Remove such code and rely on
>> skb extension API instead.
>
> Are you sure its safe?

Apparently it is not, since you found a problem with nf_bridge_alloc :)

>
>>  static inline struct nf_bridge_info *nf_bridge_alloc(struct sk_buff *skb)
>>  {
>>  #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
>> -	struct nf_bridge_info *b = skb_ext_add(skb, SKB_EXT_BRIDGE_NF);
>> -
>> -	if (b)
>> -		memset(b, 0, sizeof(*b));
>> -
>> -	return b;
>> +	return skb_ext_add(skb, SKB_EXT_BRIDGE_NF);
>
> So in the (unlikely) case where skb_ext_add did not allocate a new
> extension, the memory is no longer cleared.
>
> If the skb had an nf_bridge_info extension previously that got
> discarded earlier via skb_ext_del() this now leaks the old content.

So what would you suggest: provide a dedicated wrapper for TC skb
extension that will memset resulting extension to zero or refactor my
patch to zero-initialize specific skb extension in skb_ext_add() (only
the extension requested and also when previously discarded extension is
reused)?