All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/1] net: Fix use of proc_fs
@ 2020-12-11 16:37 Yonatan Linik
  2020-12-11 16:37 ` [PATCH 1/1] " Yonatan Linik
  0 siblings, 1 reply; 9+ messages in thread
From: Yonatan Linik @ 2020-12-11 16:37 UTC (permalink / raw)
  To: davem, kuba
  Cc: ast, daniel, kafai, songliubraving, yhs, andriin, john.fastabend,
	kpsingh, willemb, john.ogness, arnd, maowenan, colin.king,
	orcohen, netdev, linux-kernel, bpf, Yonatan Linik

This patch fixes the failure of af_packet module initialization when
CONFIG_PROC_FS=n.

The commit message itself has a pretty thorough explanation.
I will just add that I made sure this fixes the problem, both by
using socket from userspace and by looking at kernel logs.

Yonatan Linik (1):
  net: Fix use of proc_fs

 net/packet/af_packet.c | 2 ++
 1 file changed, 2 insertions(+)


base-commit: bbf5c979011a099af5dc76498918ed7df445635b
-- 
2.25.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/1] net: Fix use of proc_fs
  2020-12-11 16:37 [PATCH 0/1] net: Fix use of proc_fs Yonatan Linik
@ 2020-12-11 16:37 ` Yonatan Linik
  2020-12-11 20:59   ` Arnd Bergmann
  2020-12-12 19:48   ` Jakub Kicinski
  0 siblings, 2 replies; 9+ messages in thread
From: Yonatan Linik @ 2020-12-11 16:37 UTC (permalink / raw)
  To: davem, kuba
  Cc: ast, daniel, kafai, songliubraving, yhs, andriin, john.fastabend,
	kpsingh, willemb, john.ogness, arnd, maowenan, colin.king,
	orcohen, netdev, linux-kernel, bpf, Yonatan Linik

proc_fs was used, in af_packet, without a surrounding #ifdef,
although there is no hard dependency on proc_fs.
That caused the initialization of the af_packet module to fail
when CONFIG_PROC_FS=n.

Specifically, proc_create_net() was used in af_packet.c,
and when it fails, packet_net_init() returns -ENOMEM.
It will always fail when the kernel is compiled without proc_fs,
because, proc_create_net() for example always returns NULL.

The calling order that starts in af_packet.c is as follows:
packet_init()
register_pernet_subsys()
register_pernet_operations()
__register_pernet_operations()
ops_init()
ops->init() (packet_net_ops.init=packet_net_init())
proc_create_net()

It worked in the past because register_pernet_subsys()'s return value
wasn't checked before this Commit 36096f2f4fa0 ("packet: Fix error path in
packet_init.").
It always returned an error, but was not checked before, so everything
was working even when CONFIG_PROC_FS=n.

The fix here is simply to add the necessary #ifdef.

Signed-off-by: Yonatan Linik <yonatanlinik@gmail.com>
---
 net/packet/af_packet.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 2b33e977a905..031f2b593720 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -4612,9 +4612,11 @@ static int __net_init packet_net_init(struct net *net)
 	mutex_init(&net->packet.sklist_lock);
 	INIT_HLIST_HEAD(&net->packet.sklist);
 
+#ifdef CONFIG_PROC_FS
 	if (!proc_create_net("packet", 0, net->proc_net, &packet_seq_ops,
 			sizeof(struct seq_net_private)))
 		return -ENOMEM;
+#endif /* CONFIG_PROC_FS */
 
 	return 0;
 }
-- 
2.25.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1] net: Fix use of proc_fs
  2020-12-11 16:37 ` [PATCH 1/1] " Yonatan Linik
@ 2020-12-11 20:59   ` Arnd Bergmann
  2020-12-12  8:39     ` Yonatan Linik
  2020-12-12 19:48   ` Jakub Kicinski
  1 sibling, 1 reply; 9+ messages in thread
From: Arnd Bergmann @ 2020-12-11 20:59 UTC (permalink / raw)
  To: Yonatan Linik
  Cc: David Miller, Jakub Kicinski, Alexei Starovoitov,
	Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
	Andrii Nakryiko, John Fastabend, KP Singh, Willem de Bruijn,
	john.ogness, Arnd Bergmann, Mao Wenan, Colin Ian King, orcohen,
	Networking, linux-kernel, bpf

