Skip to contentSkip to conversation

Refine the nat helper codes

PATCH In review [email protected] 3 participants 7 messages Mar 17, 2017 – Mar 27, 2017

Subsystems (2)
Networking netfilter
lore
Share & tools
lore

[email protected]

author
#0 lore raw

cover Refine the nat helper codes

From: Gao Feng <[email protected]>

There are some netfilter functions which return the literal 1 and 0 as the bool value. We should use bool instead of them.
And even though some calleris treat the return value as Neftiler action. For example, the caller would compare the return value with NF_ACCEPT.

Luckily the value of NF_ACCEPT is 1, so there is no any real issue.

Gao Feng (4):
netfilter: amanda: Correct the return value comparison of the func
nf_nat_mangle_udp_packet
netfilter: irc: Correct the return value comparison of the func
nf_nat_mangle_tcp_packet
netfilter: helper: Use the bool instead of int type
netfilter: sip: Use NF_DROP and NF_ACCEPT instead of 0 and 1

 include/net/netfilter/nf_nat_helper.h | 36 +++++++++++++++----------------
 net/netfilter/nf_nat_amanda.c         | 11 +++++-----
 net/netfilter/nf_nat_helper.c         | 40 +++++++++++++++++------------------
 net/netfilter/nf_nat_irc.c            |  9 ++++----
 net/netfilter/nf_nat_sip.c            |  6 +++---
 5 files changed, 50 insertions(+), 52 deletions(-)
Signature · 1 line
1.9.1
author
#1 lore raw

PATCH 1/4 netfilter: amanda: Correct the return value comparison of the func nf_nat_mangle_udp_packet

From: Gao Feng <[email protected]>

The return value of nf_nat_mangle_udp_packet actually is 1 and 0 as bool type. But the amanda codes compare it with NF_ACCEPT.

Signed-off-by Gao Feng <[email protected]>
---
 net/netfilter/nf_nat_amanda.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)
