Re: [RFC PATCH 1/1] netfilter: nat: restore default DNAT behavior

[Kyle Swenson <kyle.swenson@est.tech>]

On Fri, Jan 26, 2024 at 04:57:20PM +0100, Florian Westphal wrote:
> Kyle Swenson <kyle.swenson@est.tech> wrote:
> > When a DNAT rule is configured via iptables with different port ranges,
> > 
> > iptables -t nat -A PREROUTING -p tcp -d 10.0.0.2 -m tcp --dport 32000:32010
> > -j DNAT --to-destination 192.168.0.10:21000-21010
> > 
> > we seem to be DNATing to some random port on the LAN side. While this is
> > expected if --random is passed to the iptables command, it is not
> > expected without passing --random.  The expected behavior (and the
> > observed behavior in v4.4) is the traffic will be DNAT'd to
> > 192.168.0.10:21000 unless there is a tuple collision with that
> > destination.  In that case, we expect the traffic to be instead DNAT'd
> > to 192.168.0.10:21001, so on so forth until the end of the range.
> > 
> > This patch is a naive attempt to restore the behavior seen in v4.4.  I'm
> > hopeful folks will point out problems and regressions this could cause
> > elsewhere, since I've little experience in the net tree.
> > 
> > Signed-off-by: Kyle Swenson <kyle.swenson@est.tech>
> > ---
> >  net/netfilter/nf_nat_core.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c
> > index c3d7ecbc777c..bd275c3906f7 100644
> > --- a/net/netfilter/nf_nat_core.c
> > +++ b/net/netfilter/nf_nat_core.c
> > @@ -549,12 +549,14 @@ static void nf_nat_l4proto_unique_tuple(struct nf_conntrack_tuple *tuple,
> >  	}
> >  
> >  find_free_id:
> >  	if (range->flags & NF_NAT_RANGE_PROTO_OFFSET)
> >  		off = (ntohs(*keyptr) - ntohs(range->base_proto.all));
> > -	else
> > +	else if (range->flags & NF_NAT_RANGE_PROTO_RANDOM)
> >  		off = get_random_u16();
> > +	else
> > +		off = 0;
> 
> Can you restrict this to NF_NAT_MANIP_DST?
> I don't want predictable src port conflict resolution.
> 
> Probably something like (untested):
> 
> find_free_id:
>  	if (range->flags & NF_NAT_RANGE_PROTO_OFFSET)
>  		off = (ntohs(*keyptr) - ntohs(range->base_proto.all));
> +	else if ((range->flags & NF_NAT_RANGE_PROTO_SPECIFIED) &&
> +	  	  maniptype == NF_NAT_MANIP_DST))
> + 		off = 1;
> 	else
>   		off = get_random_u16();

Yes, absolutely.  I'll test out the change and send a v2 next week.

Thanks,
Kyle