* [PATCH 1/1] arch:hexagon/powerpc: use KSYM_NAME_LEN in array size
[not found] <CGME20230529052832epcas5p4fa1b8cf25d9810d32bd2ccf012086fb3@epcas5p4.samsung.com>
@ 2023-05-29 5:28 ` Maninder Singh
2023-05-29 10:45 ` Miguel Ojeda
[not found] ` <CGME20230529052832epcas5p4fa1b8cf25d9810d32bd2ccf012086fb3@epcms5p1>
0 siblings, 2 replies; 8+ messages in thread
From: Maninder Singh @ 2023-05-29 5:28 UTC (permalink / raw)
To: bcain, mpe, npiggin, christophe.leroy, keescook, nathanl,
ustavoars, alex.gaynor, gary, ojeda, pmladek, wedsonaf
Cc: linux-hexagon, linux-kernel, linuxppc-dev, Maninder Singh, Onkarnath
kallsyms_lookup which in turn calls for kallsyms_lookup_buildid()
writes on index "KSYM_NAME_LEN - 1".
Thus array size should be KSYM_NAME_LEN.
for powerpc and hexagon it was defined as "128" directly.
and commit '61968dbc2d5d' changed define value to 512,
So both were missed to update with new size.
Fixes: 61968dbc2d5d ("kallsyms: increase maximum kernel symbol length to 512")
Signed-off-by: Onkarnath <onkarnath.1@samsung.com>
Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
---
arch/hexagon/kernel/traps.c | 2 +-
arch/powerpc/xmon/xmon.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/hexagon/kernel/traps.c b/arch/hexagon/kernel/traps.c
index 6447763ce5a9..65b30b6ea226 100644
--- a/arch/hexagon/kernel/traps.c
+++ b/arch/hexagon/kernel/traps.c
@@ -82,7 +82,7 @@ static void do_show_stack(struct task_struct *task, unsigned long *fp,
const char *name = NULL;
unsigned long *newfp;
unsigned long low, high;
- char tmpstr[128];
+ char tmpstr[KSYM_NAME_LEN];
char *modname;
int i;
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index 728d3c257e4a..70c4c59a1a8f 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -88,7 +88,7 @@ static unsigned long ndump = 64;
static unsigned long nidump = 16;
static unsigned long ncsum = 4096;
static int termch;
-static char tmpstr[128];
+static char tmpstr[KSYM_NAME_LEN];
static int tracing_enabled;
static long bus_error_jmp[JMP_BUF_LEN];
--
2.17.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] arch:hexagon/powerpc: use KSYM_NAME_LEN in array size
2023-05-29 5:28 ` [PATCH 1/1] arch:hexagon/powerpc: use KSYM_NAME_LEN in array size Maninder Singh
@ 2023-05-29 10:45 ` Miguel Ojeda
[not found] ` <CGME20230529052832epcas5p4fa1b8cf25d9810d32bd2ccf012086fb3@epcms5p1>
1 sibling, 0 replies; 8+ messages in thread
From: Miguel Ojeda @ 2023-05-29 10:45 UTC (permalink / raw)
To: Maninder Singh
Cc: bcain, mpe, npiggin, christophe.leroy, keescook, nathanl,
ustavoars, alex.gaynor, gary, ojeda, pmladek, wedsonaf,
linux-hexagon, linux-kernel, linuxppc-dev, Onkarnath
On Mon, May 29, 2023 at 7:44 AM Maninder Singh <maninder1.s@samsung.com> wrote:
>
> kallsyms_lookup which in turn calls for kallsyms_lookup_buildid()
> writes on index "KSYM_NAME_LEN - 1".
>
> Thus array size should be KSYM_NAME_LEN.
>
> for powerpc and hexagon it was defined as "128" directly.
> and commit '61968dbc2d5d' changed define value to 512,
> So both were missed to update with new size.
>
> Fixes: 61968dbc2d5d ("kallsyms: increase maximum kernel symbol length to 512")
> Signed-off-by: Onkarnath <onkarnath.1@samsung.com>
> Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
Thanks for this!
There is no `From:` at the top. Since I cannot locate the patch in
Lore, did you mean to put both of you as authors perhaps? In that
case, please use a `Co-developed-by` as needed.
Perhaps it is a good idea to submit each arch independently, too.
The changes themselves look fine on a quick inspection, though the
`xmon.c` one is a global buffer (and there is another equally-sized
buffer in `xmon.c` with a hard-coded `128` constant that would be nice
to clarify).
Cheers,
Miguel
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH 1/1] arch:hexagon/powerpc: use KSYM_NAME_LEN in array size
[not found] ` <CGME20230529052832epcas5p4fa1b8cf25d9810d32bd2ccf012086fb3@epcms5p1>
@ 2023-05-29 10:57 ` Maninder Singh
2023-05-29 14:50 ` Miguel Ojeda
0 siblings, 1 reply; 8+ messages in thread
From: Maninder Singh @ 2023-05-29 10:57 UTC (permalink / raw)
To: Miguel Ojeda
Cc: bcain, mpe, npiggin, christophe.leroy, keescook, nathanl,
ustavoars, alex.gaynor, gary, ojeda, pmladek, wedsonaf,
linux-hexagon, linux-kernel, linuxppc-dev, Onkarnath
Hi,
>>
>> kallsyms_lookup which in turn calls for kallsyms_lookup_buildid()
>> writes on index "KSYM_NAME_LEN - 1".
>>
>> Thus array size should be KSYM_NAME_LEN.
>>
>> for powerpc and hexagon it was defined as "128" directly.
>> and commit '61968dbc2d5d' changed define value to 512,
>> So both were missed to update with new size.
>>
>> Fixes: 61968dbc2d5d ("kallsyms: increase maximum kernel symbol length to 512")
>> Signed-off-by: Onkarnath <onkarnath.1@samsung.com>
>> Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
> Thanks for this!
>
> There is no `From:` at the top. Since I cannot locate the patch in
> Lore, did you mean to put both of you as authors perhaps? In that
> case, please use a `Co-developed-by` as needed.
>
I Will add co-developed-by` tag.
because this change was identified while we were working on kallsyms some time back.
https://lore.kernel.org/lkml/YonTOL4zC4CytVrn@infradead.org/t/
this patch set is pending and we will start working on that again, so i thought better
to send bugfix first.
> Perhaps it is a good idea to submit each arch independently, too.
>
ok, I will share 2 separate patches.
> The changes themselves look fine on a quick inspection, though the
> `xmon.c` one is a global buffer (and there is another equally-sized
> buffer in `xmon.c` with a hard-coded `128` constant that would be nice
> to clarify).
Yes, I think second buffer was not related to kallsyms, so I have not touched that.
Thanks,
Maninder Singh
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] arch:hexagon/powerpc: use KSYM_NAME_LEN in array size
2023-05-29 10:57 ` Maninder Singh
@ 2023-05-29 14:50 ` Miguel Ojeda
2023-05-30 8:06 ` Petr Mladek
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Miguel Ojeda @ 2023-05-29 14:50 UTC (permalink / raw)
To: maninder1.s, keescook, Steven Rostedt, Masami Hiramatsu
Cc: bcain, mpe, npiggin, christophe.leroy, nathanl, ustavoars,
alex.gaynor, gary, ojeda, pmladek, linux-hexagon, linux-kernel,
linuxppc-dev, Onkarnath, Wedson Almeida Filho
On Mon, May 29, 2023 at 1:08 PM Maninder Singh <maninder1.s@samsung.com> wrote:
>
> I Will add co-developed-by` tag.
> because this change was identified while we were working on kallsyms some time back.
> https://lore.kernel.org/lkml/YonTOL4zC4CytVrn@infradead.org/t/
>
> this patch set is pending and we will start working on that again, so i thought better
> to send bugfix first.
Sounds good to me!
(Fixed Wedson's email address)
> Yes, I think second buffer was not related to kallsyms, so I have not touched that.
Kees: what is the current stance on `[static N]` parameters? Something like:
const char *kallsyms_lookup(unsigned long addr,
unsigned long *symbolsize,
unsigned long *offset,
- char **modname, char *namebuf);
+ char **modname, char namebuf[static
KSYM_NAME_LEN]);
makes the compiler complain about cases like these (even if trivial):
arch/powerpc/xmon/xmon.c:1711:10: error: array argument is too small;
contains 128 elements, callee requires at least 512
[-Werror,-Warray-bounds]
name = kallsyms_lookup(pc, &size, &offset, NULL, tmpstr);
^ ~~~~~~
./include/linux/kallsyms.h:86:29: note: callee declares array
parameter as static here
char **modname, char namebuf[static KSYM_NAME_LEN]);
^ ~~~~~~~~~~~~~~~~~~~~~~
But I only see 2 files in the kernel using `[static N]` (from 2020 and
2021). Should something else be used instead (e.g. `__counted_by`),
even if constexpr-sized?.
Also, I went through the other callers to `kallsyms_lookup` to see
other issues -- one I am not sure about is `fetch_store_symstring` in
`kernel/trace/trace_probe_tmpl.h`. Steven/Masami: is that "with max
length" in the function docs enough? Is it 0xffff?
Thanks!
Cheers,
Miguel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] arch:hexagon/powerpc: use KSYM_NAME_LEN in array size
2023-05-29 14:50 ` Miguel Ojeda
@ 2023-05-30 8:06 ` Petr Mladek
[not found] ` <CGME20230529052832epcas5p4fa1b8cf25d9810d32bd2ccf012086fb3@epcms5p8>
2023-05-30 23:14 ` Kees Cook
2 siblings, 0 replies; 8+ messages in thread
From: Petr Mladek @ 2023-05-30 8:06 UTC (permalink / raw)
To: Miguel Ojeda
Cc: maninder1.s, keescook, Steven Rostedt, Masami Hiramatsu, bcain,
mpe, npiggin, christophe.leroy, nathanl, ustavoars, alex.gaynor,
gary, ojeda, linux-hexagon, linux-kernel, linuxppc-dev,
Onkarnath, Wedson Almeida Filho
On Mon 2023-05-29 16:50:45, Miguel Ojeda wrote:
> On Mon, May 29, 2023 at 1:08 PM Maninder Singh <maninder1.s@samsung.com> wrote:
> >
> > I Will add co-developed-by` tag.
> > because this change was identified while we were working on kallsyms some time back.
> > https://lore.kernel.org/lkml/YonTOL4zC4CytVrn@infradead.org/t/
> >
> > this patch set is pending and we will start working on that again, so i thought better
> > to send bugfix first.
>
> Sounds good to me!
>
> (Fixed Wedson's email address)
>
> > Yes, I think second buffer was not related to kallsyms, so I have not touched that.
>
> Kees: what is the current stance on `[static N]` parameters? Something like:
>
> const char *kallsyms_lookup(unsigned long addr,
> unsigned long *symbolsize,
> unsigned long *offset,
> - char **modname, char *namebuf);
> + char **modname, char namebuf[static
> KSYM_NAME_LEN]);
>
> makes the compiler complain about cases like these (even if trivial):
>
> arch/powerpc/xmon/xmon.c:1711:10: error: array argument is too small;
> contains 128 elements, callee requires at least 512
> [-Werror,-Warray-bounds]
> name = kallsyms_lookup(pc, &size, &offset, NULL, tmpstr);
> ^ ~~~~~~
> ./include/linux/kallsyms.h:86:29: note: callee declares array
> parameter as static here
> char **modname, char namebuf[static KSYM_NAME_LEN]);
> ^ ~~~~~~~~~~~~~~~~~~~~~~
>
> But I only see 2 files in the kernel using `[static N]` (from 2020 and
> 2021). Should something else be used instead (e.g. `__counted_by`),
> even if constexpr-sized?.
>
> Also, I went through the other callers to `kallsyms_lookup` to see
> other issues -- one I am not sure about is `fetch_store_symstring` in
> `kernel/trace/trace_probe_tmpl.h`. Steven/Masami: is that "with max
> length" in the function docs enough? Is it 0xffff?
The best solution would be to pass the buffer size as an extra
parameter. Especially when some code passes buffers that are
allocated/reserved dynamically.
Sigh, I am not sure how many changes it would require in kallsyms
API and all the callers. But it would be really appreciated, IMHO.
Best Regards,
Petr
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH 1/1] arch:hexagon/powerpc: use KSYM_NAME_LEN in array size
[not found] ` <CGME20230529052832epcas5p4fa1b8cf25d9810d32bd2ccf012086fb3@epcms5p8>
@ 2023-05-30 8:14 ` Maninder Singh
0 siblings, 0 replies; 8+ messages in thread
From: Maninder Singh @ 2023-05-30 8:14 UTC (permalink / raw)
To: Petr Mladek
Cc: Miguel Ojeda, keescook, Steven Rostedt, Masami Hiramatsu, bcain,
mpe, npiggin, christophe.leroy, nathanl, ustavoars, alex.gaynor,
gary, ojeda, linux-hexagon, linux-kernel, linuxppc-dev,
Onkarnath, Wedson Almeida Filho
Hi Peter,
>
> The best solution would be to pass the buffer size as an extra
> parameter. Especially when some code passes buffers that are
> allocated/reserved dynamically.
>
> Sigh, I am not sure how many changes it would require in kallsyms
> API and all the callers. But it would be really appreciated, IMHO.
>
yes we already prepared size changes 5-6 months back:
https://lore.kernel.org/lkml/YonTOL4zC4CytVrn@infradead.org/t/
[PATCH 1/5] kallsyms: pass buffer size in sprint_* APIs
But at that time new API development(for replacement of seq_buf) was in progress and we decided to wait for that completion.
https://lore.kernel.org/r/20220604193042.1674951-2-kent.overstreet@gmail.com
https://lore.kernel.org/r/20220604193042.1674951-4-kent.overstreet@gmail.com
As I checeked these APIs are not pushed to mainline.
we will try to prepare new patch set for kallsym changes again
with seq_buf to take care of length argument.
Thanks,
Maninder Singh
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] arch:hexagon/powerpc: use KSYM_NAME_LEN in array size
2023-05-29 14:50 ` Miguel Ojeda
2023-05-30 8:06 ` Petr Mladek
[not found] ` <CGME20230529052832epcas5p4fa1b8cf25d9810d32bd2ccf012086fb3@epcms5p8>
@ 2023-05-30 23:14 ` Kees Cook
2023-06-18 14:20 ` Miguel Ojeda
2 siblings, 1 reply; 8+ messages in thread
From: Kees Cook @ 2023-05-30 23:14 UTC (permalink / raw)
To: Miguel Ojeda
Cc: maninder1.s, Steven Rostedt, Masami Hiramatsu, bcain, mpe,
npiggin, christophe.leroy, nathanl, ustavoars, alex.gaynor, gary,
ojeda, pmladek, linux-hexagon, linux-kernel, linuxppc-dev,
Onkarnath, Wedson Almeida Filho
On Mon, May 29, 2023 at 04:50:45PM +0200, Miguel Ojeda wrote:
> Kees: what is the current stance on `[static N]` parameters? Something like:
>
> const char *kallsyms_lookup(unsigned long addr,
> unsigned long *symbolsize,
> unsigned long *offset,
> - char **modname, char *namebuf);
> + char **modname, char namebuf[static KSYM_NAME_LEN]);
>
> makes the compiler complain about cases like these (even if trivial):
>
> arch/powerpc/xmon/xmon.c:1711:10: error: array argument is too small;
> contains 128 elements, callee requires at least 512
> [-Werror,-Warray-bounds]
> name = kallsyms_lookup(pc, &size, &offset, NULL, tmpstr);
> ^ ~~~~~~
> ./include/linux/kallsyms.h:86:29: note: callee declares array
> parameter as static here
> char **modname, char namebuf[static KSYM_NAME_LEN]);
> ^ ~~~~~~~~~~~~~~~~~~~~~~
Wouldn't that be a good thing? (I.e. complain about the size mismatch?)
> But I only see 2 files in the kernel using `[static N]` (from 2020 and
> 2021). Should something else be used instead (e.g. `__counted_by`),
> even if constexpr-sized?.
Yeah, it seems pretty uncommon. I'd say traditionally arrays aren't
based too often, rather structs containing them.
But ultimately, yeah, everything could gain __counted_by and friends in
the future.
--
Kees Cook
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] arch:hexagon/powerpc: use KSYM_NAME_LEN in array size
2023-05-30 23:14 ` Kees Cook
@ 2023-06-18 14:20 ` Miguel Ojeda
0 siblings, 0 replies; 8+ messages in thread
From: Miguel Ojeda @ 2023-06-18 14:20 UTC (permalink / raw)
To: Kees Cook
Cc: maninder1.s, Steven Rostedt, Masami Hiramatsu, bcain, mpe,
npiggin, christophe.leroy, nathanl, ustavoars, alex.gaynor, gary,
ojeda, pmladek, linux-hexagon, linux-kernel, linuxppc-dev,
Onkarnath, Wedson Almeida Filho
On Wed, May 31, 2023 at 1:14 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Mon, May 29, 2023 at 04:50:45PM +0200, Miguel Ojeda wrote:
> > Kees: what is the current stance on `[static N]` parameters? Something like:
> >
> > const char *kallsyms_lookup(unsigned long addr,
> > unsigned long *symbolsize,
> > unsigned long *offset,
> > - char **modname, char *namebuf);
> > + char **modname, char namebuf[static KSYM_NAME_LEN]);
> >
> > makes the compiler complain about cases like these (even if trivial):
> >
> > arch/powerpc/xmon/xmon.c:1711:10: error: array argument is too small;
> > contains 128 elements, callee requires at least 512
> > [-Werror,-Warray-bounds]
> > name = kallsyms_lookup(pc, &size, &offset, NULL, tmpstr);
> > ^ ~~~~~~
> > ./include/linux/kallsyms.h:86:29: note: callee declares array
> > parameter as static here
> > char **modname, char namebuf[static KSYM_NAME_LEN]);
> > ^ ~~~~~~~~~~~~~~~~~~~~~~
>
> Wouldn't that be a good thing? (I.e. complain about the size mismatch?)
Yeah, I would say so (i.e. I meant it as a good thing).
> > But I only see 2 files in the kernel using `[static N]` (from 2020 and
> > 2021). Should something else be used instead (e.g. `__counted_by`),
> > even if constexpr-sized?.
>
> Yeah, it seems pretty uncommon. I'd say traditionally arrays aren't
> based too often, rather structs containing them.
>
> But ultimately, yeah, everything could gain __counted_by and friends in
> the future.
That would be nice!
Cheers,
Miguel
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-06-18 14:20 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CGME20230529052832epcas5p4fa1b8cf25d9810d32bd2ccf012086fb3@epcas5p4.samsung.com>
2023-05-29 5:28 ` [PATCH 1/1] arch:hexagon/powerpc: use KSYM_NAME_LEN in array size Maninder Singh
2023-05-29 10:45 ` Miguel Ojeda
[not found] ` <CGME20230529052832epcas5p4fa1b8cf25d9810d32bd2ccf012086fb3@epcms5p1>
2023-05-29 10:57 ` Maninder Singh
2023-05-29 14:50 ` Miguel Ojeda
2023-05-30 8:06 ` Petr Mladek
[not found] ` <CGME20230529052832epcas5p4fa1b8cf25d9810d32bd2ccf012086fb3@epcms5p8>
2023-05-30 8:14 ` Maninder Singh
2023-05-30 23:14 ` Kees Cook
2023-06-18 14:20 ` Miguel Ojeda
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).