[nf-next PATCH 0/4] netfilter: Conntrack counter review

[nf-next PATCH 0/4] netfilter: Conntrack counter review

[Phil Sutter]

The intention behind the various conntrack counters was recently
discussed as part of a conntrack-tools.8 man page update review.

This series adjusts counters accordingly.

Phil Sutter (4):
  netfilter: conntrack: Untangle insert_failed counter from others
  netfilter: conntrack: Untangle drop and invalid counters
  netfilter: conntrack: nf_ct_seq_adjust to return error cause
  netfilter: conntrack: Improve invalid packet stats

 net/netfilter/nf_conntrack_core.c   | 12 ++++++++----
 net/netfilter/nf_conntrack_ovs.c    |  2 +-
 net/netfilter/nf_conntrack_proto.c  | 14 ++++++++++----
 net/netfilter/nf_conntrack_seqadj.c | 24 +++++++++++-------------
 4 files changed, 30 insertions(+), 22 deletions(-)

-- 
2.54.0

[nf-next PATCH 1/4] netfilter: conntrack: Untangle insert_failed counter from others

[Phil Sutter]

There is not much sense in incrementing multiple conntrack counters for
the same situation. Defining insert_failed as a situation where a valid
packet's conntrack can't be confirmed due to unresolvable clash sets it
apart from 'drop' (ENOMEM situation) and 'chaintoolong'.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 net/netfilter/nf_conntrack_core.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index d0d9e5ea84a0..53401e21ad99 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -1169,7 +1169,6 @@ nf_ct_resolve_clash(struct sk_buff *skb, struct nf_conntrack_tuple_hash *h,
 		return ret;
 
 drop:
-	NF_CT_STAT_INC(net, drop);
 	NF_CT_STAT_INC(net, insert_failed);
 	return NF_DROP;
 }
@@ -1259,7 +1258,6 @@ __nf_conntrack_confirm(struct sk_buff *skb)
 		if (chainlen++ > max_chainlen) {
 chaintoolong:
 			NF_CT_STAT_INC(net, chaintoolong);
-			NF_CT_STAT_INC(net, insert_failed);
 			ret = NF_DROP;
 			goto dying;
 		}
-- 
2.54.0

[nf-next PATCH 2/4] netfilter: conntrack: Untangle drop and invalid counters

[Phil Sutter]

