On 20/09/2018 13:59, Prarit Bhargava wrote:
> gcc 8.1.0 warns with:
>
> kernel/debug/kdb/kdb_support.c: In function ‘kallsyms_symbol_next’:
> kernel/debug/kdb/kdb_support.c:239:4: warning: ‘strncpy’ specified bound depends on the length of the source argument [-Wstringop-overflow=]
> strncpy(prefix_name, name, strlen(name)+1);
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> kernel/debug/kdb/kdb_support.c:239:31: note: length computed here
>
> Use strscpy() with the destination buffer size, and use ellipses when
> displaying truncated symbols.
>
> v2: Use strscpy()
>
> Signed-off-by: Prarit Bhargava <[email protected]>
> Cc: Jonathan Toppins <[email protected]>
> Cc: Jason Wessel <[email protected]>
> Cc: Daniel Thompson <[email protected]>
> Cc: [email protected]
> ---
> kernel/debug/kdb/kdb_io.c | 15 +++++++++------
> kernel/debug/kdb/kdb_private.h | 2 +-
> kernel/debug/kdb/kdb_support.c | 10 +++++-----
> 3 files changed, 15 insertions(+), 12 deletions(-)
>
> diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> index ed5d34925ad0..6a4b41484afe 100644
> --- a/kernel/debug/kdb/kdb_io.c
> +++ b/kernel/debug/kdb/kdb_io.c
> @@ -216,7 +216,7 @@ static char *kdb_read(char *buffer, size_t bufsize)
> int count;
> int i;
> int diag, dtab_count;
> - int key;
> + int key, buf_size, ret;
>
>
> diag = kdbgetintenv("DTABCOUNT", &dtab_count);
> @@ -336,9 +336,8 @@ static char *kdb_read(char *buffer, size_t bufsize)
> else
> p_tmp = tmpbuffer;
> len = strlen(p_tmp);
> - count = kallsyms_symbol_complete(p_tmp,
> - sizeof(tmpbuffer) -
> - (p_tmp - tmpbuffer));
> + buf_size = sizeof(tmpbuffer) - (p_tmp - tmpbuffer);
> + count = kallsyms_symbol_complete(p_tmp, buf_size);
> if (tab == 2 && count > 0) {
> kdb_printf("\n%d symbols are found.", count);
> if (count > dtab_count) {
> @@ -350,9 +349,13 @@ static char *kdb_read(char *buffer, size_t bufsize)
> }
> kdb_printf("\n");
> for (i = 0; i < count; i++) {
> - if (WARN_ON(!kallsyms_symbol_next(p_tmp, i)))
> + ret = kallsyms_symbol_next(p_tmp, i, buf_size);
> + if (WARN_ON(!ret))
> break;
I'm getting confused by having two different branches on ret.
Don't get a WARN_ON() when ret == -E2BIG?
Daniel.
> - kdb_printf("%s ", p_tmp);
> + if (ret != -E2BIG)
> + kdb_printf("%s ", p_tmp);
> + else
> + kdb_printf("%s... ", p_tmp);
> *(p_tmp + len) = '\0';
> }
> if (i >= dtab_count)
On 10/02/2018 11:53 AM, Daniel Thompson wrote:
> On 20/09/2018 13:59, Prarit Bhargava wrote:
>> gcc 8.1.0 warns with:
>>
>> kernel/debug/kdb/kdb_support.c: In function ‘kallsyms_symbol_next’:
>> kernel/debug/kdb/kdb_support.c:239:4: warning: ‘strncpy’ specified bound
>> depends on the length of the source argument [-Wstringop-overflow=]
>> strncpy(prefix_name, name, strlen(name)+1);
>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> kernel/debug/kdb/kdb_support.c:239:31: note: length computed here
>>
>> Use strscpy() with the destination buffer size, and use ellipses when
>> displaying truncated symbols.
>>
>> v2: Use strscpy()
>>
>> Signed-off-by: Prarit Bhargava <[email protected]>
>> Cc: Jonathan Toppins <[email protected]>
>> Cc: Jason Wessel <[email protected]>
>> Cc: Daniel Thompson <[email protected]>
>> Cc: [email protected]
>> ---
>> kernel/debug/kdb/kdb_io.c | 15 +++++++++------
>> kernel/debug/kdb/kdb_private.h | 2 +-
>> kernel/debug/kdb/kdb_support.c | 10 +++++-----
>> 3 files changed, 15 insertions(+), 12 deletions(-)
>>
>> diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
>> index ed5d34925ad0..6a4b41484afe 100644
>> --- a/kernel/debug/kdb/kdb_io.c
>> +++ b/kernel/debug/kdb/kdb_io.c
>> @@ -216,7 +216,7 @@ static char *kdb_read(char *buffer, size_t bufsize)
>> int count;
>> int i;
>> int diag, dtab_count;
>> - int key;
>> + int key, buf_size, ret;
>> diag = kdbgetintenv("DTABCOUNT", &dtab_count);
>> @@ -336,9 +336,8 @@ static char *kdb_read(char *buffer, size_t bufsize)
>> else
>> p_tmp = tmpbuffer;
>> len = strlen(p_tmp);
>> - count = kallsyms_symbol_complete(p_tmp,
>> - sizeof(tmpbuffer) -
>> - (p_tmp - tmpbuffer));
>> + buf_size = sizeof(tmpbuffer) - (p_tmp - tmpbuffer);
>> + count = kallsyms_symbol_complete(p_tmp, buf_size);
>> if (tab == 2 && count > 0) {
>> kdb_printf("\n%d symbols are found.", count);
>> if (count > dtab_count) {
>> @@ -350,9 +349,13 @@ static char *kdb_read(char *buffer, size_t bufsize)
>> }
>> kdb_printf("\n");
>> for (i = 0; i < count; i++) {
>> - if (WARN_ON(!kallsyms_symbol_next(p_tmp, i)))
>> + ret = kallsyms_symbol_next(p_tmp, i, buf_size);
>> + if (WARN_ON(!ret))
>> break;
> I'm getting confused by having two different branches on ret.
>
> Don't get a WARN_ON() when ret == -E2BIG?
>
>
Should we WARN on a really long symbol? I don't think we should as we're
handling that by truncating the output and adding ellipses below.
P.
> Daniel.
>
>
>> - kdb_printf("%s ", p_tmp);
>> + if (ret != -E2BIG)
>> + kdb_printf("%s ", p_tmp);
>> + else
>> + kdb_printf("%s... ", p_tmp);
>> *(p_tmp + len) = '\0';
>> }
>> if (i >= dtab_count)
On Thu, Oct 04, 2018 at 08:25:30AM -0400, Prarit Bhargava wrote:
> On 10/02/2018 11:53 AM, Daniel Thompson wrote:
> > On 20/09/2018 13:59, Prarit Bhargava wrote:
> >> gcc 8.1.0 warns with:
> >>
> >> kernel/debug/kdb/kdb_support.c: In function ‘kallsyms_symbol_next’:
> >> kernel/debug/kdb/kdb_support.c:239:4: warning: ‘strncpy’ specified bound
> >> depends on the length of the source argument [-Wstringop-overflow=]
> >> strncpy(prefix_name, name, strlen(name)+1);
> >> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >> kernel/debug/kdb/kdb_support.c:239:31: note: length computed here
> >>
> >> Use strscpy() with the destination buffer size, and use ellipses when
> >> displaying truncated symbols.
> >>
> >> v2: Use strscpy()
> >>
> >> Signed-off-by: Prarit Bhargava <[email protected]>
> >> Cc: Jonathan Toppins <[email protected]>
> >> Cc: Jason Wessel <[email protected]>
> >> Cc: Daniel Thompson <[email protected]>
> >> Cc: [email protected]
> >> ---
> >> kernel/debug/kdb/kdb_io.c | 15 +++++++++------
> >> kernel/debug/kdb/kdb_private.h | 2 +-
> >> kernel/debug/kdb/kdb_support.c | 10 +++++-----
> >> 3 files changed, 15 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> >> index ed5d34925ad0..6a4b41484afe 100644
> >> --- a/kernel/debug/kdb/kdb_io.c
> >> +++ b/kernel/debug/kdb/kdb_io.c
> >> @@ -216,7 +216,7 @@ static char *kdb_read(char *buffer, size_t bufsize)
> >> int count;
> >> int i;
> >> int diag, dtab_count;
> >> - int key;
> >> + int key, buf_size, ret;
> >> diag = kdbgetintenv("DTABCOUNT", &dtab_count);
> >> @@ -336,9 +336,8 @@ static char *kdb_read(char *buffer, size_t bufsize)
> >> else
> >> p_tmp = tmpbuffer;
> >> len = strlen(p_tmp);
> >> - count = kallsyms_symbol_complete(p_tmp,
> >> - sizeof(tmpbuffer) -
> >> - (p_tmp - tmpbuffer));
> >> + buf_size = sizeof(tmpbuffer) - (p_tmp - tmpbuffer);
> >> + count = kallsyms_symbol_complete(p_tmp, buf_size);
> >> if (tab == 2 && count > 0) {
> >> kdb_printf("\n%d symbols are found.", count);
> >> if (count > dtab_count) {
> >> @@ -350,9 +349,13 @@ static char *kdb_read(char *buffer, size_t bufsize)
> >> }
> >> kdb_printf("\n");
> >> for (i = 0; i < count; i++) {
> >> - if (WARN_ON(!kallsyms_symbol_next(p_tmp, i)))
> >> + ret = kallsyms_symbol_next(p_tmp, i, buf_size);
> >> + if (WARN_ON(!ret))
> >> break;
> > I'm getting confused by having two different branches on ret.
> >
> > Don't get a WARN_ON() when ret == -E2BIG?
> >
> >
>
> Should we WARN on a really long symbol? I don't think we should as we're
> handling that by truncating the output and adding ellipses below.
It's OK. You describe the behaviour I expect but I was misreading the
code (not realizing that kallsyms_symbol_next() had not become a 0 on
success success function).
However after reviewing the code (properly this time) I wonder if the
WARN_ON() should be improved to match the return value for the function:
WARN_ON(ret >= 0 && ret < len)
That said, checking the symbol length is pretty paranoid and getting
close to nitpicking so I'll leave it up to you and, with or without, the
change:
Reviewed-by: Daniel Thompson <[email protected]>
Daniel.
On 10/04/2018 08:45 AM, Daniel Thompson wrote:
> On Thu, Oct 04, 2018 at 08:25:30AM -0400, Prarit Bhargava wrote:
>> On 10/02/2018 11:53 AM, Daniel Thompson wrote:
>>> On 20/09/2018 13:59, Prarit Bhargava wrote:
>>>> gcc 8.1.0 warns with:
>>>>
>>>> kernel/debug/kdb/kdb_support.c: In function ‘kallsyms_symbol_next’:
>>>> kernel/debug/kdb/kdb_support.c:239:4: warning: ‘strncpy’ specified bound
>>>> depends on the length of the source argument [-Wstringop-overflow=]
>>>> strncpy(prefix_name, name, strlen(name)+1);
>>>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>> kernel/debug/kdb/kdb_support.c:239:31: note: length computed here
>>>>
>>>> Use strscpy() with the destination buffer size, and use ellipses when
>>>> displaying truncated symbols.
>>>>
>>>> v2: Use strscpy()
>>>>
>>>> Signed-off-by: Prarit Bhargava <[email protected]>
>>>> Cc: Jonathan Toppins <[email protected]>
>>>> Cc: Jason Wessel <[email protected]>
>>>> Cc: Daniel Thompson <[email protected]>
>>>> Cc: [email protected]
>>>> ---
>>>> kernel/debug/kdb/kdb_io.c | 15 +++++++++------
>>>> kernel/debug/kdb/kdb_private.h | 2 +-
>>>> kernel/debug/kdb/kdb_support.c | 10 +++++-----
>>>> 3 files changed, 15 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
>>>> index ed5d34925ad0..6a4b41484afe 100644
>>>> --- a/kernel/debug/kdb/kdb_io.c
>>>> +++ b/kernel/debug/kdb/kdb_io.c
>>>> @@ -216,7 +216,7 @@ static char *kdb_read(char *buffer, size_t bufsize)
>>>> int count;
>>>> int i;
>>>> int diag, dtab_count;
>>>> - int key;
>>>> + int key, buf_size, ret;
>>>> diag = kdbgetintenv("DTABCOUNT", &dtab_count);
>>>> @@ -336,9 +336,8 @@ static char *kdb_read(char *buffer, size_t bufsize)
>>>> else
>>>> p_tmp = tmpbuffer;
>>>> len = strlen(p_tmp);
>>>> - count = kallsyms_symbol_complete(p_tmp,
>>>> - sizeof(tmpbuffer) -
>>>> - (p_tmp - tmpbuffer));
>>>> + buf_size = sizeof(tmpbuffer) - (p_tmp - tmpbuffer);
>>>> + count = kallsyms_symbol_complete(p_tmp, buf_size);
>>>> if (tab == 2 && count > 0) {
>>>> kdb_printf("\n%d symbols are found.", count);
>>>> if (count > dtab_count) {
>>>> @@ -350,9 +349,13 @@ static char *kdb_read(char *buffer, size_t bufsize)
>>>> }
>>>> kdb_printf("\n");
>>>> for (i = 0; i < count; i++) {
>>>> - if (WARN_ON(!kallsyms_symbol_next(p_tmp, i)))
>>>> + ret = kallsyms_symbol_next(p_tmp, i, buf_size);
>>>> + if (WARN_ON(!ret))
>>>> break;
>>> I'm getting confused by having two different branches on ret.
>>>
>>> Don't get a WARN_ON() when ret == -E2BIG?
>>>
>>>
>>
>> Should we WARN on a really long symbol? I don't think we should as we're
>> handling that by truncating the output and adding ellipses below.
>
> It's OK. You describe the behaviour I expect but I was misreading the
> code (not realizing that kallsyms_symbol_next() had not become a 0 on
> success success function).
>
> However after reviewing the code (properly this time) I wonder if the
> WARN_ON() should be improved to match the return value for the function:
>
> WARN_ON(ret >= 0 && ret < len)
>
> That said, checking the symbol length is pretty paranoid and getting
> close to nitpicking so I'll leave it up to you and, with or without, the
> change:
>
> Reviewed-by: Daniel Thompson <[email protected]>
>
>
I think we should leave it as is for now. OOC was this patch picked up by
anyone's repo for staging for 4.20?
P.
> Daniel.
>