net/netfilter/nf_nat_amanda.c +5 -6
index eb77238..e4d61a7 100644
@@ -33,7 +33,6 @@ static unsigned int help(struct sk_buff *skb,
 {
 	char buffer[sizeof("65535")];
 	u_int16_t port;
-	unsigned int ret;
 
 	/* Connection comes from client. */
 	exp->saved_proto.tcp.port = exp->tuple.dst.u.tcp.port;

@@ -63,14 +62,14 @@ static unsigned int help(struct sk_buff *skb,
 	}
 
 	sprintf(buffer, "%u", port);
-	ret = nf_nat_mangle_udp_packet(skb, exp->master, ctinfo,
-				       protoff, matchoff, matchlen,
-				       buffer, strlen(buffer));
-	if (ret != NF_ACCEPT) {
+	if (!nf_nat_mangle_udp_packet(skb, exp->master, ctinfo,
+				      protoff, matchoff, matchlen,
+				      buffer, strlen(buffer))) {
 		nf_ct_helper_log(skb, exp->master, "cannot mangle packet");
 		nf_ct_unexpect_related(exp);
+		return NF_DROP;
 	}
-	return ret;
+	return NF_ACCEPT;
 }
 
 static void __exit nf_nat_amanda_fini(void)
Signature · 1 line
1.9.1
maintainer
#2 lore raw

Re: [PATCH nf-next 1/4] netfilter: amanda: Correct the return value comparison of the func nf_nat_mangle_udp_packet

Quoted 29 lines · On Fri, Mar 17, 2017 at 02:47:19PM +0800, [email protected] wrote:

From: Gao Feng <[email protected]>

The return value of nf_nat_mangle_udp_packet actually is 1 and 0 as bool type. But the amanda codes compare it with NF_ACCEPT.

Signed-off-by Gao Feng <[email protected]>
---
 net/netfilter/nf_nat_amanda.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)
net/netfilter/nf_nat_amanda.c +0 -4
index eb77238..e4d61a7 100644
@@ -33,7 +33,6 @@ static unsigned int help(struct sk_buff *skb,
 {
 	char buffer[sizeof("65535")];
 	u_int16_t port;
-	unsigned int ret;
 
 	/* Connection comes from client. */
 	exp->saved_proto.tcp.port = exp->tuple.dst.u.tcp.port;

@@ -63,14 +62,14 @@ static unsigned int help(struct sk_buff *skb,
 	}
 
 	sprintf(buffer, "%u", port);
-	ret = nf_nat_mangle_udp_packet(skb, exp->master, ctinfo,
-				       protoff, matchoff, matchlen,
-				       buffer, strlen(buffer));

net/netfilter/nf_nat_amanda.c static unsigned int help(struct sk_buff *skb, · line 72

Quoted 10 lines
net/netfilter/nf_nat_amanda.c +5 -2
-	if (ret != NF_ACCEPT) {
+	if (!nf_nat_mangle_udp_packet(skb, exp->master, ctinfo,
+				      protoff, matchoff, matchlen,
+				      buffer, strlen(buffer))) {
 		nf_ct_helper_log(skb, exp->master, "cannot mangle packet");
 		nf_ct_unexpect_related(exp);
+		return NF_DROP;
 	}
-	return ret;
+	return NF_ACCEPT;

This cleanup patches are a bit oversplit.

Better, send one patch where you update nf_nat_mangle_udp_packet() and nf_nat_mangle_tcp_packet() to return boolean and update *all of the netfilter spots* where we use them accordingly.

Please be careful on this...

#3 lore raw

RE: [PATCH nf-next 1/4] netfilter: amanda: Correct the return value comparison of the func nf_nat_mangle_udp_packet

Quoted 6 lines

-----Original Message-----
From: Pablo Neira Ayuso [mailto:[email protected]]
Sent: Monday, March 27, 2017 8:13 PM
To: [email protected]
Cc: [email protected]; [email protected]
Subject: Re: [PATCH nf-next 1/4] netfilter: amanda: Correct the return

value

Quoted 44 lines

comparison of the func nf_nat_mangle_udp_packet

Quoted 28 lines · On Fri, Mar 17, 2017 at 02:47:19PM +0800, [email protected] wrote:

From: Gao Feng <[email protected]>

The return value of nf_nat_mangle_udp_packet actually is 1 and 0 as bool type. But the amanda codes compare it with NF_ACCEPT.

Signed-off-by Gao Feng <[email protected]>
---
 net/netfilter/nf_nat_amanda.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)
(quoted diff) +0 -0

b/net/netfilter/nf_nat_amanda.c index eb77238..e4d61a7 100644

net/netfilter/nf_nat_amanda.c +1 -4
@@ -33,7 +33,6 @@ static unsigned int help(struct sk_buff *skb,  {
 	char buffer[sizeof("65535")];
 	u_int16_t port;
-	unsigned int ret;
 
 	/* Connection comes from client. */
 	exp->saved_proto.tcp.port = exp->tuple.dst.u.tcp.port; @@ -63,14
+62,14 @@ static unsigned int help(struct sk_buff *skb,
 	}
 
 	sprintf(buffer, "%u", port);
-	ret = nf_nat_mangle_udp_packet(skb, exp->master, ctinfo,
-				       protoff, matchoff, matchlen,
-				       buffer, strlen(buffer));

net/netfilter/nf_nat_amanda.c static unsigned int help(struct sk_buff *skb, {

Quoted 10 lines
net/netfilter/nf_nat_amanda.c +5 -2
-	if (ret != NF_ACCEPT) {
+	if (!nf_nat_mangle_udp_packet(skb, exp->master, ctinfo,
+				      protoff, matchoff, matchlen,
+				      buffer, strlen(buffer))) {
 		nf_ct_helper_log(skb, exp->master, "cannot mangle packet");
 		nf_ct_unexpect_related(exp);
+		return NF_DROP;
 	}
-	return ret;
+	return NF_ACCEPT;

This cleanup patches are a bit oversplit.

Quoted 2 lines

Better, send one patch where you update nf_nat_mangle_udp_packet() and nf_nat_mangle_tcp_packet() to return boolean and update *all of the

netfilter

Quoted 3 lines

spots* where we use them accordingly.

Please be careful on this...

Ok, I would merge them into one patch.
I already checked them before, but I would check it again after merge.

Regards
Feng

author
#4 lore raw

PATCH 2/4 netfilter: irc: Correct the return value comparison of the func nf_nat_mangle_tcp_packet

In reply to [email protected]

From: Gao Feng <[email protected]>

The return value of nf_nat_mangle_tcp_packet actually is 1 and 0 as bool type. But the irc codes compare it with NF_ACCEPT.

Signed-off-by Gao Feng <[email protected]>
---
 net/netfilter/nf_nat_irc.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)
net/netfilter/nf_nat_irc.c +4 -5
index 1fb2258..0648cb0 100644
@@ -37,7 +37,6 @@ static unsigned int help(struct sk_buff *skb,
 	struct nf_conn *ct = exp->master;
 	union nf_inet_addr newaddr;
 	u_int16_t port;
-	unsigned int ret;
 
 	/* Reply comes from server. */
 	newaddr = ct->tuplehash[IP_CT_DIR_REPLY].tuple.dst.u3;

@@ -83,14 +82,14 @@ static unsigned int help(struct sk_buff *skb,
 	pr_debug("nf_nat_irc: inserting '%s' == %pI4, port %u\n",
 		 buffer, &newaddr.ip, port);
 
-	ret = nf_nat_mangle_tcp_packet(skb, ct, ctinfo, protoff, matchoff,
-				       matchlen, buffer, strlen(buffer));
-	if (ret != NF_ACCEPT) {
+	if (!nf_nat_mangle_tcp_packet(skb, ct, ctinfo, protoff, matchoff,
+				      matchlen, buffer, strlen(buffer))) {
 		nf_ct_helper_log(skb, ct, "cannot mangle packet");
 		nf_ct_unexpect_related(exp);
+		return NF_DROP;
 	}
 
-	return ret;
+	return NF_ACCEPT;
 }
 
 static void __exit nf_nat_irc_fini(void)
Signature · 1 line
1.9.1
author
#5 lore raw

PATCH 3/4 netfilter: helper: Use the bool instead of int type

In reply to [email protected]

From: Gao Feng <[email protected]>

Use the bool type instead of the int type in the codes of nf_nat_helper.h/c.

Signed-off-by Gao Feng <[email protected]>
---
 include/net/netfilter/nf_nat_helper.h | 36 +++++++++++++++----------------
 net/netfilter/nf_nat_helper.c         | 40 +++++++++++++++++------------------
 2 files changed, 38 insertions(+), 38 deletions(-)
include/net/netfilter/nf_nat_helper.h +18 -18
index 01bcc6b..fbfa5ac 100644
@@ -7,31 +7,31 @@
 struct sk_buff;
 
 /* These return true or false. */
-int __nf_nat_mangle_tcp_packet(struct sk_buff *skb, struct nf_conn *ct,
-			       enum ip_conntrack_info ctinfo,
-			       unsigned int protoff, unsigned int match_offset,
-			       unsigned int match_len, const char *rep_buffer,
-			       unsigned int rep_len, bool adjust);
+bool __nf_nat_mangle_tcp_packet(struct sk_buff *skb, struct nf_conn *ct,
+				enum ip_conntrack_info ctinfo,
+				unsigned int protoff, unsigned int match_offset,
+				unsigned int match_len, const char *rep_buffer,
+				unsigned int rep_len, bool adjust);
 
-static inline int nf_nat_mangle_tcp_packet(struct sk_buff *skb,
-					   struct nf_conn *ct,
-					   enum ip_conntrack_info ctinfo,
-					   unsigned int protoff,
-					   unsigned int match_offset,
-					   unsigned int match_len,
-					   const char *rep_buffer,
-					   unsigned int rep_len)
+static inline bool nf_nat_mangle_tcp_packet(struct sk_buff *skb,
+					    struct nf_conn *ct,
+					    enum ip_conntrack_info ctinfo,
+					    unsigned int protoff,
+					    unsigned int match_offset,
+					    unsigned int match_len,
+					    const char *rep_buffer,
+					    unsigned int rep_len)
 {
 	return __nf_nat_mangle_tcp_packet(skb, ct, ctinfo, protoff,
 					  match_offset, match_len,
 					  rep_buffer, rep_len, true);
 }
 
-int nf_nat_mangle_udp_packet(struct sk_buff *skb, struct nf_conn *ct,
-			     enum ip_conntrack_info ctinfo,
-			     unsigned int protoff, unsigned int match_offset,
-			     unsigned int match_len, const char *rep_buffer,
-			     unsigned int rep_len);
+bool nf_nat_mangle_udp_packet(struct sk_buff *skb, struct nf_conn *ct,
+			      enum ip_conntrack_info ctinfo,
+			      unsigned int protoff, unsigned int match_offset,
+			      unsigned int match_len, const char *rep_buffer,
+			      unsigned int rep_len);
 
 /* Setup NAT on this expected conntrack so it follows master, but goes
  * to port ct->master->saved_proto. */
net/netfilter/nf_nat_helper.c +20 -20
index 211661c..607a373 100644
@@ -70,15 +70,15 @@ static void mangle_contents(struct sk_buff *skb,
 }
 
 /* Unusual, but possible case. */
-static int enlarge_skb(struct sk_buff *skb, unsigned int extra)
+static bool enlarge_skb(struct sk_buff *skb, unsigned int extra)
 {
 	if (skb->len + extra > 65535)
-		return 0;
+		return false;
 
 	if (pskb_expand_head(skb, 0, extra - skb_tailroom(skb), GFP_ATOMIC))
-		return 0;
+		return false;
 
-	return 1;
+	return true;
 }
 
 /* Generic function for mangling variable-length address changes inside

@@ -89,26 +89,26 @@ static int enlarge_skb(struct sk_buff *skb, unsigned int extra)
  * skb enlargement, ...
  *
  * */
-int __nf_nat_mangle_tcp_packet(struct sk_buff *skb,
-			       struct nf_conn *ct,
-			       enum ip_conntrack_info ctinfo,
-			       unsigned int protoff,
-			       unsigned int match_offset,
-			       unsigned int match_len,
-			       const char *rep_buffer,
-			       unsigned int rep_len, bool adjust)
+bool __nf_nat_mangle_tcp_packet(struct sk_buff *skb,
+				struct nf_conn *ct,
+				enum ip_conntrack_info ctinfo,
+				unsigned int protoff,
+				unsigned int match_offset,
+				unsigned int match_len,
+				const char *rep_buffer,
+				unsigned int rep_len, bool adjust)
 {
 	const struct nf_nat_l3proto *l3proto;
 	struct tcphdr *tcph;
 	int oldlen, datalen;
 
 	if (!skb_make_writable(skb, skb->len))
-		return 0;
+		return false;
 
 	if (rep_len > match_len &&
 	    rep_len - match_len > skb_tailroom(skb) &&
 	    !enlarge_skb(skb, rep_len - match_len))
-		return 0;
+		return false;
 
 	SKB_LINEAR_ASSERT(skb);
 

@@ -128,7 +128,7 @@ int __nf_nat_mangle_tcp_packet(struct sk_buff *skb,
 		nf_ct_seqadj_set(ct, ctinfo, tcph->seq,
 				 (int)rep_len - (int)match_len);
 
-	return 1;
+	return true;
 }
 EXPORT_SYMBOL(__nf_nat_mangle_tcp_packet);
 

@@ -142,7 +142,7 @@ int __nf_nat_mangle_tcp_packet(struct sk_buff *skb,
  * XXX - This function could be merged with nf_nat_mangle_tcp_packet which
  *       should be fairly easy to do.
  */
-int
+bool
 nf_nat_mangle_udp_packet(struct sk_buff *skb,
 			 struct nf_conn *ct,
 			 enum ip_conntrack_info ctinfo,

@@ -157,12 +157,12 @@ int __nf_nat_mangle_tcp_packet(struct sk_buff *skb,
 	int datalen, oldlen;
 
 	if (!skb_make_writable(skb, skb->len))
-		return 0;
+		return false;
 
 	if (rep_len > match_len &&
 	    rep_len - match_len > skb_tailroom(skb) &&
 	    !enlarge_skb(skb, rep_len - match_len))
-		return 0;
+		return false;
 
 	udph = (void *)skb->data + protoff;
 

@@ -176,13 +176,13 @@ int __nf_nat_mangle_tcp_packet(struct sk_buff *skb,
 
 	/* fix udp checksum if udp checksum was previously calculated */
 	if (!udph->check && skb->ip_summed != CHECKSUM_PARTIAL)
-		return 1;
+		return true;
 
 	l3proto = __nf_nat_l3proto_find(nf_ct_l3num(ct));
 	l3proto->csum_recalc(skb, IPPROTO_UDP, udph, &udph->check,
 			     datalen, oldlen);
 
-	return 1;
+	return true;
 }
 EXPORT_SYMBOL(nf_nat_mangle_udp_packet);
Signature · 1 line
1.9.1
author
#6 lore raw

PATCH 4/4 netfilter: sip: Use NF_DROP and NF_ACCEPT instead of 0 and 1

In reply to [email protected]

From: Gao Feng <[email protected]>

The current sip codes should use NF_DROP and NF_ACCEPT instead of the literal number 0 and 1.

Signed-off-by Gao Feng <[email protected]>
---
 net/netfilter/nf_nat_sip.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
net/netfilter/nf_nat_sip.c +3 -3
index 791fac4..a093979 100644
@@ -48,7 +48,7 @@ static unsigned int mangle_packet(struct sk_buff *skb, unsigned int protoff,
 		if (!__nf_nat_mangle_tcp_packet(skb, ct, ctinfo,
 						protoff, matchoff, matchlen,
 						buffer, buflen, false))
-			return 0;
+			return NF_DROP;
 	} else {
 		baseoff = protoff + sizeof(struct udphdr);
 		matchoff += dataoff - baseoff;

@@ -56,13 +56,13 @@ static unsigned int mangle_packet(struct sk_buff *skb, unsigned int protoff,
 		if (!nf_nat_mangle_udp_packet(skb, ct, ctinfo,
 					      protoff, matchoff, matchlen,
 					      buffer, buflen))
-			return 0;
+			return NF_DROP;
 	}
 
 	/* Reload data pointer and adjust datalen value */
 	*dptr = skb->data + dataoff;
 	*datalen += buflen - matchlen;
-	return 1;
+	return NF_ACCEPT;
 }
 
 static int sip_sprintf_addr(const struct nf_conn *ct, char *buffer,
Signature · 1 line
1.9.1