[PATCH net] mpls: fix af_mpls dependencies

[PATCH net] mpls: fix af_mpls dependencies

[Matteo Croce]

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(+)

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.
 
-- 
2.21.0

Re: [PATCH net] mpls: fix af_mpls dependencies

[David Miller]

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.

Re: [PATCH net] mpls: fix af_mpls dependencies

[Randy Dunlap]

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.


-- 
~Randy

Re: [PATCH net] mpls: fix af_mpls dependencies

[Matteo Croce]

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,
-- 
Matteo Croce
per aspera ad upstream

Re: [PATCH net] mpls: fix af_mpls dependencies

[Randy Dunlap]

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.

-- 
~Randy

Re: [PATCH net] mpls: fix af_mpls dependencies

[Arnd Bergmann]

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;

Re: [PATCH net] mpls: fix af_mpls dependencies

[David Ahern]

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.

Re: [PATCH net] mpls: fix af_mpls dependencies

[Arnd Bergmann]

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

Re: [PATCH net] mpls: fix af_mpls dependencies

[Eric W. Biederman]

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

Re: [PATCH net] mpls: fix af_mpls dependencies

[Randy Dunlap]

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.

-- 
~Randy