Patchwork [2/2] Handle the routing changes in the MASQUERADE target

login
register
mail settings
Submitter Jozsef Kadlecsik
Date Nov. 13, 2012, 8:17 p.m.
Message ID <1352837857-22087-3-git-send-email-kadlec@blackhole.kfki.hu>
Download mbox | patch
Permalink /patch/198775/
State Not Applicable
Headers show

Comments

Jozsef Kadlecsik - Nov. 13, 2012, 8:17 p.m.
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(-)
Jan Engelhardt - Nov. 13, 2012, 11:49 p.m.
On Tuesday 2012-11-13 21:17, Jozsef Kadlecsik wrote:

>+static int
>+route_cmp(struct nf_conn *i, const struct fib_info *fi)
>+{  [...]
>+}
>+
>+static int masq_route_event(struct notifier_block *this,
>+			    unsigned long event,
>+			    void *ptr)
>+{
>+	if (event == NETDEV_ROUTE_CHANGED) {
>+		/* Routing changed, delete marked entries */
>+		nf_ct_iterate_cleanup(net, route_cmp,
>+				      (const struct fib_info)ptr);

It would seem you forget a '*' near fib_info)ptr.
The cast is pointless though, since ptr is already of type void *
which nf_ct_iterate_cleanup expects.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jozsef Kadlecsik - Nov. 14, 2012, 7:55 a.m.
On Wed, 14 Nov 2012, Jan Engelhardt wrote:

> On Tuesday 2012-11-13 21:17, Jozsef Kadlecsik wrote:
> 
> >+static int
> >+route_cmp(struct nf_conn *i, const struct fib_info *fi)
> >+{  [...]
> >+}
> >+
> >+static int masq_route_event(struct notifier_block *this,
> >+			    unsigned long event,
> >+			    void *ptr)
> >+{
> >+	if (event == NETDEV_ROUTE_CHANGED) {
> >+		/* Routing changed, delete marked entries */
> >+		nf_ct_iterate_cleanup(net, route_cmp,
> >+				      (const struct fib_info)ptr);
> 
> It would seem you forget a '*' near fib_info)ptr.
> The cast is pointless though, since ptr is already of type void *
> which nf_ct_iterate_cleanup expects.

You are right and it was not tested at all. Just a quick code to discuss 
not only in theory but something real.

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences
          H-1525 Budapest 114, POB. 49, Hungary
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pablo Neira - Nov. 15, 2012, 11:44 a.m.
Hi Jozsef,

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.

>  };
>  
>  /* 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

Regards.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jozsef Kadlecsik - Nov. 15, 2012, 2:42 p.m.
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.

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences
          H-1525 Budapest 114, POB. 49, Hungary
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pablo Neira - Nov. 16, 2012, 10:09 a.m.
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.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jozsef Kadlecsik - Nov. 16, 2012, 9:38 p.m.
Hi Pablo,

On Fri, 16 Nov 2012, Pablo Neira Ayuso wrote:

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

I realized that there's no point to store the extra information in an 
extension: changing for example the weight of another routing rule may 
effect the conntrack entry. So we have to recheck the routing. Sigh.

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences
          H-1525 Budapest 114, POB. 49, Hungary
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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),
 };
 
 /* 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
@@ -19,6 +19,7 @@ 
 #include <net/ip.h>
 #include <net/checksum.h>
 #include <net/route.h>
+#include <net/ip_fib.h>
 #include <linux/netfilter_ipv4.h>
 #include <linux/netfilter/x_tables.h>
 #include <net/netfilter/nf_nat.h>
@@ -88,6 +89,9 @@  masquerade_tg(struct sk_buff *skb, const struct xt_action_param *par)
 	newrange.min_proto   = mr->range[0].min;
 	newrange.max_proto   = mr->range[0].max;
 
+	if (mr->range[0].flags & NF_NAT_ROUTING_DEPENDENT)
+		set_bit(IPS_ROUTING_DEPENDENT, &ct->status);
+
 	/* Hand modified range to generic setup. */
 	return nf_nat_setup_info(ct, &newrange, NF_NAT_MANIP_SRC);
 }
@@ -132,6 +136,35 @@  static int masq_inet_event(struct notifier_block *this,
 	return masq_device_event(this, event, dev);
 }
 
+static int
+route_cmp(struct nf_conn *i, const struct fib_info *fi)
+{
+	const struct nf_conn_nat *nat = nfct_nat(i);
+
+	if (!nat)
+		return 0;
+	if (nf_ct_l3num(i) != NFPROTO_IPV4)
+		return 0;
+	if (!test_bit(IPS_ROUTING_DEPENDENT, &ct->status))
+		return 0;
+
+	/* We should check whether the SNAT address changed. */
+	return 1;
+}
+
+static int masq_route_event(struct notifier_block *this,
+			    unsigned long event,
+			    void *ptr)
+{
+	if (event == NETDEV_ROUTE_CHANGED) {
+		/* Routing changed, delete marked entries */
+		nf_ct_iterate_cleanup(net, route_cmp,
+				      (const struct fib_info)ptr);
+	}
+
+	return NOTIFY_DONE;
+}
+
 static struct notifier_block masq_dev_notifier = {
 	.notifier_call	= masq_device_event,
 };
@@ -140,6 +173,10 @@  static struct notifier_block masq_inet_notifier = {
 	.notifier_call	= masq_inet_event,
 };
 
+static struct notifier_block masq_route_notifier = {
+	.notifier_call	= masq_route_event,
+};
+
 static struct xt_target masquerade_tg_reg __read_mostly = {
 	.name		= "MASQUERADE",
 	.family		= NFPROTO_IPV4,
@@ -162,6 +199,8 @@  static int __init masquerade_tg_init(void)
 		register_netdevice_notifier(&masq_dev_notifier);
 		/* Register IP address change reports */
 		register_inetaddr_notifier(&masq_inet_notifier);
+		/* Register route change reports */
+		register_iproute_notifier(&masq_route_notifier);
 	}
 
 	return ret;
@@ -172,6 +211,7 @@  static void __exit masquerade_tg_exit(void)
 	xt_unregister_target(&masquerade_tg_reg);
 	unregister_netdevice_notifier(&masq_dev_notifier);
 	unregister_inetaddr_notifier(&masq_inet_notifier);
+	unregister_iproute_notifier(&masq_route_notifier);
 }
 
 module_init(masquerade_tg_init);