[PATCH] libkmod: allow to fallback to user-space decompression

[PATCH] libkmod: allow to fallback to user-space decompression

[Andrea Righi]

Fallback to user-space decompression when the kernel cannot allocate
enough memory to perform the decompression.

This can happen with large modules, such as xfs on linux 6.5 for
example, an ENOMEM would be returned and the module fails to load.

It seems more reliable to try again with user-space decompression
rather than reporting an error and failing to load the module.

Fixes: 09c9f8c ("libkmod: Use kernel decompression when available")
Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
---
 libkmod/libkmod-module.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
index 585da41..d2d4815 100644
--- a/libkmod/libkmod-module.c
+++ b/libkmod/libkmod-module.c
@@ -978,7 +978,7 @@ KMOD_EXPORT int kmod_module_insert_module(struct kmod_module *mod,
 	}
 
 	err = do_finit_module(mod, flags, args);
-	if (err == -ENOSYS)
+	if (err == -ENOSYS || err == -ENOMEM)
 		err = do_init_module(mod, flags, args);
 
 	if (err < 0)
-- 
2.40.1

Re: [PATCH] libkmod: allow to fallback to user-space decompression

[Lucas De Marchi]

On Tue, Aug 29, 2023 at 02:38:08PM +0200, Andrea Righi wrote:
>Fallback to user-space decompression when the kernel cannot allocate
>enough memory to perform the decompression.
>
>This can happen with large modules, such as xfs on linux 6.5 for
>example, an ENOMEM would be returned and the module fails to load.
>
>It seems more reliable to try again with user-space decompression
>rather than reporting an error and failing to load the module.
>
>Fixes: 09c9f8c ("libkmod: Use kernel decompression when available")
>Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
>---
> libkmod/libkmod-module.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
>index 585da41..d2d4815 100644
>--- a/libkmod/libkmod-module.c
>+++ b/libkmod/libkmod-module.c
>@@ -978,7 +978,7 @@ KMOD_EXPORT int kmod_module_insert_module(struct kmod_module *mod,
> 	}
>
> 	err = do_finit_module(mod, flags, args);
>-	if (err == -ENOSYS)
>+	if (err == -ENOSYS || err == -ENOMEM)

oh... ENOMEM can be returned in several places. I don't think it's a great
interface to just retry in userspace.

Luis, can we do better on the kernel side?

Andrea, did you track the exact location triggering the ENOMEM? Is it
the return of kvmalloc_array() or alloc_page() from
module_get_next_page()?  Or one of the previous kmalloc we use for the
different deflate methods?


thanks
Lucas De Marchi

> 		err = do_init_module(mod, flags, args);
>
> 	if (err < 0)
>-- 
>2.40.1
>

Re: [PATCH] libkmod: allow to fallback to user-space decompression

[Andrea Righi]

On Tue, Aug 29, 2023 at 07:47:05AM -0700, Lucas De Marchi wrote:
> On Tue, Aug 29, 2023 at 02:38:08PM +0200, Andrea Righi wrote:
> > Fallback to user-space decompression when the kernel cannot allocate
> > enough memory to perform the decompression.
> > 
> > This can happen with large modules, such as xfs on linux 6.5 for
> > example, an ENOMEM would be returned and the module fails to load.
> > 
> > It seems more reliable to try again with user-space decompression
> > rather than reporting an error and failing to load the module.
> > 
> > Fixes: 09c9f8c ("libkmod: Use kernel decompression when available")
> > Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
> > ---
> > libkmod/libkmod-module.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
> > index 585da41..d2d4815 100644
> > --- a/libkmod/libkmod-module.c
> > +++ b/libkmod/libkmod-module.c
> > @@ -978,7 +978,7 @@ KMOD_EXPORT int kmod_module_insert_module(struct kmod_module *mod,
> > 	}
> > 
> > 	err = do_finit_module(mod, flags, args);
> > -	if (err == -ENOSYS)
> > +	if (err == -ENOSYS || err == -ENOMEM)
> 
> oh... ENOMEM can be returned in several places. I don't think it's a great
> interface to just retry in userspace.
> 
> Luis, can we do better on the kernel side?
> 
> Andrea, did you track the exact location triggering the ENOMEM? Is it
> the return of kvmalloc_array() or alloc_page() from
> module_get_next_page()?  Or one of the previous kmalloc we use for the
> different deflate methods?

