[PATCH net-next v2 0/2] net: flow dissector: allow explicit passing of netns

[PATCH net-next v2 0/2] net: flow dissector: allow explicit passing of netns

[Florian Westphal]

Change since last version:
 fix kdoc comment warning reported by kbuild robot, no other changes,
 thus retaining RvB tags from Eric and Willem.
 v1: https://lore.kernel.org/netdev/20240607083205.3000-1-fw@strlen.de/

Years ago flow dissector gained ability to delegate flow dissection
to a bpf program, scoped per netns.

The netns is derived from skb->dev, and if that is not available, from
skb->sk.  If neither is set, we hit a (benign) WARN_ON_ONCE().

This WARN_ON_ONCE can be triggered from netfilter.
Known skb origins are nf_send_reset and ipv4 stack generated IGMP
messages.

Lets allow callers to pass the current netns explicitly and make
nf_tables use those instead.

This targets net-next instead of net because the WARN is benign and this
is not a regression.

Florian Westphal (2):
  net: add and use skb_get_hash_net
  net: add and use __skb_get_hash_symmetric_net

 include/linux/skbuff.h          | 20 +++++++++++++++++---
 net/core/flow_dissector.c       | 21 ++++++++++++++-------
 net/netfilter/nf_tables_trace.c |  2 +-
 net/netfilter/nft_hash.c        |  3 ++-
 4 files changed, 34 insertions(+), 12 deletions(-)

-- 
2.44.2

[PATCH net-next v2 1/2] net: add and use skb_get_hash_net

[Florian Westphal]

Years ago flow dissector gained ability to delegate flow dissection
to a bpf program, scoped per netns.

Unfortunately, skb_get_hash() only gets an sk_buff argument instead
of both net+skb.  This means the flow dissector needs to obtain the
netns pointer from somewhere else.

The netns is derived from skb->dev, and if that is not available, from
skb->sk.  If neither is set, we hit a (benign) WARN_ON_ONCE().

Trying both dev and sk covers most cases, but not all, as recently
reported by Christoph Paasch.

In case of nf-generated tcp reset, both sk and dev are NULL:

WARNING: .. net/core/flow_dissector.c:1104
 skb_flow_dissect_flow_keys include/linux/skbuff.h:1536 [inline]
 skb_get_hash include/linux/skbuff.h:1578 [inline]
 nft_trace_init+0x7d/0x120 net/netfilter/nf_tables_trace.c:320
 nft_do_chain+0xb26/0xb90 net/netfilter/nf_tables_core.c:268
 nft_do_chain_ipv4+0x7a/0xa0 net/netfilter/nft_chain_filter.c:23
 nf_hook_slow+0x57/0x160 net/netfilter/core.c:626
 __ip_local_out+0x21d/0x260 net/ipv4/ip_output.c:118
 ip_local_out+0x26/0x1e0 net/ipv4/ip_output.c:127
 nf_send_reset+0x58c/0x700 net/ipv4/netfilter/nf_reject_ipv4.c:308
 nft_reject_ipv4_eval+0x53/0x90 net/ipv4/netfilter/nft_reject_ipv4.c:30
 [..]

syzkaller did something like this:
table inet filter {
  chain input {
    type filter hook input priority filter; policy accept;
    meta nftrace set 1
    tcp dport 42 reject with tcp reset
   }
   chain output {
    type filter hook output priority filter; policy accept;
    # empty chain is enough
   }
}

... then sends a tcp packet to port 42.

Initial attempt to simply set skb->dev from nf_reject_ipv4 doesn't cover
all cases: skbs generated via ipv4 igmp_send_report trigger similar splat.

Moreover, Pablo Neira found that nft_hash.c uses __skb_get_hash_symmetric()
which would trigger same warn splat for such skbs.

Lets allow callers to pass the current netns explicitly.
The nf_trace infrastructure is adjusted to use the new helper.

__skb_get_hash_symmetric is handled in the next patch.

