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

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

[Xie XiuQi]

From: Wanlong Gao <gaowanlong@huawei.com>

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@huawei.com>
Signed-off-by: Xie XiuQi <xiexiuqi@huawei.com>
---
 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;
+	}
 
 	for (s = mod->unres; s; s = s->next) {
 		exp = find_symbol(s->name);
-- 
1.8.3.1

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

[Wanlong Gao]

Folks,

Any comments?


On 2017/5/20 15:46, Xie XiuQi wrote:
> From: Wanlong Gao <gaowanlong@huawei.com>
> 
> 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@huawei.com>
> Signed-off-by: Xie XiuQi <xiexiuqi@huawei.com>
> ---
>  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;
> +	}
>  
>  	for (s = mod->unres; s; s = s->next) {
>  		exp = find_symbol(s->name);
> 

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

[Rusty Russell]

Wanlong Gao <gaowanlong@huawei.com> writes:
> Folks,
>
> Any comments?

I've CC'd the module maintainer, who can help with this :_

Cheers,
Rusty.

> On 2017/5/20 15:46, Xie XiuQi wrote:
>> From: Wanlong Gao <gaowanlong@huawei.com>
>> 
>> 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@huawei.com>
>> Signed-off-by: Xie XiuQi <xiexiuqi@huawei.com>
>> ---
>>  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;
>> +	}
>>  
>>  	for (s = mod->unres; s; s = s->next) {
>>  		exp = find_symbol(s->name);
>> 

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

[Jessica Yu]

+++ Xie XiuQi [20/05/17 15:46 +0800]:
>From: Wanlong Gao <gaowanlong@huawei.com>
>
>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@huawei.com>
>Signed-off-by: Xie XiuQi <xiexiuqi@huawei.com>
>---
> 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?

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
>

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

[Wanlong Gao]

Hi Jessica,

On 2017/5/29 17:10, Jessica Yu wrote:
> +++ Xie XiuQi [20/05/17 15:46 +0800]:
>> From: Wanlong Gao <gaowanlong@huawei.com>
>>
>> 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@huawei.com>
>> Signed-off-by: Xie XiuQi <xiexiuqi@huawei.com>
>> ---
>> 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@huawei.com>
Tested-by: Wanlong Gao <gaowanlong@huawei.com>


Thanks,
Wanlong Gao

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

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

[Jessica Yu]

+++ 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@huawei.com>
>>>
>>> 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@huawei.com>
>>> Signed-off-by: Xie XiuQi <xiexiuqi@huawei.com>
>>> ---
>>> 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@huawei.com>
>Tested-by: Wanlong Gao <gaowanlong@huawei.com>

Sure, thanks for testing. I'll go ahead and format this into a proper
patch and resend.

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

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

[Wanlong Gao]

Hi Jessica,

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@huawei.com>
>>>>
>>>> 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@huawei.com>
>>>> Signed-off-by: Xie XiuQi <xiexiuqi@huawei.com>
>>>> ---
>>>> 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@huawei.com>
>> Tested-by: Wanlong Gao <gaowanlong@huawei.com>
> 
> Sure, thanks for testing. I'll go ahead and format this into a proper
> patch and resend.

Nice, thank you.

Cheers,
Wanlong Gao

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

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

[Wanlong Gao]

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@huawei.com>
>>>>
>>>> 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@huawei.com>
>>>> Signed-off-by: Xie XiuQi <xiexiuqi@huawei.com>
>>>> ---
>>>> 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@huawei.com>
>> Tested-by: Wanlong Gao <gaowanlong@huawei.com>
> 
> 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)


Thanks,
Wanlong Gao

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

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

[Wanlong Gao]

Jessica,

On 2017/5/31 11:48, Wanlong Gao wrote:
> 
> 
> 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@huawei.com>
>>>>>
>>>>> 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@huawei.com>
>>>>> Signed-off-by: Xie XiuQi <xiexiuqi@huawei.com>
>>>>> ---
>>>>> 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@huawei.com>
>>> Tested-by: Wanlong Gao <gaowanlong@huawei.com>
>>
>> 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)
> 
> 