It was this kmalloc() right here:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/module/decompress.c?h=v6.5#n244

For the records, I've also sent this patch to address the issue from a
kernel perspective:
https://lore.kernel.org/lkml/20230829120508.317611-1-andrea.righi@canonical.com/T/#u

That seems to solve the issue in my particular case, but I'm not sure if
that's a valid solution across all architectures.

Thanks,
-Andrea

Re: [PATCH] libkmod: allow to fallback to user-space decompression

[Luis Chamberlain]

On Tue, Aug 29, 2023 at 05:23:25PM +0200, Andrea Righi wrote:
> For the records, I've also sent this patch to address the issue from a
> kernel perspective:
> https://lore.kernel.org/lkml/20230829120508.317611-1-andrea.righi@canonical.com/T/#u
> 
> That seems to solve the issue in my particular case, but I'm not sure if
> that's a valid solution across all architectures.

I can't think of a reason why it would regress on other architectures.
We'll give it a spin soon fast as it will be on its way to Linus today.

  Luis

Re: [PATCH] libkmod: allow to fallback to user-space decompression

[Andrea Righi]

On Tue, Aug 29, 2023 at 09:42:43AM -0700, Luis Chamberlain wrote:
> On Tue, Aug 29, 2023 at 05:23:25PM +0200, Andrea Righi wrote:
> > For the records, I've also sent this patch to address the issue from a
> > kernel perspective:
> > https://lore.kernel.org/lkml/20230829120508.317611-1-andrea.righi@canonical.com/T/#u
> > 
> > That seems to solve the issue in my particular case, but I'm not sure if
> > that's a valid solution across all architectures.
> 
> I can't think of a reason why it would regress on other architectures.
> We'll give it a spin soon fast as it will be on its way to Linus today.

Cool! Yes, if this patch can be applied we don't really need to change
libkmod and if there are other spots in the decompress code that can use
vmalloc() we can adjust them in a similar way.

Thanks,
-Andrea

Re: [PATCH] libkmod: allow to fallback to user-space decompression

[Lucas De Marchi]

On Tue, Aug 29, 2023 at 05:23:25PM +0200, Andrea Righi wrote:
>On Tue, Aug 29, 2023 at 07:47:05AM -0700, Lucas De Marchi wrote:
>> On Tue, Aug 29, 2023 at 02:38:08PM +0200, Andrea Righi wrote:
>> > Fallback to user-space decompression when the kernel cannot allocate
>> > enough memory to perform the decompression.
>> >
>> > This can happen with large modules, such as xfs on linux 6.5 for
>> > example, an ENOMEM would be returned and the module fails to load.
>> >
>> > It seems more reliable to try again with user-space decompression
>> > rather than reporting an error and failing to load the module.
>> >
>> > Fixes: 09c9f8c ("libkmod: Use kernel decompression when available")
>> > Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
>> > ---
>> > libkmod/libkmod-module.c | 2 +-
>> > 1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
>> > index 585da41..d2d4815 100644
>> > --- a/libkmod/libkmod-module.c
>> > +++ b/libkmod/libkmod-module.c
>> > @@ -978,7 +978,7 @@ KMOD_EXPORT int kmod_module_insert_module(struct kmod_module *mod,
>> > 	}
>> >
>> > 	err = do_finit_module(mod, flags, args);
>> > -	if (err == -ENOSYS)
>> > +	if (err == -ENOSYS || err == -ENOMEM)
>>
>> oh... ENOMEM can be returned in several places. I don't think it's a great
>> interface to just retry in userspace.
>>
>> Luis, can we do better on the kernel side?
>>
>> Andrea, did you track the exact location triggering the ENOMEM? Is it
>> the return of kvmalloc_array() or alloc_page() from
>> module_get_next_page()?  Or one of the previous kmalloc we use for the
>> different deflate methods?
>
>It was this kmalloc() right here:
>https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/module/decompress.c?h=v6.5#n244
>
>For the records, I've also sent this patch to address the issue from a
>kernel perspective:
>https://lore.kernel.org/lkml/20230829120508.317611-1-andrea.righi@canonical.com/T/#u

That patch seems better. We will probably have to move other decompress
algorithms to vmalloc too.

thanks
Lucas De Marchi

>
>That seems to solve the issue in my particular case, but I'm not sure if
>that's a valid solution across all architectures.
>
>Thanks,
>-Andrea