| Message ID | 20190608125019.417-1-mcroce@redhat.com |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] mpls: fix af_mpls dependencies | expand |
From: Matteo Croce <mcroce@redhat.com> Date: Sat, 8 Jun 2019 14:50:19 +0200 > MPLS routing code relies on sysctl to work, so let it select PROC_SYSCTL. > > Reported-by: Randy Dunlap <rdunlap@infradead.org> > Suggested-by: David Ahern <dsahern@gmail.com> > Signed-off-by: Matteo Croce <mcroce@redhat.com> Applied, thanks.
On 6/9/19 7:57 PM, David Miller wrote: > From: Matteo Croce <mcroce@redhat.com> > Date: Sat, 8 Jun 2019 14:50:19 +0200 > >> MPLS routing code relies on sysctl to work, so let it select PROC_SYSCTL. >> >> Reported-by: Randy Dunlap <rdunlap@infradead.org> >> Suggested-by: David Ahern <dsahern@gmail.com> >> Signed-off-by: Matteo Croce <mcroce@redhat.com> > > Applied, thanks. > This patch causes build errors when # CONFIG_PROC_FS is not set because PROC_SYSCTL depends on PROC_FS. The build errors are not in fs/proc/ but in other places in the kernel that never expect to see PROC_FS not set but PROC_SYSCTL=y. I see the following 2 build errors: ../kernel/sysctl_binary.c: In function 'binary_sysctl': ../kernel/sysctl_binary.c:1305:37: error: 'struct pid_namespace' has no member named 'proc_mnt'; did you mean 'proc_work'? mnt = task_active_pid_ns(current)->proc_mnt; ^~~~~~~~ ../fs/xfs/xfs_sysctl.c:80:19: error: 'xfs_panic_mask_proc_handler' undeclared here (not in a function); did you mean 'xfs_panic_mask'? .proc_handler = xfs_panic_mask_proc_handler, ^~~~~~~~~~~~~~~~~~~~~~~~~~~ The patch's line: + select PROC_SYSCTL should not be done unless PROC_FS is enabled, e.g.: select PROC_SYSCTL if PROC_FS but that still doesn't help the mpls driver operate as it should. The patch should have been depends on PROC_SYSCTL As it stands now (in linux-next), this patch should be reverted IMO.
On Wed, Jun 12, 2019 at 1:07 AM Randy Dunlap <rdunlap@infradead.org> wrote: > > On 6/9/19 7:57 PM, David Miller wrote: > > From: Matteo Croce <mcroce@redhat.com> > > Date: Sat, 8 Jun 2019 14:50:19 +0200 > > > >> MPLS routing code relies on sysctl to work, so let it select PROC_SYSCTL. > >> > >> Reported-by: Randy Dunlap <rdunlap@infradead.org> > >> Suggested-by: David Ahern <dsahern@gmail.com> > >> Signed-off-by: Matteo Croce <mcroce@redhat.com> > > > > Applied, thanks. > > > > This patch causes build errors when > # CONFIG_PROC_FS is not set > because PROC_SYSCTL depends on PROC_FS. The build errors are not > in fs/proc/ but in other places in the kernel that never expect to see > PROC_FS not set but PROC_SYSCTL=y. > Hi, Maybe I'm missing something, if PROC_SYSCTL depends on PROC_FS, how is possible to have PROC_FS not set but PROC_SYSCTL=y? I tried it by manually editing .config. but make oldconfig warns: WARNING: unmet direct dependencies detected for PROC_SYSCTL Depends on [n]: PROC_FS [=n] Selected by [m]: - MPLS_ROUTING [=m] && NET [=y] && MPLS [=y] && (NET_IP_TUNNEL [=n] || NET_IP_TUNNEL [=n]=n) * * Restart config... * * * Configure standard kernel features (expert users) * Configure standard kernel features (expert users) (EXPERT) [Y/?] y Multiple users, groups and capabilities support (MULTIUSER) [Y/n/?] y sgetmask/ssetmask syscalls support (SGETMASK_SYSCALL) [N/y/?] n Sysfs syscall support (SYSFS_SYSCALL) [N/y/?] n Sysctl syscall support (SYSCTL_SYSCALL) [N/y/?] (NEW) Regards,
On 6/11/19 5:08 PM, Matteo Croce wrote: > On Wed, Jun 12, 2019 at 1:07 AM Randy Dunlap <rdunlap@infradead.org> wrote: >> >> On 6/9/19 7:57 PM, David Miller wrote: >>> From: Matteo Croce <mcroce@redhat.com> >>> Date: Sat, 8 Jun 2019 14:50:19 +0200 >>> >>>> MPLS routing code relies on sysctl to work, so let it select PROC_SYSCTL. >>>> >>>> Reported-by: Randy Dunlap <rdunlap@infradead.org> >>>> Suggested-by: David Ahern <dsahern@gmail.com> >>>> Signed-off-by: Matteo Croce <mcroce@redhat.com> >>> >>> Applied, thanks. >>> >> >> This patch causes build errors when >> # CONFIG_PROC_FS is not set >> because PROC_SYSCTL depends on PROC_FS. The build errors are not >> in fs/proc/ but in other places in the kernel that never expect to see >> PROC_FS not set but PROC_SYSCTL=y. >> > > Hi, > > Maybe I'm missing something, if PROC_SYSCTL depends on PROC_FS, how is > possible to have PROC_FS not set but PROC_SYSCTL=y? When MPLS=y and MPLS_ROUTING=[y|m], MPLS_ROUTING selects PROC_SYSCTL. That enables PROC_SYSCTL, whether PROC_FS is set/enabled or not. There is a warning about this in Documentation/kbuild/kconfig-language.rst: Note: select should be used with care. select will force a symbol to a value without visiting the dependencies. By abusing select you are able to select a symbol FOO even if FOO depends on BAR that is not set. In general use select only for non-visible symbols (no prompts anywhere) and for symbols with no dependencies. That will limit the usefulness but on the other hand avoid the illegal configurations all over. > I tried it by manually editing .config. but make oldconfig warns: > > WARNING: unmet direct dependencies detected for PROC_SYSCTL > Depends on [n]: PROC_FS [=n] > Selected by [m]: > - MPLS_ROUTING [=m] && NET [=y] && MPLS [=y] && (NET_IP_TUNNEL [=n] > || NET_IP_TUNNEL [=n]=n) Yes, I get this also. > * > * Restart config... > * > * > * Configure standard kernel features (expert users) > * > Configure standard kernel features (expert users) (EXPERT) [Y/?] y > Multiple users, groups and capabilities support (MULTIUSER) [Y/n/?] y > sgetmask/ssetmask syscalls support (SGETMASK_SYSCALL) [N/y/?] n > Sysfs syscall support (SYSFS_SYSCALL) [N/y/?] n > Sysctl syscall support (SYSCTL_SYSCALL) [N/y/?] (NEW) So I still say that MPLS_ROUTING should depend on PROC_SYSCTL, not select it.
On Wed, Jun 12, 2019 at 9:41 AM Randy Dunlap <rdunlap@infradead.org> wrote: > On 6/11/19 5:08 PM, Matteo Croce wrote: > > On Wed, Jun 12, 2019 at 1:07 AM Randy Dunlap <rdunlap@infradead.org> wrote: > > * Configure standard kernel features (expert users) > > * > > Configure standard kernel features (expert users) (EXPERT) [Y/?] y > > Multiple users, groups and capabilities support (MULTIUSER) [Y/n/?] y > > sgetmask/ssetmask syscalls support (SGETMASK_SYSCALL) [N/y/?] n > > Sysfs syscall support (SYSFS_SYSCALL) [N/y/?] n > > Sysctl syscall support (SYSCTL_SYSCALL) [N/y/?] (NEW) > > So I still say that MPLS_ROUTING should depend on PROC_SYSCTL, > not select it. It clearly shouldn't select PROC_SYSCTL, but I think it should not have a 'depends on' statement either. I think the correct fix for the original problem would have been something like --- a/net/mpls/af_mpls.c +++ b/net/mpls/af_mpls.c @@ -2659,6 +2659,9 @@ static int mpls_net_init(struct net *net) net->mpls.ip_ttl_propagate = 1; net->mpls.default_ttl = 255; + if (!IS_ENABLED(CONFIG_PROC_SYSCTL)) + return 0; + table = kmemdup(mpls_table, sizeof(mpls_table), GFP_KERNEL); if (table == NULL) return -ENOMEM;
On 6/14/19 8:01 AM, Arnd Bergmann wrote: > On Wed, Jun 12, 2019 at 9:41 AM Randy Dunlap <rdunlap@infradead.org> wrote: >> On 6/11/19 5:08 PM, Matteo Croce wrote: >>> On Wed, Jun 12, 2019 at 1:07 AM Randy Dunlap <rdunlap@infradead.org> wrote: >>> * Configure standard kernel features (expert users) >>> * >>> Configure standard kernel features (expert users) (EXPERT) [Y/?] y >>> Multiple users, groups and capabilities support (MULTIUSER) [Y/n/?] y >>> sgetmask/ssetmask syscalls support (SGETMASK_SYSCALL) [N/y/?] n >>> Sysfs syscall support (SYSFS_SYSCALL) [N/y/?] n >>> Sysctl syscall support (SYSCTL_SYSCALL) [N/y/?] (NEW) >> >> So I still say that MPLS_ROUTING should depend on PROC_SYSCTL, >> not select it. > > It clearly shouldn't select PROC_SYSCTL, but I think it should not > have a 'depends on' statement either. I think the correct fix for the > original problem would have been something like > > --- a/net/mpls/af_mpls.c > +++ b/net/mpls/af_mpls.c > @@ -2659,6 +2659,9 @@ static int mpls_net_init(struct net *net) > net->mpls.ip_ttl_propagate = 1; > net->mpls.default_ttl = 255; > > + if (!IS_ENABLED(CONFIG_PROC_SYSCTL)) > + return 0; > + > table = kmemdup(mpls_table, sizeof(mpls_table), GFP_KERNEL); > if (table == NULL) > return -ENOMEM; > Without sysctl, the entire mpls_router code is disabled. So if sysctl is not enabled there is no point in building this file.
On Fri, Jun 14, 2019 at 4:07 PM David Ahern <dsahern@gmail.com> wrote: > On 6/14/19 8:01 AM, Arnd Bergmann wrote: > > On Wed, Jun 12, 2019 at 9:41 AM Randy Dunlap <rdunlap@infradead.org> wrote: > >> On 6/11/19 5:08 PM, Matteo Croce wrote: > > > > It clearly shouldn't select PROC_SYSCTL, but I think it should not > > have a 'depends on' statement either. I think the correct fix for the > > original problem would have been something like > > > > --- a/net/mpls/af_mpls.c > > +++ b/net/mpls/af_mpls.c > > @@ -2659,6 +2659,9 @@ static int mpls_net_init(struct net *net) > > net->mpls.ip_ttl_propagate = 1; > > net->mpls.default_ttl = 255; > > > > + if (!IS_ENABLED(CONFIG_PROC_SYSCTL)) > > + return 0; > > + > > table = kmemdup(mpls_table, sizeof(mpls_table), GFP_KERNEL); > > if (table == NULL) > > return -ENOMEM; > > > > Without sysctl, the entire mpls_router code is disabled. So if sysctl is > not enabled there is no point in building this file. Ok, I see. There are a couple of other drivers that use 'depends on SYSCTL', which may be the right thing to do here. In theory, one can still build a kernel with CONFIG_SYSCTRL_SYSCALL=y and no procfs. Arnd
Arnd Bergmann <arnd@arndb.de> writes: > On Fri, Jun 14, 2019 at 4:07 PM David Ahern <dsahern@gmail.com> wrote: >> On 6/14/19 8:01 AM, Arnd Bergmann wrote: >> > On Wed, Jun 12, 2019 at 9:41 AM Randy Dunlap <rdunlap@infradead.org> wrote: >> >> On 6/11/19 5:08 PM, Matteo Croce wrote: >> > >> > It clearly shouldn't select PROC_SYSCTL, but I think it should not >> > have a 'depends on' statement either. I think the correct fix for the >> > original problem would have been something like >> > >> > --- a/net/mpls/af_mpls.c >> > +++ b/net/mpls/af_mpls.c >> > @@ -2659,6 +2659,9 @@ static int mpls_net_init(struct net *net) >> > net->mpls.ip_ttl_propagate = 1; >> > net->mpls.default_ttl = 255; >> > >> > + if (!IS_ENABLED(CONFIG_PROC_SYSCTL)) >> > + return 0; >> > + >> > table = kmemdup(mpls_table, sizeof(mpls_table), GFP_KERNEL); >> > if (table == NULL) >> > return -ENOMEM; >> > >> >> Without sysctl, the entire mpls_router code is disabled. So if sysctl is >> not enabled there is no point in building this file. > > Ok, I see. > > There are a couple of other drivers that use 'depends on SYSCTL', > which may be the right thing to do here. In theory, one can still > build a kernel with CONFIG_SYSCTRL_SYSCALL=y and no > procfs. Which reminds me. I really need to write the patch to remove CONFIG_SYSCTL_SYSCALL. Unless I have missed something we have finally reached default off in all of the distributions so no one should care. Eric
On 6/14/19 7:26 AM, Arnd Bergmann wrote: > On Fri, Jun 14, 2019 at 4:07 PM David Ahern <dsahern@gmail.com> wrote: >> On 6/14/19 8:01 AM, Arnd Bergmann wrote: >>> On Wed, Jun 12, 2019 at 9:41 AM Randy Dunlap <rdunlap@infradead.org> wrote: >>>> On 6/11/19 5:08 PM, Matteo Croce wrote: >>> >>> It clearly shouldn't select PROC_SYSCTL, but I think it should not >>> have a 'depends on' statement either. I think the correct fix for the >>> original problem would have been something like >>> >>> --- a/net/mpls/af_mpls.c >>> +++ b/net/mpls/af_mpls.c >>> @@ -2659,6 +2659,9 @@ static int mpls_net_init(struct net *net) >>> net->mpls.ip_ttl_propagate = 1; >>> net->mpls.default_ttl = 255; >>> >>> + if (!IS_ENABLED(CONFIG_PROC_SYSCTL)) >>> + return 0; >>> + >>> table = kmemdup(mpls_table, sizeof(mpls_table), GFP_KERNEL); >>> if (table == NULL) >>> return -ENOMEM; >>> >> >> Without sysctl, the entire mpls_router code is disabled. So if sysctl is >> not enabled there is no point in building this file. > > Ok, I see. > > There are a couple of other drivers that use 'depends on SYSCTL', > which may be the right thing to do here. In theory, one can still > build a kernel with CONFIG_SYSCTRL_SYSCALL=y and no > procfs. Yes, that makes sense.
diff --git a/net/mpls/Kconfig b/net/mpls/Kconfig index d9391beea980..2b802a48d5a6 100644 --- a/net/mpls/Kconfig +++ b/net/mpls/Kconfig @@ -26,6 +26,7 @@ config NET_MPLS_GSO config MPLS_ROUTING tristate "MPLS: routing support" depends on NET_IP_TUNNEL || NET_IP_TUNNEL=n + select PROC_SYSCTL ---help--- Add support for forwarding of mpls packets.
MPLS routing code relies on sysctl to work, so let it select PROC_SYSCTL. Reported-by: Randy Dunlap <rdunlap@infradead.org> Suggested-by: David Ahern <dsahern@gmail.com> Signed-off-by: Matteo Croce <mcroce@redhat.com> --- net/mpls/Kconfig | 1 + 1 file changed, 1 insertion(+)