| Message ID | 20180709213537.2748896-1-arnd@arndb.de |
|---|---|
| State | Accepted |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | netfilter: NFT_SOCKET don't use NF_SOCKET_IPV6 without NF_TABLES_IPV6 | expand |
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. -- 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 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. -- 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 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 -- 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 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 -- 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 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! -- 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 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! -- 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 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. -- 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 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. -- 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 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. -- 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/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:
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(-)