netfilter-devel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next 0/2] net: flow dissector: allow explicit passing of netns
@ 2024-06-07  8:31 Florian Westphal
  2024-06-07  8:31 ` [PATCH net-next 1/2] net: add and use skb_get_hash_net Florian Westphal
  2024-06-07  8:32 ` [PATCH net-next 2/2] net: add and use __skb_get_hash_symmetric_net Florian Westphal
  0 siblings, 2 replies; 9+ messages in thread
From: Florian Westphal @ 2024-06-07  8:31 UTC (permalink / raw)
  To: netdev
  Cc: Paolo Abeni, David S. Miller, Eric Dumazet, Jakub Kicinski,
	netfilter-devel, pablo, willemb

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       | 20 +++++++++++++-------
 net/netfilter/nf_tables_trace.c |  2 +-
 net/netfilter/nft_hash.c        |  3 ++-
 4 files changed, 33 insertions(+), 12 deletions(-)

-- 
2.44.2


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH net-next 1/2] net: add and use skb_get_hash_net
  2024-06-07  8:31 [PATCH net-next 0/2] net: flow dissector: allow explicit passing of netns Florian Westphal
@ 2024-06-07  8:31 ` Florian Westphal
  2024-06-07  9:25   ` Eric Dumazet
  2024-06-07 12:33   ` kernel test robot
  2024-06-07  8:32 ` [PATCH net-next 2/2] net: add and use __skb_get_hash_symmetric_net Florian Westphal
  1 sibling, 2 replies; 9+ messages in thread
From: Florian Westphal @ 2024-06-07  8:31 UTC (permalink / raw)
  To: netdev
  Cc: Paolo Abeni, David S. Miller, Eric Dumazet, Jakub Kicinski,
	netfilter-devel, pablo, willemb, Christoph Paasch

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			# calls skb_get_hash
    tcp dport 42 reject with tcp reset  # emits skb with NULL skb dev/sk
   }
   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
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/linux/skbuff.h          | 12 ++++++++++--
 net/core/flow_dissector.c       | 14 ++++++++++----
 net/netfilter/nf_tables_trace.c |  2 +-
 3 files changed, 21 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..32454181be60 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -1860,7 +1860,7 @@ 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
  * @skb: sk_buff to calculate flow hash from
  *
  * This function calculates a flow hash based on src/dst addresses
@@ -1868,18 +1868,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


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH net-next 2/2] net: add and use __skb_get_hash_symmetric_net
  2024-06-07  8:31 [PATCH net-next 0/2] net: flow dissector: allow explicit passing of netns Florian Westphal
  2024-06-07  8:31 ` [PATCH net-next 1/2] net: add and use skb_get_hash_net Florian Westphal
@ 2024-06-07  8:32 ` Florian Westphal
  2024-06-07  9:26   ` Eric Dumazet
  1 sibling, 1 reply; 9+ messages in thread
From: Florian Westphal @ 2024-06-07  8:32 UTC (permalink / raw)
  To: netdev
  Cc: Paolo Abeni, David S. Miller, Eric Dumazet, Jakub Kicinski,
	netfilter-devel, pablo, willemb

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.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 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 32454181be60..f6a97aec38da 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


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH net-next 1/2] net: add and use skb_get_hash_net
  2024-06-07  8:31 ` [PATCH net-next 1/2] net: add and use skb_get_hash_net Florian Westphal
@ 2024-06-07  9:25   ` Eric Dumazet
  2024-06-07 14:13     ` Willem de Bruijn
  2024-06-07 12:33   ` kernel test robot
  1 sibling, 1 reply; 9+ messages in thread
From: Eric Dumazet @ 2024-06-07  9:25 UTC (permalink / raw)
  To: Florian Westphal
  Cc: netdev, Paolo Abeni, David S. Miller, Jakub Kicinski,
	netfilter-devel, pablo, willemb, Christoph Paasch

On Fri, Jun 7, 2024 at 10:36 AM Florian Westphal <fw@strlen.de> wrote:
>
> 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                  # calls skb_get_hash
>     tcp dport 42 reject with tcp reset  # emits skb with NULL skb dev/sk
>    }
>    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
> Signed-off-by: Florian Westphal <fw@strlen.de>

Nice, I had an internal syzbot report about the same issue.

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net-next 2/2] net: add and use __skb_get_hash_symmetric_net
  2024-06-07  8:32 ` [PATCH net-next 2/2] net: add and use __skb_get_hash_symmetric_net Florian Westphal
@ 2024-06-07  9:26   ` Eric Dumazet
  2024-06-07 14:14     ` Willem de Bruijn
  0 siblings, 1 reply; 9+ messages in thread
