Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Florian Westphal <fw@strlen.de>
To: Cole Dishington <Cole.Dishington@alliedtelesis.co.nz>
Cc: pablo@netfilter.org,
	Anthony Lineham <anthony.lineham@alliedtelesis.co.nz>,
	Scott Parlane <scott.parlane@alliedtelesis.co.nz>,
	Blair Steven <blair.steven@alliedtelesis.co.nz>,
	Jozsef Kadlecsik <kadlec@netfilter.org>,
	Florian Westphal <fw@strlen.de>,
	"David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	netfilter-devel@vger.kernel.org, coreteam@netfilter.org,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] net: netfilter: Add RFC-7597 Section 5.1 PSID support
Date: Wed, 30 Jun 2021 16:20:49 +0200	[thread overview]
Message-ID: <20210630142049.GC18022@breakpoint.cc> (raw)
In-Reply-To: <20210629004819.4750-1-Cole.Dishington@alliedtelesis.co.nz>

Cole Dishington <Cole.Dishington@alliedtelesis.co.nz> wrote:
>     Comments:
>     Selecting the ports for psid needs to be in nf_nat_core since the PSID ranges are not a single range. e.g. offset=1024, PSID=0, psid_length=8 generates the ranges 1024-1027, 2048-2051, ..., 63488-63491, ... (example taken from RFC7597 B.2).
>     This is why it is enough to set NF_NAT_RANGE_PROTO_SPECIFIED and init upper/lower boundaries.

I suspect this misses a NOT.  But current algorithm has problems, see
below.

> +	if (range->flags & NF_NAT_RANGE_PSID) {
> +		/* PSID defines a group of port ranges, per PSID. PSID
> +		 * is already contained in min and max.
> +		 */
> +		unsigned int min_to_max, base;
> +
> +		min = ntohs(range->min_proto.all);
> +		max = ntohs(range->max_proto.all);
> +		base = ntohs(range->base_proto.all);
> +		min_to_max = max - min;
> +		for (; max <= (1 << 16) - 1; min += base, max = min + min_to_max) {
> +			for (off = 0; off <= min_to_max; off++) {
> +				*keyptr = htons(min + off);
> +				if (!nf_nat_used_tuple(tuple, ct))
> +					return;
> +			}
> +		}
> +	}

I fear this searches waaaay to many ports.
We had softlockups in the past because of exhausive searches.

See a504b703bb1da526a01593da0e4be2af9d9f5fa8
("netfilter: nat: limit port clash resolution attempts").

I suggest you try pre-selecting one of the eligible ranges in
nf_nat_masquerade_ipv4 when the 'newrange' is filled in and set
RANGE_PROTO_SPECIFIED.

Maybe even prandom-based preselection is good enough.