On Fri, Dec 11, 2020 at 5:37 PM Yonatan Linik <yonatanlinik@gmail.com> wrote:

> index 2b33e977a905..031f2b593720 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -4612,9 +4612,11 @@ static int __net_init packet_net_init(struct net *net)
>         mutex_init(&net->packet.sklist_lock);
>         INIT_HLIST_HEAD(&net->packet.sklist);
>
> +#ifdef CONFIG_PROC_FS
>         if (!proc_create_net("packet", 0, net->proc_net, &packet_seq_ops,
>                         sizeof(struct seq_net_private)))
>                 return -ENOMEM;
> +#endif /* CONFIG_PROC_FS */
>

Another option would be to just ignore the return code here
and continue without a procfs file, regardless of whether procfs
is enabled or not.

       Arnd

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1] net: Fix use of proc_fs
  2020-12-11 20:59   ` Arnd Bergmann
@ 2020-12-12  8:39     ` Yonatan Linik
  0 siblings, 0 replies; 9+ messages in thread
From: Yonatan Linik @ 2020-12-12  8:39 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: David Miller, Jakub Kicinski, Alexei Starovoitov,
	Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
	Andrii Nakryiko, John Fastabend, KP Singh, Willem de Bruijn,
	john.ogness, Arnd Bergmann, Mao Wenan, Colin Ian King, orcohen,
	Networking, linux-kernel, bpf

On Fri, Dec 11, 2020 at 11:00 PM Arnd Bergmann <arnd@kernel.org> wrote:
>
> Another option would be to just ignore the return code here
> and continue without a procfs file, regardless of whether procfs
> is enabled or not.
>
>        Arnd

Yes I thought about that, but I didn't want to make changes to the way
it behaved when procfs was enabled.
If you decide that's a better solution I will happily change it.

-- 
Yonatan Linik

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1] net: Fix use of proc_fs
  2020-12-11 16:37 ` [PATCH 1/1] " Yonatan Linik
  2020-12-11 20:59   ` Arnd Bergmann
@ 2020-12-12 19:48   ` Jakub Kicinski
  2020-12-12 21:39     ` Yonatan Linik
  1 sibling, 1 reply; 9+ messages in thread
From: Jakub Kicinski @ 2020-12-12 19:48 UTC (permalink / raw)
  To: Yonatan Linik
  Cc: davem, ast, daniel, kafai, songliubraving, yhs, andriin,
	john.fastabend, kpsingh, willemb, john.ogness, arnd, maowenan,
	colin.king, orcohen, netdev, linux-kernel, bpf

On Fri, 11 Dec 2020 18:37:49 +0200 Yonatan Linik wrote:
> proc_fs was used, in af_packet, without a surrounding #ifdef,
> although there is no hard dependency on proc_fs.
> That caused the initialization of the af_packet module to fail
> when CONFIG_PROC_FS=n.
> 
> Specifically, proc_create_net() was used in af_packet.c,
> and when it fails, packet_net_init() returns -ENOMEM.
> It will always fail when the kernel is compiled without proc_fs,
> because, proc_create_net() for example always returns NULL.
> 
> The calling order that starts in af_packet.c is as follows:
> packet_init()
> register_pernet_subsys()
> register_pernet_operations()
> __register_pernet_operations()
> ops_init()
> ops->init() (packet_net_ops.init=packet_net_init())
> proc_create_net()
> 
> It worked in the past because register_pernet_subsys()'s return value
> wasn't checked before this Commit 36096f2f4fa0 ("packet: Fix error path in
> packet_init.").
> It always returned an error, but was not checked before, so everything
> was working even when CONFIG_PROC_FS=n.
> 
> The fix here is simply to add the necessary #ifdef.
> 
> Signed-off-by: Yonatan Linik <yonatanlinik@gmail.com>

Hm, I'm guessing you hit this on a kernel upgrade of a real system?
It seems like all callers to proc_create_net (and friends) interpret
NULL as an error, but only handful is protected by an ifdef.

I checked a few and none of them cares about the proc_dir_entry pointer
that gets returned. Should we perhaps rework the return values of the
function so that we can return success if !CONFIG_PROC_FS without
having to yield a pointer?

