All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] [RFC] Get MASQUERADE target to handle routing changes
@ 2012-11-13 20:17 Jozsef Kadlecsik
  2012-11-13 20:17 ` [PATCH 1/2] Introduce notification chain for " Jozsef Kadlecsik
  2012-11-13 20:17 ` [PATCH 2/2] Handle the routing changes in the MASQUERADE target Jozsef Kadlecsik
  0 siblings, 2 replies; 11+ messages in thread
From: Jozsef Kadlecsik @ 2012-11-13 20:17 UTC (permalink / raw)
  To: netfilter-devel


The MASQUERADE target does not handle the cases when the routing changes.
(See thread "UDP packets sent with wrong source address after routing change
[AV#3431]").

The first patch introduces a new in-kernel notification chain for the
routing changes.  The second one registers the MASQUERADE target to this
events and adds the new "--route-dependent" flag (actually, the value of the
flag) and conntrack flag to mark conntrack entries which may be affected by
routing changes.  As the first step, when routing changes, marked entries
are simply deleted.

Best regards,
Jozsef

Jozsef Kadlecsik (2):
  Introduce notification chain for routing changes
  Handle the routing changes in the MASQUERADE target

 include/linux/inetdevice.h                         |    2 +
 include/linux/netdevice.h                          |    1 +
 include/uapi/linux/netfilter/nf_conntrack_common.h |    4 ++
 include/uapi/linux/netfilter/nf_nat.h              |    1 +
 net/ipv4/fib_trie.c                                |   18 +++++++++
 net/ipv4/netfilter/ipt_MASQUERADE.c                |   40 ++++++++++++++++++++
 6 files changed, 66 insertions(+), 0 deletions(-)


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/2] Introduce notification chain for routing changes
  2012-11-13 20:17 [PATCH 0/2] [RFC] Get MASQUERADE target to handle routing changes Jozsef Kadlecsik
@ 2012-11-13 20:17 ` Jozsef Kadlecsik
  2012-11-13 22:15   ` David Miller
  2012-11-13 20:17 ` [PATCH 2/2] Handle the routing changes in the MASQUERADE target Jozsef Kadlecsik
  1 sibling, 1 reply; 11+ messages in thread
From: Jozsef Kadlecsik @ 2012-11-13 20:17 UTC (permalink / raw)
  To: netfilter-devel; +Cc: Jozsef Kadlecsik


Signed-off-by: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
---
 include/linux/inetdevice.h |    2 ++
 include/linux/netdevice.h  |    1 +
 net/ipv4/fib_trie.c        |   18 ++++++++++++++++++
 3 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/include/linux/inetdevice.h b/include/linux/inetdevice.h
index d032780..cf16dab 100644
--- a/include/linux/inetdevice.h
+++ b/include/linux/inetdevice.h
@@ -170,6 +170,8 @@ struct in_ifaddr {
 
 extern int register_inetaddr_notifier(struct notifier_block *nb);
 extern int unregister_inetaddr_notifier(struct notifier_block *nb);
+extern int register_iproute_notifier(struct notifier_block *nb);
+extern int unregister_iproute_notifier(struct notifier_block *nb);
 
 extern struct net_device *__ip_dev_find(struct net *net, __be32 addr, bool devref);
 static inline struct net_device *ip_dev_find(struct net *net, __be32 addr)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index f8eda02..f61b5f4 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1540,6 +1540,7 @@ struct packet_type {
 #define NETDEV_RELEASE		0x0012
 #define NETDEV_NOTIFY_PEERS	0x0013
 #define NETDEV_JOIN		0x0014
+#define NETDEV_ROUTE_CHANGED	0x0015
 
 extern int register_netdevice_notifier(struct notifier_block *nb);
 extern int unregister_netdevice_notifier(struct notifier_block *nb);
diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
index 31d771c..5caf26b 100644
--- a/net/ipv4/fib_trie.c
+++ b/net/ipv4/fib_trie.c
@@ -73,6 +73,7 @@
 #include <linux/slab.h>
 #include <linux/prefetch.h>
 #include <linux/export.h>
+#include <linux/notifier.h>
 #include <net/net_namespace.h>
 #include <net/ip.h>
 #include <net/protocol.h>
@@ -178,6 +179,8 @@ static const int sync_pages = 128;
 static struct kmem_cache *fn_alias_kmem __read_mostly;
 static struct kmem_cache *trie_leaf_kmem __read_mostly;
 
+static BLOCKING_NOTIFIER_HEAD(iproute_chain);
+
 /*
  * caller must hold RTNL
  */
@@ -1337,6 +1340,8 @@ int fib_table_insert(struct fib_table *tb, struct fib_config *cfg)
 	rtmsg_fib(RTM_NEWROUTE, htonl(key), new_fa, plen, tb->tb_id,
 		  &cfg->fc_nlinfo, 0);
 succeeded:
+	blocking_notifier_call_chain(&iproute_chain,
+				     NETDEV_ROUTE_CHANGED, fi);
 	return 0;
 
 out_free_new_fa:
@@ -1713,6 +1718,8 @@ int fib_table_delete(struct fib_table *tb, struct fib_config *cfg)
 	if (fa->fa_state & FA_S_ACCESSED)
 		rt_cache_flush(cfg->fc_nlinfo.nl_net);
 
+	blocking_notifier_call_chain(&iproute_chain,
+				     NETDEV_ROUTE_CHANGED, fa->fa_info);
 	fib_release_info(fa->fa_info);
 	alias_free_mem_rcu(fa);
 	return 0;
@@ -1979,6 +1986,17 @@ void __init fib_trie_init(void)
 					   0, SLAB_PANIC, NULL);
 }
 
+int register_iproute_notifier(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_register(&iproute_chain, nb);
+}
+EXPORT_SYMBOL(register_iproute_notifier);
+
+int unregister_iproute_notifier(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_unregister(&iproute_chain, nb);
+}
+EXPORT_SYMBOL(unregister_iproute_notifier);
 
 struct fib_table *fib_trie_table(u32 id)
 {
-- 
1.7.0.4


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/2] Handle the routing changes in the MASQUERADE target
  2012-11-13 20:17 [PATCH 0/2] [RFC] Get MASQUERADE target to handle routing changes Jozsef Kadlecsik
  2012-11-13 20:17 ` [PATCH 1/2] Introduce notification chain for " Jozsef Kadlecsik
@ 2012-11-13 20:17 ` Jozsef Kadlecsik
  2012-11-13 23:49   ` Jan Engelhardt
  2012-11-15 11:44   ` Pablo Neira Ayuso
  1 sibling, 2 replies; 11+ messages in thread
From: Jozsef Kadlecsik @ 2012-11-13 20:17 UTC (permalink / raw)
  To: netfilter-devel; +Cc: Jozsef Kadlecsik

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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] Introduce notification chain for routing changes
  2012-11-13 20:17 ` [PATCH 1/2] Introduce notification chain for " Jozsef Kadlecsik
@ 2012-11-13 22:15   ` David Miller
  2012-11-14  7:57     ` Jozsef Kadlecsik
  0 siblings, 1 reply; 11+ messages in thread
From: David Miller @ 2012-11-13 22:15 UTC (permalink / raw)
  To: kadlec; +Cc: netfilter-devel

From: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
Date: Tue, 13 Nov 2012 21:17:36 +0100

> 
> Signed-off-by: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
> ---
>  include/linux/inetdevice.h |    2 ++
>  include/linux/netdevice.h  |    1 +
>  net/ipv4/fib_trie.c        |   18 ++++++++++++++++++
>  3 files changed, 21 insertions(+), 0 deletions(-)

A change like this needs to be reviewed on netdev, not
netfilter-devel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] Handle the routing changes in the MASQUERADE target
  2012-11-13 20:17 ` [PATCH 2/2] Handle the routing changes in the MASQUERADE target Jozsef Kadlecsik
@ 2012-11-13 23:49   ` Jan Engelhardt
  2012-11-14  7:55     ` Jozsef Kadlecsik
  2012-11-15 11:44   ` Pablo Neira Ayuso
  1 sibling, 1 reply; 11+ messages in thread
