[PATCH nf-next 0/4] Refine the nat helper codes

[PATCH nf-next 0/4] Refine the nat helper codes

[fgao]

From: Gao Feng <fgao@ikuai8.com>

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(-)

-- 
1.9.1

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

[fgao]

From: Gao Feng <fgao@ikuai8.com>

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 <fgao@ikuai8.com>
---
 net/netfilter/nf_nat_amanda.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/net/netfilter/nf_nat_amanda.c b/net/netfilter/nf_nat_amanda.c
index eb77238..e4d61a7 100644
--- a/net/netfilter/nf_nat_amanda.c
+++ b/net/netfilter/nf_nat_amanda.c
@@ -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)
-- 
1.9.1

[PATCH nf-next 2/4] netfilter: irc: Correct the return value comparison of the func nf_nat_mangle_tcp_packet

[fgao]

From: Gao Feng <fgao@ikuai8.com>

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 <fgao@ikuai8.com>
---
 net/netfilter/nf_nat_irc.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/net/netfilter/nf_nat_irc.c b/net/netfilter/nf_nat_irc.c
index 1fb2258..0648cb0 100644
--- a/net/netfilter/nf_nat_irc.c
+++ b/net/netfilter/nf_nat_irc.c
@@ -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)
-- 
1.9.1

[PATCH nf-next 3/4] netfilter: helper: Use the bool instead of int type

[fgao]

From: Gao Feng <fgao@ikuai8.com>

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

Signed-off-by: Gao Feng <fgao@ikuai8.com>
---
 include/net/netfilter/nf_nat_helper.h | 36 +++++++++++++++----------------
 net/netfilter/nf_nat_helper.c         | 40 +++++++++++++++++------------------
 2 files changed, 38 insertions(+), 38 deletions(-)

diff --git a/include/net/netfilter/nf_nat_helper.h b/include/net/netfilter/nf_nat_helper.h
index 01bcc6b..fbfa5ac 100644
--- a/include/net/netfilter/nf_nat_helper.h
+++ b/include/net/netfilter/nf_nat_helper.h
@@ -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. */
diff --git a/net/netfilter/nf_nat_helper.c b/net/netfilter/nf_nat_helper.c
index 211661c..607a373 100644
--- a/net/netfilter/nf_nat_helper.c
+++ b/net/netfilter/nf_nat_helper.c
@@ -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);
 
-- 
1.9.1

[PATCH nf-next 4/4] netfilter: sip: Use NF_DROP and NF_ACCEPT instead of 0 and 1

[fgao]

From: Gao Feng <fgao@ikuai8.com>

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

Signed-off-by: Gao Feng <fgao@ikuai8.com>
---
 net/netfilter/nf_nat_sip.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/netfilter/nf_nat_sip.c b/net/netfilter/nf_nat_sip.c
index 791fac4..a093979 100644
--- a/net/netfilter/nf_nat_sip.c
+++ b/net/netfilter/nf_nat_sip.c
@@ -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,
-- 
1.9.1

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

[Pablo Neira Ayuso]

On Fri, Mar 17, 2017 at 02:47:19PM +0800, fgao@ikuai8.com wrote:
> From: Gao Feng <fgao@ikuai8.com>
> 
> 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 <fgao@ikuai8.com>
> ---
>  net/netfilter/nf_nat_amanda.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/net/netfilter/nf_nat_amanda.c b/net/netfilter/nf_nat_amanda.c
> index eb77238..e4d61a7 100644
> --- a/net/netfilter/nf_nat_amanda.c
> +++ b/net/netfilter/nf_nat_amanda.c
> @@ -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;

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...

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

[Gao Feng]

> -----Original Message-----
> From: Pablo Neira Ayuso [mailto:pablo@netfilter.org]
> Sent: Monday, March 27, 2017 8:13 PM
> To: fgao@ikuai8.com
> Cc: netfilter-devel@vger.kernel.org; gfree.wind@gmail.com
> Subject: Re: [PATCH nf-next 1/4] netfilter: amanda: Correct the return
value
> comparison of the func nf_nat_mangle_udp_packet
> 
> On Fri, Mar 17, 2017 at 02:47:19PM +0800, fgao@ikuai8.com wrote:
> > From: Gao Feng <fgao@ikuai8.com>
> >
> > 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 <fgao@ikuai8.com>
> > ---
> >  net/netfilter/nf_nat_amanda.c | 11 +++++------
> >  1 file changed, 5 insertions(+), 6 deletions(-)
> >
> > diff --git a/net/netfilter/nf_nat_amanda.c
> > b/net/netfilter/nf_nat_amanda.c index eb77238..e4d61a7 100644
> > --- a/net/netfilter/nf_nat_amanda.c
> > +++ b/net/netfilter/nf_nat_amanda.c
> > @@ -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;
> 
> 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...
Ok, I would merge them into one patch.
I already checked them before, but I would check it again after merge.

Regards
Feng