Re: [PATCH 2/2] Handle the routing changes in the MASQUERADE target

[Pablo Neira Ayuso <pablo@netfilter.org>]

Hi Jozsef,

On Thu, Nov 15, 2012 at 03:42:14PM +0100, Jozsef Kadlecsik wrote:
> On Thu, 15 Nov 2012, Pablo Neira Ayuso wrote:
> 
> > Two comments on this patch:
> > 
> > On Tue, Nov 13, 2012 at 09:17:37PM +0100, Jozsef Kadlecsik wrote:
> > > When the routing changes, MASQUERADE should delete the conntrack
> > > entries where the source NATed address changes due to the routing
> > > change. As a first approximation, delete all entries which are
> > > marked with the new "--route-dependent" flag of the MASQUERADE
> > > target.
> > > 
> > > Signed-off-by: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
> > > ---
> > >  include/uapi/linux/netfilter/nf_conntrack_common.h |    4 ++
> > >  include/uapi/linux/netfilter/nf_nat.h              |    1 +
> > >  net/ipv4/netfilter/ipt_MASQUERADE.c                |   40 ++++++++++++++++++++
> > >  3 files changed, 45 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/include/uapi/linux/netfilter/nf_conntrack_common.h b/include/uapi/linux/netfilter/nf_conntrack_common.h
> > > index 1644cdd..1c698b5 100644
> > > --- a/include/uapi/linux/netfilter/nf_conntrack_common.h
> > > +++ b/include/uapi/linux/netfilter/nf_conntrack_common.h
> > > @@ -87,6 +87,10 @@ enum ip_conntrack_status {
> > >  	/* Conntrack got a helper explicitly attached via CT target. */
> > >  	IPS_HELPER_BIT = 13,
> > >  	IPS_HELPER = (1 << IPS_HELPER_BIT),
> > > +
> > > +	/* Conntrack must be deleted when routing changed (NAT) */
> > > +	IPS_ROUTING_DEPENDENT_BIT = 14,
> > > +	IPS_ROUTING_DEPENDENT = (1 << IPS_ROUTING_DEPENDENT_BIT),
> > 
> > This seems to me a bit too specific for the masquerade module. I've
> > been checking the struct nf_conn_nat to squash that information there,
> > but I don't find the way to make it without increasing the length of
> > the NAT area.
> 
> I know, we waste a status bit. But I couldn't find a better way to store 
> the information in conntrack.
>
> > >  };
> > >  
> > >  /* Connection tracking event types */
> > > diff --git a/include/uapi/linux/netfilter/nf_nat.h b/include/uapi/linux/netfilter/nf_nat.h
> > > index bf0cc37..a0dfac7 100644
> > > --- a/include/uapi/linux/netfilter/nf_nat.h
> > > +++ b/include/uapi/linux/netfilter/nf_nat.h
> > > @@ -8,6 +8,7 @@
> > >  #define NF_NAT_RANGE_PROTO_SPECIFIED	2
> > >  #define NF_NAT_RANGE_PROTO_RANDOM	4
> > >  #define NF_NAT_RANGE_PERSISTENT		8
> > > +#define NF_NAT_ROUTING_DEPENDENT	16
> > >  
> > >  struct nf_nat_ipv4_range {
> > >  	unsigned int			flags;
> > > diff --git a/net/ipv4/netfilter/ipt_MASQUERADE.c b/net/ipv4/netfilter/ipt_MASQUERADE.c
> > > index 5d5d4d1..ecf3063 100644
> > > --- a/net/ipv4/netfilter/ipt_MASQUERADE.c
> > > +++ b/net/ipv4/netfilter/ipt_MASQUERADE.c
> > 
> > We now have IPv6 NAT support, so I guess you need to patch
> > /net/ipv6/netfilter/ip6t_MASQUERADE.c
> 
> Ohh, yes, I missed that. I'll add the required code there.
> 
> Currently I'm trying to find a way to purge just the entries which are 
> affected by the routing change (for example when there are muliple VPN 
> tunnels). However that requires a new conntrack extension and it's 
> nontrivial (at least for me) to figure out the required data from struct 
> fib_info.
> 
> If the conntrack extension is used then of course the status bit is not 
> required.

We can register now variable length conntrack extensions. I think we
can use that feature to extend nf_conn_nat to allocate extra
information for all working modes of MASQUERADE. It may require
changing the nf_nat_setup_info interface to pass some new flags.

Regarding the variable length conntrack extensions, please check
nf_ct_ext_add_length in net/netfilter/nf_conntrack_helper.c for
instance.