Obviously we can apply this fix so we can backport to 5.4 if you need
it. I think the ifdef is fine, since it's what other callers have.

> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 2b33e977a905..031f2b593720 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -4612,9 +4612,11 @@ static int __net_init packet_net_init(struct net *net)
>  	mutex_init(&net->packet.sklist_lock);
>  	INIT_HLIST_HEAD(&net->packet.sklist);
>  
> +#ifdef CONFIG_PROC_FS
>  	if (!proc_create_net("packet", 0, net->proc_net, &packet_seq_ops,
>  			sizeof(struct seq_net_private)))
>  		return -ENOMEM;
> +#endif /* CONFIG_PROC_FS */
>  
>  	return 0;
>  }


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1] net: Fix use of proc_fs
  2020-12-12 19:48   ` Jakub Kicinski
@ 2020-12-12 21:39     ` Yonatan Linik
  2020-12-12 21:51       ` Jakub Kicinski
  0 siblings, 1 reply; 9+ messages in thread
From: Yonatan Linik @ 2020-12-12 21:39 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: David Miller, Alexei Starovoitov, Daniel Borkmann,
	Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko,
	John Fastabend, KP Singh, Willem de Bruijn, john.ogness,
	Arnd Bergmann, Mao Wenan, Colin Ian King, orcohen, Networking,
	linux-kernel, bpf

On Sat, Dec 12, 2020 at 9:48 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Fri, 11 Dec 2020 18:37:49 +0200 Yonatan Linik wrote:
> > proc_fs was used, in af_packet, without a surrounding #ifdef,
> > although there is no hard dependency on proc_fs.
> > That caused the initialization of the af_packet module to fail
> > when CONFIG_PROC_FS=n.
> >
> > Specifically, proc_create_net() was used in af_packet.c,
> > and when it fails, packet_net_init() returns -ENOMEM.
> > It will always fail when the kernel is compiled without proc_fs,
> > because, proc_create_net() for example always returns NULL.
> >
> > The calling order that starts in af_packet.c is as follows:
> > packet_init()
> > register_pernet_subsys()
> > register_pernet_operations()
> > __register_pernet_operations()
> > ops_init()
> > ops->init() (packet_net_ops.init=packet_net_init())
> > proc_create_net()
> >
> > It worked in the past because register_pernet_subsys()'s return value
> > wasn't checked before this Commit 36096f2f4fa0 ("packet: Fix error path in
> > packet_init.").
> > It always returned an error, but was not checked before, so everything
> > was working even when CONFIG_PROC_FS=n.
> >
> > The fix here is simply to add the necessary #ifdef.
> >
> > Signed-off-by: Yonatan Linik <yonatanlinik@gmail.com>
>
> Hm, I'm guessing you hit this on a kernel upgrade of a real system?

Yeah, suddenly using socket with AF_PACKET didn't work,
so I checked what happened.

> It seems like all callers to proc_create_net (and friends) interpret
> NULL as an error, but only handful is protected by an ifdef.

I guess where there is no ifdef,
there should be a hard dependency on procfs,
using depends on in the Kconfig.
Maybe that's not the case everywhere it should be.

>
> I checked a few and none of them cares about the proc_dir_entry pointer
> that gets returned. Should we perhaps rework the return values of the
> function so that we can return success if !CONFIG_PROC_FS without
> having to yield a pointer?

Sometimes the pointer returned is used,
for example in drivers/acpi/button.c.
Are you suggesting returning a bool while
having the pointer as an out parameter?
Because that would still be problematic where the pointer is used.

>
> Obviously we can apply this fix so we can backport to 5.4 if you need
> it. I think the ifdef is fine, since it's what other callers have.
>

It would be great to apply this where the problem exists,
I believe this applies to other versions as well.

-- 
Yonatan Linik

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1] net: Fix use of proc_fs
  2020-12-12 21:39     ` Yonatan Linik
@ 2020-12-12 21:51       ` Jakub Kicinski
  2020-12-13  9:48         ` Yonatan Linik
  0 siblings, 1 reply; 9+ messages in thread