From: Jan Engelhardt @ 2012-11-13 23:49 UTC (permalink / raw)
  To: Jozsef Kadlecsik; +Cc: netfilter-devel


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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] Handle the routing changes in the MASQUERADE target
  2012-11-13 23:49   ` Jan Engelhardt
@ 2012-11-14  7:55     ` Jozsef Kadlecsik
  0 siblings, 0 replies; 11+ messages in thread
From: Jozsef Kadlecsik @ 2012-11-14  7:55 UTC (permalink / raw)
  To: Jan Engelhardt; +Cc: netfilter-devel

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] Introduce notification chain for routing changes
  2012-11-13 22:15   ` David Miller
@ 2012-11-14  7:57     ` Jozsef Kadlecsik
  0 siblings, 0 replies; 11+ messages in thread
From: Jozsef Kadlecsik @ 2012-11-14  7:57 UTC (permalink / raw)
  To: David Miller; +Cc: netfilter-devel

On Tue, 13 Nov 2012, David Miller wrote:

> From: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
> Date: Tue, 13 Nov 2012 21:17:36 +0100
> 
> > 
> > Signed-off-by: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
> > ---
> >  include/linux/inetdevice.h |    2 ++
> >  include/linux/netdevice.h  |    1 +
> >  net/ipv4/fib_trie.c        |   18 ++++++++++++++++++
> >  3 files changed, 21 insertions(+), 0 deletions(-)
> 
> A change like this needs to be reviewed on netdev, not
> netfilter-devel

