diff mbox series

netfilter: NFT_SOCKET don't use NF_SOCKET_IPV6 without NF_TABLES_IPV6

Message ID 20180709213537.2748896-1-arnd@arndb.de
State New
Headers show
Series netfilter: NFT_SOCKET don't use NF_SOCKET_IPV6 without NF_TABLES_IPV6 | expand

Commit Message

Arnd Bergmann July 9, 2018, 9:35 p.m. UTC 
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(-)

-- 
2.9.0

Comments

Máté Eckl July 10, 2018, 8:02 a.m. UTC | #1 
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.
Máté Eckl July 10, 2018, 8:05 a.m. UTC | #2 
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.
Arnd Bergmann July 10, 2018, 9:10 a.m. UTC | #3 
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
Máté Eckl July 10, 2018, 9:51 a.m. UTC | #4 
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
Máté Eckl July 10, 2018, 11:15 a.m. UTC | #5 
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!
Pablo Neira Ayuso July 10, 2018, 11:24 a.m. UTC | #6 
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.
Máté Eckl July 10, 2018, 11:45 a.m. UTC | #7 
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.
Pablo Neira Ayuso July 10, 2018, 12:14 p.m. UTC | #8 
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.
diff mbox series

Patch

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: