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

On Mon 24 May 2021 at 13:01, Florian Westphal [off-list ref] wrote:

Vlad Buslov [off-list ref] wrote:

quoted

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!

quoted

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

quoted

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