[PATCH] module: fix invalid ELF structure error to print error code

[PATCH] module: fix invalid ELF structure error to print error code

[Shuah Khan]

When elf_validity_check() returns error, load_module() prints an
error message without error code. It is hard to determine why the
module ELF structure is invalid without this information. Fix the
error message to print the error code.

Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
 kernel/module.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/module.c b/kernel/module.c
index 40ec9a030eec..a0d412d396d6 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3941,7 +3941,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
 	 */
 	err = elf_validity_check(info);
 	if (err) {
-		pr_err("Module has invalid ELF structures\n");
+		pr_err("Module has invalid ELF structures:errno(%ld)\n", err);
 		goto free_copy;
 	}
 
-- 
2.30.2

Re: [PATCH] module: fix invalid ELF structure error to print error code

[Luis Chamberlain]

On Wed, Sep 22, 2021 at 06:14:42PM -0600, Shuah Khan wrote:
> When elf_validity_check() returns error, load_module() prints an
> error message without error code. It is hard to determine why the
> module ELF structure is invalid without this information. Fix the
> error message to print the error code.
> 
> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
> ---
>  kernel/module.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/module.c b/kernel/module.c
> index 40ec9a030eec..a0d412d396d6 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3941,7 +3941,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
>  	 */
>  	err = elf_validity_check(info);
>  	if (err) {
> -		pr_err("Module has invalid ELF structures\n");
> +		pr_err("Module has invalid ELF structures:errno(%ld)\n", err);
>  		goto free_copy;

The subject seems to indicate a fix is being made, and I think that the
bots that pick up fixes through language might find that this is a
candidate because of that. It is not, and so if you can change the
subject to indicate that this is just expanding the error print message
with the actual error code passed it would be better.

Now, if you look at elf_validity_check() and how it can fail, it would
seem tons of errors are due to -ENOEXEC. Even a function it calls,
validate_section_offset() also uses that return code. So on failure,
you likely won't still know exactly where this failed as you likely
will juse see the -ENOEXEC value, but that won't tell you much I think.

So, it would seem a bit more useful instead to add some pr_debug()s
on the elf_validity_check() and friends so that dynamic debug could
be used to debug and figure out where this did fail, when needed?
Thoughts?

  Luis

Re: [PATCH] module: fix invalid ELF structure error to print error code

[Shuah Khan]

On 9/22/21 8:59 PM, Luis Chamberlain wrote:
> On Wed, Sep 22, 2021 at 06:14:42PM -0600, Shuah Khan wrote:
>> When elf_validity_check() returns error, load_module() prints an
>> error message without error code. It is hard to determine why the
>> module ELF structure is invalid without this information. Fix the
>> error message to print the error code.
>>
>> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
>> ---
>>   kernel/module.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/kernel/module.c b/kernel/module.c
>> index 40ec9a030eec..a0d412d396d6 100644
>> --- a/kernel/module.c
>> +++ b/kernel/module.c
>> @@ -3941,7 +3941,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
>>   	 */
>>   	err = elf_validity_check(info);
>>   	if (err) {
>> -		pr_err("Module has invalid ELF structures\n");
>> +		pr_err("Module has invalid ELF structures:errno(%ld)\n", err);
>>   		goto free_copy;
> 
> The subject seems to indicate a fix is being made, and I think that the
> bots that pick up fixes through language might find that this is a
> candidate because of that. It is not, and so if you can change the
> subject to indicate that this is just expanding the error print message
> with the actual error code passed it would be better.
> 

I call this a fix because this message should have included the error code
to being with and is useless without it. Having bots pick up is one of the
reasons I called this it a fix. I am debugging a problem that a module is
failing to load, it would have been helpful to know the error code.

> Now, if you look at elf_validity_check() and how it can fail, it would
> seem tons of errors are due to -ENOEXEC. Even a function it calls,
> validate_section_offset() also uses that return code. So on failure,
> you likely won't still know exactly where this failed as you likely
> will juse see the -ENOEXEC value, but that won't tell you much I think.
> 

That is correct. This code path returns error without any indication
of what happened and needs fixing. I can do that.

> So, it would seem a bit more useful instead to add some pr_debug()s
> on the elf_validity_check() and friends so that dynamic debug could
> be used to debug and figure out where this did fail, when needed?
> Thoughts?
> 

I considered adding messages to elf_validity_check() error paths. I would
add them as pr_err() though so that it will be easier to debug. These are
errors that indicate module load failed and pr_err() is a better choice.

It doesn't look like we have access to .modinfo for determining the module
name. This information will be very useful.

thanks,
-- Shuah

Re: [PATCH] module: fix invalid ELF structure error to print error code

[Luis Chamberlain]

On Thu, Sep 23, 2021 at 04:37:00PM -0600, Shuah Khan wrote:
> That is correct. This code path returns error without any indication
> of what happened and needs fixing. I can do that.

Great thanks!

  Luis