From: Eric Dumazet @ 2024-06-07  9:26 UTC (permalink / raw)
  To: Florian Westphal
  Cc: netdev, Paolo Abeni, David S. Miller, Jakub Kicinski,
	netfilter-devel, pablo, willemb

On Fri, Jun 7, 2024 at 10:36 AM Florian Westphal <fw@strlen.de> wrote:
>
> 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.
>
> Signed-off-by: Florian Westphal <fw@strlen.de>

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net-next 1/2] net: add and use skb_get_hash_net
  2024-06-07  8:31 ` [PATCH net-next 1/2] net: add and use skb_get_hash_net Florian Westphal
  2024-06-07  9:25   ` Eric Dumazet
@ 2024-06-07 12:33   ` kernel test robot
  1 sibling, 0 replies; 9+ messages in thread
From: kernel test robot @ 2024-06-07 12:33 UTC (permalink / raw)
  To: Florian Westphal, netdev
  Cc: oe-kbuild-all, Paolo Abeni, Eric Dumazet, Jakub Kicinski,
	netfilter-devel, pablo, willemb, Christoph Paasch

Hi Florian,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Florian-Westphal/net-add-and-use-skb_get_hash_net/20240607-163738
base:   net-next/main
patch link:    https://lore.kernel.org/r/20240607083205.3000-2-fw%40strlen.de
patch subject: [PATCH net-next 1/2] net: add and use skb_get_hash_net
config: openrisc-defconfig (https://download.01.org/0day-ci/archive/20240607/202406072022.OkRGOAuS-lkp@intel.com/config)
compiler: or1k-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240607/202406072022.OkRGOAuS-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202406072022.OkRGOAuS-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> net/core/flow_dissector.c:1872: warning: Function parameter or struct member 'net' not described in '__skb_get_hash_net'


vim +1872 net/core/flow_dissector.c

eb70db8756717b David S. Miller  2016-07-01  1861  
d4fd32757176d1 Jiri Pirko       2015-05-12  1862  /**
11b45a5b56dab6 Florian Westphal 2024-06-07  1863   * __skb_get_hash_net: calculate a flow hash
d4fd32757176d1 Jiri Pirko       2015-05-12  1864   * @skb: sk_buff to calculate flow hash from
d4fd32757176d1 Jiri Pirko       2015-05-12  1865   *
d4fd32757176d1 Jiri Pirko       2015-05-12  1866   * This function calculates a flow hash based on src/dst addresses
61b905da33ae25 Tom Herbert      2014-03-24  1867   * and src/dst port numbers.  Sets hash in skb to non-zero hash value
61b905da33ae25 Tom Herbert      2014-03-24  1868   * on success, zero indicates no valid hash.  Also, sets l4_hash in skb
441d9d327f1e77 Cong Wang        2013-01-21  1869   * if hash is a canonical 4-tuple hash over transport ports.
441d9d327f1e77 Cong Wang        2013-01-21  1870   */
11b45a5b56dab6 Florian Westphal 2024-06-07  1871  void __skb_get_hash_net(const struct net *net, struct sk_buff *skb)
441d9d327f1e77 Cong Wang        2013-01-21 @1872  {
441d9d327f1e77 Cong Wang        2013-01-21  1873  	struct flow_keys keys;
635c223cfa05af Gao Feng         2016-08-31  1874  	u32 hash;
441d9d327f1e77 Cong Wang        2013-01-21  1875  
11b45a5b56dab6 Florian Westphal 2024-06-07  1876  	memset(&keys, 0, sizeof(keys));
11b45a5b56dab6 Florian Westphal 2024-06-07  1877  
11b45a5b56dab6 Florian Westphal 2024-06-07  1878  	__skb_flow_dissect(net, skb, &flow_keys_dissector,
11b45a5b56dab6 Florian Westphal 2024-06-07  1879  			   &keys, NULL, 0, 0, 0,
11b45a5b56dab6 Florian Westphal 2024-06-07  1880  			   FLOW_DISSECTOR_F_STOP_AT_FLOW_LABEL);
11b45a5b56dab6 Florian Westphal 2024-06-07  1881  
50fb799289501c Tom Herbert      2015-05-01  1882  	__flow_hash_secret_init();
50fb799289501c Tom Herbert      2015-05-01  1883  
11b45a5b56dab6 Florian Westphal 2024-06-07  1884  	hash = __flow_hash_from_keys(&keys, &hashrnd);
635c223cfa05af Gao Feng         2016-08-31  1885  
635c223cfa05af Gao Feng         2016-08-31  1886  	__skb_set_sw_hash(skb, hash, flow_keys_have_l4(&keys));
441d9d327f1e77 Cong Wang        2013-01-21  1887  }
11b45a5b56dab6 Florian Westphal 2024-06-07  1888  EXPORT_SYMBOL(__skb_get_hash_net);
441d9d327f1e77 Cong Wang        2013-01-21  1889  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net-next 1/2] net: add and use skb_get_hash_net
  2024-06-07  9:25   ` Eric Dumazet
