[PATCH 0/5] netfilter: NAT sequence number adjustment fixes/improvements

[PATCH 0/5] netfilter: NAT sequence number adjustment fixes/improvements

[Patrick McHardy]

the following patches from my SYNPROXY tree contain some fixes and improvements
for netfilter sequence number adjustment handling and two unrelated minor
patches:

- Remove net_ratelimit() for LOG_INVALID: ratelimiting explicitly enabled
  packet logging is inconsistent with other netfilter logging behaviour and
  makes debugging harder

- Constify nf_ct_attach() source skb argument

- Fix locking in nf_nat_seq_adjust(): we need to take nf_nat_seqofs lock
  to protect against concurrent changes to the sequence adjustment data

- Increase sequence number offset size to 32 bits. When many adjustments
  happen in a single connection, the offsets can overflow and break the
  connection.

- Use per-conntrack locks to protect sequence number adjustment data.
  Should increase scalability and additionally saves one lock/unlock operation
  per TCP packet.

Please apply. thanks.

[PATCH 1/5] netfilter: nf_conntrack: remove net_ratelimit() for LOG_INVALID()

[Patrick McHardy]

Logging of invalid packets has to be explicitly enabled. Rate-limiting these
messages is inconsistent with other netfilter logging features and makes
debugging harder.

Signed-off-by: Patrick McHardy <kaber@trash.net>
---
 include/net/netfilter/nf_conntrack_l4proto.h | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/include/net/netfilter/nf_conntrack_l4proto.h b/include/net/netfilter/nf_conntrack_l4proto.h
index 914d8d9..b411d7b 100644
--- a/include/net/netfilter/nf_conntrack_l4proto.h
+++ b/include/net/netfilter/nf_conntrack_l4proto.h
@@ -148,17 +148,10 @@ extern int nf_ct_port_nlattr_tuple_size(void);
 extern const struct nla_policy nf_ct_port_nla_policy[];
 
 #ifdef CONFIG_SYSCTL
-#ifdef DEBUG_INVALID_PACKETS
 #define LOG_INVALID(net, proto)				\
 	((net)->ct.sysctl_log_invalid == (proto) ||	\
 	 (net)->ct.sysctl_log_invalid == IPPROTO_RAW)
 #else
-#define LOG_INVALID(net, proto)				\
-	(((net)->ct.sysctl_log_invalid == (proto) ||	\
-	  (net)->ct.sysctl_log_invalid == IPPROTO_RAW)	\
-	 && net_ratelimit())
-#endif
-#else
 static inline int LOG_INVALID(struct net *net, int proto) { return 0; }
 #endif /* CONFIG_SYSCTL */
 
-- 
1.8.1.4

[PATCH 2/5] netfilter: nf_conntrack: constify sk_buff argument to nf_ct_attach()

[Patrick McHardy]

Signed-off-by: Patrick McHardy <kaber@trash.net>
---
 include/linux/netfilter.h         | 4 ++--
 net/netfilter/core.c              | 7 ++++---
 net/netfilter/nf_conntrack_core.c | 2 +-
 3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/include/linux/netfilter.h b/include/linux/netfilter.h
index de70f7b..f4bbf2c 100644
--- a/include/linux/netfilter.h
+++ b/include/linux/netfilter.h
@@ -314,8 +314,8 @@ nf_nat_decode_session(struct sk_buff *skb, struct flowi *fl, u_int8_t family)
 #endif /*CONFIG_NETFILTER*/
 
 #if defined(CONFIG_NF_CONNTRACK) || defined(CONFIG_NF_CONNTRACK_MODULE)
-extern void (*ip_ct_attach)(struct sk_buff *, struct sk_buff *) __rcu;
-extern void nf_ct_attach(struct sk_buff *, struct sk_buff *);
+extern void (*ip_ct_attach)(struct sk_buff *, const struct sk_buff *) __rcu;
+extern void nf_ct_attach(struct sk_buff *, const struct sk_buff *);
 extern void (*nf_ct_destroy)(struct nf_conntrack *) __rcu;
 
 struct nf_conn;
diff --git a/net/netfilter/core.c b/net/netfilter/core.c
index 2217363..593b16e 100644
--- a/net/netfilter/core.c
+++ b/net/netfilter/core.c
@@ -234,12 +234,13 @@ EXPORT_SYMBOL(skb_make_writable);
 /* This does not belong here, but locally generated errors need it if connection
    tracking in use: without this, connection may not be in hash table, and hence
    manufactured ICMP or RST packets will not be associated with it. */
