Hi,
On Mon, Mar 13, 2023 at 01:46:46PM +0000, Jeremy Sowden wrote:
> These three patches perform refactoring in NF NAT modules to remove
> duplicate code.
Thanks for consolidating this.
I collapsed patch 1 and 3, in this particular case I think it is easy
to review to do these two things at once, I can see this:
+static unsigned int
+nf_nat_redirect(struct sk_buff *skb, const struct nf_nat_range2 *range,
+ const union nf_inet_addr *newdst)
+{
+ struct nf_nat_range2 newrange;
+ enum ip_conntrack_info ctinfo;
+ struct nf_conn *ct;
+
+ ct = nf_ct_get(skb, &ctinfo);
+ WARN_ON(!(ct && (ctinfo == IP_CT_NEW || ctinfo == IP_CT_RELATED)));
nf_nat_setup_info() calls _is_confirmed() which expect ct to be
non-null, so crash will happen right after this WARN_ON.
and _is_confirmed() implicitly means that this is either IP_CT_NEW or
IP_CT_RELATED.
This comes from 44d6e2f27328b25411 which replaced an assertion, quite
straight forward.
I'd suggest to remove this.
+ newrange.flags = range->flags | NF_NAT_RANGE_MAP_IPS;
+ newrange.min_addr = *newdst;
+ newrange.max_addr = *newdst;
+ newrange.min_proto = range->min_proto;
+ newrange.max_proto = range->max_proto;
This newrange is missing memset(), other existing nat code calls
memset(), I think it might be related to iptables which is exposing
this structure to userspace through blob. I would need to check the
NAT core to check if this memset is really required.
+ return nf_nat_setup_info(ct, &newrange, NF_NAT_MANIP_DST);
+}
May you submit v2 with these two changes?
Thanks.
