Re: [PATCH nf-next 0/3] NF NAT deduplication refactoring

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]



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

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?


[Index of Archives]     [Netfitler Users]     [Berkeley Packet Filter]     [LARTC]     [Bugtraq]     [Yosemite Forum]

  Powered by Linux