Reported-by: Christoph Paasch <cpaasch@apple.com>
Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/494
Reviewed-by: Eric Dumazet <edumazet@google.com>
Reviewed-by: Willem de Bruijn <willemb@google.com>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 Changes since v1: add @net to kdoc comment (kbuild robot warning), no
 other changes.

 include/linux/skbuff.h          | 12 ++++++++++--
 net/core/flow_dissector.c       | 15 +++++++++++----
 net/netfilter/nf_tables_trace.c |  2 +-
 3 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index fe7d8dbef77e..6e78019f899a 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -1498,7 +1498,7 @@ __skb_set_sw_hash(struct sk_buff *skb, __u32 hash, bool is_l4)
 	__skb_set_hash(skb, hash, true, is_l4);
 }
 
-void __skb_get_hash(struct sk_buff *skb);
+void __skb_get_hash_net(const struct net *net, struct sk_buff *skb);
 u32 __skb_get_hash_symmetric(const struct sk_buff *skb);
 u32 skb_get_poff(const struct sk_buff *skb);
 u32 __skb_get_poff(const struct sk_buff *skb, const void *data,
@@ -1578,10 +1578,18 @@ void skb_flow_dissect_hash(const struct sk_buff *skb,
 			   struct flow_dissector *flow_dissector,
 			   void *target_container);
 
+static inline __u32 skb_get_hash_net(const struct net *net, struct sk_buff *skb)
+{
+	if (!skb->l4_hash && !skb->sw_hash)
+		__skb_get_hash_net(net, skb);
+
+	return skb->hash;
+}
+
 static inline __u32 skb_get_hash(struct sk_buff *skb)
 {
 	if (!skb->l4_hash && !skb->sw_hash)
-		__skb_get_hash(skb);
+		__skb_get_hash_net(NULL, skb);
 
 	return skb->hash;
 }
diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 59fe46077b3c..702b4f0a70b6 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -1860,7 +1860,8 @@ u32 __skb_get_hash_symmetric(const struct sk_buff *skb)
 EXPORT_SYMBOL_GPL(__skb_get_hash_symmetric);
 
 /**
- * __skb_get_hash: calculate a flow hash
+ * __skb_get_hash_net: calculate a flow hash
+ * @net: associated network namespace, derived from @skb if NULL
  * @skb: sk_buff to calculate flow hash from
  *
  * This function calculates a flow hash based on src/dst addresses
@@ -1868,18 +1869,24 @@ EXPORT_SYMBOL_GPL(__skb_get_hash_symmetric);
  * on success, zero indicates no valid hash.  Also, sets l4_hash in skb
  * if hash is a canonical 4-tuple hash over transport ports.
  */
-void __skb_get_hash(struct sk_buff *skb)
+void __skb_get_hash_net(const struct net *net, struct sk_buff *skb)
 {
 	struct flow_keys keys;
 	u32 hash;
 
+	memset(&keys, 0, sizeof(keys));
+
+	__skb_flow_dissect(net, skb, &flow_keys_dissector,
+			   &keys, NULL, 0, 0, 0,
+			   FLOW_DISSECTOR_F_STOP_AT_FLOW_LABEL);
+
 	__flow_hash_secret_init();
 
-	hash = ___skb_get_hash(skb, &keys, &hashrnd);
+	hash = __flow_hash_from_keys(&keys, &hashrnd);
 
 	__skb_set_sw_hash(skb, hash, flow_keys_have_l4(&keys));
 }
-EXPORT_SYMBOL(__skb_get_hash);
+EXPORT_SYMBOL(__skb_get_hash_net);
 
 __u32 skb_get_hash_perturb(const struct sk_buff *skb,
 			   const siphash_key_t *perturb)
diff --git a/net/netfilter/nf_tables_trace.c b/net/netfilter/nf_tables_trace.c
index a83637e3f455..580c55268f65 100644
--- a/net/netfilter/nf_tables_trace.c
+++ b/net/netfilter/nf_tables_trace.c
@@ -317,7 +317,7 @@ void nft_trace_init(struct nft_traceinfo *info, const struct nft_pktinfo *pkt,
 	net_get_random_once(&trace_key, sizeof(trace_key));
 
 	info->skbid = (u32)siphash_3u32(hash32_ptr(skb),
-					skb_get_hash(skb),
+					skb_get_hash_net(nft_net(pkt), skb),
 					skb->skb_iif,
 					&trace_key);
 }
-- 
2.44.2

[PATCH net-next v2 2/2] net: add and use __skb_get_hash_symmetric_net

[Florian Westphal]

Similar to previous patch: apply same logic for
__skb_get_hash_symmetric and let callers pass the netns to the dissector
core.

Existing function is turned into a wrapper to avoid adjusting all
callers, nft_hash.c uses new function.

Reviewed-by: Eric Dumazet <edumazet@google.com>
Reviewed-by: Willem de Bruijn <willemb@google.com>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 No changes.

 include/linux/skbuff.h    | 8 +++++++-
 net/core/flow_dissector.c | 6 +++---
 net/netfilter/nft_hash.c  | 3 ++-
 3 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 6e78019f899a..813406a9bd6c 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -1498,8 +1498,14 @@ __skb_set_sw_hash(struct sk_buff *skb, __u32 hash, bool is_l4)
 	__skb_set_hash(skb, hash, true, is_l4);
 }
 
