[PATCH net v2] net: netfilter: Fix port selection of FTP for NF_NAT_RANGE_PROTO_SPECIFIED

[PATCH net v2] net: netfilter: Fix port selection of FTP for NF_NAT_RANGE_PROTO_SPECIFIED

[Cole Dishington]

FTP port selection ignores specified port ranges (with iptables
masquerade --to-ports) when creating an expectation, based on
FTP commands PORT or PASV, for the data connection.

Co-developed-by: Anthony Lineham <anthony.lineham@alliedtelesis.co.nz>
Signed-off-by: Anthony Lineham <anthony.lineham@alliedtelesis.co.nz>
Co-developed-by: Scott Parlane <scott.parlane@alliedtelesis.co.nz>
Signed-off-by: Scott Parlane <scott.parlane@alliedtelesis.co.nz>
Co-developed-by: Blair Steven <blair.steven@alliedtelesis.co.nz>
Signed-off-by: Blair Steven <blair.steven@alliedtelesis.co.nz>
Signed-off-by: Cole Dishington <Cole.Dishington@alliedtelesis.co.nz>
---

Notes:
    Thanks for your time reviewing!
    
    Changes:
    - Avoid storing full range structure, only store min_proto and max_proto.
    - Store min and max_proto on nf_conn_nat rather than nf_conn.
    - Only store extra range info on nf_conn if NF_NAT_RANGE_PROTO_SPECIFIED and
      fallback to selecting from full port range.
    - Try to get same port if it matches the range.
    - Added net tag to subject.

 include/net/netfilter/nf_nat.h |  6 +++++
 net/netfilter/nf_nat_core.c    | 17 +++++++++----
 net/netfilter/nf_nat_ftp.c     | 44 +++++++++++++++++++++++++---------
 net/netfilter/nf_nat_helper.c  | 10 ++++++++
 4 files changed, 62 insertions(+), 15 deletions(-)

diff --git a/include/net/netfilter/nf_nat.h b/include/net/netfilter/nf_nat.h
index 0d412dd63707..231cffc16722 100644
--- a/include/net/netfilter/nf_nat.h
+++ b/include/net/netfilter/nf_nat.h
@@ -27,12 +27,18 @@ union nf_conntrack_nat_help {
 #endif
 };
 
+struct nf_conn_nat_range_info {
+	union nf_conntrack_man_proto    min_proto;
+	union nf_conntrack_man_proto    max_proto;
+};
+
 /* The structure embedded in the conntrack structure. */
 struct nf_conn_nat {
 	union nf_conntrack_nat_help help;
 #if IS_ENABLED(CONFIG_NF_NAT_MASQUERADE)
 	int masq_index;
 #endif
+	struct nf_conn_nat_range_info range_info;
 };
 
 /* Set up the info structure to map into this range. */
diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c
index ea923f8cf9c4..5412e9cf8189 100644
--- a/net/netfilter/nf_nat_core.c
+++ b/net/netfilter/nf_nat_core.c
@@ -397,10 +397,10 @@ find_best_ips_proto(const struct nf_conntrack_zone *zone,
  *
  * Per-protocol part of tuple is initialized to the incoming packet.
  */
-static void nf_nat_l4proto_unique_tuple(struct nf_conntrack_tuple *tuple,
-					const struct nf_nat_range2 *range,
-					enum nf_nat_manip_type maniptype,
-					const struct nf_conn *ct)
+void nf_nat_l4proto_unique_tuple(struct nf_conntrack_tuple *tuple,
+				 const struct nf_nat_range2 *range,
+				 enum nf_nat_manip_type maniptype,
+				 const struct nf_conn *ct)
 {
 	unsigned int range_size, min, max, i, attempts;
 	__be16 *keyptr;
@@ -601,6 +601,7 @@ nf_nat_setup_info(struct nf_conn *ct,
 		  const struct nf_nat_range2 *range,
 		  enum nf_nat_manip_type maniptype)
 {
+	struct nf_conn_nat *nat;
 	struct net *net = nf_ct_net(ct);
 	struct nf_conntrack_tuple curr_tuple, new_tuple;
 
@@ -623,6 +624,14 @@ nf_nat_setup_info(struct nf_conn *ct,
 			   &ct->tuplehash[IP_CT_DIR_REPLY].tuple);
 
 	get_unique_tuple(&new_tuple, &curr_tuple, range, ct, maniptype);
+	if (range && (range->flags & NF_NAT_RANGE_PROTO_SPECIFIED)) {
+		nat = nf_ct_nat_ext_add(ct);
+		if (WARN_ON_ONCE(!nat))
+			return NF_DROP;
+
+		nat->range_info.min_proto = range->min_proto;
+		nat->range_info.max_proto = range->max_proto;
+	}
 
 	if (!nf_ct_tuple_equal(&new_tuple, &curr_tuple)) {
 		struct nf_conntrack_tuple reply;
diff --git a/net/netfilter/nf_nat_ftp.c b/net/netfilter/nf_nat_ftp.c
index aace6768a64e..cf675dc589be 100644
--- a/net/netfilter/nf_nat_ftp.c
+++ b/net/netfilter/nf_nat_ftp.c
@@ -17,6 +17,10 @@
 #include <net/netfilter/nf_conntrack_helper.h>
 #include <net/netfilter/nf_conntrack_expect.h>
 #include <linux/netfilter/nf_conntrack_ftp.h>
+void nf_nat_l4proto_unique_tuple(struct nf_conntrack_tuple *tuple,
+				 const struct nf_nat_range2 *range,
+				 enum nf_nat_manip_type maniptype,
+				 const struct nf_conn *ct);
 
 #define NAT_HELPER_NAME "ftp"
 
@@ -72,8 +76,14 @@ static unsigned int nf_nat_ftp(struct sk_buff *skb,
 	u_int16_t port;
 	int dir = CTINFO2DIR(ctinfo);
 	struct nf_conn *ct = exp->master;
+	struct nf_conn_nat *nat = nfct_nat(ct);
+	struct nf_nat_range2 range = {};
 	char buffer[sizeof("|1||65535|") + INET6_ADDRSTRLEN];
 	unsigned int buflen;
+	int ret;
+
+	if (WARN_ON_ONCE(!nat))
+		return NF_DROP;
 
 	pr_debug("type %i, off %u len %u\n", type, matchoff, matchlen);
 
@@ -86,21 +96,33 @@ static unsigned int nf_nat_ftp(struct sk_buff *skb,
 	 * this one. */
 	exp->expectfn = nf_nat_follow_master;
 
-	/* Try to get same port: if not, try to change it. */
-	for (port = ntohs(exp->saved_proto.tcp.port); port != 0; port++) {
-		int ret;
+	if (htons(nat->range_info.min_proto.all) == 0 ||
+	    htons(nat->range_info.max_proto.all) == 0) {
+		range.min_proto.all = htons(1);
+		range.max_proto.all = htons(65535);
+	} else {
+		range.min_proto     = nat->range_info.min_proto;
+		range.max_proto     = nat->range_info.max_proto;
+	}
+	range.flags                 = NF_NAT_RANGE_PROTO_SPECIFIED;
 
-		exp->tuple.dst.u.tcp.port = htons(port);
+	/* Try to get same port if it matches NAT rule: if not, try to change it. */
+	ret = -1;
+	port = ntohs(exp->tuple.dst.u.tcp.port);
+	if (port != 0 && ntohs(range.min_proto.all) <= port &&
+	    port <= ntohs(range.max_proto.all)) {
 		ret = nf_ct_expect_related(exp, 0);
-		if (ret == 0)
-			break;
-		else if (ret != -EBUSY) {
-			port = 0;
-			break;
+	}
+	if (ret != 0 || port == 0) {
+		if (!dir) {
+			nf_nat_l4proto_unique_tuple(&exp->tuple, &range,
+						    NF_NAT_MANIP_DST,
+						    ct);
 		}
+		port = ntohs(exp->tuple.dst.u.tcp.port);
+		ret = nf_ct_expect_related(exp, 0);
 	}
-
-	if (port == 0) {
+	if (ret != 0 || port == 0) {
 		nf_ct_helper_log(skb, ct, "all ports in use");
 		return NF_DROP;
 	}
diff --git a/net/netfilter/nf_nat_helper.c b/net/netfilter/nf_nat_helper.c
index a263505455fc..29fa78cfdb9c 100644
--- a/net/netfilter/nf_nat_helper.c
+++ b/net/netfilter/nf_nat_helper.c
@@ -188,6 +188,16 @@ void nf_nat_follow_master(struct nf_conn *ct,
 	range.flags = NF_NAT_RANGE_MAP_IPS;
 	range.min_addr = range.max_addr
 		= ct->master->tuplehash[!exp->dir].tuple.dst.u3;
+	if (exp->master && !exp->dir) {
+		struct nf_conn_nat *nat = nfct_nat(exp->master);
+
+		if (nat && nat->range_info.min_proto.all != 0 &&
+		    nat->range_info.max_proto.all != 0) {
+			range.min_proto = nat->range_info.min_proto;
+			range.max_proto = nat->range_info.max_proto;
+			range.flags |= NF_NAT_RANGE_PROTO_SPECIFIED;
+		}
+	}
 	nf_nat_setup_info(ct, &range, NF_NAT_MANIP_SRC);
 
 	/* For DST manip, map port here to where it's expected. */
-- 
2.33.0

Re: [PATCH net v2] net: netfilter: Fix port selection of FTP for NF_NAT_RANGE_PROTO_SPECIFIED

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 6053 bytes --]

Hi Cole,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on net/master]

url:    https://github.com/0day-ci/linux/commits/Cole-Dishington/net-netfilter-Fix-port-selection-of-FTP-for-NF_NAT_RANGE_PROTO_SPECIFIED/20210907-101823
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git b539c44df067ac116ec1b58b956efda51b6a7fc1
config: arm-randconfig-r003-20210906 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 9c476172b93367d2cb88d7d3f4b1b5b456fa6020)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm cross compiling tool for clang build
        # apt-get install binutils-arm-linux-gnueabi
        # https://github.com/0day-ci/linux/commit/3d790f5d7c3d6069948749b4697090adfcc48e51
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Cole-Dishington/net-netfilter-Fix-port-selection-of-FTP-for-NF_NAT_RANGE_PROTO_SPECIFIED/20210907-101823
        git checkout 3d790f5d7c3d6069948749b4697090adfcc48e51
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> net/netfilter/nf_nat_core.c:373:6: warning: no previous prototype for function 'nf_nat_l4proto_unique_tuple' [-Wmissing-prototypes]
   void nf_nat_l4proto_unique_tuple(struct nf_conntrack_tuple *tuple,
        ^
   net/netfilter/nf_nat_core.c:373:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   void nf_nat_l4proto_unique_tuple(struct nf_conntrack_tuple *tuple,
   ^
   static 
   1 warning generated.


vim +/nf_nat_l4proto_unique_tuple +373 net/netfilter/nf_nat_core.c

   367	
   368	/* Alter the per-proto part of the tuple (depending on maniptype), to
   369	 * give a unique tuple in the given range if possible.
   370	 *
   371	 * Per-protocol part of tuple is initialized to the incoming packet.
   372	 */
 > 373	void nf_nat_l4proto_unique_tuple(struct nf_conntrack_tuple *tuple,
   374					 const struct nf_nat_range2 *range,
   375					 enum nf_nat_manip_type maniptype,
   376					 const struct nf_conn *ct)
   377	{
   378		unsigned int range_size, min, max, i, attempts;
   379		__be16 *keyptr;
   380		u16 off;
   381		static const unsigned int max_attempts = 128;
   382	
   383		switch (tuple->dst.protonum) {
   384		case IPPROTO_ICMP:
   385		case IPPROTO_ICMPV6:
   386			/* id is same for either direction... */
   387			keyptr = &tuple->src.u.icmp.id;
   388			if (!(range->flags & NF_NAT_RANGE_PROTO_SPECIFIED)) {
   389				min = 0;
   390				range_size = 65536;
   391			} else {
   392				min = ntohs(range->min_proto.icmp.id);
   393				range_size = ntohs(range->max_proto.icmp.id) -
   394					     ntohs(range->min_proto.icmp.id) + 1;
   395			}
   396			goto find_free_id;
   397	#if IS_ENABLED(CONFIG_NF_CT_PROTO_GRE)
   398		case IPPROTO_GRE:
   399			/* If there is no master conntrack we are not PPTP,
   400			   do not change tuples */
   401			if (!ct->master)
   402				return;
   403	
   404			if (maniptype == NF_NAT_MANIP_SRC)
   405				keyptr = &tuple->src.u.gre.key;
   406			else
   407				keyptr = &tuple->dst.u.gre.key;
   408	
   409			if (!(range->flags & NF_NAT_RANGE_PROTO_SPECIFIED)) {
   410				min = 1;
   411				range_size = 65535;
   412			} else {
   413				min = ntohs(range->min_proto.gre.key);
   414				range_size = ntohs(range->max_proto.gre.key) - min + 1;
   415			}
   416			goto find_free_id;
   417	#endif
   418		case IPPROTO_UDP:
   419		case IPPROTO_UDPLITE:
   420		case IPPROTO_TCP:
   421		case IPPROTO_SCTP:
   422		case IPPROTO_DCCP:
   423			if (maniptype == NF_NAT_MANIP_SRC)
   424				keyptr = &tuple->src.u.all;
   425			else
   426				keyptr = &tuple->dst.u.all;
   427	
   428			break;
   429		default:
   430			return;
   431		}
   432	
   433		/* If no range specified... */
   434		if (!(range->flags & NF_NAT_RANGE_PROTO_SPECIFIED)) {
   435			/* If it's dst rewrite, can't change port */
   436			if (maniptype == NF_NAT_MANIP_DST)
   437				return;
   438	
   439			if (ntohs(*keyptr) < 1024) {
   440				/* Loose convention: >> 512 is credential passing */
   441				if (ntohs(*keyptr) < 512) {
   442					min = 1;
   443					range_size = 511 - min + 1;
   444				} else {
   445					min = 600;
   446					range_size = 1023 - min + 1;
   447				}
   448			} else {
   449				min = 1024;
   450				range_size = 65535 - 1024 + 1;
   451			}
   452		} else {
   453			min = ntohs(range->min_proto.all);
   454			max = ntohs(range->max_proto.all);
   455			if (unlikely(max < min))
   456				swap(max, min);
   457			range_size = max - min + 1;
   458		}
   459	
   460	find_free_id:
   461		if (range->flags & NF_NAT_RANGE_PROTO_OFFSET)
   462			off = (ntohs(*keyptr) - ntohs(range->base_proto.all));
   463		else
   464			off = prandom_u32();
   465	
   466		attempts = range_size;
   467		if (attempts > max_attempts)
   468			attempts = max_attempts;
   469	
   470		/* We are in softirq; doing a search of the entire range risks
   471		 * soft lockup when all tuples are already used.
   472		 *
   473		 * If we can't find any free port from first offset, pick a new
   474		 * one and try again, with ever smaller search window.
   475		 */
   476	another_round:
   477		for (i = 0; i < attempts; i++, off++) {
   478			*keyptr = htons(min + off % range_size);
   479			if (!nf_nat_used_tuple(tuple, ct))
   480				return;
   481		}
   482	
   483		if (attempts >= range_size || attempts < 16)
   484			return;
   485		attempts /= 2;
   486		off = prandom_u32();
   487		goto another_round;
   488	}
   489	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 26493 bytes --]

Re: [PATCH net v2] net: netfilter: Fix port selection of FTP for NF_NAT_RANGE_PROTO_SPECIFIED

[Florian Westphal]

Cole Dishington <Cole.Dishington@alliedtelesis.co.nz> wrote:
> index aace6768a64e..cf675dc589be 100644
> --- a/net/netfilter/nf_nat_ftp.c
> +++ b/net/netfilter/nf_nat_ftp.c
> @@ -17,6 +17,10 @@
>  #include <net/netfilter/nf_conntrack_helper.h>
>  #include <net/netfilter/nf_conntrack_expect.h>
>  #include <linux/netfilter/nf_conntrack_ftp.h>
> +void nf_nat_l4proto_unique_tuple(struct nf_conntrack_tuple *tuple,
> +				 const struct nf_nat_range2 *range,
> +				 enum nf_nat_manip_type maniptype,
> +				 const struct nf_conn *ct);

Please add this to a header, e.g. include/net/netfilter/nf_nat.h.

> -	/* Try to get same port: if not, try to change it. */
> -	for (port = ntohs(exp->saved_proto.tcp.port); port != 0; port++) {
> -		int ret;
> +	if (htons(nat->range_info.min_proto.all) == 0 ||
> +	    htons(nat->range_info.max_proto.all) == 0) {

Either use if (nat->range_info.min_proto.all || ...

or use ntohs().  I will leave it up to you if you prefer
ntohs(nat->range_info.min_proto.all) == 0 or
nat->range_info.min_proto.all == ntohs(0).

(Use of htons here will trigger endian warnings from sparse tool).

> -		exp->tuple.dst.u.tcp.port = htons(port);
> +	/* Try to get same port if it matches NAT rule: if not, try to change it. */
> +	ret = -1;
> +	port = ntohs(exp->tuple.dst.u.tcp.port);
> +	if (port != 0 && ntohs(range.min_proto.all) <= port &&
> +	    port <= ntohs(range.max_proto.all)) {
>  		ret = nf_ct_expect_related(exp, 0);
> -		if (ret == 0)
> -			break;
> -		else if (ret != -EBUSY) {
> -			port = 0;
> -			break;
> +	}
> +	if (ret != 0 || port == 0) {
> +		if (!dir) {
> +			nf_nat_l4proto_unique_tuple(&exp->tuple, &range,
> +						    NF_NAT_MANIP_DST,
> +						    ct);

A small comment that explains why nf_nat_l4proto_unique_tuple() is
called conditionally would be good.

I don't understand this new logic, can you explain?

Old:

for (port = expr>tuple.port ; port > 0 ;port++)
    nf_ct_expect_related(exp, 0);
    if (success || fatal_error)
	 break;

New:
port = exp->tuple.port;
if (port && min <= port && port <= max) // in which case is port 0 here?
	ret = nf_ct_expect_related();

if (fatal_error || port == 0)	// how can port be 0?
    if (!dir) {
	    nf_nat_l4proto_unique_tuple();
            ret = nf_ct_expect_related();
    }
}

How can this work?  This removes the loop and relies on
nf_nat_l4proto_unique_tuple(), but NF_NAT_MANIP_DST doesn't support
port rewrite in !NF_NAT_RANGE_PROTO_SPECIFIED case.

Plus, it restricts nf_nat_l4proto_unique_tuple to !dir case, which
I don't understand either.

> +		port = ntohs(exp->tuple.dst.u.tcp.port);
> +		ret = nf_ct_expect_related(exp, 0);
>  	}
> -
> -	if (port == 0) {
> +	if (ret != 0 || port == 0) {

How can port be 0?  In the old code, it becomes 0 if all attempts
to find unused port failed, but after the rewrite I don't see how it can
happen.

> @@ -188,6 +188,16 @@ void nf_nat_follow_master(struct nf_conn *ct,
>  	range.flags = NF_NAT_RANGE_MAP_IPS;
>  	range.min_addr = range.max_addr
>  		= ct->master->tuplehash[!exp->dir].tuple.dst.u3;
> +	if (exp->master && !exp->dir) {

AFAIK exp->master can't be NULL.

> +		struct nf_conn_nat *nat = nfct_nat(exp->master);
> +
> +		if (nat && nat->range_info.min_proto.all != 0 &&
> +		    nat->range_info.max_proto.all != 0) {
> +			range.min_proto = nat->range_info.min_proto;
> +			range.max_proto = nat->range_info.max_proto;
> +			range.flags |= NF_NAT_RANGE_PROTO_SPECIFIED;
> +		}
> +	}

!expr->dir means REPLY, i.e. new connection is reversed compared
to the master connection (from responder back to initiator).

So, why are we munging range in this case?

I would have expected exp->dir == IP_CT_DIR_ORIGINAL for your use case
(original connection subject to masquerade and source ports mangled to
 fall into special range, so related conntion should also be mangled
 to match said range).

Re: [PATCH net v2] net: netfilter: Fix port selection of FTP for NF_NAT_RANGE_PROTO_SPECIFIED

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 1508 bytes --]

Hi Cole,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net/master]

url:    https://github.com/0day-ci/linux/commits/Cole-Dishington/net-netfilter-Fix-port-selection-of-FTP-for-NF_NAT_RANGE_PROTO_SPECIFIED/20210907-101823
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git b539c44df067ac116ec1b58b956efda51b6a7fc1
config: s390-defconfig (attached as .config)
compiler: s390-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/3d790f5d7c3d6069948749b4697090adfcc48e51
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Cole-Dishington/net-netfilter-Fix-port-selection-of-FTP-for-NF_NAT_RANGE_PROTO_SPECIFIED/20210907-101823
        git checkout 3d790f5d7c3d6069948749b4697090adfcc48e51
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=s390 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "nf_nat_l4proto_unique_tuple" [net/netfilter/nf_nat_ftp.ko] undefined!

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 20100 bytes --]

Re: [PATCH net v2] net: netfilter: Fix port selection of FTP for NF_NAT_RANGE_PROTO_SPECIFIED

[Jan Engelhardt]

On Tuesday 2021-09-07 15:54, Florian Westphal wrote:
>> -	/* Try to get same port: if not, try to change it. */
>> -	for (port = ntohs(exp->saved_proto.tcp.port); port != 0; port++) {
>> -		int ret;
>> +	if (htons(nat->range_info.min_proto.all) == 0 ||
>> +	    htons(nat->range_info.max_proto.all) == 0) {
>
>Either use if (nat->range_info.min_proto.all || ...
>
>or use ntohs().  I will leave it up to you if you prefer
>ntohs(nat->range_info.min_proto.all) == 0 or
>nat->range_info.min_proto.all == ntohs(0).

If one has the option, one should always prefer to put htons/htonl on 
the side with the constant literal;
Propagation of constants and compile-time evaluation is the target.

That works for some other functions as well (e.g. 
strlen("fixedstring")).

Re: [PATCH net v2] net: netfilter: Fix port selection of FTP for NF_NAT_RANGE_PROTO_SPECIFIED

[Duncan Roe]

On Tue, Sep 07, 2021 at 05:11:42PM +0200, Jan Engelhardt wrote:
>
> On Tuesday 2021-09-07 15:54, Florian Westphal wrote:
> >> -	/* Try to get same port: if not, try to change it. */
> >> -	for (port = ntohs(exp->saved_proto.tcp.port); port != 0; port++) {
> >> -		int ret;
> >> +	if (htons(nat->range_info.min_proto.all) == 0 ||
> >> +	    htons(nat->range_info.max_proto.all) == 0) {
> >
> >Either use if (nat->range_info.min_proto.all || ...
> >
> >or use ntohs().  I will leave it up to you if you prefer
> >ntohs(nat->range_info.min_proto.all) == 0 or
> >nat->range_info.min_proto.all == ntohs(0).
>
> If one has the option, one should always prefer to put htons/htonl on
> the side with the constant literal;
> Propagation of constants and compile-time evaluation is the target.
>
> That works for some other functions as well (e.g.
> strlen("fixedstring")).

When comparing against constant zero, why use htons/htonl at all?

Cheers ... Duncan.

Re: [PATCH net v2] net: netfilter: Fix port selection of FTP for NF_NAT_RANGE_PROTO_SPECIFIED

[Jan Engelhardt]

On Wednesday 2021-09-08 04:22, Duncan Roe wrote:
>> >Either use if (nat->range_info.min_proto.all || ...
>> >
>> >or use ntohs().  I will leave it up to you if you prefer
>> >ntohs(nat->range_info.min_proto.all) == 0 or
>> >nat->range_info.min_proto.all == ntohs(0).
>>
>> If one has the option, one should always prefer to put htons/htonl on
>> the side with the constant literal;
>> Propagation of constants and compile-time evaluation is the target.
>>
>> That works for some other functions as well (e.g.
>> strlen("fixedstring")).
>
>When comparing against constant zero, why use htons/htonl at all?

Logical correctness.
Remember, it was the sparse tool that complained in the first place.