| Message ID | 20230529111337.352990-2-maninder1.s@samsung.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 719dfd5925e186e09a2a6f23016936ac436f3d78 |
| Headers | show |
| Series | [1/2] hexagon/traps.c: use KSYM_NAME_LEN in array size | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
| snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 24 jobs. |
| snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 6 jobs. |
Maninder Singh <maninder1.s@samsung.com> writes: > 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 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") AFAICS that's the wrong sha. That commit appears in linux-next, but the commit that actually went into mainline is: b8a94bfb3395 ("kallsyms: increase maximum kernel symbol length to 512") So I'll update the change log to refer to that. cheers > Co-developed-by: Onkarnath <onkarnath.1@samsung.com> > Signed-off-by: Onkarnath <onkarnath.1@samsung.com> > Signed-off-by: Maninder Singh <maninder1.s@samsung.com> > --- > arch/powerpc/xmon/xmon.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > 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
On Mon, May 29, 2023 at 1:14 PM Maninder Singh <maninder1.s@samsung.com> wrote: > > +static char tmpstr[KSYM_NAME_LEN]; Reviewed-by: Miguel Ojeda <ojeda@kernel.org> Side-note: in `get_function_bounds()`, I see `kallsyms_lookup()` being used, but the name seems discarded? Can `kallsyms_lookup_size_offset()` be used instead, thus avoiding the usage of the buffer there to begin with? Side-note 2: in `scanhex()`, I see a loop `i<63` using `tmpstr` which then is used to do a `kallsyms_lookup_name()`, so I guess symbols larger than 64 couldn't be found. I have no idea about what are the external constraints here, but perhaps it is possible to increase the `line` buffer etc. to then allow for bigger symbols to be found. Cheers, Miguel
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> writes: > On Mon, May 29, 2023 at 1:14 PM Maninder Singh <maninder1.s@samsung.com> wrote: >> >> +static char tmpstr[KSYM_NAME_LEN]; > > Reviewed-by: Miguel Ojeda <ojeda@kernel.org> > > Side-note: in `get_function_bounds()`, I see `kallsyms_lookup()` being > used, but the name seems discarded? Can > `kallsyms_lookup_size_offset()` be used instead, thus avoiding the > usage of the buffer there to begin with? A few lines below it uses the modname, and AFAICS there's no (easy) way to lookup the modname without also looking up the name. > Side-note 2: in `scanhex()`, I see a loop `i<63` using `tmpstr` which > then is used to do a `kallsyms_lookup_name()`, so I guess symbols > larger than 64 couldn't be found. I have no idea about what are the > external constraints here, but perhaps it is possible to increase the > `line` buffer etc. to then allow for bigger symbols to be found. Yeah that looks wrong. I don't see any symbols that long in current kernels, but we should fix it. Thanks for looking. cheers
On Thu, Jun 1, 2023 at 4:02 AM Michael Ellerman <mpe@ellerman.id.au> wrote: > > > Side-note: in `get_function_bounds()`, I see `kallsyms_lookup()` being > > used, but the name seems discarded? Can > > `kallsyms_lookup_size_offset()` be used instead, thus avoiding the > > usage of the buffer there to begin with? > > A few lines below it uses the modname, and AFAICS there's no (easy) way > to lookup the modname without also looking up the name. Hmm... I think you are looking at the `xmon_print_symbol()` one? I was referring to the `get_function_bounds()` one, where the `modname` parameter is `NULL` (and the `name` contents are not used, only whether it was found or not). > > Side-note 2: in `scanhex()`, I see a loop `i<63` using `tmpstr` which > > then is used to do a `kallsyms_lookup_name()`, so I guess symbols > > larger than 64 couldn't be found. I have no idea about what are the > > external constraints here, but perhaps it is possible to increase the > > `line` buffer etc. to then allow for bigger symbols to be found. > > Yeah that looks wrong. I don't see any symbols that long in current > kernels, but we should fix it. > > Thanks for looking. My pleasure! Cheers, Miguel
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> writes: > On Thu, Jun 1, 2023 at 4:02 AM Michael Ellerman <mpe@ellerman.id.au> wrote: >> >> > Side-note: in `get_function_bounds()`, I see `kallsyms_lookup()` being >> > used, but the name seems discarded? Can >> > `kallsyms_lookup_size_offset()` be used instead, thus avoiding the >> > usage of the buffer there to begin with? >> >> A few lines below it uses the modname, and AFAICS there's no (easy) way >> to lookup the modname without also looking up the name. > > Hmm... I think you are looking at the `xmon_print_symbol()` one? I was > referring to the `get_function_bounds()` one, where the `modname` > parameter is `NULL` (and the `name` contents are not used, only > whether it was found or not). Yes you're right, apparently I can't read :} cheers
On 30/5/23 10:54 pm, Miguel Ojeda wrote: > Side-note: in `get_function_bounds()`, I see `kallsyms_lookup()` being > used, but the name seems discarded? Can > `kallsyms_lookup_size_offset()` be used instead, thus avoiding the > usage of the buffer there to begin with? I'm not familiar with the kallsyms infrastructure, but looking over the implementations of kallsyms_lookup() and kallsyms_lookup_size_offset() it looks like the existing kallsyms_lookup() handles an extra case over kallsyms_lookup_size_offset()? kallsyms_lookup_buildid() (the implementation of kallsyms_lookup()) has /* See if it's in a module or a BPF JITed image. */ ret = module_address_lookup(addr, symbolsize, offset, modname, modbuildid, namebuf); if (!ret) ret = bpf_address_lookup(addr, symbolsize, offset, modname, namebuf); if (!ret) ret = ftrace_mod_address_lookup(addr, symbolsize, offset, modname, namebuf); while kallsyms_lookup_size_offset() is missing the ftrace case return !!module_address_lookup(addr, symbolsize, offset, NULL, NULL, namebuf) || !!__bpf_address_lookup(addr, symbolsize, offset, namebuf); Might this be a concern for xmon?
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];
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 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") Co-developed-by: Onkarnath <onkarnath.1@samsung.com> Signed-off-by: Onkarnath <onkarnath.1@samsung.com> Signed-off-by: Maninder Singh <maninder1.s@samsung.com> --- arch/powerpc/xmon/xmon.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)