[PATCH] netfilter: conntrack: add weak IPV6 dependency

[PATCH] netfilter: conntrack: add weak IPV6 dependency

[Arnd Bergmann]

Now that the conntrack module contains code for ipv6, we can no longer
have it built-in while IPv6 itself is a loadable module:

net/netfilter/nf_conntrack_proto.o: In function `nf_ct_netns_do_get':
nf_conntrack_proto.c:(.text+0x88c): undefined reference to `nf_defrag_ipv6_enable'
net/netfilter/nf_conntrack_proto.o:(.rodata+0x178): undefined reference to `nf_conntrack_l4proto_icmpv6'

This adds a dependency on IPv6 that makes it possible to still build
the conntrack module with IPv6 disabled, but avoids the broken configuration.

Fixes: 66c524acfb51 ("netfilter: conntrack: remove l3proto abstraction")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 net/netfilter/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/netfilter/Kconfig b/net/netfilter/Kconfig
index 74df382bf2ba..e42c38c99741 100644
--- a/net/netfilter/Kconfig
+++ b/net/netfilter/Kconfig
@@ -49,6 +49,7 @@ config NETFILTER_NETLINK_LOG
 config NF_CONNTRACK
 	tristate "Netfilter connection tracking support"
 	default m if NETFILTER_ADVANCED=n
+	depends on IPV6 || !IPV6
 	select NF_DEFRAG_IPV4
 	select NF_DEFRAG_IPV6 if IPV6
 	help
-- 
2.9.0

Re: [PATCH] netfilter: conntrack: add weak IPV6 dependency

[Florian Westphal]

Arnd Bergmann <arnd@arndb.de> wrote:
> Now that the conntrack module contains code for ipv6, we can no longer
> have it built-in while IPv6 itself is a loadable module:
> 
> net/netfilter/nf_conntrack_proto.o: In function `nf_ct_netns_do_get':
> nf_conntrack_proto.c:(.text+0x88c): undefined reference to `nf_defrag_ipv6_enable'

AFAICS this is caused by

CONFIG_NF_CONNTRACK=y
CONFIG_IPV6=m
CONFIG_NF_DEFRAG_IPV6=m

This is exported via nf_defrag_ipv6.ko.

nf_defrag_ipv6 has an ipv6 dependency, but i think it might be avoidable
so this would work:

CONFIG_NF_CONNTRACK=y
CONFIG_NF_DEFRAG_IPV6=y
CONFIG_IPV6=m

Re: [PATCH] netfilter: conntrack: add weak IPV6 dependency

[Arnd Bergmann]

On Fri, Jul 6, 2018 at 3:55 PM, Florian Westphal <fw@strlen.de> wrote:
> Arnd Bergmann <arnd@arndb.de> wrote:
>> Now that the conntrack module contains code for ipv6, we can no longer
>> have it built-in while IPv6 itself is a loadable module:
>>
>> net/netfilter/nf_conntrack_proto.o: In function `nf_ct_netns_do_get':
>> nf_conntrack_proto.c:(.text+0x88c): undefined reference to `nf_defrag_ipv6_enable'
>
> AFAICS this is caused by
>
> CONFIG_NF_CONNTRACK=y
> CONFIG_IPV6=m
> CONFIG_NF_DEFRAG_IPV6=m
>
> This is exported via nf_defrag_ipv6.ko.
>
> nf_defrag_ipv6 has an ipv6 dependency, but i think it might be avoidable
> so this would work:
>
> CONFIG_NF_CONNTRACK=y
> CONFIG_NF_DEFRAG_IPV6=y
> CONFIG_IPV6=m

I've tried it like this now:

diff --git a/net/ipv6/netfilter/Kconfig b/net/ipv6/netfilter/Kconfig
index 07516d5c2f80..18b9f8f37c97 100644
--- a/net/ipv6/netfilter/Kconfig
+++ b/net/ipv6/netfilter/Kconfig
@@ -5,10 +5,6 @@
 menu "IPv6: Netfilter Configuration"
        depends on INET && IPV6 && NETFILTER

-config NF_DEFRAG_IPV6
-       tristate
-       default n
-
 config NF_SOCKET_IPV6
        tristate "IPv6 socket lookup support"
        help
@@ -352,3 +348,6 @@ endif # IP6_NF_IPTABLES

 endmenu

+config NF_DEFRAG_IPV6
+       tristate
+       default n
diff --git a/net/netfilter/Kconfig b/net/netfilter/Kconfig
index e42c38c99741..51be519a3802 100644
--- a/net/netfilter/Kconfig
+++ b/net/netfilter/Kconfig
@@ -49,9 +49,8 @@ config NETFILTER_NETLINK_LOG
 config NF_CONNTRACK
        tristate "Netfilter connection tracking support"
        default m if NETFILTER_ADVANCED=n
-       depends on IPV6 || !IPV6
        select NF_DEFRAG_IPV4
-       select NF_DEFRAG_IPV6 if IPV6
+       select NF_DEFRAG_IPV6 if IPV6 != n
        help
          Connection tracking keeps a record of what packets have passed
          through your machine, in order to figure out how they are related

and that resulted in a new build failure:

net/netfilter/nf_conntrack_proto.o:(.rodata+0x788): undefined
reference to `nf_conntrack_l4proto_icmpv6'
net/ipv6/netfilter/nf_conntrack_reasm.o: In function `nf_ct_frag6_expire':
nf_conntrack_reasm.c:(.text+0x2320): undefined reference to
`ip6_expire_frag_queue'
net/ipv6/netfilter/nf_conntrack_reasm.o: In function `nf_ct_frag6_init':
nf_conntrack_reasm.c:(.text+0x2384): undefined reference to `ip6_frag_init'
nf_conntrack_reasm.c:(.text+0x2394): undefined reference to `ip6_frag_init'
nf_conntrack_reasm.c:(.text+0x2398): undefined reference to `ip6_rhash_params'
net/ipv6/netfilter/nf_conntrack_reasm.o: In function `nf_ct_frag6_expire':
nf_conntrack_reasm.c:(.text+0x10bc): undefined reference to
`ip6_expire_frag_queue'

