Re: [PATCH 1/1] net: Fix use of proc_fs

On Sat, Dec 12, 2020 at 9:48 PM Jakub Kicinski [off-list ref] wrote:

On Fri, 11 Dec 2020 18:37:49 +0200 Yonatan Linik wrote:

quoted

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 <redacted>

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