Yes, of course. I just wanted to put together a possible real solution 
which we could discuss further.

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] Handle the routing changes in the MASQUERADE target
  2012-11-13 20:17 ` [PATCH 2/2] Handle the routing changes in the MASQUERADE target Jozsef Kadlecsik
  2012-11-13 23:49   ` Jan Engelhardt
@ 2012-11-15 11:44   ` Pablo Neira Ayuso
  2012-11-15 14:42     ` Jozsef Kadlecsik
  1 sibling, 1 reply; 11+ messages in thread
From: Pablo Neira Ayuso @ 2012-11-15 11:44 UTC (permalink / raw)
  To: Jozsef Kadlecsik; +Cc: netfilter-devel

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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] Handle the routing changes in the MASQUERADE target
  2012-11-15 11:44   ` Pablo Neira Ayuso
@ 2012-11-15 14:42     ` Jozsef Kadlecsik
  2012-11-16 10:09       ` Pablo Neira Ayuso
  0 siblings, 1 reply; 11+ messages in thread
From: Jozsef Kadlecsik @ 2012-11-15 14:42 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] Handle the routing changes in the MASQUERADE target
  2012-11-15 14:42     ` Jozsef Kadlecsik
@ 2012-11-16 10:09       ` Pablo Neira Ayuso
  2012-11-16 21:38         ` Jozsef Kadlecsik
  0 siblings, 1 reply; 11+ messages in thread
From: Pablo Neira Ayuso @ 2012-11-16 10:09 UTC (permalink / raw)
  To: Jozsef Kadlecsik; +Cc: netfilter-devel

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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] Handle the routing changes in the MASQUERADE target
  2012-11-16 10:09       ` Pablo Neira Ayuso
@ 2012-11-16 21:38         ` Jozsef Kadlecsik
  0 siblings, 0 replies; 11+ messages in thread
From: Jozsef Kadlecsik @ 2012-11-16 21:38 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2012-11-16 21:38 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-11-13 20:17 [PATCH 0/2] [RFC] Get MASQUERADE target to handle routing changes Jozsef Kadlecsik
2012-11-13 20:17 ` [PATCH 1/2] Introduce notification chain for " Jozsef Kadlecsik
2012-11-13 22:15   ` David Miller
2012-11-14  7:57     ` Jozsef Kadlecsik
2012-11-13 20:17 ` [PATCH 2/2] Handle the routing changes in the MASQUERADE target Jozsef Kadlecsik
2012-11-13 23:49   ` Jan Engelhardt
2012-11-14  7:55     ` Jozsef Kadlecsik
2012-11-15 11:44   ` Pablo Neira Ayuso
2012-11-15 14:42     ` Jozsef Kadlecsik
2012-11-16 10:09       ` Pablo Neira Ayuso
2012-11-16 21:38         ` Jozsef Kadlecsik

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.