-void (*ip_ct_attach)(struct sk_buff *, struct sk_buff *) __rcu __read_mostly;
+void (*ip_ct_attach)(struct sk_buff *, const struct sk_buff *)
+		__rcu __read_mostly;
 EXPORT_SYMBOL(ip_ct_attach);
 
-void nf_ct_attach(struct sk_buff *new, struct sk_buff *skb)
+void nf_ct_attach(struct sk_buff *new, const struct sk_buff *skb)
 {
-	void (*attach)(struct sk_buff *, struct sk_buff *);
+	void (*attach)(struct sk_buff *, const struct sk_buff *);
 
 	if (skb->nfct) {
 		rcu_read_lock();
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 0283bae..d32afaf 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -1192,7 +1192,7 @@ EXPORT_SYMBOL_GPL(nf_ct_port_nlattr_tuple_size);
 #endif
 
 /* Used by ipt_REJECT and ip6t_REJECT. */
-static void nf_conntrack_attach(struct sk_buff *nskb, struct sk_buff *skb)
+static void nf_conntrack_attach(struct sk_buff *nskb, const struct sk_buff *skb)
 {
 	struct nf_conn *ct;
 	enum ip_conntrack_info ctinfo;
-- 
1.8.1.4

[PATCH 3/5] netfilter: nf_nat: fix locking in nf_nat_seq_adjust()

[Patrick McHardy]

nf_nat_seq_adjust() needs to grab nf_nat_setofs_lock to protect against
concurrent changes to the sequence adjustment data.

Signed-off-by: Patrick McHardy <kaber@trash.net>
---
 net/netfilter/nf_nat_helper.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/net/netfilter/nf_nat_helper.c b/net/netfilter/nf_nat_helper.c
index 85e20a9..a7262ed 100644
--- a/net/netfilter/nf_nat_helper.c
+++ b/net/netfilter/nf_nat_helper.c
@@ -373,6 +373,7 @@ nf_nat_seq_adjust(struct sk_buff *skb,
 	s16 seqoff, ackoff;
 	struct nf_conn_nat *nat = nfct_nat(ct);
 	struct nf_nat_seq *this_way, *other_way;
+	int res;
 
 	dir = CTINFO2DIR(ctinfo);
 
@@ -383,6 +384,7 @@ nf_nat_seq_adjust(struct sk_buff *skb,
 		return 0;
 
 	tcph = (void *)skb->data + protoff;
+	spin_lock_bh(&nf_nat_seqofs_lock);
 	if (after(ntohl(tcph->seq), this_way->correction_pos))
 		seqoff = this_way->offset_after;
 	else
@@ -407,7 +409,10 @@ nf_nat_seq_adjust(struct sk_buff *skb,
 	tcph->seq = newseq;
 	tcph->ack_seq = newack;
 
-	return nf_nat_sack_adjust(skb, protoff, tcph, ct, ctinfo);
+	res = nf_nat_sack_adjust(skb, protoff, tcph, ct, ctinfo);
+	spin_unlock_bh(&nf_nat_seqofs_lock);
+
+	return res;
 }
 
 /* Setup NAT on this expected conntrack so it follows master. */
-- 
1.8.1.4

[PATCH 4/5] netfilter: nf_nat: change sequence number adjustments to 32 bits

[Patrick McHardy]

Using 16 bits is too small, when many adjustments happen the offsets might
overflow and break the connection.

Signed-off-by: Patrick McHardy <kaber@trash.net>
---
 include/linux/netfilter.h              | 2 +-
 include/net/netfilter/nf_conntrack.h   | 2 +-
 include/net/netfilter/nf_nat.h         | 2 +-
 include/net/netfilter/nf_nat_helper.h  | 6 +++---
 net/netfilter/nf_conntrack_core.c      | 2 +-
 net/netfilter/nf_conntrack_proto_tcp.c | 4 ++--
 net/netfilter/nf_nat_helper.c          | 8 ++++----
 7 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/include/linux/netfilter.h b/include/linux/netfilter.h
index f4bbf2c..655d5d1 100644
--- a/include/linux/netfilter.h
+++ b/include/linux/netfilter.h
@@ -330,7 +330,7 @@ extern struct nfq_ct_hook __rcu *nfq_ct_hook;
 
 struct nfq_ct_nat_hook {
 	void (*seq_adjust)(struct sk_buff *skb, struct nf_conn *ct,
-			   u32 ctinfo, int off);
+			   u32 ctinfo, s32 off);
 };
 extern struct nfq_ct_nat_hook __rcu *nfq_ct_nat_hook;
 #else
diff --git a/include/net/netfilter/nf_conntrack.h b/include/net/netfilter/nf_conntrack.h
index 644d9c2..21c374c 100644
--- a/include/net/netfilter/nf_conntrack.h
+++ b/include/net/netfilter/nf_conntrack.h
@@ -235,7 +235,7 @@ static inline bool nf_ct_kill(struct nf_conn *ct)
 }
 
 /* These are for NAT.  Icky. */
-extern s16 (*nf_ct_nat_offset)(const struct nf_conn *ct,
+extern s32 (*nf_ct_nat_offset)(const struct nf_conn *ct,
 			       enum ip_conntrack_dir dir,
 			       u32 seq);
 
diff --git a/include/net/netfilter/nf_nat.h b/include/net/netfilter/nf_nat.h
index ad14a79..e244141 100644
--- a/include/net/netfilter/nf_nat.h
+++ b/include/net/netfilter/nf_nat.h
@@ -19,7 +19,7 @@ struct nf_nat_seq {
 	u_int32_t correction_pos;
 
 	/* sequence number offset before and after last modification */
-	int16_t offset_before, offset_after;
+	int32_t offset_before, offset_after;
 };
 
 #include <linux/list.h>
diff --git a/include/net/netfilter/nf_nat_helper.h b/include/net/netfilter/nf_nat_helper.h
index b4d6bfc..194c347 100644
--- a/include/net/netfilter/nf_nat_helper.h
+++ b/include/net/netfilter/nf_nat_helper.h
@@ -41,7 +41,7 @@ extern int nf_nat_mangle_udp_packet(struct sk_buff *skb,
 
 extern void nf_nat_set_seq_adjust(struct nf_conn *ct,
 				  enum ip_conntrack_info ctinfo,
-				  __be32 seq, s16 off);
+				  __be32 seq, s32 off);
 extern int nf_nat_seq_adjust(struct sk_buff *skb,
 			     struct nf_conn *ct,
 			     enum ip_conntrack_info ctinfo,
@@ -56,11 +56,11 @@ extern int (*nf_nat_seq_adjust_hook)(struct sk_buff *skb,
 extern void nf_nat_follow_master(struct nf_conn *ct,
 				 struct nf_conntrack_expect *this);
 
-extern s16 nf_nat_get_offset(const struct nf_conn *ct,
+extern s32 nf_nat_get_offset(const struct nf_conn *ct,
 			     enum ip_conntrack_dir dir,
 			     u32 seq);
 
 extern void nf_nat_tcp_seq_adjust(struct sk_buff *skb, struct nf_conn *ct,
-				  u32 dir, int off);
+				  u32 dir, s32 off);
 
 #endif
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index d32afaf..26c5b37 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -1692,7 +1692,7 @@ err_stat:
 	return ret;
 }
 
-s16 (*nf_ct_nat_offset)(const struct nf_conn *ct,
+s32 (*nf_ct_nat_offset)(const struct nf_conn *ct,
 			enum ip_conntrack_dir dir,
 			u32 seq);
 EXPORT_SYMBOL_GPL(nf_ct_nat_offset);
diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
index 7dcc376..8f308d8 100644
--- a/net/netfilter/nf_conntrack_proto_tcp.c
+++ b/net/netfilter/nf_conntrack_proto_tcp.c
@@ -496,7 +496,7 @@ static void tcp_sack(const struct sk_buff *skb, unsigned int dataoff,
 }
 
 #ifdef CONFIG_NF_NAT_NEEDED
-static inline s16 nat_offset(const struct nf_conn *ct,
+static inline s32 nat_offset(const struct nf_conn *ct,
 			     enum ip_conntrack_dir dir,
 			     u32 seq)
 {
@@ -525,7 +525,7 @@ static bool tcp_in_window(const struct nf_conn *ct,
 	struct ip_ct_tcp_state *receiver = &state->seen[!dir];
 	const struct nf_conntrack_tuple *tuple = &ct->tuplehash[dir].tuple;
 	__u32 seq, ack, sack, end, win, swin;
-	s16 receiver_offset;
+	s32 receiver_offset;
 	bool res;
 
 	/*
diff --git a/net/netfilter/nf_nat_helper.c b/net/netfilter/nf_nat_helper.c
index a7262ed..ff4a589 100644
--- a/net/netfilter/nf_nat_helper.c
+++ b/net/netfilter/nf_nat_helper.c
@@ -68,13 +68,13 @@ adjust_tcp_sequence(u32 seq,
 }
 
 /* Get the offset value, for conntrack */
-s16 nf_nat_get_offset(const struct nf_conn *ct,
+s32 nf_nat_get_offset(const struct nf_conn *ct,
 		      enum ip_conntrack_dir dir,
 		      u32 seq)
 {
 	struct nf_conn_nat *nat = nfct_nat(ct);
 	struct nf_nat_seq *this_way;
-	s16 offset;
+	s32 offset;
 
 	if (!nat)
 		return 0;
@@ -143,7 +143,7 @@ static int enlarge_skb(struct sk_buff *skb, unsigned int extra)
 }
 
 void nf_nat_set_seq_adjust(struct nf_conn *ct, enum ip_conntrack_info ctinfo,
-			   __be32 seq, s16 off)
+			   __be32 seq, s32 off)
 {
 	if (!off)
 		return;
@@ -370,7 +370,7 @@ nf_nat_seq_adjust(struct sk_buff *skb,
 	struct tcphdr *tcph;
 	int dir;
 	__be32 newseq, newack;
-	s16 seqoff, ackoff;
+	s32 seqoff, ackoff;
 	struct nf_conn_nat *nat = nfct_nat(ct);
 	struct nf_nat_seq *this_way, *other_way;
 	int res;
-- 
1.8.1.4

[PATCH 5/5] netfilter: nf_nat: use per-conntrack locking for sequence number adjustments

[Patrick McHardy]

Get rid of the global lock and use per-conntrack locks for protecting the
sequencen number adjustment data. Additionally saves one lock/unlock
operation for every TCP packet.

Signed-off-by: Patrick McHardy <kaber@trash.net>
---
 net/netfilter/nf_nat_helper.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/net/netfilter/nf_nat_helper.c b/net/netfilter/nf_nat_helper.c
index ff4a589..46b9baa 100644
--- a/net/netfilter/nf_nat_helper.c
+++ b/net/netfilter/nf_nat_helper.c
@@ -30,8 +30,6 @@
 	pr_debug("offset_before=%d, offset_after=%d, correction_pos=%u\n", \
 		 x->offset_before, x->offset_after, x->correction_pos);
 
-static DEFINE_SPINLOCK(nf_nat_seqofs_lock);
-
 /* Setup TCP sequence correction given this change at this sequence */
 static inline void
 adjust_tcp_sequence(u32 seq,
@@ -49,7 +47,7 @@ adjust_tcp_sequence(u32 seq,
 	pr_debug("adjust_tcp_sequence: Seq_offset before: ");
 	DUMP_OFFSET(this_way);
 
-	spin_lock_bh(&nf_nat_seqofs_lock);
+	spin_lock_bh(&ct->lock);
 
 	/* SYN adjust. If it's uninitialized, or this is after last
 	 * correction, record it: we don't handle more than one
@@ -61,31 +59,26 @@ adjust_tcp_sequence(u32 seq,
 		this_way->offset_before = this_way->offset_after;
 		this_way->offset_after += sizediff;
 	}
-	spin_unlock_bh(&nf_nat_seqofs_lock);
+	spin_unlock_bh(&ct->lock);
 
 	pr_debug("adjust_tcp_sequence: Seq_offset after: ");
 	DUMP_OFFSET(this_way);
 }
 
-/* Get the offset value, for conntrack */
+/* Get the offset value, for conntrack. Caller must have the conntrack locked */
 s32 nf_nat_get_offset(const struct nf_conn *ct,
 		      enum ip_conntrack_dir dir,
 		      u32 seq)
 {
 	struct nf_conn_nat *nat = nfct_nat(ct);
 	struct nf_nat_seq *this_way;
-	s32 offset;
 
 	if (!nat)
 		return 0;
 
 	this_way = &nat->seq[dir];
-	spin_lock_bh(&nf_nat_seqofs_lock);
-	offset = after(seq, this_way->correction_pos)
+	return after(seq, this_way->correction_pos)
 		 ? this_way->offset_after : this_way->offset_before;
-	spin_unlock_bh(&nf_nat_seqofs_lock);
-
-	return offset;
 }
 
 /* Frobs data inside this packet, which is linear. */
@@ -384,7 +377,7 @@ nf_nat_seq_adjust(struct sk_buff *skb,
 		return 0;
 
 	tcph = (void *)skb->data + protoff;
-	spin_lock_bh(&nf_nat_seqofs_lock);
+	spin_lock_bh(&ct->lock);
 	if (after(ntohl(tcph->seq), this_way->correction_pos))
 		seqoff = this_way->offset_after;
 	else
@@ -410,7 +403,7 @@ nf_nat_seq_adjust(struct sk_buff *skb,
 	tcph->ack_seq = newack;
 
 	res = nf_nat_sack_adjust(skb, protoff, tcph, ct, ctinfo);
-	spin_unlock_bh(&nf_nat_seqofs_lock);
+	spin_unlock_bh(&ct->lock);
 
 	return res;
 }
-- 
1.8.1.4

Re: [PATCH 1/5] netfilter: nf_conntrack: remove net_ratelimit() for LOG_INVALID()

[Pablo Neira Ayuso]

On Sun, Jul 28, 2013 at 10:54:07PM +0200, Patrick McHardy wrote:
> Logging of invalid packets has to be explicitly enabled. Rate-limiting these
> messages is inconsistent with other netfilter logging features and makes
> debugging harder.

Applied, thanks Patrick!

Re: [PATCH 2/5] netfilter: nf_conntrack: constify sk_buff argument to nf_ct_attach()

[Pablo Neira Ayuso]

Also applied, thanks

Re: [PATCH 3/5] netfilter: nf_nat: fix locking in nf_nat_seq_adjust()

[Pablo Neira Ayuso]

On Sun, Jul 28, 2013 at 10:54:09PM +0200, Patrick McHardy wrote:
> nf_nat_seq_adjust() needs to grab nf_nat_setofs_lock to protect against
> concurrent changes to the sequence adjustment data.

Applied, thanks.

I have enqueued this to nf so it hits 3.11. I have to wait to take 4/5
and 5/5 for a while until this patch hits Linus tree.

Re: [PATCH 3/5] netfilter: nf_nat: fix locking in nf_nat_seq_adjust()

[Patrick McHardy]

On Wed, Jul 31, 2013 at 06:55:23PM +0200, Pablo Neira Ayuso wrote:
> On Sun, Jul 28, 2013 at 10:54:09PM +0200, Patrick McHardy wrote:
> > nf_nat_seq_adjust() needs to grab nf_nat_setofs_lock to protect against
> > concurrent changes to the sequence adjustment data.
> 
> Applied, thanks.
> 
> I have enqueued this to nf so it hits 3.11. I have to wait to take 4/5
> and 5/5 for a while until this patch hits Linus tree.

We've had this problem since the beginning of netfilter, so I think
this can go to -next. So far I haven't seen any bugreports related to
this, but its clearly wrong.

A follow up patch will move the entire sequence number adjustment data
to an extend, so the size increase will only affect the quite rare case
of connections using a helper.

Re: [PATCH 3/5] netfilter: nf_nat: fix locking in nf_nat_seq_adjust()

[Pablo Neira Ayuso]

On Wed, Jul 31, 2013 at 07:51:18PM +0200, Patrick McHardy wrote:
> On Wed, Jul 31, 2013 at 06:55:23PM +0200, Pablo Neira Ayuso wrote:
> > On Sun, Jul 28, 2013 at 10:54:09PM +0200, Patrick McHardy wrote:
> > > nf_nat_seq_adjust() needs to grab nf_nat_setofs_lock to protect against
> > > concurrent changes to the sequence adjustment data.
> > 
> > Applied, thanks.
> > 
> > I have enqueued this to nf so it hits 3.11. I have to wait to take 4/5
> > and 5/5 for a while until this patch hits Linus tree.
> 
> We've had this problem since the beginning of netfilter, so I think
> this can go to -next. So far I haven't seen any bugreports related to
> this, but its clearly wrong.

Fair enough, will enqueue this to -next then.

> A follow up patch will move the entire sequence number adjustment data
> to an extend, so the size increase will only affect the quite rare case
> of connections using a helper.

Great to know, thanks.

Re: [PATCH 4/5] netfilter: nf_nat: change sequence number adjustments to 32 bits

[Pablo Neira Ayuso]

On Sun, Jul 28, 2013 at 10:54:10PM +0200, Patrick McHardy wrote:
> Using 16 bits is too small, when many adjustments happen the offsets might
> overflow and break the connection.

Applied, thanks.

Re: [PATCH 5/5] netfilter: nf_nat: use per-conntrack locking for sequence number adjustments

[Pablo Neira Ayuso]

On Sun, Jul 28, 2013 at 10:54:11PM +0200, Patrick McHardy wrote:
> Get rid of the global lock and use per-conntrack locks for protecting the
> sequencen number adjustment data. Additionally saves one lock/unlock
> operation for every TCP packet.

also applied, thanks.