I don't think we can get CONFIG_NF_DEFRAG_IPV6=y to work with IPV6=m.

      Arnd

Re: [PATCH] netfilter: conntrack: add weak IPV6 dependency

[Florian Westphal]

Arnd Bergmann <arnd@arndb.de> wrote:
> and that resulted in a new build failure:
> 
> net/netfilter/nf_conntrack_proto.o:(.rodata+0x788): undefined
> reference to `nf_conntrack_l4proto_icmpv6'
> net/ipv6/netfilter/nf_conntrack_reasm.o: In function `nf_ct_frag6_expire':
> nf_conntrack_reasm.c:(.text+0x2320): undefined reference to
> `ip6_expire_frag_queue'
> net/ipv6/netfilter/nf_conntrack_reasm.o: In function `nf_ct_frag6_init':
> nf_conntrack_reasm.c:(.text+0x2384): undefined reference to `ip6_frag_init'
> nf_conntrack_reasm.c:(.text+0x2394): undefined reference to `ip6_frag_init'
> nf_conntrack_reasm.c:(.text+0x2398): undefined reference to `ip6_rhash_params'
> net/ipv6/netfilter/nf_conntrack_reasm.o: In function `nf_ct_frag6_expire':
> nf_conntrack_reasm.c:(.text+0x10bc): undefined reference to
> `ip6_expire_frag_queue'
> 
> I don't think we can get CONFIG_NF_DEFRAG_IPV6=y to work with IPV6=m.

Yes, not with current implementation, but I still don't think this
is unavoidable.

In case this is urgent I'm fine with the patch that adds the dependency,
otherwise I'd like to try and disentangle nf_conntrack_reasm and ipv6.

Re: [PATCH] netfilter: conntrack: add weak IPV6 dependency

[Arnd Bergmann]

On Fri, Jul 6, 2018 at 5:00 PM, Florian Westphal <fw@strlen.de> wrote:
> Arnd Bergmann <arnd@arndb.de> wrote:
>> and that resulted in a new build failure:
>>
>> net/netfilter/nf_conntrack_proto.o:(.rodata+0x788): undefined
>> reference to `nf_conntrack_l4proto_icmpv6'
>> net/ipv6/netfilter/nf_conntrack_reasm.o: In function `nf_ct_frag6_expire':
>> nf_conntrack_reasm.c:(.text+0x2320): undefined reference to
>> `ip6_expire_frag_queue'
>> net/ipv6/netfilter/nf_conntrack_reasm.o: In function `nf_ct_frag6_init':
>> nf_conntrack_reasm.c:(.text+0x2384): undefined reference to `ip6_frag_init'
>> nf_conntrack_reasm.c:(.text+0x2394): undefined reference to `ip6_frag_init'
>> nf_conntrack_reasm.c:(.text+0x2398): undefined reference to `ip6_rhash_params'
>> net/ipv6/netfilter/nf_conntrack_reasm.o: In function `nf_ct_frag6_expire':
>> nf_conntrack_reasm.c:(.text+0x10bc): undefined reference to
>> `ip6_expire_frag_queue'
>>
>> I don't think we can get CONFIG_NF_DEFRAG_IPV6=y to work with IPV6=m.
>
> Yes, not with current implementation, but I still don't think this
> is unavoidable.
>
> In case this is urgent I'm fine with the patch that adds the dependency,
> otherwise I'd like to try and disentangle nf_conntrack_reasm and ipv6.