From: Jakub Kicinski @ 2020-12-12 21:51 UTC (permalink / raw)
  To: Yonatan Linik
  Cc: David Miller, Alexei Starovoitov, Daniel Borkmann,
	Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko,
	John Fastabend, KP Singh, Willem de Bruijn, john.ogness,
	Arnd Bergmann, Mao Wenan, Colin Ian King, orcohen, Networking,
	linux-kernel, bpf

On Sat, 12 Dec 2020 23:39:20 +0200 Yonatan Linik wrote:
> On Sat, Dec 12, 2020 at 9:48 PM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > On Fri, 11 Dec 2020 18:37:49 +0200 Yonatan Linik wrote:  
> > > proc_fs was used, in af_packet, without a surrounding #ifdef,
> > > although there is no hard dependency on proc_fs.
> > > That caused the initialization of the af_packet module to fail
> > > when CONFIG_PROC_FS=n.
> > >
> > > Specifically, proc_create_net() was used in af_packet.c,
> > > and when it fails, packet_net_init() returns -ENOMEM.
> > > It will always fail when the kernel is compiled without proc_fs,
> > > because, proc_create_net() for example always returns NULL.
> > >
> > > The calling order that starts in af_packet.c is as follows:
> > > packet_init()
> > > register_pernet_subsys()
> > > register_pernet_operations()
> > > __register_pernet_operations()
> > > ops_init()
> > > ops->init() (packet_net_ops.init=packet_net_init())
> > > proc_create_net()
> > >
> > > It worked in the past because register_pernet_subsys()'s return value
> > > wasn't checked before this Commit 36096f2f4fa0 ("packet: Fix error path in
> > > packet_init.").
> > > It always returned an error, but was not checked before, so everything
> > > was working even when CONFIG_PROC_FS=n.
> > >
> > > The fix here is simply to add the necessary #ifdef.
> > >
> > > Signed-off-by: Yonatan Linik <yonatanlinik@gmail.com>  
> >
> > Hm, I'm guessing you hit this on a kernel upgrade of a real system?  
> 
> Yeah, suddenly using socket with AF_PACKET didn't work,
> so I checked what happened.
> 
> > It seems like all callers to proc_create_net (and friends) interpret
> > NULL as an error, but only handful is protected by an ifdef.  
> 
> I guess where there is no ifdef,
> there should be a hard dependency on procfs,
> using depends on in the Kconfig.
> Maybe that's not the case everywhere it should be.

