Message ID | 20210907021415.962-1-Cole.Dishington@alliedtelesis.co.nz (mailing list archive) |
---|---|
State | Superseded |
Delegated to: | Netdev Maintainers |
Headers | show |
Series | [net,v2] net: netfilter: Fix port selection of FTP for NF_NAT_RANGE_PROTO_SPECIFIED | expand |
Context | Check | Description |
---|---|---|
netdev/cover_letter | success | Link |
netdev/fixes_present | fail | Series targets non-next tree, but doesn't contain any Fixes tags |
netdev/patch_count | success | Link |
netdev/tree_selection | success | Clearly marked for net |
netdev/subject_prefix | success | Link |
netdev/cc_maintainers | success | CCed 8 of 8 maintainers |
netdev/source_inline | success | Was 0 now: 0 |
netdev/verify_signedoff | success | Link |
netdev/module_param | success | Was 0 now: 0 |
netdev/build_32bit | fail | Errors and warnings before: 52 this patch: 60 |
netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
netdev/verify_fixes | success | Link |
netdev/checkpatch | warning | WARNING: line length of 84 exceeds 80 columns |
netdev/build_allmodconfig_warn | fail | Errors and warnings before: 50 this patch: 58 |
netdev/header_inline | success | Link |
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
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).
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
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")).
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.
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.
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. */