+u32 __skb_get_hash_symmetric_net(const struct net *net, const struct sk_buff *skb);
+
+static inline u32 __skb_get_hash_symmetric(const struct sk_buff *skb)
+{
+	return __skb_get_hash_symmetric_net(NULL, skb);
+}
+
 void __skb_get_hash_net(const struct net *net, struct sk_buff *skb);
-u32 __skb_get_hash_symmetric(const struct sk_buff *skb);
 u32 skb_get_poff(const struct sk_buff *skb);
 u32 __skb_get_poff(const struct sk_buff *skb, const void *data,
 		   const struct flow_keys_basic *keys, int hlen);
diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 702b4f0a70b6..e479790db0f7 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -1845,19 +1845,19 @@ EXPORT_SYMBOL(make_flow_keys_digest);
 
 static struct flow_dissector flow_keys_dissector_symmetric __read_mostly;
 
-u32 __skb_get_hash_symmetric(const struct sk_buff *skb)
+u32 __skb_get_hash_symmetric_net(const struct net *net, const struct sk_buff *skb)
 {
 	struct flow_keys keys;
 
 	__flow_hash_secret_init();
 
 	memset(&keys, 0, sizeof(keys));
-	__skb_flow_dissect(NULL, skb, &flow_keys_dissector_symmetric,
+	__skb_flow_dissect(net, skb, &flow_keys_dissector_symmetric,
 			   &keys, NULL, 0, 0, 0, 0);
 
 	return __flow_hash_from_keys(&keys, &hashrnd);
 }
-EXPORT_SYMBOL_GPL(__skb_get_hash_symmetric);
+EXPORT_SYMBOL_GPL(__skb_get_hash_symmetric_net);
 
 /**
  * __skb_get_hash_net: calculate a flow hash
diff --git a/net/netfilter/nft_hash.c b/net/netfilter/nft_hash.c
index 92d47e469204..868d68302d22 100644
--- a/net/netfilter/nft_hash.c
+++ b/net/netfilter/nft_hash.c
@@ -51,7 +51,8 @@ static void nft_symhash_eval(const struct nft_expr *expr,
 	struct sk_buff *skb = pkt->skb;
 	u32 h;
 
-	h = reciprocal_scale(__skb_get_hash_symmetric(skb), priv->modulus);
+	h = reciprocal_scale(__skb_get_hash_symmetric_net(nft_net(pkt), skb),
+			     priv->modulus);
 
 	regs->data[priv->dreg] = h + priv->offset;
 }
-- 
2.44.2

Re: [PATCH net-next v2 0/2] net: flow dissector: allow explicit passing of netns

[Eric Dumazet]

On Sun, Jun 9, 2024 at 12:20 AM Florian Westphal <fw@strlen.de> wrote:
>
> Change since last version:
>  fix kdoc comment warning reported by kbuild robot, no other changes,
>  thus retaining RvB tags from Eric and Willem.
>  v1: https://lore.kernel.org/netdev/20240607083205.3000-1-fw@strlen.de/

Thanks Florian
Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH net-next v2 0/2] net: flow dissector: allow explicit passing of netns

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Sun,  9 Jun 2024 00:10:38 +0200 you wrote:
> Change since last version:
>  fix kdoc comment warning reported by kbuild robot, no other changes,
>  thus retaining RvB tags from Eric and Willem.
>  v1: https://lore.kernel.org/netdev/20240607083205.3000-1-fw@strlen.de/
> 
> Years ago flow dissector gained ability to delegate flow dissection
> to a bpf program, scoped per netns.
> 
> [...]

Here is the summary with links:
  - [net-next,v2,1/2] net: add and use skb_get_hash_net
    https://git.kernel.org/netdev/net-next/c/b975d3ee5962
  - [net-next,v2,2/2] net: add and use __skb_get_hash_symmetric_net
    https://git.kernel.org/netdev/net-next/c/d1dab4f71d37

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next v2 0/2] net: flow dissector: allow explicit passing of netns

[Pablo Neira Ayuso]

Hi,

This series got applied to net-next.

But I can trigger this splat via nftables/tests/shell in net.git (6.10-rc).

As well as in -stable 6.1.x:

Jun 26 02:19:26 curiosity kernel: [ 1211.840595] ------------[ cut here ]------------
Jun 26 02:19:26 curiosity kernel: [ 1211.840605] WARNING: CPU: 0 PID: 70274 at net/core/flow_dissector.c:1016 __skb_flow_dissect+0x107e/0x2860
[...]
Jun 26 02:19:26 curiosity kernel: [ 1211.841240] CPU: 0 PID: 70274 Comm: socat Not tainted 6.1.93+ #18

I think that turning this into DEBUG_NET_WARN_ON_ONCE as Willem
suggested provides a workaround for net.git until Florian's fixes in
net-next hit -stable.

Would you accept such a patch?

Thanks.

On Sun, Jun 09, 2024 at 12:10:38AM +0200, Florian Westphal wrote:
> Change since last version:
>  fix kdoc comment warning reported by kbuild robot, no other changes,
>  thus retaining RvB tags from Eric and Willem.
>  v1: https://lore.kernel.org/netdev/20240607083205.3000-1-fw@strlen.de/
> 
> Years ago flow dissector gained ability to delegate flow dissection
> to a bpf program, scoped per netns.
> 
> The netns is derived from skb->dev, and if that is not available, from
> skb->sk.  If neither is set, we hit a (benign) WARN_ON_ONCE().
> 
> This WARN_ON_ONCE can be triggered from netfilter.
> Known skb origins are nf_send_reset and ipv4 stack generated IGMP
> messages.
> 
> Lets allow callers to pass the current netns explicitly and make
> nf_tables use those instead.
> 
> This targets net-next instead of net because the WARN is benign and this
> is not a regression.
> 
> Florian Westphal (2):
>   net: add and use skb_get_hash_net
>   net: add and use __skb_get_hash_symmetric_net
> 
>  include/linux/skbuff.h          | 20 +++++++++++++++++---
>  net/core/flow_dissector.c       | 21 ++++++++++++++-------
>  net/netfilter/nf_tables_trace.c |  2 +-
>  net/netfilter/nft_hash.c        |  3 ++-
>  4 files changed, 34 insertions(+), 12 deletions(-)
> 
> -- 
> 2.44.2
> 

Re: [PATCH net-next v2 0/2] net: flow dissector: allow explicit passing of netns

[Paolo Abeni]

On Thu, 2024-06-27 at 01:49 +0200, Pablo Neira Ayuso wrote:
> This series got applied to net-next.
> 
> But I can trigger this splat via nftables/tests/shell in net.git (6.10-rc).
> 
> As well as in -stable 6.1.x:
> 
> Jun 26 02:19:26 curiosity kernel: [ 1211.840595] ------------[ cut here ]------------
> Jun 26 02:19:26 curiosity kernel: [ 1211.840605] WARNING: CPU: 0 PID: 70274 at net/core/flow_dissector.c:1016 __skb_flow_dissect+0x107e/0x2860
> [...]
> Jun 26 02:19:26 curiosity kernel: [ 1211.841240] CPU: 0 PID: 70274 Comm: socat Not tainted 6.1.93+ #18
> 
> I think that turning this into DEBUG_NET_WARN_ON_ONCE as Willem
> suggested provides a workaround for net.git until Florian's fixes in
> net-next hit -stable.
> 
> Would you accept such a patch?

FWISW I think it makes sense.

Thanks,

Paolo