@ 2024-06-07 14:13     ` Willem de Bruijn
  2024-06-08 22:17       ` Florian Westphal
  0 siblings, 1 reply; 9+ messages in thread
From: Willem de Bruijn @ 2024-06-07 14:13 UTC (permalink / raw)
  To: Eric Dumazet, Florian Westphal
  Cc: netdev, Paolo Abeni, David S. Miller, Jakub Kicinski,
	netfilter-devel, pablo, willemb, Christoph Paasch

Eric Dumazet wrote:
> On Fri, Jun 7, 2024 at 10:36 AM Florian Westphal <fw@strlen.de> wrote:
> >
> > 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                  # calls skb_get_hash
> >     tcp dport 42 reject with tcp reset  # emits skb with NULL skb dev/sk
> >    }
> >    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.

Does this mean we have more non-nf callsites to convert?

> >
> > 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
> > Signed-off-by: Florian Westphal <fw@strlen.de>
> 
> Nice, I had an internal syzbot report about the same issue.
> 
> Reviewed-by: Eric Dumazet <edumazet@google.com>

Subject to the documentation warning from the bot

Reviewed-by: Willem de Bruijn <willemb@google.com>

Thanks for fixing this, Florian.


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net-next 2/2] net: add and use __skb_get_hash_symmetric_net
  2024-06-07  9:26   ` Eric Dumazet
@ 2024-06-07 14:14     ` Willem de Bruijn
  0 siblings, 0 replies; 9+ messages in thread
From: Willem de Bruijn @ 2024-06-07 14:14 UTC (permalink / raw)
  To: Eric Dumazet, Florian Westphal
  Cc: netdev, Paolo Abeni, David S. Miller, Jakub Kicinski,
	netfilter-devel, pablo, willemb

Eric Dumazet wrote:
> On Fri, Jun 7, 2024 at 10:36 AM Florian Westphal <fw@strlen.de> wrote:
> >
> > 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.
> >
> > Signed-off-by: Florian Westphal <fw@strlen.de>
> 
> Reviewed-by: Eric Dumazet <edumazet@google.com>

Reviewed-by: Willem de Bruijn <willemb@google.com>


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net-next 1/2] net: add and use skb_get_hash_net
  2024-06-07 14:13     ` Willem de Bruijn
@ 2024-06-08 22:17       ` Florian Westphal
  0 siblings, 0 replies; 9+ messages in thread
From: Florian Westphal @ 2024-06-08 22:17 UTC (permalink / raw)
  To: Willem de Bruijn
  Cc: Eric Dumazet, Florian Westphal, netdev, Paolo Abeni,
	David S. Miller, Jakub Kicinski, netfilter-devel, pablo, willemb,
	Christoph Paasch

Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote:
> > > syzkaller did something like this:
> > > table inet filter {
> > >   chain input {
> > >     type filter hook input priority filter; policy accept;
> > >     meta nftrace set 1                  # calls skb_get_hash
> > >     tcp dport 42 reject with tcp reset  # emits skb with NULL skb dev/sk
> > >    }
> > >    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.
> 
> Does this mean we have more non-nf callsites to convert?

There might be non-nf call sites that need skb_get_hash_net(),
but I don't know of any.

The above comment was meant to say that I tried to patch this
outside of flow dissector by setting skb->dev properly in nf_reject,
but that still triggers a slightly different WARN trace, this time
due to igmp_send_report also sending skb without dev+sk pointers.

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2024-06-08 22:53 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-06-07  8:31 [PATCH net-next 0/2] net: flow dissector: allow explicit passing of netns Florian Westphal
2024-06-07  8:31 ` [PATCH net-next 1/2] net: add and use skb_get_hash_net Florian Westphal
2024-06-07  9:25   ` Eric Dumazet
2024-06-07 14:13     ` Willem de Bruijn
2024-06-08 22:17       ` Florian Westphal
2024-06-07 12:33   ` kernel test robot
2024-06-07  8:32 ` [PATCH net-next 2/2] net: add and use __skb_get_hash_symmetric_net Florian Westphal
2024-06-07  9:26   ` Eric Dumazet
2024-06-07 14:14     ` Willem de Bruijn

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).