Any idea about this error?

Thanks,
Wanlong Gao

> Thanks,
> Wanlong Gao
> 
>>
>> 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
>>>>>
>>>>
>>>> .
>>>>
>>>
>>
>> .
>>
> 
> 
> .
> 

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

[Jessica Yu]

+++ 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@huawei.com>
>>>>>
>>>>> 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@huawei.com>
>>>>> Signed-off-by: Xie XiuQi <xiexiuqi@huawei.com>
>>>>> ---
>>>>> 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@huawei.com>
>>> Tested-by: Wanlong Gao <gaowanlong@huawei.com>
>>
>> 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`?

Does your module correspond to the running kernel version?

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

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

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

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@huawei.com>
>>>>>>
>>>>>> 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@huawei.com>
>>>>>> Signed-off-by: Xie XiuQi <xiexiuqi@huawei.com>
>>>>>> ---
>>>>>> 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@huawei.com>
>>>> Tested-by: Wanlong Gao <gaowanlong@huawei.com>
>>>
>>> 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
>>>>>>
>>>>>
>>>>> .
>>>>>
>>>>
>>>
>>> .
>>>
>>
> 
> .
> 

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

[Wanlong Gao]

On 2017/6/2 11:04, Wanlong Gao wrote:
> 
> 
> 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@huawei.com>
>>>>>>>
>>>>>>> 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@huawei.com>
>>>>>>> Signed-off-by: Xie XiuQi <xiexiuqi@huawei.com>
>>>>>>> ---
>>>>>>> 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@huawei.com>
>>>>> Tested-by: Wanlong Gao <gaowanlong@huawei.com>
>>>>
>>>> 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?


And FYI, if I removed the "used" in attribute, this problem goes away, the
compiled module can be inserted without error. expect that there is a
"_modname_test defined but not used" warning:

  CC      /root/kprobe_example_abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz/abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz.mod.o
/root/kprobe_example_abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz/abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz.mod.c:18:38: warning: ‘__modname_test’ defined but not used [-Wunused-function]
 __attribute__((section(".discard"))) __modname_test(void)


Any idea?

Thanks,
Wanlong Gao

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

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

[Jessica Yu]

+++ Wanlong Gao [02/06/17 11:04 +0800]:
>
>
>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@huawei.com>
>>>>>>>
>>>>>>> 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@huawei.com>
>>>>>>> Signed-off-by: Xie XiuQi <xiexiuqi@huawei.com>
>>>>>>> ---
>>>>>>> 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@huawei.com>
>>>>> Tested-by: Wanlong Gao <gaowanlong@huawei.com>
>>>>
>>>> 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?

Hm, I am still unable to reproduce this on my system with modversions
enabled and the -rc2 kernel. But judging by the errno (-22) it looks
like this is failing in check_version()/resolve_symbol() for you,
which leads me to think that this is somehow messing with the
__versions table generated by modpost (not sure why).

Does the  ____versions[] array in the generated *.mod.c file for your
test module look different with and without the patch? Also: what
version of gcc and binutils are you using, and what kernel version are
you testing on?

If you could also send me off-list the *.mod.c files generated by
modpost with and without the patch applied, that'd also help.

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

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

[Wanlong Gao]

On 2017/6/5 10:09, Jessica Yu wrote:
> +++ Wanlong Gao [02/06/17 11:04 +0800]:
>>
>>
>> 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@huawei.com>
>>>>>>>>
>>>>>>>> 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@huawei.com>
>>>>>>>> Signed-off-by: Xie XiuQi <xiexiuqi@huawei.com>
>>>>>>>> ---
>>>>>>>> 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@huawei.com>
>>>>>> Tested-by: Wanlong Gao <gaowanlong@huawei.com>
>>>>>
>>>>> 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?
> 
> Hm, I am still unable to reproduce this on my system with modversions
> enabled and the -rc2 kernel. But judging by the errno (-22) it looks
> like this is failing in check_version()/resolve_symbol() for you,
> which leads me to think that this is somehow messing with the
> __versions table generated by modpost (not sure why).
> 
> Does the  ____versions[] array in the generated *.mod.c file for your
> test module look different with and without the patch? Also: what
> version of gcc and binutils are you using, and what kernel version are
> you testing on?

The *.mod.c file are same except the added __modname_test section, the gcc
,binutils and kernel are all from centos 7.2 (3.10.0-327).

Thanks,
Wanlong Gao

> 
> If you could also send me off-list the *.mod.c files generated by
> modpost with and without the patch applied, that'd also help.
> 
> 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
>>>>>>>>
>>>>>>>
>>>>>>> .
>>>>>>>
>>>>>>
>>>>>
>>>>> .
>>>>>
>>>>
>>>
>>> .
>>>
>>
> 
> .
> 

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

[Jessica Yu]

+++ Wanlong Gao [06/06/17 09:07 +0800]:
>
>
>On 2017/6/5 10:09, Jessica Yu wrote:
>> +++ Wanlong Gao [02/06/17 11:04 +0800]:
>>>
>>>
>>> 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@huawei.com>
>>>>>>>>>
>>>>>>>>> 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@huawei.com>
>>>>>>>>> Signed-off-by: Xie XiuQi <xiexiuqi@huawei.com>
>>>>>>>>> ---
>>>>>>>>> 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@huawei.com>
>>>>>>> Tested-by: Wanlong Gao <gaowanlong@huawei.com>
>>>>>>
>>>>>> 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?
>>
>> Hm, I am still unable to reproduce this on my system with modversions
>> enabled and the -rc2 kernel. But judging by the errno (-22) it looks
>> like this is failing in check_version()/resolve_symbol() for you,
>> which leads me to think that this is somehow messing with the
>> __versions table generated by modpost (not sure why).
>>
>> Does the  ____versions[] array in the generated *.mod.c file for your
>> test module look different with and without the patch? Also: what
>> version of gcc and binutils are you using, and what kernel version are
>> you testing on?
>
>The *.mod.c file are same except the added __modname_test section, the gcc
>,binutils and kernel are all from centos 7.2 (3.10.0-327).

Thanks for the additional info. Just FYI, I'm going to be out this
week and part of next week due to travelling, but I'll be able to take
another look at this next Thurs/Fri. If we can't resolve the issue, we
can just work on your original patch.

Thanks!

Jessica

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

[Jessica Yu]

+++ Jessica Yu [06/06/17 20:41 -0700]:
>+++ Wanlong Gao [06/06/17 09:07 +0800]:
>>
>>
>>On 2017/6/5 10:09, Jessica Yu wrote:
>>>+++ Wanlong Gao [02/06/17 11:04 +0800]:
>>>>
>>>>
>>>>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@huawei.com>
>>>>>>>>>>
>>>>>>>>>>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@huawei.com>
>>>>>>>>>>Signed-off-by: Xie XiuQi <xiexiuqi@huawei.com>
>>>>>>>>>>---
>>>>>>>>>>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@huawei.com>
>>>>>>>>Tested-by: Wanlong Gao <gaowanlong@huawei.com>
>>>>>>>
>>>>>>>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?
>>>
>>>Hm, I am still unable to reproduce this on my system with modversions
>>>enabled and the -rc2 kernel. But judging by the errno (-22) it looks
>>>like this is failing in check_version()/resolve_symbol() for you,
>>>which leads me to think that this is somehow messing with the
>>>__versions table generated by modpost (not sure why).
>>>
>>>Does the  ____versions[] array in the generated *.mod.c file for your
>>>test module look different with and without the patch? Also: what
>>>version of gcc and binutils are you using, and what kernel version are
>>>you testing on?
>>
>>The *.mod.c file are same except the added __modname_test section, the gcc
>>,binutils and kernel are all from centos 7.2 (3.10.0-327).
>
>Thanks for the additional info. Just FYI, I'm going to be out this
>week and part of next week due to travelling, but I'll be able to take
>another look at this next Thurs/Fri. If we can't resolve the issue, we
>can just work on your original patch.

Thanks for your patience, I've just moved abroad and getting to stable
internet has been a challenge :-/

Here's my last attempt at fixing the BUILD_BUG_ON patch (I am not sure
why it seems to be messing with the __versions table on your setup,
perhaps it is related to .discard usage?).

Do either of the patches below work on your setup? (try one or the
other and let me know if either of them work..)

Thanks!

---
Variant #1 - don't put __modname_check in .discard...
---

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 48397fe..c88a5a7 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 __used __modname_check(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)

---
Variant #2 - put __modname_check at the very end of .mod.c
---

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 48397fe..37cdf36 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -2263,6 +2263,13 @@ static void add_srcversion(struct buffer *b, struct module *mod)
 	}
 }
 
+static void add_modname_check(struct buffer *b, struct module *mod)
+{
+	buf_printf(b, "\n");
+	buf_printf(b, "static void __attribute__((section(\".discard\"), used)) __modname_check(void)\n");
+	buf_printf(b, "{ BUILD_BUG_ON(sizeof(KBUILD_MODNAME) > MODULE_NAME_LEN); }\n");
+}
+
 static void write_if_changed(struct buffer *b, const char *fname)
 {
 	char *tmp;
@@ -2497,6 +2504,7 @@ int main(int argc, char **argv)
 		add_depends(&buf, mod, modules);
 		add_moddevtable(&buf, mod);
 		add_srcversion(&buf, mod);
+		add_modname_check(&buf, mod);
 
 		sprintf(fname, "%s.mod.c", mod->name);
 		write_if_changed(&buf, fname);

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

[Wanlong Gao]

On 2017/6/22 0:09, Jessica Yu wrote:
> +++ Jessica Yu [06/06/17 20:41 -0700]:
>> +++ Wanlong Gao [06/06/17 09:07 +0800]:
>>>
>>>
>>> On 2017/6/5 10:09, Jessica Yu wrote:
>>>> +++ Wanlong Gao [02/06/17 11:04 +0800]:
>>>>>
>>>>>
>>>>> 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@huawei.com>
>>>>>>>>>>>
>>>>>>>>>>> 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@huawei.com>
>>>>>>>>>>> Signed-off-by: Xie XiuQi <xiexiuqi@huawei.com>
>>>>>>>>>>> ---
>>>>>>>>>>> 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@huawei.com>
>>>>>>>>> Tested-by: Wanlong Gao <gaowanlong@huawei.com>
>>>>>>>>
>>>>>>>> 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?
>>>>
>>>> Hm, I am still unable to reproduce this on my system with modversions
>>>> enabled and the -rc2 kernel. But judging by the errno (-22) it looks
>>>> like this is failing in check_version()/resolve_symbol() for you,
>>>> which leads me to think that this is somehow messing with the
>>>> __versions table generated by modpost (not sure why).
>>>>
>>>> Does the  ____versions[] array in the generated *.mod.c file for your
>>>> test module look different with and without the patch? Also: what
>>>> version of gcc and binutils are you using, and what kernel version are
>>>> you testing on?
>>>
>>> The *.mod.c file are same except the added __modname_test section, the gcc
>>> ,binutils and kernel are all from centos 7.2 (3.10.0-327).
>>
>> Thanks for the additional info. Just FYI, I'm going to be out this
>> week and part of next week due to travelling, but I'll be able to take
>> another look at this next Thurs/Fri. If we can't resolve the issue, we
>> can just work on your original patch.
> 
> Thanks for your patience, I've just moved abroad and getting to stable
> internet has been a challenge :-/
> 
> Here's my last attempt at fixing the BUILD_BUG_ON patch (I am not sure
> why it seems to be messing with the __versions table on your setup,
> perhaps it is related to .discard usage?).
> 
> Do either of the patches below work on your setup? (try one or the
> other and let me know if either of them work..)

Sorry to say that neither ;< It seems not to add section in mod.c
is more safe.


Thanks,
Wanlong Gao

> 
> Thanks!
> 
> ---
> Variant #1 - don't put __modname_check in .discard...
> ---
> 
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 48397fe..c88a5a7 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 __used __modname_check(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)
> 
> ---
> Variant #2 - put __modname_check at the very end of .mod.c
> ---
> 
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 48397fe..37cdf36 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -2263,6 +2263,13 @@ static void add_srcversion(struct buffer *b, struct module *mod)
>     }
> }
> 
> +static void add_modname_check(struct buffer *b, struct module *mod)
> +{
> +    buf_printf(b, "\n");
> +    buf_printf(b, "static void __attribute__((section(\".discard\"), used)) __modname_check(void)\n");
> +    buf_printf(b, "{ BUILD_BUG_ON(sizeof(KBUILD_MODNAME) > MODULE_NAME_LEN); }\n");
> +}
> +
> static void write_if_changed(struct buffer *b, const char *fname)
> {
>     char *tmp;
> @@ -2497,6 +2504,7 @@ int main(int argc, char **argv)
>         add_depends(&buf, mod, modules);
>         add_moddevtable(&buf, mod);
>         add_srcversion(&buf, mod);
> +        add_modname_check(&buf, mod);
> 
>         sprintf(fname, "%s.mod.c", mod->name);
>         write_if_changed(&buf, fname);
> 
> 
> .
> 

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

[Jessica Yu]

+++ Wanlong Gao [22/06/17 09:11 +0800]:
>
>
>On 2017/6/22 0:09, Jessica Yu wrote:
>> +++ Jessica Yu [06/06/17 20:41 -0700]:
>>> +++ Wanlong Gao [06/06/17 09:07 +0800]:
>>>>
>>>>
>>>> On 2017/6/5 10:09, Jessica Yu wrote:
>>>>> +++ Wanlong Gao [02/06/17 11:04 +0800]:
>>>>>>
>>>>>>
>>>>>> 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@huawei.com>
>>>>>>>>>>>>
>>>>>>>>>>>> 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@huawei.com>
>>>>>>>>>>>> Signed-off-by: Xie XiuQi <xiexiuqi@huawei.com>
>>>>>>>>>>>> ---
>>>>>>>>>>>> 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@huawei.com>
>>>>>>>>>> Tested-by: Wanlong Gao <gaowanlong@huawei.com>
>>>>>>>>>
>>>>>>>>> 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?
>>>>>
>>>>> Hm, I am still unable to reproduce this on my system with modversions
>>>>> enabled and the -rc2 kernel. But judging by the errno (-22) it looks
>>>>> like this is failing in check_version()/resolve_symbol() for you,
>>>>> which leads me to think that this is somehow messing with the
>>>>> __versions table generated by modpost (not sure why).
>>>>>
>>>>> Does the  ____versions[] array in the generated *.mod.c file for your
>>>>> test module look different with and without the patch? Also: what
>>>>> version of gcc and binutils are you using, and what kernel version are
>>>>> you testing on?
>>>>
>>>> The *.mod.c file are same except the added __modname_test section, the gcc
>>>> ,binutils and kernel are all from centos 7.2 (3.10.0-327).
>>>
>>> Thanks for the additional info. Just FYI, I'm going to be out this
>>> week and part of next week due to travelling, but I'll be able to take
>>> another look at this next Thurs/Fri. If we can't resolve the issue, we
>>> can just work on your original patch.
>>
>> Thanks for your patience, I've just moved abroad and getting to stable
>> internet has been a challenge :-/
>>
>> Here's my last attempt at fixing the BUILD_BUG_ON patch (I am not sure
>> why it seems to be messing with the __versions table on your setup,
>> perhaps it is related to .discard usage?).
>>
>> Do either of the patches below work on your setup? (try one or the
>> other and let me know if either of them work..)
>
>Sorry to say that neither ;< It seems not to add section in mod.c
>is more safe.

No problem, thanks for verifying! I originally liked the build bug
patch because it worked directly with the in-kernel module name,
instead of the filename (they can differ slightly, but the number of
chars should remain the same anyway..). In any case, could you modify
your original patch to put the modname check in a separate function,
maybe named check_modname_len(), and have it be called before
check_exports()?

Thanks!

Jessica

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

[Wanlong Gao]

On 2017/6/22 22:25, Jessica Yu wrote:
> +++ Wanlong Gao [22/06/17 09:11 +0800]:
>>
>>
>> On 2017/6/22 0:09, Jessica Yu wrote:
>>> +++ Jessica Yu [06/06/17 20:41 -0700]:
>>>> +++ Wanlong Gao [06/06/17 09:07 +0800]:
>>>>>
>>>>>
>>>>> On 2017/6/5 10:09, Jessica Yu wrote:
>>>>>> +++ Wanlong Gao [02/06/17 11:04 +0800]:
>>>>>>>
>>>>>>>
>>>>>>> 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@huawei.com>
>>>>>>>>>>>>>
>>>>>>>>>>>>> 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@huawei.com>
>>>>>>>>>>>>> Signed-off-by: Xie XiuQi <xiexiuqi@huawei.com>
>>>>>>>>>>>>> ---
>>>>>>>>>>>>> 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@huawei.com>
>>>>>>>>>>> Tested-by: Wanlong Gao <gaowanlong@huawei.com>
>>>>>>>>>>
>>>>>>>>>> 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?
>>>>>>
>>>>>> Hm, I am still unable to reproduce this on my system with modversions
>>>>>> enabled and the -rc2 kernel. But judging by the errno (-22) it looks
>>>>>> like this is failing in check_version()/resolve_symbol() for you,
>>>>>> which leads me to think that this is somehow messing with the
>>>>>> __versions table generated by modpost (not sure why).
>>>>>>
>>>>>> Does the  ____versions[] array in the generated *.mod.c file for your
>>>>>> test module look different with and without the patch? Also: what
>>>>>> version of gcc and binutils are you using, and what kernel version are
>>>>>> you testing on?
>>>>>
>>>>> The *.mod.c file are same except the added __modname_test section, the gcc
>>>>> ,binutils and kernel are all from centos 7.2 (3.10.0-327).
>>>>
>>>> Thanks for the additional info. Just FYI, I'm going to be out this
>>>> week and part of next week due to travelling, but I'll be able to take
>>>> another look at this next Thurs/Fri. If we can't resolve the issue, we
>>>> can just work on your original patch.
>>>
>>> Thanks for your patience, I've just moved abroad and getting to stable
>>> internet has been a challenge :-/
>>>
>>> Here's my last attempt at fixing the BUILD_BUG_ON patch (I am not sure
>>> why it seems to be messing with the __versions table on your setup,
>>> perhaps it is related to .discard usage?).
>>>
>>> Do either of the patches below work on your setup? (try one or the
>>> other and let me know if either of them work..)
>>
>> Sorry to say that neither ;< It seems not to add section in mod.c
>> is more safe.
> 
> No problem, thanks for verifying! I originally liked the build bug
> patch because it worked directly with the in-kernel module name,
> instead of the filename (they can differ slightly, but the number of
> chars should remain the same anyway..). In any case, could you modify
> your original patch to put the modname check in a separate function,
> maybe named check_modname_len(), and have it be called before
> check_exports()?


Sure, will send V2 ;)

Thanks,
Wanlong Gao

> 
> Thanks!
> 
> Jessica
> 
> 
> .
>