You're right, on a closer look most of the places have a larger #ifdef
block (which my grep didn't catch) or are under Kconfig. Of those I
checked only TLS looks wrong (good job me) - would you care to fix that
one as well, or should I?

> > I checked a few and none of them cares about the proc_dir_entry pointer
> > that gets returned. Should we perhaps rework the return values of the
> > function so that we can return success if !CONFIG_PROC_FS without
> > having to yield a pointer?  
> 
> Sometimes the pointer returned is used,
> for example in drivers/acpi/button.c.
> Are you suggesting returning a bool while
> having the pointer as an out parameter?
> Because that would still be problematic where the pointer is used.

Ack, I was only thinking of changing proc_create_net* but as you
rightly pointed out most callers already deal with the problem, 
so maybe it's not worth refactoring.

> > Obviously we can apply this fix so we can backport to 5.4 if you need
> > it. I think the ifdef is fine, since it's what other callers have.
> 
> It would be great to apply this where the problem exists,
> I believe this applies to other versions as well.

Will do. Linus is likely to cut the final 5.11 release on Sunday, so
it needs to wait until next week for process reasons but it won't get
lost. For the record:

Fixes: 36096f2f4fa0 ("packet: Fix error path in packet_init")

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1] net: Fix use of proc_fs
  2020-12-12 21:51       ` Jakub Kicinski
@ 2020-12-13  9:48         ` Yonatan Linik
  2020-12-14 19:07           ` Jakub Kicinski
  0 siblings, 1 reply; 9+ messages in thread
From: Yonatan Linik @ 2020-12-13  9:48 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: David Miller, Alexei Starovoitov, Daniel Borkmann,
	Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko,
	John Fastabend, KP Singh, Willem de Bruijn, john.ogness,
	Arnd Bergmann, Mao Wenan, Colin Ian King, orcohen, Networking,
	linux-kernel, bpf

On Sat, Dec 12, 2020 at 11:51 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Sat, 12 Dec 2020 23:39:20 +0200 Yonatan Linik wrote:
> > On Sat, Dec 12, 2020 at 9:48 PM Jakub Kicinski <kuba@kernel.org> wrote:
> > >
> > > On Fri, 11 Dec 2020 18:37:49 +0200 Yonatan Linik wrote:
> > > > proc_fs was used, in af_packet, without a surrounding #ifdef,
> > > > although there is no hard dependency on proc_fs.
> > > > That caused the initialization of the af_packet module to fail
> > > > when CONFIG_PROC_FS=n.
> > > >
> > > > Specifically, proc_create_net() was used in af_packet.c,
> > > > and when it fails, packet_net_init() returns -ENOMEM.
> > > > It will always fail when the kernel is compiled without proc_fs,
> > > > because, proc_create_net() for example always returns NULL.
> > > >
> > > > The calling order that starts in af_packet.c is as follows:
> > > > packet_init()
> > > > register_pernet_subsys()
> > > > register_pernet_operations()
> > > > __register_pernet_operations()
> > > > ops_init()
> > > > ops->init() (packet_net_ops.init=packet_net_init())
> > > > proc_create_net()
> > > >
> > > > It worked in the past because register_pernet_subsys()'s return value
> > > > wasn't checked before this Commit 36096f2f4fa0 ("packet: Fix error path in
> > > > packet_init.").
> > > > It always returned an error, but was not checked before, so everything
> > > > was working even when CONFIG_PROC_FS=n.
> > > >
> > > > The fix here is simply to add the necessary #ifdef.
> > > >
> > > > Signed-off-by: Yonatan Linik <yonatanlinik@gmail.com>
> > >
> > > Hm, I'm guessing you hit this on a kernel upgrade of a real system?
> >
> > Yeah, suddenly using socket with AF_PACKET didn't work,
> > so I checked what happened.
> >
> > > It seems like all callers to proc_create_net (and friends) interpret
> > > NULL as an error, but only handful is protected by an ifdef.
> >
> > I guess where there is no ifdef,
> > there should be a hard dependency on procfs,
> > using depends on in the Kconfig.
> > Maybe that's not the case everywhere it should be.
>
> You're right, on a closer look most of the places have a larger #ifdef
> block (which my grep didn't catch) or are under Kconfig. Of those I
> checked only TLS looks wrong (good job me) - would you care to fix that
> one as well, or should I?

I can fix that as well, you are talking about tls_proc.c, right?

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1] net: Fix use of proc_fs
  2020-12-13  9:48         ` Yonatan Linik
@ 2020-12-14 19:07           ` Jakub Kicinski
  0 siblings, 0 replies; 9+ messages in thread
From: Jakub Kicinski @ 2020-12-14 19:07 UTC (permalink / raw)
  To: Yonatan Linik
  Cc: David Miller, Alexei Starovoitov, Daniel Borkmann,
	Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko,
	John Fastabend, KP Singh, Willem de Bruijn, john.ogness,
	Arnd Bergmann, Mao Wenan, Colin Ian King, orcohen, Networking,
	linux-kernel, bpf

On Sun, 13 Dec 2020 11:48:15 +0200 Yonatan Linik wrote:
> > You're right, on a closer look most of the places have a larger #ifdef
> > block (which my grep didn't catch) or are under Kconfig. Of those I
> > checked only TLS looks wrong (good job me) - would you care to fix that
> > one as well, or should I?  
> 
> I can fix that as well, you are talking about tls_proc.c, right?

Yup, thanks!

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2020-12-14 19:18 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-11 16:37 [PATCH 0/1] net: Fix use of proc_fs Yonatan Linik
2020-12-11 16:37 ` [PATCH 1/1] " Yonatan Linik
2020-12-11 20:59   ` Arnd Bergmann
2020-12-12  8:39     ` Yonatan Linik
2020-12-12 19:48   ` Jakub Kicinski
2020-12-12 21:39     ` Yonatan Linik
2020-12-12 21:51       ` Jakub Kicinski
2020-12-13  9:48         ` Yonatan Linik
2020-12-14 19:07           ` Jakub Kicinski

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.