| Message ID | 1352837857-22087-3-git-send-email-kadlec@blackhole.kfki.hu |
|---|---|
| State | Not Applicable |
| Headers | show |
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
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
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
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
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
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
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);
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(-)