* [PATCH net 1/3] netfilter: ctnetlink: revert to dumping mark regardless of event type
2023-03-07 10:04 [PATCH net 0/3] Netfilter fixes for net Pablo Neira Ayuso
@ 2023-03-07 10:04 ` Pablo Neira Ayuso
2023-03-07 14:30 ` patchwork-bot+netdevbpf
2023-03-07 10:04 ` [PATCH net 2/3] netfilter: tproxy: fix deadlock due to missing BH disable Pablo Neira Ayuso
` (2 subsequent siblings)
3 siblings, 1 reply; 8+ messages in thread
From: Pablo Neira Ayuso @ 2023-03-07 10:04 UTC (permalink / raw)
To: netfilter-devel; +Cc: davem, netdev, kuba, pabeni, edumazet
From: Ivan Delalande <colona@arista.com>
It seems that change was unintentional, we have userspace code that
needs the mark while listening for events like REPLY, DESTROY, etc.
Also include 0-marks in requested dumps, as they were before that fix.
Fixes: 1feeae071507 ("netfilter: ctnetlink: fix compilation warning after data race fixes in ct mark")
Signed-off-by: Ivan Delalande <colona@arista.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
net/netfilter/nf_conntrack_netlink.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index c11dff91d52d..bfc3aaa2c872 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -328,11 +328,12 @@ ctnetlink_dump_timestamp(struct sk_buff *skb, const struct nf_conn *ct)
}
#ifdef CONFIG_NF_CONNTRACK_MARK
-static int ctnetlink_dump_mark(struct sk_buff *skb, const struct nf_conn *ct)
+static int ctnetlink_dump_mark(struct sk_buff *skb, const struct nf_conn *ct,
+ bool dump)
{
u32 mark = READ_ONCE(ct->mark);
- if (!mark)
+ if (!mark && !dump)
return 0;
if (nla_put_be32(skb, CTA_MARK, htonl(mark)))
@@ -343,7 +344,7 @@ static int ctnetlink_dump_mark(struct sk_buff *skb, const struct nf_conn *ct)
return -1;
}
#else
-#define ctnetlink_dump_mark(a, b) (0)
+#define ctnetlink_dump_mark(a, b, c) (0)
#endif
#ifdef CONFIG_NF_CONNTRACK_SECMARK
@@ -548,7 +549,7 @@ static int ctnetlink_dump_extinfo(struct sk_buff *skb,
static int ctnetlink_dump_info(struct sk_buff *skb, struct nf_conn *ct)
{
if (ctnetlink_dump_status(skb, ct) < 0 ||
- ctnetlink_dump_mark(skb, ct) < 0 ||
+ ctnetlink_dump_mark(skb, ct, true) < 0 ||
ctnetlink_dump_secctx(skb, ct) < 0 ||
ctnetlink_dump_id(skb, ct) < 0 ||
ctnetlink_dump_use(skb, ct) < 0 ||
@@ -831,8 +832,7 @@ ctnetlink_conntrack_event(unsigned int events, const struct nf_ct_event *item)
}
#ifdef CONFIG_NF_CONNTRACK_MARK
- if (events & (1 << IPCT_MARK) &&
- ctnetlink_dump_mark(skb, ct) < 0)
+ if (ctnetlink_dump_mark(skb, ct, events & (1 << IPCT_MARK)))
goto nla_put_failure;
#endif
nlmsg_end(skb, nlh);
@@ -2735,7 +2735,7 @@ static int __ctnetlink_glue_build(struct sk_buff *skb, struct nf_conn *ct)
goto nla_put_failure;
#ifdef CONFIG_NF_CONNTRACK_MARK
- if (ctnetlink_dump_mark(skb, ct) < 0)
+ if (ctnetlink_dump_mark(skb, ct, true) < 0)
goto nla_put_failure;
#endif
if (ctnetlink_dump_labels(skb, ct) < 0)
--
2.30.2
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH net 1/3] netfilter: ctnetlink: revert to dumping mark regardless of event type
2023-03-07 10:04 ` [PATCH net 1/3] netfilter: ctnetlink: revert to dumping mark regardless of event type Pablo Neira Ayuso
@ 2023-03-07 14:30 ` patchwork-bot+netdevbpf
0 siblings, 0 replies; 8+ messages in thread
From: patchwork-bot+netdevbpf @ 2023-03-07 14:30 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: netfilter-devel, davem, netdev, kuba, pabeni, edumazet
Hello:
This series was applied to netdev/net.git (main)
by Pablo Neira Ayuso <pablo@netfilter.org>:
On Tue, 7 Mar 2023 11:04:22 +0100 you wrote:
> From: Ivan Delalande <colona@arista.com>
>
> It seems that change was unintentional, we have userspace code that
> needs the mark while listening for events like REPLY, DESTROY, etc.
> Also include 0-marks in requested dumps, as they were before that fix.
>
> Fixes: 1feeae071507 ("netfilter: ctnetlink: fix compilation warning after data race fixes in ct mark")
> Signed-off-by: Ivan Delalande <colona@arista.com>
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
>
> [...]
Here is the summary with links:
- [net,1/3] netfilter: ctnetlink: revert to dumping mark regardless of event type
https://git.kernel.org/netdev/net/c/9f7dd42f0db1
- [net,2/3] netfilter: tproxy: fix deadlock due to missing BH disable
https://git.kernel.org/netdev/net/c/4a02426787bf
- [net,3/3] netfilter: conntrack: adopt safer max chain length
https://git.kernel.org/netdev/net/c/c77737b736ce
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH net 2/3] netfilter: tproxy: fix deadlock due to missing BH disable
2023-03-07 10:04 [PATCH net 0/3] Netfilter fixes for net Pablo Neira Ayuso
2023-03-07 10:04 ` [PATCH net 1/3] netfilter: ctnetlink: revert to dumping mark regardless of event type Pablo Neira Ayuso
@ 2023-03-07 10:04 ` Pablo Neira Ayuso
2023-03-07 10:04 ` [PATCH net 3/3] netfilter: conntrack: adopt safer max chain length Pablo Neira Ayuso
2023-03-07 12:57 ` [PATCH net 0/3] Netfilter fixes for net Paolo Abeni
3 siblings, 0 replies; 8+ messages in thread
From: Pablo Neira Ayuso @ 2023-03-07 10:04 UTC (permalink / raw)
To: netfilter-devel; +Cc: davem, netdev, kuba, pabeni, edumazet
From: Florian Westphal <fw@strlen.de>
The xtables packet traverser performs an unconditional local_bh_disable(),
but the nf_tables evaluation loop does not.
Functions that are called from either xtables or nftables must assume
that they can be called in process context.
inet_twsk_deschedule_put() assumes that no softirq interrupt can occur.
If tproxy is used from nf_tables its possible that we'll deadlock
trying to aquire a lock already held in process context.
Add a small helper that takes care of this and use it.
Link: https://lore.kernel.org/netfilter-devel/401bd6ed-314a-a196-1cdc-e13c720cc8f2@balasys.hu/
Fixes: 4ed8eb6570a4 ("netfilter: nf_tables: Add native tproxy support")
Reported-and-tested-by: Major Dávid <major.david@balasys.hu>
Signed-off-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
include/net/netfilter/nf_tproxy.h | 7 +++++++
net/ipv4/netfilter/nf_tproxy_ipv4.c | 2 +-
net/ipv6/netfilter/nf_tproxy_ipv6.c | 2 +-
3 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/include/net/netfilter/nf_tproxy.h b/include/net/netfilter/nf_tproxy.h
index 82d0e41b76f2..faa108b1ba67 100644
--- a/include/net/netfilter/nf_tproxy.h
+++ b/include/net/netfilter/nf_tproxy.h
@@ -17,6 +17,13 @@ static inline bool nf_tproxy_sk_is_transparent(struct sock *sk)
return false;
}
+static inline void nf_tproxy_twsk_deschedule_put(struct inet_timewait_sock *tw)
+{
+ local_bh_disable();
+ inet_twsk_deschedule_put(tw);
+ local_bh_enable();
+}
+
/* assign a socket to the skb -- consumes sk */
static inline void nf_tproxy_assign_sock(struct sk_buff *skb, struct sock *sk)
{
diff --git a/net/ipv4/netfilter/nf_tproxy_ipv4.c b/net/ipv4/netfilter/nf_tproxy_ipv4.c
index b22b2c745c76..69e331799604 100644
--- a/net/ipv4/netfilter/nf_tproxy_ipv4.c
+++ b/net/ipv4/netfilter/nf_tproxy_ipv4.c
@@ -38,7 +38,7 @@ nf_tproxy_handle_time_wait4(struct net *net, struct sk_buff *skb,
hp->source, lport ? lport : hp->dest,
skb->dev, NF_TPROXY_LOOKUP_LISTENER);
if (sk2) {
- inet_twsk_deschedule_put(inet_twsk(sk));
+ nf_tproxy_twsk_deschedule_put(inet_twsk(sk));
sk = sk2;
}
}
diff --git a/net/ipv6/netfilter/nf_tproxy_ipv6.c b/net/ipv6/netfilter/nf_tproxy_ipv6.c
index 929502e51203..52f828bb5a83 100644
--- a/net/ipv6/netfilter/nf_tproxy_ipv6.c
+++ b/net/ipv6/netfilter/nf_tproxy_ipv6.c
@@ -63,7 +63,7 @@ nf_tproxy_handle_time_wait6(struct sk_buff *skb, int tproto, int thoff,
lport ? lport : hp->dest,
skb->dev, NF_TPROXY_LOOKUP_LISTENER);
if (sk2) {
- inet_twsk_deschedule_put(inet_twsk(sk));
+ nf_tproxy_twsk_deschedule_put(inet_twsk(sk));
sk = sk2;
}
}
--
2.30.2
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH net 3/3] netfilter: conntrack: adopt safer max chain length
2023-03-07 10:04 [PATCH net 0/3] Netfilter fixes for net Pablo Neira Ayuso
2023-03-07 10:04 ` [PATCH net 1/3] netfilter: ctnetlink: revert to dumping mark regardless of event type Pablo Neira Ayuso
2023-03-07 10:04 ` [PATCH net 2/3] netfilter: tproxy: fix deadlock due to missing BH disable Pablo Neira Ayuso
@ 2023-03-07 10:04 ` Pablo Neira Ayuso
2023-03-07 12:57 ` [PATCH net 0/3] Netfilter fixes for net Paolo Abeni
3 siblings, 0 replies; 8+ messages in thread
From: Pablo Neira Ayuso @ 2023-03-07 10:04 UTC (permalink / raw)
To: netfilter-devel; +Cc: davem, netdev, kuba, pabeni, edumazet
From: Eric Dumazet <edumazet@google.com>
Customers using GKE 1.25 and 1.26 are facing conntrack issues
root caused to commit c9c3b6811f74 ("netfilter: conntrack: make
max chain length random").
Even if we assume Uniform Hashing, a bucket often reachs 8 chained
items while the load factor of the hash table is smaller than 0.5
With a limit of 16, we reach load factors of 3.
With a limit of 32, we reach load factors of 11.
With a limit of 40, we reach load factors of 15.
With a limit of 50, we reach load factors of 24.
This patch changes MIN_CHAINLEN to 50, to minimize risks.
Ideally, we could in the future add a cushion based on expected
load factor (2 * nf_conntrack_max / nf_conntrack_buckets),
because some setups might expect unusual values.
Fixes: c9c3b6811f74 ("netfilter: conntrack: make max chain length random")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
net/netfilter/nf_conntrack_core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 7250082e7de5..c6a6a6099b4e 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -96,8 +96,8 @@ static DEFINE_MUTEX(nf_conntrack_mutex);
#define GC_SCAN_MAX_DURATION msecs_to_jiffies(10)
#define GC_SCAN_EXPIRED_MAX (64000u / HZ)
-#define MIN_CHAINLEN 8u
-#define MAX_CHAINLEN (32u - MIN_CHAINLEN)
+#define MIN_CHAINLEN 50u
+#define MAX_CHAINLEN (80u - MIN_CHAINLEN)
static struct conntrack_gc_work conntrack_gc_work;
--
2.30.2
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH net 0/3] Netfilter fixes for net
2023-03-07 10:04 [PATCH net 0/3] Netfilter fixes for net Pablo Neira Ayuso
` (2 preceding siblings ...)
2023-03-07 10:04 ` [PATCH net 3/3] netfilter: conntrack: adopt safer max chain length Pablo Neira Ayuso
@ 2023-03-07 12:57 ` Paolo Abeni
2023-03-07 17:26 ` Jakub Kicinski
3 siblings, 1 reply; 8+ messages in thread
From: Paolo Abeni @ 2023-03-07 12:57 UTC (permalink / raw)
To: Pablo Neira Ayuso, netfilter-devel; +Cc: davem, netdev, kuba, edumazet
On Tue, 2023-03-07 at 11:04 +0100, Pablo Neira Ayuso wrote:
> Hi,
>
> The following patchset contains Netfilter fixes for net:
>
> 1) Restore ctnetlink zero mark in events and dump, from Ivan Delalande.
>
> 2) Fix deadlock due to missing disabled bh in tproxy, from Florian Westphal.
>
> 3) Safer maximum chain load in conntrack, from Eric Dumazet.
>
> Please, pull these changes from:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/netfilter/nf.git
>
> Thanks.
>
> ----------------------------------------------------------------
>
> The following changes since commit 528125268588a18a2f257002af051b62b14bb282:
>
> Merge branch 'nfp-ipsec-csum' (2023-03-03 08:28:44 +0000)
>
> are available in the Git repository at:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/netfilter/nf.git HEAD
It's not clear to me the root cause, but pulling from the above ref.
yields nothing. I have to replace 'HEAD' with main to get the expected
patches.
Cheers,
Paolo
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net 0/3] Netfilter fixes for net
2023-03-07 12:57 ` [PATCH net 0/3] Netfilter fixes for net Paolo Abeni
@ 2023-03-07 17:26 ` Jakub Kicinski
2023-03-08 9:34 ` Pablo Neira Ayuso
0 siblings, 1 reply; 8+ messages in thread
From: Jakub Kicinski @ 2023-03-07 17:26 UTC (permalink / raw)
To: Paolo Abeni, Pablo Neira Ayuso; +Cc: netfilter-devel, davem, netdev, edumazet
On Tue, 07 Mar 2023 13:57:07 +0100 Paolo Abeni wrote:
> On Tue, 2023-03-07 at 11:04 +0100, Pablo Neira Ayuso wrote:
> > The following changes since commit 528125268588a18a2f257002af051b62b14bb282:
> >
> > Merge branch 'nfp-ipsec-csum' (2023-03-03 08:28:44 +0000)
> >
> > are available in the Git repository at:
> >
> > git://git.kernel.org/pub/scm/linux/kernel/git/netfilter/nf.git HEAD
>
> It's not clear to me the root cause, but pulling from the above ref.
> yields nothing. I have to replace 'HEAD' with main to get the expected
> patches.
Possibly netfilter folks did not update HEAD to point to main?
ssh git@gitolite.kernel.org symbolic-ref \
pub/scm/linux/kernel/git/netfilter/nf \
HEAD refs/heads/main
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net 0/3] Netfilter fixes for net
2023-03-07 17:26 ` Jakub Kicinski
@ 2023-03-08 9:34 ` Pablo Neira Ayuso
0 siblings, 0 replies; 8+ messages in thread
From: Pablo Neira Ayuso @ 2023-03-08 9:34 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: Paolo Abeni, netfilter-devel, davem, netdev, edumazet
On Tue, Mar 07, 2023 at 09:26:04AM -0800, Jakub Kicinski wrote:
> On Tue, 07 Mar 2023 13:57:07 +0100 Paolo Abeni wrote:
> > On Tue, 2023-03-07 at 11:04 +0100, Pablo Neira Ayuso wrote:
> > > The following changes since commit 528125268588a18a2f257002af051b62b14bb282:
> > >
> > > Merge branch 'nfp-ipsec-csum' (2023-03-03 08:28:44 +0000)
> > >
> > > are available in the Git repository at:
> > >
> > > git://git.kernel.org/pub/scm/linux/kernel/git/netfilter/nf.git HEAD
> >
> > It's not clear to me the root cause, but pulling from the above ref.
> > yields nothing. I have to replace 'HEAD' with main to get the expected
> > patches.
>
> Possibly netfilter folks did not update HEAD to point to main?
>
> ssh git@gitolite.kernel.org symbolic-ref \
> pub/scm/linux/kernel/git/netfilter/nf \
> HEAD refs/heads/main
Fixed, thanks.
I will also review my pull request scripts to check if someone got
unadjusted after the switch to the main branch.
^ permalink raw reply [flat|nested] 8+ messages in thread