I don't think any real users depend on it, but I hit the problem quite
frequently in randconfig testing on linux-next.

       Arnd

Re: [PATCH] netfilter: conntrack: add weak IPV6 dependency

[Florian Westphal]

Florian Westphal <fw@strlen.de> wrote:
> Arnd Bergmann <arnd@arndb.de> wrote:
> > and that resulted in a new build failure:
> > 
> > net/netfilter/nf_conntrack_proto.o:(.rodata+0x788): undefined
> > reference to `nf_conntrack_l4proto_icmpv6'
> > net/ipv6/netfilter/nf_conntrack_reasm.o: In function `nf_ct_frag6_expire':
> > nf_conntrack_reasm.c:(.text+0x2320): undefined reference to
> > `ip6_expire_frag_queue'
> > net/ipv6/netfilter/nf_conntrack_reasm.o: In function `nf_ct_frag6_init':
> > nf_conntrack_reasm.c:(.text+0x2384): undefined reference to `ip6_frag_init'
> > nf_conntrack_reasm.c:(.text+0x2394): undefined reference to `ip6_frag_init'
> > nf_conntrack_reasm.c:(.text+0x2398): undefined reference to `ip6_rhash_params'
> > net/ipv6/netfilter/nf_conntrack_reasm.o: In function `nf_ct_frag6_expire':
> > nf_conntrack_reasm.c:(.text+0x10bc): undefined reference to
> > `ip6_expire_frag_queue'
> > 
> > I don't think we can get CONFIG_NF_DEFRAG_IPV6=y to work with IPV6=m.
> 
> Yes, not with current implementation, but I still don't think this
> is unavoidable.
> 
> In case this is urgent I'm fine with the patch that adds the dependency,
> otherwise I'd like to try and disentangle nf_conntrack_reasm and ipv6.

This links fine for me with IPV6=m:

Pablo, do you think this is too ugly?
It requires some copypastry from ipv6 defrag into nfct ipv6 defrag
to avoid the link errors outlined above.

Rest of defrag uses generic inet_defrag routines.

diff --git a/net/ipv6/netfilter/Kconfig b/net/ipv6/netfilter/Kconfig
index 07516d5c2f80..339d0762b027 100644
--- a/net/ipv6/netfilter/Kconfig
+++ b/net/ipv6/netfilter/Kconfig
@@ -5,10 +5,6 @@
 menu "IPv6: Netfilter Configuration"
 	depends on INET && IPV6 && NETFILTER
 
-config NF_DEFRAG_IPV6
-	tristate
-	default n
-
 config NF_SOCKET_IPV6
 	tristate "IPv6 socket lookup support"
 	help
@@ -349,6 +345,7 @@ config IP6_NF_TARGET_NPT
 endif # IP6_NF_NAT
 
 endif # IP6_NF_IPTABLES
-
 endmenu
 
+config NF_DEFRAG_IPV6
+	tristate
diff --git a/net/ipv6/netfilter/nf_conntrack_reasm.c b/net/ipv6/netfilter/nf_conntrack_reasm.c
index 5e0332014c17..9973aadc6b71 100644
--- a/net/ipv6/netfilter/nf_conntrack_reasm.c
+++ b/net/ipv6/netfilter/nf_conntrack_reasm.c
@@ -142,6 +142,50 @@ static inline u8 ip6_frag_ecn(const struct ipv6hdr *ipv6h)
 	return 1 << (ipv6_get_dsfield(ipv6h) & INET_ECN_MASK);
 }
 
+static void nfct_expire_frag_queue(struct net *net, struct frag_queue *fq)
+{
+	struct net_device *dev = NULL;
+	struct sk_buff *head;
+
+	rcu_read_lock();
+	spin_lock(&fq->q.lock);
+
+	if (fq->q.flags & INET_FRAG_COMPLETE)
+		goto out;
+
+	inet_frag_kill(&fq->q);
+
+	dev = dev_get_by_index_rcu(net, fq->iif);
+	if (!dev)
+		goto out;
+
+	__IP6_INC_STATS(net, __in6_dev_get(dev), IPSTATS_MIB_REASMFAILS);
+	__IP6_INC_STATS(net, __in6_dev_get(dev), IPSTATS_MIB_REASMTIMEOUT);
+
+	/* Don't send error if the first segment did not arrive. */
+	head = fq->q.fragments;
+	if (!(fq->q.flags & INET_FRAG_FIRST_IN) || !head)
+		goto out;
+
+	/* But use as source device on which LAST ARRIVED
+	 * segment was received. And do not use fq->dev
+	 * pointer directly, device might already disappeared.
+	 */
+	head->dev = dev;
+	skb_get(head);
+	spin_unlock(&fq->q.lock);
+
+	icmpv6_send(head, ICMPV6_TIME_EXCEED, ICMPV6_EXC_FRAGTIME, 0);
+	kfree_skb(head);
+	goto out_rcu_unlock;
+
+out:
+	spin_unlock(&fq->q.lock);
+out_rcu_unlock:
+	rcu_read_unlock();
+	inet_frag_put(&fq->q);
+}
+
 static void nf_ct_frag6_expire(struct timer_list *t)
 {
 	struct inet_frag_queue *frag = from_timer(frag, t, timer);
@@ -151,7 +195,7 @@ static void nf_ct_frag6_expire(struct timer_list *t)
 	fq = container_of(frag, struct frag_queue, q);
 	net = container_of(fq->q.net, struct net, nf_frag.frags);
 
-	ip6_expire_frag_queue(net, fq);
+	nfct_expire_frag_queue(net, fq);
 }
 
 /* Creation primitives. */
@@ -622,16 +666,55 @@ static struct pernet_operations nf_ct_net_ops = {
 	.exit = nf_ct_net_exit,
 };
 
+static u32 ip6_key_hashfn(const void *data, u32 len, u32 seed)
+{
+	return jhash2(data,
+		      sizeof(struct frag_v6_compare_key) / sizeof(u32), seed);
+}
+
+static u32 ip6_obj_hashfn(const void *data, u32 len, u32 seed)
+{
+	const struct inet_frag_queue *fq = data;
+
+	return jhash2((const u32 *)&fq->key.v6,
+		      sizeof(struct frag_v6_compare_key) / sizeof(u32), seed);
+}
+
+static int ip6_obj_cmpfn(struct rhashtable_compare_arg *arg, const void *ptr)
+{
+	const struct frag_v6_compare_key *key = arg->key;
+	const struct inet_frag_queue *fq = ptr;
+
+	return !!memcmp(&fq->key, key, sizeof(*key));
+}
+
+static const struct rhashtable_params nfct_rhash_params = {
+	.head_offset		= offsetof(struct inet_frag_queue, node),
+	.hashfn			= ip6_key_hashfn,
+	.obj_hashfn		= ip6_obj_hashfn,
+	.obj_cmpfn		= ip6_obj_cmpfn,
+	.automatic_shrinking	= true,
+};
+
+static void nfct_frag_init(struct inet_frag_queue *q, const void *a)
+{
+	struct frag_queue *fq = container_of(q, struct frag_queue, q);
+	const struct frag_v6_compare_key *key = a;
+
+	q->key.v6 = *key;
+	fq->ecn = 0;
+}
+
 int nf_ct_frag6_init(void)
 {
 	int ret = 0;
 
-	nf_frags.constructor = ip6_frag_init;
+	nf_frags.constructor = nfct_frag_init;
 	nf_frags.destructor = NULL;
 	nf_frags.qsize = sizeof(struct frag_queue);
 	nf_frags.frag_expire = nf_ct_frag6_expire;
 	nf_frags.frags_cache_name = nf_frags_cache_name;
-	nf_frags.rhash_params = ip6_rhash_params;
+	nf_frags.rhash_params = nfct_rhash_params;
 	ret = inet_frags_init(&nf_frags);
 	if (ret)
 		goto out;
diff --git a/net/netfilter/Kconfig b/net/netfilter/Kconfig
index 6c65d756e603..e0ab50c58dc4 100644
--- a/net/netfilter/Kconfig
+++ b/net/netfilter/Kconfig
@@ -50,7 +50,7 @@ config NF_CONNTRACK
 	tristate "Netfilter connection tracking support"
 	default m if NETFILTER_ADVANCED=n
 	select NF_DEFRAG_IPV4
-	select NF_DEFRAG_IPV6 if IPV6
+	select NF_DEFRAG_IPV6 if IPV6 != n
 	help
 	  Connection tracking keeps a record of what packets have passed
 	  through your machine, in order to figure out how they are related
diff --git a/net/netfilter/Makefile b/net/netfilter/Makefile
index 0b3851e825fa..53bd1ed1228a 100644
--- a/net/netfilter/Makefile
+++ b/net/netfilter/Makefile
@@ -6,7 +6,7 @@ nf_conntrack-y	:= nf_conntrack_core.o nf_conntrack_standalone.o nf_conntrack_exp
 		   nf_conntrack_proto_icmp.o \
 		   nf_conntrack_extend.o nf_conntrack_acct.o nf_conntrack_seqadj.o
 
-nf_conntrack-$(CONFIG_IPV6) += nf_conntrack_proto_icmpv6.o
+nf_conntrack-$(subst m,y,$(CONFIG_IPV6)) += nf_conntrack_proto_icmpv6.o
 nf_conntrack-$(CONFIG_NF_CONNTRACK_TIMEOUT) += nf_conntrack_timeout.o
 nf_conntrack-$(CONFIG_NF_CONNTRACK_TIMESTAMP) += nf_conntrack_timestamp.o
 nf_conntrack-$(CONFIG_NF_CONNTRACK_EVENTS) += nf_conntrack_ecache.o

Re: [PATCH] netfilter: conntrack: add weak IPV6 dependency

[Pablo Neira Ayuso]

On Fri, Jul 06, 2018 at 08:35:48PM +0200, Florian Westphal wrote:
> Florian Westphal <fw@strlen.de> wrote:
> > Arnd Bergmann <arnd@arndb.de> wrote:
> > > and that resulted in a new build failure:
> > > 
> > > net/netfilter/nf_conntrack_proto.o:(.rodata+0x788): undefined
> > > reference to `nf_conntrack_l4proto_icmpv6'
> > > net/ipv6/netfilter/nf_conntrack_reasm.o: In function `nf_ct_frag6_expire':
> > > nf_conntrack_reasm.c:(.text+0x2320): undefined reference to
> > > `ip6_expire_frag_queue'
> > > net/ipv6/netfilter/nf_conntrack_reasm.o: In function `nf_ct_frag6_init':
> > > nf_conntrack_reasm.c:(.text+0x2384): undefined reference to `ip6_frag_init'
> > > nf_conntrack_reasm.c:(.text+0x2394): undefined reference to `ip6_frag_init'
> > > nf_conntrack_reasm.c:(.text+0x2398): undefined reference to `ip6_rhash_params'
> > > net/ipv6/netfilter/nf_conntrack_reasm.o: In function `nf_ct_frag6_expire':
> > > nf_conntrack_reasm.c:(.text+0x10bc): undefined reference to
> > > `ip6_expire_frag_queue'
> > > 
> > > I don't think we can get CONFIG_NF_DEFRAG_IPV6=y to work with IPV6=m.
> > 
> > Yes, not with current implementation, but I still don't think this
> > is unavoidable.
> > 
> > In case this is urgent I'm fine with the patch that adds the dependency,
> > otherwise I'd like to try and disentangle nf_conntrack_reasm and ipv6.
> 
> This links fine for me with IPV6=m:
> 
> Pablo, do you think this is too ugly?

