Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH net v2] net: netfilter: Fix port selection of FTP for NF_NAT_RANGE_PROTO_SPECIFIED
@ 2021-09-07  2:14 Cole Dishington
  2021-09-07  5:14 ` kernel test robot
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Cole Dishington @ 2021-09-07  2:14 UTC (permalink / raw)
  To: pablo, kadlec, fw, davem, kuba, shuah
  Cc: linux-kernel, netfilter-devel, coreteam, netdev, Cole Dishington,
	Anthony Lineham, Scott Parlane, Blair Steven

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net v2] net: netfilter: Fix port selection of FTP for NF_NAT_RANGE_PROTO_SPECIFIED
  2021-09-07  2:14 [PATCH net v2] net: netfilter: Fix port selection of FTP for NF_NAT_RANGE_PROTO_SPECIFIED Cole Dishington
@ 2021-09-07  5:14 ` kernel test robot
  2021-09-07 13:54 ` Florian Westphal
  2021-09-07 14:11 ` kernel test robot
  2 siblings, 0 replies; 7+ messages in thread
From: kernel test robot @ 2021-09-07  5:14 UTC (permalink / raw)
  To: Cole Dishington, pablo, kadlec, fw, davem, kuba, shuah
  Cc: llvm, kbuild-all, linux-kernel, netfilter-devel, coreteam, netdev

[-- 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 --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net v2] net: netfilter: Fix port selection of FTP for NF_NAT_RANGE_PROTO_SPECIFIED
  2021-09-07  2:14 [PATCH net v2] net: netfilter: Fix port selection of FTP for NF_NAT_RANGE_PROTO_SPECIFIED Cole Dishington
  2021-09-07  5:14 ` kernel test robot
@ 2021-09-07 13:54 ` Florian Westphal
  2021-09-07 15:11   ` Jan Engelhardt
  2021-09-07 14:11 ` kernel test robot
  2 siblings, 1 reply; 7+ messages in thread
From: Florian Westphal @ 2021-09-07 13:54 UTC (permalink / raw)
  To: Cole Dishington
  Cc: pablo, kadlec, fw, davem, kuba, shuah, linux-kernel,
	netfilter-devel, coreteam, netdev, Anthony Lineham,
	Scott Parlane, Blair Steven

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net v2] net: netfilter: Fix port selection of FTP for NF_NAT_RANGE_PROTO_SPECIFIED
  2021-09-07  2:14 [PATCH net v2] net: netfilter: Fix port selection of FTP for NF_NAT_RANGE_PROTO_SPECIFIED Cole Dishington
  2021-09-07  5:14 ` kernel test robot
  2021-09-07 13:54 ` Florian Westphal
@ 2021-09-07 14:11 ` kernel test robot
  2 siblings, 0 replies; 7+ messages in thread
From: kernel test robot @ 2021-09-07 14:11 UTC (permalink / raw)
  To: Cole Dishington, pablo, kadlec, fw, davem, kuba, shuah
  Cc: kbuild-all, linux-kernel, netfilter-devel, coreteam, netdev

[-- 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 --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net v2] net: netfilter: Fix port selection of FTP for NF_NAT_RANGE_PROTO_SPECIFIED
  2021-09-07 13:54 ` Florian Westphal
@ 2021-09-07 15:11   ` Jan Engelhardt
  2021-09-08  2:22     ` Duncan Roe
  0 siblings, 1 reply; 7+ messages in thread
From: Jan Engelhardt @ 2021-09-07 15:11 UTC (permalink / raw)
  To: Florian Westphal
  Cc: Cole Dishington, pablo, kadlec, davem, kuba, shuah, linux-kernel,
	netfilter-devel, coreteam, netdev, Anthony Lineham,
	Scott Parlane, Blair Steven


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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net v2] net: netfilter: Fix port selection of FTP for NF_NAT_RANGE_PROTO_SPECIFIED
  2021-09-07 15:11   ` Jan Engelhardt
@ 2021-09-08  2:22     ` Duncan Roe
  2021-09-08  6:52       ` Jan Engelhardt
  0 siblings, 1 reply; 7+ messages in thread
From: Duncan Roe @ 2021-09-08  2:22 UTC (permalink / raw)
  To: Jan Engelhardt
  Cc: Florian Westphal, Cole Dishington, pablo, kadlec, davem, kuba,
	shuah, linux-kernel, netfilter-devel, coreteam, netdev,
	Anthony Lineham, Scott Parlane, Blair Steven

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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net v2] net: netfilter: Fix port selection of FTP for NF_NAT_RANGE_PROTO_SPECIFIED
  2021-09-08  2:22     ` Duncan Roe
@ 2021-09-08  6:52       ` Jan Engelhardt
  0 siblings, 0 replies; 7+ messages in thread
From: Jan Engelhardt @ 2021-09-08  6:52 UTC (permalink / raw)
  To: Duncan Roe
  Cc: Florian Westphal, Cole Dishington, pablo, kadlec, davem, kuba,
	shuah, linux-kernel, netfilter-devel, coreteam, netdev,
	Anthony Lineham, Scott Parlane, Blair Steven


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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-09-08  6:53 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-07  2:14 [PATCH net v2] net: netfilter: Fix port selection of FTP for NF_NAT_RANGE_PROTO_SPECIFIED Cole Dishington
2021-09-07  5:14 ` kernel test robot
2021-09-07 13:54 ` Florian Westphal
2021-09-07 15:11   ` Jan Engelhardt
2021-09-08  2:22     ` Duncan Roe
2021-09-08  6:52       ` Jan Engelhardt
2021-09-07 14:11 ` kernel test robot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).