>  	/* If no range specified... */
>  	if (!(range->flags & NF_NAT_RANGE_PROTO_SPECIFIED)) {
>  		/* If it's dst rewrite, can't change port */
> @@ -529,11 +572,19 @@ get_unique_tuple(struct nf_conntrack_tuple *tuple,
>  
>  	/* Only bother mapping if it's not already in range and unique */
>  	if (!(range->flags & NF_NAT_RANGE_PROTO_RANDOM_ALL)) {
> -		if (range->flags & NF_NAT_RANGE_PROTO_SPECIFIED) {
> +		/* PSID mode is present always needs to check
> +		 * to see if the source ports are in range.
> +		 */
> +		if (range->flags & NF_NAT_RANGE_PROTO_SPECIFIED ||
> +		    (range->flags & NF_NAT_RANGE_PSID &&

Why the extra check?
Can't you set NF_NAT_RANGE_PROTO_SPECIFIED in case PSID is requested by
userspace?

> diff --git a/net/netfilter/nf_nat_ftp.c b/net/netfilter/nf_nat_ftp.c
> index aace6768a64e..f65163278db0 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,13 @@ 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);
>  	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,18 +95,14 @@ 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;
> -
> -		exp->tuple.dst.u.tcp.port = htons(port);
> -		ret = nf_ct_expect_related(exp, 0);
> -		if (ret == 0)
> -			break;
> -		else if (ret != -EBUSY) {
> -			port = 0;
> -			break;
> -		}
> +	/* Find a port that matches the MASQ rule. */
> +	nf_nat_l4proto_unique_tuple(&exp->tuple, nat->range,
> +				    dir ? NF_NAT_MANIP_SRC : NF_NAT_MANIP_DST,
> +				    ct);

Hmm, I am ingorant on details here, but is this correct?

This could be an inbound connection, rather than outbound.

> diff --git a/net/netfilter/nf_nat_helper.c b/net/netfilter/nf_nat_helper.c
> index a263505455fc..2d105e4eb8f8 100644
> --- a/net/netfilter/nf_nat_helper.c
> +++ b/net/netfilter/nf_nat_helper.c
> @@ -179,15 +179,23 @@ EXPORT_SYMBOL(nf_nat_mangle_udp_packet);
>  void nf_nat_follow_master(struct nf_conn *ct,
>  			  struct nf_conntrack_expect *exp)
>  {
> +	struct nf_conn_nat *nat = NULL;
>  	struct nf_nat_range2 range;
>  
>  	/* This must be a fresh one. */
>  	BUG_ON(ct->status & IPS_NAT_DONE_MASK);
>  
> -	/* Change src to where master sends to */
> -	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) {
> +		nat = nfct_nat(exp->master);
> +		if (nat)
> +			range = *nat->range;

Can't you store the psid-relevant parts of the range struct only?
Non-PSID doesn't need the original range, so why do you?

> diff --git a/net/netfilter/nf_nat_masquerade.c b/net/netfilter/nf_nat_masquerade.c
> index 8e8a65d46345..d83cd3d8ad3f 100644
> --- a/net/netfilter/nf_nat_masquerade.c
> +++ b/net/netfilter/nf_nat_masquerade.c
> @@ -45,10 +45,6 @@ nf_nat_masquerade_ipv4(struct sk_buff *skb, unsigned int hooknum,
>  		return NF_DROP;
>  	}
>  
> -	nat = nf_ct_nat_ext_add(ct);
> -	if (nat)
> -		nat->masq_index = out->ifindex;
> -
>  	/* Transfer from original range. */
>  	memset(&newrange.min_addr, 0, sizeof(newrange.min_addr));
>  	memset(&newrange.max_addr, 0, sizeof(newrange.max_addr));
> @@ -57,6 +53,15 @@ nf_nat_masquerade_ipv4(struct sk_buff *skb, unsigned int hooknum,
>  	newrange.max_addr.ip = newsrc;
>  	newrange.min_proto   = range->min_proto;
>  	newrange.max_proto   = range->max_proto;
> +	newrange.base_proto  = range->base_proto;
> +
> +	nat = nf_ct_nat_ext_add(ct);
> +	if (nat) {
> +		nat->masq_index = out->ifindex;
> +		if (!nat->range)
> +			nat->range = kmalloc(sizeof(*nat->range), 0);
> +		memcpy(nat->range, &newrange, sizeof(*nat->range));

kmemdup.  Also misses error handling.  Should use GFP_ATOMIC.
Where is this free'd again?

It would be good if you could chop this up in smaller chunks.
A selftest would be nice as well (see tools/testing/selftests/netfilter).

  reply	other threads:[~2021-06-30 14:21 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-22  2:35 Cole Dishington
2021-04-26 12:23 ` Florian Westphal
2021-06-29  0:48   ` Cole Dishington
2021-06-30 14:20     ` Florian Westphal [this message]
     [not found]       ` <20210705040856.25191-1-Cole.Dishington@alliedtelesis.co.nz>
2021-07-05  4:08         ` [PATCH] net: netfilter: Add RFC-7597 Section 5.1 PSID support xtables API Cole Dishington
2021-07-05  4:08         ` [PATCH] net: netfilter: Add RFC-7597 Section 5.1 PSID support Cole Dishington
2021-07-05 10:39           ` Florian Westphal
2021-07-16  0:27             ` [PATCH 0/3] " Cole Dishington
2021-07-16  0:27               ` [PATCH 1/3] net: netfilter: Add RFC-7597 Section 5.1 PSID support xtables API Cole Dishington
2021-07-16  0:27               ` [PATCH 2/3] net: netfilter: Add RFC-7597 Section 5.1 PSID support Cole Dishington
2021-07-16 15:18                 ` Florian Westphal
2021-07-19  1:21                   ` Cole Dishington
2021-07-22  7:17                     ` Florian Westphal
2021-07-25 23:28                       ` Cole Dishington
2021-07-26 14:37                         ` Florian Westphal
2021-08-09  4:10                           ` [PATCH net-next 0/3] " Cole Dishington
2021-08-09  4:10                             ` [PATCH net-next 1/3] net: netfilter: Add RFC-7597 Section 5.1 PSID support xtables API Cole Dishington
2021-08-09  4:10                             ` [PATCH net-next 2/3] net: netfilter: Add RFC-7597 Section 5.1 PSID support Cole Dishington
2021-08-25 17:05                               ` Pablo Neira Ayuso
2021-08-29 21:30                                 ` Cole Dishington
2021-08-09  4:10                             ` [PATCH net-next 3/3] selftests: netfilter: Add RFC-7597 Section 5.1 PSID selftests Cole Dishington
2021-07-16  0:27               ` [PATCH " Cole Dishington

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210630142049.GC18022@breakpoint.cc \
    --to=fw@strlen.de \
    --cc=Cole.Dishington@alliedtelesis.co.nz \
    --cc=anthony.lineham@alliedtelesis.co.nz \
    --cc=blair.steven@alliedtelesis.co.nz \
    --cc=coreteam@netfilter.org \
    --cc=davem@davemloft.net \
    --cc=kadlec@netfilter.org \
    --cc=kuba@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=netfilter-devel@vger.kernel.org \
    --cc=pablo@netfilter.org \
    --cc=scott.parlane@alliedtelesis.co.nz \
    --subject='Re: [PATCH] net: netfilter: Add RFC-7597 Section 5.1 PSID support' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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