Well, yes...

> It requires some copypastry from ipv6 defrag into nfct ipv6 defrag
> to avoid the link errors outlined above.

This is a bit of a regression I think.

While I think a bit of demodularization is fine, if things like this
start to kick in, it's probably good if we go make a step back...

Let me know, thanks.

Re: [PATCH] netfilter: conntrack: add weak IPV6 dependency

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Fri, Jul 06, 2018 at 08:35:48PM +0200, Florian Westphal wrote:
> > Florian Westphal <fw@strlen.de> wrote:
> > > Arnd Bergmann <arnd@arndb.de> wrote:
> > > > and that resulted in a new build failure:
> > > > 
> > > > net/netfilter/nf_conntrack_proto.o:(.rodata+0x788): undefined
> > > > reference to `nf_conntrack_l4proto_icmpv6'
> > > > net/ipv6/netfilter/nf_conntrack_reasm.o: In function `nf_ct_frag6_expire':
> > > > nf_conntrack_reasm.c:(.text+0x2320): undefined reference to
> > > > `ip6_expire_frag_queue'
> > > > net/ipv6/netfilter/nf_conntrack_reasm.o: In function `nf_ct_frag6_init':
> > > > nf_conntrack_reasm.c:(.text+0x2384): undefined reference to `ip6_frag_init'
> > > > nf_conntrack_reasm.c:(.text+0x2394): undefined reference to `ip6_frag_init'
> > > > nf_conntrack_reasm.c:(.text+0x2398): undefined reference to `ip6_rhash_params'
> > > > net/ipv6/netfilter/nf_conntrack_reasm.o: In function `nf_ct_frag6_expire':
> > > > nf_conntrack_reasm.c:(.text+0x10bc): undefined reference to
> > > > `ip6_expire_frag_queue'
> > > > 
> > > > I don't think we can get CONFIG_NF_DEFRAG_IPV6=y to work with IPV6=m.
> > > 
> > > Yes, not with current implementation, but I still don't think this
> > > is unavoidable.
> > > 
> > > In case this is urgent I'm fine with the patch that adds the dependency,
> > > otherwise I'd like to try and disentangle nf_conntrack_reasm and ipv6.
> > 
> > This links fine for me with IPV6=m:
> > 
> > Pablo, do you think this is too ugly?
> 
> Well, yes...
> 
> > It requires some copypastry from ipv6 defrag into nfct ipv6 defrag
> > to avoid the link errors outlined above.
> 
> This is a bit of a regression I think.

nf_defrag_ipv6 has always depended on ipv6.

Previously dependency chain is

nf_conntrack_ipv6 -> ipv6
             +-----> nf_defrag_ipv6 > ipv6
	     +-----> nf_conntrack

new dependency chain is:
nf_conntrack --> nf_defrag_ipv6 --> ipv6

The alternate fix is the one from Arnd to disallow
NF_CONNTRACK=y
IPV6=m

> While I think a bit of demodularization is fine, if things like this
> start to kick in, it's probably good if we go make a step back...

If you want to revert, ok.  It will prevent all l4 unification patches
plus any attempt to get rid of the nf_hook for defrag too however so I
do not like it.

> Let me know, thanks.

Yet another alternative would be to remove the

nf_conntrack -> nf_defrag_ipv6

dependency, e.g. by adding a new rcu-protected function pointer.

nf_conntrack, when ipv6 conntrack is requested, would check if its null,
and, if so, issue request_module() for ipv6 defrag before failing the
request.

However, I find it even more ugly than my patch.