In nf_conntrack_in, 'drop' counter increments if
nf_conntrack_handle_packet returns NF_DROP. This is a special case with
TCP packets (added by commit 6b69fe0c73c0 ("netfilter: nf_conntrack_tcp:
fix endless loop") and not related to invalid packets which are
responsible for all the other <=0 returns. So don't increment 'invalid'
ounter in this case.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 net/netfilter/nf_conntrack_core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 53401e21ad99..f9b8927327ba 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -2065,9 +2065,10 @@ nf_conntrack_in(struct sk_buff *skb, const struct nf_hook_state *state)
 		if (ret == -NF_REPEAT)
 			goto repeat;
 
-		NF_CT_STAT_INC_ATOMIC(state->net, invalid);
 		if (ret == NF_DROP)
 			NF_CT_STAT_INC_ATOMIC(state->net, drop);
+		else
+			NF_CT_STAT_INC_ATOMIC(state->net, invalid);
 
 		ret = -ret;
 		goto out;
-- 
2.54.0

[nf-next PATCH 3/4] netfilter: conntrack: nf_ct_seq_adjust to return error cause

[Phil Sutter]

Some callers will want to distinguish between malformed TCP packets and
other failure reasons, so invert the return code logic.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 net/netfilter/nf_conntrack_core.c   |  2 +-
 net/netfilter/nf_conntrack_ovs.c    |  2 +-
 net/netfilter/nf_conntrack_proto.c  |  2 +-
 net/netfilter/nf_conntrack_seqadj.c | 24 +++++++++++-------------
 4 files changed, 14 insertions(+), 16 deletions(-)

diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index f9b8927327ba..b0cd530cf8fe 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -2241,7 +2241,7 @@ static int nf_confirm_cthelper(struct sk_buff *skb, struct nf_conn *ct,
 
 	if (test_bit(IPS_SEQ_ADJUST_BIT, &ct->status) &&
 	    !nf_is_loopback_packet(skb)) {
-		if (!nf_ct_seq_adjust(skb, ct, ctinfo, protoff)) {
+		if (nf_ct_seq_adjust(skb, ct, ctinfo, protoff) < 0) {
 			NF_CT_STAT_INC_ATOMIC(nf_ct_net(ct), drop);
 			return NF_DROP;
 		}
diff --git a/net/netfilter/nf_conntrack_ovs.c b/net/netfilter/nf_conntrack_ovs.c
index b4085af3ad1c..d64379e203eb 100644
--- a/net/netfilter/nf_conntrack_ovs.c
+++ b/net/netfilter/nf_conntrack_ovs.c
@@ -76,7 +76,7 @@ int nf_ct_helper(struct sk_buff *skb, struct nf_conn *ct,
 	 * addresses and/or port numbers in the text-based control connection.
 	 */
 	if (test_bit(IPS_SEQ_ADJUST_BIT, &ct->status) &&
-	    !nf_ct_seq_adjust(skb, ct, ctinfo, protoff))
+	    nf_ct_seq_adjust(skb, ct, ctinfo, protoff) < 0)
 		return NF_DROP;
 	return NF_ACCEPT;
 }
diff --git a/net/netfilter/nf_conntrack_proto.c b/net/netfilter/nf_conntrack_proto.c
index 7a40e4e0e33e..0db404f662de 100644
--- a/net/netfilter/nf_conntrack_proto.c
+++ b/net/netfilter/nf_conntrack_proto.c
@@ -195,7 +195,7 @@ unsigned int nf_confirm(void *priv,
 	}
 
 	if (seqadj_needed &&
-	    !nf_ct_seq_adjust(skb, ct, ctinfo, protoff)) {
+	    nf_ct_seq_adjust(skb, ct, ctinfo, protoff) < 0) {
 		NF_CT_STAT_INC_ATOMIC(nf_ct_net(ct), drop);
 		return NF_DROP;
 	}
diff --git a/net/netfilter/nf_conntrack_seqadj.c b/net/netfilter/nf_conntrack_seqadj.c
index 220216a4edc5..c95b6383a330 100644
--- a/net/netfilter/nf_conntrack_seqadj.c
+++ b/net/netfilter/nf_conntrack_seqadj.c
@@ -117,23 +117,21 @@ static void nf_ct_sack_block_adjust(struct sk_buff *skb,
 }
 
 /* TCP SACK sequence number adjustment */
-static unsigned int nf_ct_sack_adjust(struct sk_buff *skb,
-				      unsigned int protoff,
-				      struct nf_conn *ct,
-				      enum ip_conntrack_info ctinfo)
+static int nf_ct_sack_adjust(struct sk_buff *skb, unsigned int protoff,
+			     struct nf_conn *ct, enum ip_conntrack_info ctinfo)
 {
 	struct tcphdr *tcph = (void *)skb->data + protoff;
 	struct nf_conn_seqadj *seqadj = nfct_seqadj(ct);
 	unsigned int dir, optoff, optend;
 
 	if (!seqadj)
-		return 0;
+		return -ENOSPC;
 
 	optoff = protoff + sizeof(struct tcphdr);
 	optend = protoff + tcph->doff * 4;
 
 	if (skb_ensure_writable(skb, optend))
-		return 0;
+		return -ENOMEM;
 
 	tcph = (void *)skb->data + protoff;
 	dir = CTINFO2DIR(ctinfo);
@@ -144,7 +142,7 @@ static unsigned int nf_ct_sack_adjust(struct sk_buff *skb,
 
 		switch (op[0]) {
 		case TCPOPT_EOL:
-			return 1;
+			return 0;
 		case TCPOPT_NOP:
 			optoff++;
 			continue;
@@ -153,7 +151,7 @@ static unsigned int nf_ct_sack_adjust(struct sk_buff *skb,
 			if (optoff + 1 == optend ||
 			    optoff + op[1] > optend ||
 			    op[1] < 2)
-				return 0;
+				return -EINVAL;
 			if (op[0] == TCPOPT_SACK &&
 			    op[1] >= 2+TCPOLEN_SACK_PERBLOCK &&
 			    ((op[1] - 2) % TCPOLEN_SACK_PERBLOCK) == 0)
@@ -163,10 +161,10 @@ static unsigned int nf_ct_sack_adjust(struct sk_buff *skb,
 			optoff += op[1];
 		}
 	}
-	return 1;
+	return 0;
 }
 
-/* TCP sequence number adjustment.  Returns 1 on success, 0 on failure */
+/* TCP sequence number adjustment.  Returns 0 on success, -err on failure */
 int nf_ct_seq_adjust(struct sk_buff *skb,
 		     struct nf_conn *ct, enum ip_conntrack_info ctinfo,
 		     unsigned int protoff)
@@ -177,16 +175,16 @@ int nf_ct_seq_adjust(struct sk_buff *skb,
 	s32 seqoff, ackoff;
 	struct nf_conn_seqadj *seqadj = nfct_seqadj(ct);
 	struct nf_ct_seqadj *this_way, *other_way;
-	int res = 1;
+	int res = 0;
 
 	if (!seqadj)
-		return 0;
+		return -ENOSPC;
 
 	this_way  = &seqadj->seq[dir];
 	other_way = &seqadj->seq[!dir];
 
 	if (skb_ensure_writable(skb, protoff + sizeof(*tcph)))
-		return 0;
+		return -ENOMEM;
 
 	tcph = (void *)skb->data + protoff;
 	spin_lock_bh(&ct->lock);
-- 
2.54.0

[nf-next PATCH 4/4] netfilter: conntrack: Improve invalid packet stats

[Phil Sutter]

If nf_ct_seq_adjust detects a malformed packet, increment 'invalid'
counter instead of 'drop'.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 net/netfilter/nf_conntrack_core.c  |  7 ++++++-
 net/netfilter/nf_conntrack_proto.c | 14 ++++++++++----
 2 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index b0cd530cf8fe..45e0580e72d2 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -2241,7 +2241,12 @@ static int nf_confirm_cthelper(struct sk_buff *skb, struct nf_conn *ct,
 
 	if (test_bit(IPS_SEQ_ADJUST_BIT, &ct->status) &&
 	    !nf_is_loopback_packet(skb)) {
-		if (nf_ct_seq_adjust(skb, ct, ctinfo, protoff) < 0) {
+		int ret = nf_ct_seq_adjust(skb, ct, ctinfo, protoff);
+
+		if (ret == -EINVAL) {
+			NF_CT_STAT_INC_ATOMIC(nf_ct_net(ct), invalid);
+			return NF_DROP;
+		} else if (ret < 0) {
 			NF_CT_STAT_INC_ATOMIC(nf_ct_net(ct), drop);
 			return NF_DROP;
 		}
diff --git a/net/netfilter/nf_conntrack_proto.c b/net/netfilter/nf_conntrack_proto.c
index 0db404f662de..e4cc9a69494e 100644
--- a/net/netfilter/nf_conntrack_proto.c
+++ b/net/netfilter/nf_conntrack_proto.c
@@ -194,10 +194,16 @@ unsigned int nf_confirm(void *priv,
 		}
 	}
 
-	if (seqadj_needed &&
-	    nf_ct_seq_adjust(skb, ct, ctinfo, protoff) < 0) {
-		NF_CT_STAT_INC_ATOMIC(nf_ct_net(ct), drop);
-		return NF_DROP;
+	if (seqadj_needed) {
+		int ret = nf_ct_seq_adjust(skb, ct, ctinfo, protoff);
+
+		if (ret == -EINVAL) {
+			NF_CT_STAT_INC_ATOMIC(nf_ct_net(ct), invalid);
+			return NF_DROP;
+		} else if (ret < 0) {
+			NF_CT_STAT_INC_ATOMIC(nf_ct_net(ct), drop);
+			return NF_DROP;
+		}
 	}
 
 	/* We've seen it coming out the other side: confirm it */
-- 
2.54.0