[PATCH] netfilter: NFT_SOCKET don't use NF_SOCKET_IPV6 without NF_TABLES_IPV6

[PATCH] netfilter: NFT_SOCKET don't use NF_SOCKET_IPV6 without NF_TABLES_IPV6

[Arnd Bergmann]

It is now possible to build the nft_socket module as built-in when
NF_TABLES_IPV6 is disabled, and have NF_SOCKET_IPV6=m set manually.

In this case, the NF_SOCKET_IPV6 functionality will be useless according
to the explanation in commit 35bf1ccecaaa ("netfilter: Kconfig: Change
IPv6 select dependencies"), but on top of that it also causes a link
error:

net/netfilter/nft_socket.o: In function `nft_socket_eval':
nft_socket.c:(.text+0x162): undefined reference to `nf_sk_lookup_slow_v6'

This changes the compile-time check so we don't attempt to use
the NF_SOCKET_IPV6 code when it cannot be used, and make it all
compile again. That may lead to unexpected behavior when a user
enables NF_SOCKET_IPV6 but cannot use it, but seems to be the
logical conclusion of the 35bf1ccecaaa change.

Fixes: 35bf1ccecaaa ("netfilter: Kconfig: Change IPv6 select dependencies")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
---
 net/netfilter/nft_socket.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/nft_socket.c b/net/netfilter/nft_socket.c
index 998c2b546f6d..e43c1939d25f 100644
--- a/net/netfilter/nft_socket.c
+++ b/net/netfilter/nft_socket.c
@@ -31,7 +31,7 @@ static void nft_socket_eval(const struct nft_expr *expr,
 		case NFPROTO_IPV4:
 			sk = nf_sk_lookup_slow_v4(nft_net(pkt), skb, nft_in(pkt));
 			break;
-#if IS_ENABLED(CONFIG_NF_SOCKET_IPV6)
+#if IS_ENABLED(CONFIG_NF_TABLES_IPV6)
 		case NFPROTO_IPV6:
 			sk = nf_sk_lookup_slow_v6(nft_net(pkt), skb, nft_in(pkt));
 			break;
@@ -77,7 +77,7 @@ static int nft_socket_init(const struct nft_ctx *ctx,
 
 	switch(ctx->family) {
 	case NFPROTO_IPV4:
-#if IS_ENABLED(CONFIG_NF_SOCKET_IPV6)
+#if IS_ENABLED(CONFIG_NF_TABLES_IPV6)
 	case NFPROTO_IPV6:
 #endif
 	case NFPROTO_INET:
-- 
2.9.0

Re: [PATCH] netfilter: NFT_SOCKET don't use NF_SOCKET_IPV6 without NF_TABLES_IPV6

[Máté Eckl]

On Mon, Jul 09, 2018 at 11:35:09PM +0200, Arnd Bergmann wrote:
> It is now possible to build the nft_socket module as built-in when
> NF_TABLES_IPV6 is disabled, and have NF_SOCKET_IPV6=m set manually.
> 
> In this case, the NF_SOCKET_IPV6 functionality will be useless according
> to the explanation in commit 35bf1ccecaaa ("netfilter: Kconfig: Change
> IPv6 select dependencies"), but on top of that it also causes a link
> error:
> 
> net/netfilter/nft_socket.o: In function `nft_socket_eval':
> nft_socket.c:(.text+0x162): undefined reference to `nf_sk_lookup_slow_v6'
> 
> This changes the compile-time check so we don't attempt to use
> the NF_SOCKET_IPV6 code when it cannot be used, and make it all
> compile again. That may lead to unexpected behavior when a user
> enables NF_SOCKET_IPV6 but cannot use it, but seems to be the
> logical conclusion of the 35bf1ccecaaa change.
> 
> Fixes: 35bf1ccecaaa ("netfilter: Kconfig: Change IPv6 select dependencies")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

I think this should be fixed in the Kconfig rather than inside the module(s).

I did some investigation and it turns out that you missed a circumstance. This
link error occures only if NFT_SOCKET=y && NF_SOCKET_IPV6=m && NF_TABLES_IPV6=y
(cannot be m here if NFT_SOCKET is y). And probably the same with
iptables-related modules. Probably this possibility should be eliminated.

Re: [PATCH] netfilter: NFT_SOCKET don't use NF_SOCKET_IPV6 without NF_TABLES_IPV6

[Máté Eckl]

On Tue, Jul 10, 2018 at 10:02:27AM +0200, Máté Eckl wrote:
> On Mon, Jul 09, 2018 at 11:35:09PM +0200, Arnd Bergmann wrote:
> > It is now possible to build the nft_socket module as built-in when
> > NF_TABLES_IPV6 is disabled, and have NF_SOCKET_IPV6=m set manually.
> > 
> > In this case, the NF_SOCKET_IPV6 functionality will be useless according
> > to the explanation in commit 35bf1ccecaaa ("netfilter: Kconfig: Change
> > IPv6 select dependencies"), but on top of that it also causes a link
> > error:
> > 
> > net/netfilter/nft_socket.o: In function `nft_socket_eval':
> > nft_socket.c:(.text+0x162): undefined reference to `nf_sk_lookup_slow_v6'
> > 
> > This changes the compile-time check so we don't attempt to use
> > the NF_SOCKET_IPV6 code when it cannot be used, and make it all
> > compile again. That may lead to unexpected behavior when a user
> > enables NF_SOCKET_IPV6 but cannot use it, but seems to be the
> > logical conclusion of the 35bf1ccecaaa change.
> > 
> > Fixes: 35bf1ccecaaa ("netfilter: Kconfig: Change IPv6 select dependencies")
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> 
> I think this should be fixed in the Kconfig rather than inside the module(s).
> 
> I did some investigation and it turns out that you missed a circumstance. This
> link error occures only if NFT_SOCKET=y && NF_SOCKET_IPV6=m && NF_TABLES_IPV6=y
> (cannot be m here if NFT_SOCKET is y). And probably the same with
> iptables-related modules. Probably this possibility should be eliminated.

NF_TPROXY_IPV6 might be in the same situation.

Re: [PATCH] netfilter: NFT_SOCKET don't use NF_SOCKET_IPV6 without NF_TABLES_IPV6

[Arnd Bergmann]

On Tue, Jul 10, 2018 at 10:05 AM, Máté Eckl <ecklm94@gmail.com> wrote:
> On Tue, Jul 10, 2018 at 10:02:27AM +0200, Máté Eckl wrote:
>> On Mon, Jul 09, 2018 at 11:35:09PM +0200, Arnd Bergmann wrote:
>> > It is now possible to build the nft_socket module as built-in when
>> > NF_TABLES_IPV6 is disabled, and have NF_SOCKET_IPV6=m set manually.
>> >
>> > In this case, the NF_SOCKET_IPV6 functionality will be useless according
>> > to the explanation in commit 35bf1ccecaaa ("netfilter: Kconfig: Change
>> > IPv6 select dependencies"), but on top of that it also causes a link
>> > error:
>> >
>> > net/netfilter/nft_socket.o: In function `nft_socket_eval':
>> > nft_socket.c:(.text+0x162): undefined reference to `nf_sk_lookup_slow_v6'
>> >
>> > This changes the compile-time check so we don't attempt to use
>> > the NF_SOCKET_IPV6 code when it cannot be used, and make it all
>> > compile again. That may lead to unexpected behavior when a user
>> > enables NF_SOCKET_IPV6 but cannot use it, but seems to be the
>> > logical conclusion of the 35bf1ccecaaa change.
>> >
>> > Fixes: 35bf1ccecaaa ("netfilter: Kconfig: Change IPv6 select dependencies")
>> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>>
>> I think this should be fixed in the Kconfig rather than inside the module(s).

Should we revert your patch then, or do you have a better idea?

>> I did some investigation and it turns out that you missed a circumstance. This
>> link error occures only if NFT_SOCKET=y && NF_SOCKET_IPV6=m && NF_TABLES_IPV6=y
>> (cannot be m here if NFT_SOCKET is y).

No, if NF_TABLES_IPV6=y the problem cannot happen, since NFT_SOCKET then
selects NF_SOCKET_IPV6=y as well. Before your patch, it would always select
NF_SOCKET_IPV6 when it could, so it worked in all configurations.

>>  And probably the same with
>> iptables-related modules. Probably this possibility should be eliminated.
>
> NF_TPROXY_IPV6 might be in the same situation.

I tried coming up with a combination that is broken for NF_TPROXY_IPV6=m
but could not. From what I can see with

config NETFILTER_XT_TARGET_TPROXY
       tristate '"TPROXY" target transparent proxying support'
       depends on IP6_NF_IPTABLES || IP6_NF_IPTABLES=n
       select NF_TPROXY_IPV6 if IP6_NF_IPTABLES

and

#if IS_ENABLED(CONFIG_IP6_NF_IPTABLES)

inside of net/netfilter/xt_TPROXY.c, there is no way we can end up with
xt_TPROXY calling into the nf_tproxy_ipv6 loadable module from
a built-in context. This is the same approach I used in my patch,
just with IP6_NF_IPTABLES instead of NF_SOCKET_IPV6, in both
the Kconfig dependency and the module.

      Arnd

Re: [PATCH] netfilter: NFT_SOCKET don't use NF_SOCKET_IPV6 without NF_TABLES_IPV6

[Máté Eckl]

On Tue, Jul 10, 2018 at 11:10:40AM +0200, Arnd Bergmann wrote:
> On Tue, Jul 10, 2018 at 10:05 AM, Máté Eckl <ecklm94@gmail.com> wrote:
> > On Tue, Jul 10, 2018 at 10:02:27AM +0200, Máté Eckl wrote:
> >> On Mon, Jul 09, 2018 at 11:35:09PM +0200, Arnd Bergmann wrote:
> >> > It is now possible to build the nft_socket module as built-in when
> >> > NF_TABLES_IPV6 is disabled, and have NF_SOCKET_IPV6=m set manually.
> >> >
> >> > In this case, the NF_SOCKET_IPV6 functionality will be useless according
> >> > to the explanation in commit 35bf1ccecaaa ("netfilter: Kconfig: Change
> >> > IPv6 select dependencies"), but on top of that it also causes a link
> >> > error:
> >> >
> >> > net/netfilter/nft_socket.o: In function `nft_socket_eval':
> >> > nft_socket.c:(.text+0x162): undefined reference to `nf_sk_lookup_slow_v6'
> >> >
> >> > This changes the compile-time check so we don't attempt to use
> >> > the NF_SOCKET_IPV6 code when it cannot be used, and make it all
> >> > compile again. That may lead to unexpected behavior when a user
> >> > enables NF_SOCKET_IPV6 but cannot use it, but seems to be the
> >> > logical conclusion of the 35bf1ccecaaa change.
> >> >
> >> > Fixes: 35bf1ccecaaa ("netfilter: Kconfig: Change IPv6 select dependencies")
> >> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> >>
> >> I think this should be fixed in the Kconfig rather than inside the module(s).
> 
> Should we revert your patch then, or do you have a better idea?
> 
> >> I did some investigation and it turns out that you missed a circumstance. This
> >> link error occures only if NFT_SOCKET=y && NF_SOCKET_IPV6=m && NF_TABLES_IPV6=y
> >> (cannot be m here if NFT_SOCKET is y).
> 
> No, if NF_TABLES_IPV6=y the problem cannot happen, since NFT_SOCKET then
> selects NF_SOCKET_IPV6=y as well. Before your patch, it would always select
> NF_SOCKET_IPV6 when it could, so it worked in all configurations.

Sorry I wanted to write NF_TABLES_IPV6=n...
So: NFT_SOCKET=y && NF_SOCKET_IPV6=m && NF_TABLES_IPV6=n causes linkage error.
NFT_SOCKET=m && NF_SOCKET_IPV6=m && NF_TABLES_IPV6=n compiles fine.

> >>  And probably the same with
> >> iptables-related modules. Probably this possibility should be eliminated.
> >
> > NF_TPROXY_IPV6 might be in the same situation.
> 
> I tried coming up with a combination that is broken for NF_TPROXY_IPV6=m
> but could not. From what I can see with
> 
> config NETFILTER_XT_TARGET_TPROXY
>        tristate '"TPROXY" target transparent proxying support'
>        depends on IP6_NF_IPTABLES || IP6_NF_IPTABLES=n
>        select NF_TPROXY_IPV6 if IP6_NF_IPTABLES
> 
> and
> 
> #if IS_ENABLED(CONFIG_IP6_NF_IPTABLES)
> 
> inside of net/netfilter/xt_TPROXY.c, there is no way we can end up with
> xt_TPROXY calling into the nf_tproxy_ipv6 loadable module from
> a built-in context. This is the same approach I used in my patch,
> just with IP6_NF_IPTABLES instead of NF_SOCKET_IPV6, in both
> the Kconfig dependency and the module.

Right, I see your point. I have an alternative solution which seems more robust
to me, but I might be overthinking this situation.

So
Accepted-by: Máté Eckl <ecklm94@gmail.com>

>       Arnd

Re: [PATCH] netfilter: NFT_SOCKET don't use NF_SOCKET_IPV6 without NF_TABLES_IPV6

[Pablo Neira Ayuso]

On Tue, Jul 10, 2018 at 11:10:40AM +0200, Arnd Bergmann wrote:
> On Tue, Jul 10, 2018 at 10:05 AM, Máté Eckl <ecklm94@gmail.com> wrote:
> > On Tue, Jul 10, 2018 at 10:02:27AM +0200, Máté Eckl wrote:
> >> On Mon, Jul 09, 2018 at 11:35:09PM +0200, Arnd Bergmann wrote:
> >> > It is now possible to build the nft_socket module as built-in when
> >> > NF_TABLES_IPV6 is disabled, and have NF_SOCKET_IPV6=m set manually.
> >> >
> >> > In this case, the NF_SOCKET_IPV6 functionality will be useless according
> >> > to the explanation in commit 35bf1ccecaaa ("netfilter: Kconfig: Change
> >> > IPv6 select dependencies"), but on top of that it also causes a link
> >> > error:
> >> >
> >> > net/netfilter/nft_socket.o: In function `nft_socket_eval':
> >> > nft_socket.c:(.text+0x162): undefined reference to `nf_sk_lookup_slow_v6'
> >> >
> >> > This changes the compile-time check so we don't attempt to use
> >> > the NF_SOCKET_IPV6 code when it cannot be used, and make it all
> >> > compile again. That may lead to unexpected behavior when a user
> >> > enables NF_SOCKET_IPV6 but cannot use it, but seems to be the
> >> > logical conclusion of the 35bf1ccecaaa change.
> >> >
> >> > Fixes: 35bf1ccecaaa ("netfilter: Kconfig: Change IPv6 select dependencies")
> >> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> >>
> >> I think this should be fixed in the Kconfig rather than inside the module(s).
> 
> Should we revert your patch then, or do you have a better idea?

Máté, would you resubmit a new patch that addresses all the problems
that Arnd is reporting in one go?

I think it's better if we toss your original patch in the tree and
rebase, ie. take the new one that fixes all issues that Arnd is
reporting. It would be good if we can sort out this before I send the
next pull request for net-next stuff.

I was afraid of fallout like this when I saw your original patch,
kbuild is always tricky.

Please Cc Arnd, Florian and me for review.

Thanks!

Re: [PATCH] netfilter: NFT_SOCKET don't use NF_SOCKET_IPV6 without NF_TABLES_IPV6

[Máté Eckl]

On Tue, Jul 10, 2018 at 12:56:05PM +0200, Pablo Neira Ayuso wrote:
> On Tue, Jul 10, 2018 at 11:10:40AM +0200, Arnd Bergmann wrote:
> > On Tue, Jul 10, 2018 at 10:05 AM, Máté Eckl <ecklm94@gmail.com> wrote:
> > > On Tue, Jul 10, 2018 at 10:02:27AM +0200, Máté Eckl wrote:
> > >> On Mon, Jul 09, 2018 at 11:35:09PM +0200, Arnd Bergmann wrote:
> > >> > It is now possible to build the nft_socket module as built-in when
> > >> > NF_TABLES_IPV6 is disabled, and have NF_SOCKET_IPV6=m set manually.
> > >> >
> > >> > In this case, the NF_SOCKET_IPV6 functionality will be useless according
> > >> > to the explanation in commit 35bf1ccecaaa ("netfilter: Kconfig: Change
> > >> > IPv6 select dependencies"), but on top of that it also causes a link
> > >> > error:
> > >> >
> > >> > net/netfilter/nft_socket.o: In function `nft_socket_eval':
> > >> > nft_socket.c:(.text+0x162): undefined reference to `nf_sk_lookup_slow_v6'
> > >> >
> > >> > This changes the compile-time check so we don't attempt to use
> > >> > the NF_SOCKET_IPV6 code when it cannot be used, and make it all
> > >> > compile again. That may lead to unexpected behavior when a user
> > >> > enables NF_SOCKET_IPV6 but cannot use it, but seems to be the
> > >> > logical conclusion of the 35bf1ccecaaa change.
> > >> >
> > >> > Fixes: 35bf1ccecaaa ("netfilter: Kconfig: Change IPv6 select dependencies")
> > >> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > >>
> > >> I think this should be fixed in the Kconfig rather than inside the module(s).
> > 
> > Should we revert your patch then, or do you have a better idea?
> 
> Máté, would you resubmit a new patch that addresses all the problems
> that Arnd is reporting in one go?

This patch only solves the nf_socket and nft_socket modules problem so I can
only submit a v2 for 'netfilter: Kconfig: Change IPv6 select dependencies' but
you already applied it so it would meen a force push. Should I do this?

I think Arnd's patch solves these problems in case we don't want to force-push
or rebase.

> I think it's better if we toss your original patch in the tree and
> rebase, ie. take the new one that fixes all issues that Arnd is
> reporting. It would be good if we can sort out this before I send the
> next pull request for net-next stuff.
> 
> I was afraid of fallout like this when I saw your original patch,
> kbuild is always tricky.

This patch is not related to the nft_tproxy module (it seems that you refer to
that) as Arnd didn't have that in the tree when doing this. I'll send a v4 fot
the tproxy module, but that cannot be related to this one as it is not in tree
yet.

> Please Cc Arnd, Florian and me for review.
> 
> Thanks!

Re: [PATCH] netfilter: NFT_SOCKET don't use NF_SOCKET_IPV6 without NF_TABLES_IPV6

[Pablo Neira Ayuso]

On Tue, Jul 10, 2018 at 01:15:36PM +0200, Máté Eckl wrote:
> On Tue, Jul 10, 2018 at 12:56:05PM +0200, Pablo Neira Ayuso wrote:
[...]
> This patch only solves the nf_socket and nft_socket modules problem so I can
> only submit a v2 for 'netfilter: Kconfig: Change IPv6 select dependencies' but
> you already applied it so it would meen a force push. Should I do this?
> 
> I think Arnd's patch solves these problems in case we don't want to force-push
> or rebase.

You are refering to these two patches, right?

https://patchwork.ozlabs.org/patch/941374/
https://patchwork.ozlabs.org/patch/941696/

> > I think it's better if we toss your original patch in the tree and
> > rebase, ie. take the new one that fixes all issues that Arnd is
> > reporting. It would be good if we can sort out this before I send the
> > next pull request for net-next stuff.
> > 
> > I was afraid of fallout like this when I saw your original patch,
> > kbuild is always tricky.
> 
> This patch is not related to the nft_tproxy module (it seems that you refer to
> that) as Arnd didn't have that in the tree when doing this. I'll send a v4 fot
> the tproxy module, but that cannot be related to this one as it is not in tree
> yet.

No, I'm refering to 35bf1ccecaaa ("netfilter: Kconfig: Change IPv6
select dependencies"), that is causing the issues.

Thanks.

Re: [PATCH] netfilter: NFT_SOCKET don't use NF_SOCKET_IPV6 without NF_TABLES_IPV6

[Máté Eckl]

On Tue, Jul 10, 2018 at 01:24:59PM +0200, Pablo Neira Ayuso wrote:
> On Tue, Jul 10, 2018 at 01:15:36PM +0200, Máté Eckl wrote:
> > On Tue, Jul 10, 2018 at 12:56:05PM +0200, Pablo Neira Ayuso wrote:
> [...]
> > This patch only solves the nf_socket and nft_socket modules problem so I can
> > only submit a v2 for 'netfilter: Kconfig: Change IPv6 select dependencies' but
> > you already applied it so it would meen a force push. Should I do this?
> > 
> > I think Arnd's patch solves these problems in case we don't want to force-push
> > or rebase.
> 
> You are refering to these two patches, right?
> 
> https://patchwork.ozlabs.org/patch/941374/
> https://patchwork.ozlabs.org/patch/941696/

Oh, I missed the first one. So basically I should just squash them and resubmit
right? Or even replace ("netfilter: Kconfig: Change IPv6 select
dependencies") with these changes?

> > > I think it's better if we toss your original patch in the tree and
> > > rebase, ie. take the new one that fixes all issues that Arnd is
> > > reporting. It would be good if we can sort out this before I send the
> > > next pull request for net-next stuff.
> > > 
> > > I was afraid of fallout like this when I saw your original patch,
> > > kbuild is always tricky.
> > 
> > This patch is not related to the nft_tproxy module (it seems that you refer to
> > that) as Arnd didn't have that in the tree when doing this. I'll send a v4 fot
> > the tproxy module, but that cannot be related to this one as it is not in tree
> > yet.
> 
> No, I'm refering to 35bf1ccecaaa ("netfilter: Kconfig: Change IPv6
> select dependencies"), that is causing the issues.
> 
> Thanks.

Re: [PATCH] netfilter: NFT_SOCKET don't use NF_SOCKET_IPV6 without NF_TABLES_IPV6

[Pablo Neira Ayuso]

On Tue, Jul 10, 2018 at 01:45:17PM +0200, Máté Eckl wrote:
> On Tue, Jul 10, 2018 at 01:24:59PM +0200, Pablo Neira Ayuso wrote:
> > On Tue, Jul 10, 2018 at 01:15:36PM +0200, Máté Eckl wrote:
> > > On Tue, Jul 10, 2018 at 12:56:05PM +0200, Pablo Neira Ayuso wrote:
> > [...]
> > > This patch only solves the nf_socket and nft_socket modules problem so I can
> > > only submit a v2 for 'netfilter: Kconfig: Change IPv6 select dependencies' but
> > > you already applied it so it would meen a force push. Should I do this?
> > > 
> > > I think Arnd's patch solves these problems in case we don't want to force-push
> > > or rebase.
> > 
> > You are refering to these two patches, right?
> > 
> > https://patchwork.ozlabs.org/patch/941374/
> > https://patchwork.ozlabs.org/patch/941696/
> 
> Oh, I missed the first one. So basically I should just squash them and resubmit
> right? Or even replace ("netfilter: Kconfig: Change IPv6 select
> dependencies") with these changes?

That's one possibility, yes.