Re: [PATCH] modpost: abort if a module name is too long

[Wanlong Gao]

On 2017/6/2 7:23, Jessica Yu wrote:
> +++ Wanlong Gao [31/05/17 11:48 +0800]:
>>
>>
>> On 2017/5/31 11:30, Jessica Yu wrote:
>>> +++ Wanlong Gao [31/05/17 10:23 +0800]:
>>>> Hi Jessica,
>>>>
>>>> On 2017/5/29 17:10, Jessica Yu wrote:
>>>>> +++ Xie XiuQi [20/05/17 15:46 +0800]:
>>>>>> From: Wanlong Gao <gaowanlong@xxxxxxxxxx>
>>>>>>
>>>>>> Module name has a limited length, but currently the build system
>>>>>> allows the build finishing even if the module name is too long.
>>>>>>
>>>>>>  CC      /root/kprobe_example/abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz.mod.o
>>>>>> /root/kprobe_example/abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz.mod.c:9:2:
>>>>>> warning: initializer-string for array of chars is too long [enabled by default]
>>>>>>  .name = KBUILD_MODNAME,
>>>>>>  ^
>>>>>>
>>>>>> but it's merely a warning.
>>>>>>
>>>>>> This patch adds the check of the module name length in modpost and stops
>>>>>> the build properly.
>>>>>>
>>>>>> Signed-off-by: Wanlong Gao <gaowanlong@xxxxxxxxxx>
>>>>>> Signed-off-by: Xie XiuQi <xiexiuqi@xxxxxxxxxx>
>>>>>> ---
>>>>>> scripts/mod/modpost.c | 11 +++++++++++
>>>>>> 1 file changed, 11 insertions(+)
>>>>>>
>>>>>> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
>>>>>> index 30d752a..db11c57 100644
>>>>>> --- a/scripts/mod/modpost.c
>>>>>> +++ b/scripts/mod/modpost.c
>>>>>> @@ -2166,6 +2166,17 @@ static int add_versions(struct buffer *b, struct module *mod)
>>>>>> {
>>>>>>     struct symbol *s, *exp;
>>>>>>     int err = 0;
>>>>>> +    const char *mod_name;
>>>>>> +
>>>>>> +    mod_name = strrchr(mod->name, '/');
>>>>>> +    if (mod_name == NULL)
>>>>>> +        mod_name = mod->name;
>>>>>> +    else
>>>>>> +        mod_name++;
>>>>>> +    if (strlen(mod_name) >= MODULE_NAME_LEN) {
>>>>>> +        merror("module name is too long [%s.ko]\n", mod->name);
>>>>>> +        return 1;
>>>>>> +    }
>>>>>
>>>>> Hi Xie,
>>>>>
>>>>> This check shouldn't be in add_versions() (which does something else entirely),
>>>>> it should probably be put in a separate helper function called from main. But
>>>>> I'm not a big fan of the extra string manipulation to do something this simple.
>>>>>
>>>>> I think this check can be vastly simplified, how about something like the
>>>>> following?
>>>>
>>>> This looks better, would you apply your following patch?
>>>>
>>>> Reviewed-by: Wanlong Gao <gaowanlong@xxxxxxxxxx>
>>>> Tested-by: Wanlong Gao <gaowanlong@xxxxxxxxxx>
>>>
>>> Sure, thanks for testing. I'll go ahead and format this into a proper
>>> patch and resend.
>>
>> Please wait, I just found that this patch makes the built module can't
>> be inserted by the following error:
>>
>> # insmod abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabc.ko
>> insmod: ERROR: could not insert module abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabc.ko: Invalid parameters
>>
>> # dmesg
>> abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabc: Unknown symbol __fentry__ (err -22)
> 
> Hm, I am unable to reproduce this. It looks like __fentry__ is missing
> from your kernel, you may have a mismatch between the kernel config
> that you're running and the config you are using to build the module.
> In other words, it seems like you might have built the module with
> CONFIG_FTRACE but built the kernel without.
> 
> Few questions -
> 
> What is the output of running `grep __fentry__ /proc/kallsyms`?
> 

Sure it has.

> Does your module correspond to the running kernel version?

Sure.

> 
> Do you have CONFIG_FTRACE/FUNCTION_TRACER enabled in your running
> kernel?
> 

Sure.


> Is that the full dmesg output (are there any other error messages)?

Even when I compiled the kernel with your patch, the kernel module load
failed at the boot time with the following error:

[    1.656708] libcrc32c: no symbol version for __fentry__
[    1.656709] libcrc32c: Unknown symbol __fentry__ (err -22)

But my above patch in add_versions() doesn't have such problem, I've no
idea why. Maybe your patch breaks some sections?

Thanks,
Wanlong Gao

> 
> Thanks,
> 
> Jessica
> 
>>>>> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
>>>>> index 48397fe..bb09fc7 100644
>>>>> --- a/scripts/mod/modpost.c
>>>>> +++ b/scripts/mod/modpost.c
>>>>> @@ -2139,6 +2139,9 @@ static void add_header(struct buffer *b, struct module *mod)
>>>>>                   "#endif\n");
>>>>>     buf_printf(b, "\t.arch = MODULE_ARCH_INIT,\n");
>>>>>     buf_printf(b, "};\n");
>>>>> +    buf_printf(b, "\n");
>>>>> +    buf_printf(b, "static void __attribute__((section(\".discard\"), used)) __modname_test(void)\n");
>>>>> +    buf_printf(b, "{ BUILD_BUG_ON(sizeof(KBUILD_MODNAME) > MODULE_NAME_LEN); }\n");
>>>>> }
>>>>>
>>>>> static void add_intree_flag(struct buffer *b, int is_intree)
>>>>>
>>>>> This simply checks if KBUILD_MODNAME > MODULE_NAME_LEN and breaks the build if
>>>>> it does.
>>>>>
>>>>> Jessica
>>>>>
>>>>>>     for (s = mod->unres; s; s = s->next) {
>>>>>>         exp = find_symbol(s->name);
>>>>>> -- 
>>>>>> 1.8.3.1
>>>>>>
>>>>>
>>>>> .
>>>>>
>>>>
>>>
>>> .
>>>
>>
> 
> .
>