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

Message ID fdfd3853391d136ba390dfdc4dc6884f552e540e.1489726438.git.fgao@ikuai8.com
State Changes Requested
Delegated to: Pablo Neira
Headers show

Commit Message

高峰 March 17, 2017, 6:47 a.m.
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(-)

Comments

Pablo Neira Ayuso March 27, 2017, 12:12 p.m. | #1
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...
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Gao Feng March 27, 2017, 12:48 p.m. | #2
> -----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



--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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)