| Message ID | 20250407184730.3568147-1-song@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | netfs: Let netfs depends on PROC_FS | expand |
Song Liu <song@kernel.org> writes: > When testing a special config: > > CONFIG_NETFS_SUPPORTS=y > CONFIG_PROC_FS=n > > The system crashes with something like: > > [ 3.766197] ------------[ cut here ]------------ > [ 3.766484] kernel BUG at mm/mempool.c:560! > [ 3.766789] Oops: invalid opcode: 0000 [#1] SMP NOPTI > [ 3.767123] CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Tainted: G W > [ 3.767777] Tainted: [W]=WARN > [ 3.767968] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), > [ 3.768523] RIP: 0010:mempool_alloc_slab.cold+0x17/0x19 > [ 3.768847] Code: 50 fe ff 58 5b 5d 41 5c 41 5d 41 5e 41 5f e9 93 95 13 00 > [ 3.769977] RSP: 0018:ffffc90000013998 EFLAGS: 00010286 > [ 3.770315] RAX: 000000000000002f RBX: ffff888100ba8640 RCX: 0000000000000000 > [ 3.770749] RDX: 0000000000000000 RSI: 0000000000000003 RDI: 00000000ffffffff > [ 3.771217] RBP: 0000000000092880 R08: 0000000000000000 R09: ffffc90000013828 > [ 3.771664] R10: 0000000000000001 R11: 00000000ffffffea R12: 0000000000092cc0 > [ 3.772117] R13: 0000000000000400 R14: ffff8881004b1620 R15: ffffea0004ef7e40 > [ 3.772554] FS: 0000000000000000(0000) GS:ffff8881b5f3c000(0000) knlGS:0000000000000000 > [ 3.773061] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > [ 3.773443] CR2: ffffffff830901b4 CR3: 0000000004296001 CR4: 0000000000770ef0 > [ 3.773884] PKRU: 55555554 > [ 3.774058] Call Trace: > [ 3.774232] <TASK> > [ 3.774371] mempool_alloc_noprof+0x6a/0x190 > [ 3.774649] ? _printk+0x57/0x80 > [ 3.774862] netfs_alloc_request+0x85/0x2ce > [ 3.775147] netfs_readahead+0x28/0x170 > [ 3.775395] read_pages+0x6c/0x350 > [ 3.775623] ? srso_alias_return_thunk+0x5/0xfbef5 > [ 3.775928] page_cache_ra_unbounded+0x1bd/0x2a0 > [ 3.776247] filemap_get_pages+0x139/0x970 > [ 3.776510] ? srso_alias_return_thunk+0x5/0xfbef5 > [ 3.776820] filemap_read+0xf9/0x580 > [ 3.777054] ? srso_alias_return_thunk+0x5/0xfbef5 > [ 3.777368] ? srso_alias_return_thunk+0x5/0xfbef5 > [ 3.777674] ? find_held_lock+0x32/0x90 > [ 3.777929] ? netfs_start_io_read+0x19/0x70 > [ 3.778221] ? netfs_start_io_read+0x19/0x70 > [ 3.778489] ? srso_alias_return_thunk+0x5/0xfbef5 > [ 3.778800] ? lock_acquired+0x1e6/0x450 > [ 3.779054] ? srso_alias_return_thunk+0x5/0xfbef5 > [ 3.779379] netfs_buffered_read_iter+0x57/0x80 > [ 3.779670] __kernel_read+0x158/0x2c0 > [ 3.779927] bprm_execve+0x300/0x7a0 > [ 3.780185] kernel_execve+0x10c/0x140 > [ 3.780423] ? __pfx_kernel_init+0x10/0x10 > [ 3.780690] kernel_init+0xd5/0x150 > [ 3.780910] ret_from_fork+0x2d/0x50 > [ 3.781156] ? __pfx_kernel_init+0x10/0x10 > [ 3.781414] ret_from_fork_asm+0x1a/0x30 > [ 3.781677] </TASK> > [ 3.781823] Modules linked in: > [ 3.782065] ---[ end trace 0000000000000000 ]--- > > This is caused by the following error path in netfs_init(): > > if (!proc_mkdir("fs/netfs", NULL)) > goto error_proc; > > Fix this by letting netfs depends on PROC_FS. > > Signed-off-by: Song Liu <song@kernel.org> > --- > fs/netfs/Kconfig | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) Reviewed-by: Paulo Alcantara (Red Hat) <pc@manguebit.com>
Hi Song, Paulo Alcantara <pc@manguebit.com> writes: > Song Liu <song@kernel.org> writes: > >> When testing a special config: >> >> CONFIG_NETFS_SUPPORTS=y >> CONFIG_PROC_FS=n >> >> The system crashes with something like: >> >> [ 3.766197] ------------[ cut here ]------------ >> [ 3.766484] kernel BUG at mm/mempool.c:560! >> [ 3.766789] Oops: invalid opcode: 0000 [#1] SMP NOPTI >> [ 3.767123] CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Tainted: G W >> [ 3.767777] Tainted: [W]=WARN >> [ 3.767968] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), >> [ 3.768523] RIP: 0010:mempool_alloc_slab.cold+0x17/0x19 >> [ 3.768847] Code: 50 fe ff 58 5b 5d 41 5c 41 5d 41 5e 41 5f e9 93 95 13 00 >> [ 3.769977] RSP: 0018:ffffc90000013998 EFLAGS: 00010286 >> [ 3.770315] RAX: 000000000000002f RBX: ffff888100ba8640 RCX: 0000000000000000 >> [ 3.770749] RDX: 0000000000000000 RSI: 0000000000000003 RDI: 00000000ffffffff >> [ 3.771217] RBP: 0000000000092880 R08: 0000000000000000 R09: ffffc90000013828 >> [ 3.771664] R10: 0000000000000001 R11: 00000000ffffffea R12: 0000000000092cc0 >> [ 3.772117] R13: 0000000000000400 R14: ffff8881004b1620 R15: ffffea0004ef7e40 >> [ 3.772554] FS: 0000000000000000(0000) GS:ffff8881b5f3c000(0000) knlGS:0000000000000000 >> [ 3.773061] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 >> [ 3.773443] CR2: ffffffff830901b4 CR3: 0000000004296001 CR4: 0000000000770ef0 >> [ 3.773884] PKRU: 55555554 >> [ 3.774058] Call Trace: >> [ 3.774232] <TASK> >> [ 3.774371] mempool_alloc_noprof+0x6a/0x190 >> [ 3.774649] ? _printk+0x57/0x80 >> [ 3.774862] netfs_alloc_request+0x85/0x2ce >> [ 3.775147] netfs_readahead+0x28/0x170 >> [ 3.775395] read_pages+0x6c/0x350 >> [ 3.775623] ? srso_alias_return_thunk+0x5/0xfbef5 >> [ 3.775928] page_cache_ra_unbounded+0x1bd/0x2a0 >> [ 3.776247] filemap_get_pages+0x139/0x970 >> [ 3.776510] ? srso_alias_return_thunk+0x5/0xfbef5 >> [ 3.776820] filemap_read+0xf9/0x580 >> [ 3.777054] ? srso_alias_return_thunk+0x5/0xfbef5 >> [ 3.777368] ? srso_alias_return_thunk+0x5/0xfbef5 >> [ 3.777674] ? find_held_lock+0x32/0x90 >> [ 3.777929] ? netfs_start_io_read+0x19/0x70 >> [ 3.778221] ? netfs_start_io_read+0x19/0x70 >> [ 3.778489] ? srso_alias_return_thunk+0x5/0xfbef5 >> [ 3.778800] ? lock_acquired+0x1e6/0x450 >> [ 3.779054] ? srso_alias_return_thunk+0x5/0xfbef5 >> [ 3.779379] netfs_buffered_read_iter+0x57/0x80 >> [ 3.779670] __kernel_read+0x158/0x2c0 >> [ 3.779927] bprm_execve+0x300/0x7a0 >> [ 3.780185] kernel_execve+0x10c/0x140 >> [ 3.780423] ? __pfx_kernel_init+0x10/0x10 >> [ 3.780690] kernel_init+0xd5/0x150 >> [ 3.780910] ret_from_fork+0x2d/0x50 >> [ 3.781156] ? __pfx_kernel_init+0x10/0x10 >> [ 3.781414] ret_from_fork_asm+0x1a/0x30 >> [ 3.781677] </TASK> >> [ 3.781823] Modules linked in: >> [ 3.782065] ---[ end trace 0000000000000000 ]--- >> >> This is caused by the following error path in netfs_init(): >> >> if (!proc_mkdir("fs/netfs", NULL)) >> goto error_proc; >> >> Fix this by letting netfs depends on PROC_FS. >> >> Signed-off-by: Song Liu <song@kernel.org> >> --- >> fs/netfs/Kconfig | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) > > Reviewed-by: Paulo Alcantara (Red Hat) <pc@manguebit.com> Sorry, I take it back. Reviewed it way too soon :-/ It wouldn't make sense to make it depend on PROC_FS. I see two problems here: (1) We shouldn't be creating /proc/fs/netfs if CONFIG_PROC_FS=n (2) There's a wrong assumption in the API that @netfs_request_pool and @netfs_subrequest_pool will always be initialized. For example, we should return an error from netfs_alloc_[sub]rquest() functions in case @mempool == NULL. Dave, any thoughts?
Paulo Alcantara <pc@manguebit.com> wrote: > It wouldn't make sense to make it depend on PROC_FS. Correct. > I see two problems here: > > (1) We shouldn't be creating /proc/fs/netfs if CONFIG_PROC_FS=n Yes, the proc_*() calls will all fail if CONFIG_PROC_FS=n and so need to be #ifdef'd around. > (2) There's a wrong assumption in the API that @netfs_request_pool and > @netfs_subrequest_pool will always be initialized. For example, we > should return an error from netfs_alloc_[sub]rquest() functions in case > @mempool == NULL. No. The assumption is correct. The problem is that if the module is built in (ie. CONFIG_NETFS_SUPPORT=y), then there is no consequence of netfs_init() failing - and fail it does if CONFIG_PROC_FS=n - and 9p, afs and cifs will call into it anyway, despite the fact it deinitialised itself. It should marked be module_init(), not fs_initcall(). David
David Howells <dhowells@redhat.com> writes: > Paulo Alcantara <pc@manguebit.com> wrote: > >> (2) There's a wrong assumption in the API that @netfs_request_pool and >> @netfs_subrequest_pool will always be initialized. For example, we >> should return an error from netfs_alloc_[sub]rquest() functions in case >> @mempool == NULL. > > No. The assumption is correct. The problem is that if the module is built in > (ie. CONFIG_NETFS_SUPPORT=y), then there is no consequence of netfs_init() > failing - and fail it does if CONFIG_PROC_FS=n - and 9p, afs and cifs will > call into it anyway, despite the fact it deinitialised itself. > > It should marked be module_init(), not fs_initcall(). Makes sense, thanks. I tried to reproduce it with cifs.ko and it didn't oops as netfslib ended up not using the default memory pools as cifs.ko already provide its own memory pools via netfs_request_ops.
Hi Paulo and David, > On Apr 8, 2025, at 8:05 AM, Paulo Alcantara <pc@manguebit.com> wrote: > > David Howells <dhowells@redhat.com> writes: > >> Paulo Alcantara <pc@manguebit.com> wrote: >> >>> (2) There's a wrong assumption in the API that @netfs_request_pool and >>> @netfs_subrequest_pool will always be initialized. For example, we >>> should return an error from netfs_alloc_[sub]rquest() functions in case >>> @mempool == NULL. >> >> No. The assumption is correct. The problem is that if the module is built in >> (ie. CONFIG_NETFS_SUPPORT=y), then there is no consequence of netfs_init() >> failing - and fail it does if CONFIG_PROC_FS=n - and 9p, afs and cifs will >> call into it anyway, despite the fact it deinitialised itself. >> >> It should marked be module_init(), not fs_initcall(). > > Makes sense, thanks. I tried to reproduce it with cifs.ko and it didn't > oops as netfslib ended up not using the default memory pools as cifs.ko > already provide its own memory pools via netfs_request_ops. Thanks for the review. I guess we will need the following changes, probably in two separate patches? Thanks, Song diff --git c/fs/netfs/main.c w/fs/netfs/main.c index 4e3e62040831..e12f7575e657 100644 --- c/fs/netfs/main.c +++ w/fs/netfs/main.c @@ -127,11 +127,13 @@ static int __init netfs_init(void) if (mempool_init_slab_pool(&netfs_subrequest_pool, 100, netfs_subrequest_slab) < 0) goto error_subreqpool; +#ifdef CONFIG_PROC_FS if (!proc_mkdir("fs/netfs", NULL)) goto error_proc; if (!proc_create_seq("fs/netfs/requests", S_IFREG | 0444, NULL, &netfs_requests_seq_ops)) goto error_procfile; +#endif #ifdef CONFIG_FSCACHE_STATS if (!proc_create_single("fs/netfs/stats", S_IFREG | 0444, NULL, netfs_stats_show)) @@ -157,7 +159,7 @@ static int __init netfs_init(void) error_req: return ret; } -fs_initcall(netfs_init); +module_init(netfs_init); static void __exit netfs_exit(void) {
Song Liu <songliubraving@meta.com> wrote: > Thanks for the review. I guess we will need the following changes, You need a little bit more. The error handling in the init function also needs #ifdef'ing too lest the compiler warn about unused goto labels. > probably in two separate patches? Hmmm... yes. I'm suddenly uncertain about the order modules are inited. It might actually need to be fs_initcall(). David
David Howells <dhowells@redhat.com> wrote:
> It should marked be module_init(), not fs_initcall().
Actually, it does need to use fs_initcall() so that when it's built in, it
gets initialised before any filesystems that use module_init().
David
diff --git a/fs/netfs/Kconfig b/fs/netfs/Kconfig index 7701c037c328..df9d2a8739e7 100644 --- a/fs/netfs/Kconfig +++ b/fs/netfs/Kconfig @@ -2,6 +2,7 @@ config NETFS_SUPPORT tristate + depends on PROC_FS help This option enables support for network filesystems, including helpers for high-level buffered I/O, abstracting out read @@ -9,7 +10,7 @@ config NETFS_SUPPORT config NETFS_STATS bool "Gather statistical information on local caching" - depends on NETFS_SUPPORT && PROC_FS + depends on NETFS_SUPPORT help This option causes statistical information to be gathered on local caching and exported through file:
When testing a special config: CONFIG_NETFS_SUPPORTS=y CONFIG_PROC_FS=n The system crashes with something like: [ 3.766197] ------------[ cut here ]------------ [ 3.766484] kernel BUG at mm/mempool.c:560! [ 3.766789] Oops: invalid opcode: 0000 [#1] SMP NOPTI [ 3.767123] CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Tainted: G W [ 3.767777] Tainted: [W]=WARN [ 3.767968] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), [ 3.768523] RIP: 0010:mempool_alloc_slab.cold+0x17/0x19 [ 3.768847] Code: 50 fe ff 58 5b 5d 41 5c 41 5d 41 5e 41 5f e9 93 95 13 00 [ 3.769977] RSP: 0018:ffffc90000013998 EFLAGS: 00010286 [ 3.770315] RAX: 000000000000002f RBX: ffff888100ba8640 RCX: 0000000000000000 [ 3.770749] RDX: 0000000000000000 RSI: 0000000000000003 RDI: 00000000ffffffff [ 3.771217] RBP: 0000000000092880 R08: 0000000000000000 R09: ffffc90000013828 [ 3.771664] R10: 0000000000000001 R11: 00000000ffffffea R12: 0000000000092cc0 [ 3.772117] R13: 0000000000000400 R14: ffff8881004b1620 R15: ffffea0004ef7e40 [ 3.772554] FS: 0000000000000000(0000) GS:ffff8881b5f3c000(0000) knlGS:0000000000000000 [ 3.773061] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 3.773443] CR2: ffffffff830901b4 CR3: 0000000004296001 CR4: 0000000000770ef0 [ 3.773884] PKRU: 55555554 [ 3.774058] Call Trace: [ 3.774232] <TASK> [ 3.774371] mempool_alloc_noprof+0x6a/0x190 [ 3.774649] ? _printk+0x57/0x80 [ 3.774862] netfs_alloc_request+0x85/0x2ce [ 3.775147] netfs_readahead+0x28/0x170 [ 3.775395] read_pages+0x6c/0x350 [ 3.775623] ? srso_alias_return_thunk+0x5/0xfbef5 [ 3.775928] page_cache_ra_unbounded+0x1bd/0x2a0 [ 3.776247] filemap_get_pages+0x139/0x970 [ 3.776510] ? srso_alias_return_thunk+0x5/0xfbef5 [ 3.776820] filemap_read+0xf9/0x580 [ 3.777054] ? srso_alias_return_thunk+0x5/0xfbef5 [ 3.777368] ? srso_alias_return_thunk+0x5/0xfbef5 [ 3.777674] ? find_held_lock+0x32/0x90 [ 3.777929] ? netfs_start_io_read+0x19/0x70 [ 3.778221] ? netfs_start_io_read+0x19/0x70 [ 3.778489] ? srso_alias_return_thunk+0x5/0xfbef5 [ 3.778800] ? lock_acquired+0x1e6/0x450 [ 3.779054] ? srso_alias_return_thunk+0x5/0xfbef5 [ 3.779379] netfs_buffered_read_iter+0x57/0x80 [ 3.779670] __kernel_read+0x158/0x2c0 [ 3.779927] bprm_execve+0x300/0x7a0 [ 3.780185] kernel_execve+0x10c/0x140 [ 3.780423] ? __pfx_kernel_init+0x10/0x10 [ 3.780690] kernel_init+0xd5/0x150 [ 3.780910] ret_from_fork+0x2d/0x50 [ 3.781156] ? __pfx_kernel_init+0x10/0x10 [ 3.781414] ret_from_fork_asm+0x1a/0x30 [ 3.781677] </TASK> [ 3.781823] Modules linked in: [ 3.782065] ---[ end trace 0000000000000000 ]--- This is caused by the following error path in netfs_init(): if (!proc_mkdir("fs/netfs", NULL)) goto error_proc; Fix this by letting netfs depends on PROC_FS. Signed-off-by: Song Liu <song@kernel.org> --- fs/netfs/Kconfig | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)