On Mon 2020-06-29 16:59:24, Cengiz Can wrote:
> `kdb_msg_write` operates on a global `struct kgdb_io *` called
> `dbg_io_ops`.
>
> Although it is initialized in `debug_core.c`, there's a null check in
> `kdb_msg_write` which implies that it can be null whenever we dereference
> it in this function call.
>
> Coverity scanner caught this as CID 1465042.
>
> I have modified the function to bail out if `dbg_io_ops` is not properly
> initialized.
>
> Signed-off-by: Cengiz Can <[email protected]>
> ---
> kernel/debug/kdb/kdb_io.c | 15 ++++++++-------
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> index 683a799618ad..85e579812458 100644
> --- a/kernel/debug/kdb/kdb_io.c
> +++ b/kernel/debug/kdb/kdb_io.c
> @@ -549,14 +549,15 @@ static void kdb_msg_write(const char *msg, int msg_len)
> if (msg_len == 0)
> return;
>
> - if (dbg_io_ops) {
> - const char *cp = msg;
> - int len = msg_len;
> + if (!dbg_io_ops)
> + return;
This looks wrong. The message should be printed to the consoles
even when dbg_io_ops is NULL. I mean that the for_each_console(c)
cycle should always get called.
Well, the code really looks racy. dbg_io_ops is set under
kgdb_registration_lock. IMHO, it should also get accessed under this lock.
It seems that the race is possible. kdb_msg_write() is called from
vkdb_printf(). This function is serialized on more CPUs using
kdb_printf_cpu lock. But it is not serialized with
kgdb_register_io_module() and kgdb_unregister_io_module() calls.
But I might miss something. dbg_io_ops is dereferenced on many other
locations without any check.
>
> - while (len--) {
> - dbg_io_ops->write_char(*cp);
> - cp++;
> - }
> + const char *cp = msg;
> + int len = msg_len;
> +
> + while (len--) {
> + dbg_io_ops->write_char(*cp);
> + cp++;
> }
>
> for_each_console(c) {
You probably got confused by this new code:
if (c == dbg_io_ops->cons)
continue;
It dereferences dbg_io_ops without NULL check. It should probably
get replaced by:
if (dbg_io_ops && c == dbg_io_ops->cons)
continue;
Daniel, Sumit, could you please put some light on this?
Best Regards,
Petr
On Mon 2020-06-29 16:50:20, Petr Mladek wrote:
> On Mon 2020-06-29 16:59:24, Cengiz Can wrote:
> > `kdb_msg_write` operates on a global `struct kgdb_io *` called
> > `dbg_io_ops`.
> >
> > Although it is initialized in `debug_core.c`, there's a null check in
> > `kdb_msg_write` which implies that it can be null whenever we dereference
> > it in this function call.
> >
> > Coverity scanner caught this as CID 1465042.
> >
> > I have modified the function to bail out if `dbg_io_ops` is not properly
> > initialized.
> >
> > Signed-off-by: Cengiz Can <[email protected]>
> > ---
> > kernel/debug/kdb/kdb_io.c | 15 ++++++++-------
> > 1 file changed, 8 insertions(+), 7 deletions(-)
> >
> > diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> > index 683a799618ad..85e579812458 100644
> > --- a/kernel/debug/kdb/kdb_io.c
> > +++ b/kernel/debug/kdb/kdb_io.c
> > @@ -549,14 +549,15 @@ static void kdb_msg_write(const char *msg, int msg_len)
> > if (msg_len == 0)
> > return;
> >
> > - if (dbg_io_ops) {
> > - const char *cp = msg;
> > - int len = msg_len;
> > + if (!dbg_io_ops)
> > + return;
>
> This looks wrong. The message should be printed to the consoles
> even when dbg_io_ops is NULL. I mean that the for_each_console(c)
> cycle should always get called.
Please, forget this mail. Daniel explained that dbg_io_ops must have
been set when this function gets called.
I am sorry for the noise.
Best Regards,
Petr
On Mon, Jun 29, 2020 at 04:50:20PM +0200, Petr Mladek wrote:
> On Mon 2020-06-29 16:59:24, Cengiz Can wrote:
> > `kdb_msg_write` operates on a global `struct kgdb_io *` called
> > `dbg_io_ops`.
> >
> > Although it is initialized in `debug_core.c`, there's a null check in
> > `kdb_msg_write` which implies that it can be null whenever we dereference
> > it in this function call.
> >
> > Coverity scanner caught this as CID 1465042.
> >
> > I have modified the function to bail out if `dbg_io_ops` is not properly
> > initialized.
> >
> > Signed-off-by: Cengiz Can <[email protected]>
> > ---
> > kernel/debug/kdb/kdb_io.c | 15 ++++++++-------
> > 1 file changed, 8 insertions(+), 7 deletions(-)
> >
> > diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> > index 683a799618ad..85e579812458 100644
> > --- a/kernel/debug/kdb/kdb_io.c
> > +++ b/kernel/debug/kdb/kdb_io.c
> > @@ -549,14 +549,15 @@ static void kdb_msg_write(const char *msg, int msg_len)
> > if (msg_len == 0)
> > return;
> >
> > - if (dbg_io_ops) {
> > - const char *cp = msg;
> > - int len = msg_len;
> > + if (!dbg_io_ops)
> > + return;
>
> This looks wrong. The message should be printed to the consoles
> even when dbg_io_ops is NULL. I mean that the for_each_console(c)
> cycle should always get called.
>
> Well, the code really looks racy. dbg_io_ops is set under
> kgdb_registration_lock. IMHO, it should also get accessed under this lock.
>
> It seems that the race is possible. kdb_msg_write() is called from
> vkdb_printf(). This function is serialized on more CPUs using
> kdb_printf_cpu lock. But it is not serialized with
> kgdb_register_io_module() and kgdb_unregister_io_module() calls.
We can't take the lock from the trap handler itself since we cannot
have spinlocks contended between regular threads and the debug trap
(which could be an NMI).
Instead, the call to kgdb_unregister_callbacks() at the beginning
of kgdb_unregister_io_module() should render kdb_msg_write()
unreachable prior to dbg_io_ops becoming NULL.
As it happens I am starting to believe there is a race in this area but
the race is between register/unregister calls rather than against the
trap handler (if there were register/unregister races then the trap
handler is be a potential victim of the race though).
> But I might miss something. dbg_io_ops is dereferenced on many other
> locations without any check.
There is already a paranoid "just in case there are bugs" check in
kgdb_io_ready() so in any case I think the check in kdb_msg_write() can
simply be removed.
As I said in my other post, if dbg_io_ops were ever NULL then the
system is completely hosed anyway: we can never receive the keystroke
needed to leave the debugger... and may not be able to tell anybody
why.
> >
> > - while (len--) {
> > - dbg_io_ops->write_char(*cp);
> > - cp++;
> > - }
> > + const char *cp = msg;
> > + int len = msg_len;
> > +
> > + while (len--) {
> > + dbg_io_ops->write_char(*cp);
> > + cp++;
> > }
> >
> > for_each_console(c) {
>
> You probably got confused by this new code:
>
> if (c == dbg_io_ops->cons)
> continue;
>
> It dereferences dbg_io_ops without NULL check. It should probably
> get replaced by:
>
> if (dbg_io_ops && c == dbg_io_ops->cons)
> continue;
>
> Daniel, Sumit, could you please put some light on this?
As above, I think the NULL check that confuses coverity can simply be
removed.
Daniel.
`kdb_msg_write` operates on a global `struct kgdb_io *` called
`dbg_io_ops`.
It's initialized in `debug_core.c` and checked throughout the debug
flow.
There's a null check in `kdb_msg_write` which triggers static analyzers
and gives the (almost entirely wrong) impression that it can be null.
Coverity scanner caught this as CID 1465042.
I have removed the unnecessary null check and eliminated false-positive
forward null dereference warning.
Signed-off-by: Cengiz Can <[email protected]>
---
kernel/debug/kdb/kdb_io.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
index 683a799618ad..4ac59a4fbeec 100644
--- a/kernel/debug/kdb/kdb_io.c
+++ b/kernel/debug/kdb/kdb_io.c
@@ -549,14 +549,12 @@ static void kdb_msg_write(const char *msg, int msg_len)
if (msg_len == 0)
return;
- if (dbg_io_ops) {
- const char *cp = msg;
- int len = msg_len;
+ const char *cp = msg;
+ int len = msg_len;
- while (len--) {
- dbg_io_ops->write_char(*cp);
- cp++;
- }
+ while (len--) {
+ dbg_io_ops->write_char(*cp);
+ cp++;
}
for_each_console(c) {
--
2.27.0
Hi,
On Mon, Jun 29, 2020 at 1:50 PM Cengiz Can <[email protected]> wrote:
>
> `kdb_msg_write` operates on a global `struct kgdb_io *` called
> `dbg_io_ops`.
>
> It's initialized in `debug_core.c` and checked throughout the debug
> flow.
>
> There's a null check in `kdb_msg_write` which triggers static analyzers
> and gives the (almost entirely wrong) impression that it can be null.
>
> Coverity scanner caught this as CID 1465042.
>
> I have removed the unnecessary null check and eliminated false-positive
> forward null dereference warning.
>
> Signed-off-by: Cengiz Can <[email protected]>
> ---
> kernel/debug/kdb/kdb_io.c | 12 +++++-------
> 1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> index 683a799618ad..4ac59a4fbeec 100644
> --- a/kernel/debug/kdb/kdb_io.c
> +++ b/kernel/debug/kdb/kdb_io.c
> @@ -549,14 +549,12 @@ static void kdb_msg_write(const char *msg, int msg_len)
> if (msg_len == 0)
> return;
>
> - if (dbg_io_ops) {
> - const char *cp = msg;
> - int len = msg_len;
> + const char *cp = msg;
> + int len = msg_len;
kernel/debug/kdb/kdb_io.c:552:14:
warning: ISO C90 forbids mixing declarations and code
[-Wdeclaration-after-statement]
-Doug
On June 30, 2020 00:16:54 Doug Anderson <[email protected]> wrote:
> Hi,
>
> On Mon, Jun 29, 2020 at 1:50 PM Cengiz Can <[email protected]> wrote:
>>
>> `kdb_msg_write` operates on a global `struct kgdb_io *` called
>> `dbg_io_ops`.
>>
>> It's initialized in `debug_core.c` and checked throughout the debug
>> flow.
>>
>> There's a null check in `kdb_msg_write` which triggers static analyzers
>> and gives the (almost entirely wrong) impression that it can be null.
>>
>> Coverity scanner caught this as CID 1465042.
>>
>> I have removed the unnecessary null check and eliminated false-positive
>> forward null dereference warning.
>>
>> Signed-off-by: Cengiz Can <[email protected]>
>> ---
>> kernel/debug/kdb/kdb_io.c | 12 +++++-------
>> 1 file changed, 5 insertions(+), 7 deletions(-)
>>
>> diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
>> index 683a799618ad..4ac59a4fbeec 100644
>> --- a/kernel/debug/kdb/kdb_io.c
>> +++ b/kernel/debug/kdb/kdb_io.c
>> @@ -549,14 +549,12 @@ static void kdb_msg_write(const char *msg, int msg_len)
>> if (msg_len == 0)
>> return;
>>
>> - if (dbg_io_ops) {
>> - const char *cp = msg;
>> - int len = msg_len;
>> + const char *cp = msg;
>> + int len = msg_len;
>
> kernel/debug/kdb/kdb_io.c:552:14:
> warning: ISO C90 forbids mixing declarations and code
> [-Wdeclaration-after-statement]
Oops!
Sorry for that..
I admit I didn't compile check my second attempt.
/me ducks and covers
Will fix it asap.
Thanks!
>
> -Doug
On Mon, 29 Jun 2020 at 21:07, Daniel Thompson
<[email protected]> wrote:
>
> On Mon, Jun 29, 2020 at 04:50:20PM +0200, Petr Mladek wrote:
> > On Mon 2020-06-29 16:59:24, Cengiz Can wrote:
> > > `kdb_msg_write` operates on a global `struct kgdb_io *` called
> > > `dbg_io_ops`.
> > >
> > > Although it is initialized in `debug_core.c`, there's a null check in
> > > `kdb_msg_write` which implies that it can be null whenever we dereference
> > > it in this function call.
> > >
> > > Coverity scanner caught this as CID 1465042.
> > >
> > > I have modified the function to bail out if `dbg_io_ops` is not properly
> > > initialized.
> > >
> > > Signed-off-by: Cengiz Can <[email protected]>
> > > ---
> > > kernel/debug/kdb/kdb_io.c | 15 ++++++++-------
> > > 1 file changed, 8 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> > > index 683a799618ad..85e579812458 100644
> > > --- a/kernel/debug/kdb/kdb_io.c
> > > +++ b/kernel/debug/kdb/kdb_io.c
> > > @@ -549,14 +549,15 @@ static void kdb_msg_write(const char *msg, int msg_len)
> > > if (msg_len == 0)
> > > return;
> > >
> > > - if (dbg_io_ops) {
> > > - const char *cp = msg;
> > > - int len = msg_len;
> > > + if (!dbg_io_ops)
> > > + return;
> >
> > This looks wrong. The message should be printed to the consoles
> > even when dbg_io_ops is NULL. I mean that the for_each_console(c)
> > cycle should always get called.
> >
> > Well, the code really looks racy. dbg_io_ops is set under
> > kgdb_registration_lock. IMHO, it should also get accessed under this lock.
> >
> > It seems that the race is possible. kdb_msg_write() is called from
> > vkdb_printf(). This function is serialized on more CPUs using
> > kdb_printf_cpu lock. But it is not serialized with
> > kgdb_register_io_module() and kgdb_unregister_io_module() calls.
>
> We can't take the lock from the trap handler itself since we cannot
> have spinlocks contended between regular threads and the debug trap
> (which could be an NMI).
>
> Instead, the call to kgdb_unregister_callbacks() at the beginning
> of kgdb_unregister_io_module() should render kdb_msg_write()
> unreachable prior to dbg_io_ops becoming NULL.
>
> As it happens I am starting to believe there is a race in this area but
> the race is between register/unregister calls rather than against the
> trap handler (if there were register/unregister races then the trap
> handler is be a potential victim of the race though).
>
>
> > But I might miss something. dbg_io_ops is dereferenced on many other
> > locations without any check.
>
> There is already a paranoid "just in case there are bugs" check in
> kgdb_io_ready() so in any case I think the check in kdb_msg_write() can
> simply be removed.
>
> As I said in my other post, if dbg_io_ops were ever NULL then the
> system is completely hosed anyway: we can never receive the keystroke
> needed to leave the debugger... and may not be able to tell anybody
> why.
>
>
> > >
> > > - while (len--) {
> > > - dbg_io_ops->write_char(*cp);
> > > - cp++;
> > > - }
> > > + const char *cp = msg;
> > > + int len = msg_len;
> > > +
> > > + while (len--) {
> > > + dbg_io_ops->write_char(*cp);
> > > + cp++;
> > > }
> > >
> > > for_each_console(c) {
> >
> > You probably got confused by this new code:
> >
> > if (c == dbg_io_ops->cons)
> > continue;
> >
> > It dereferences dbg_io_ops without NULL check. It should probably
> > get replaced by:
> >
> > if (dbg_io_ops && c == dbg_io_ops->cons)
> > continue;
> >
> > Daniel, Sumit, could you please put some light on this?
>
> As above, I think the NULL check that confuses coverity can simply be
> removed.
>
+1
-Sumit
>
> Daniel.
`kdb_msg_write` operates on a global `struct kgdb_io *` called
`dbg_io_ops`.
It's initialized in `debug_core.c` and checked throughout the debug
flow.
There's a null check in `kdb_msg_write` which triggers static analyzers
and gives the (almost entirely wrong) impression that it can be null.
Coverity scanner caught this as CID 1465042.
I have removed the unnecessary null check and eliminated false-positive
forward null dereference warning.
Signed-off-by: Cengiz Can <[email protected]>
---
kernel/debug/kdb/kdb_io.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
index 683a799618ad..81783ecaec58 100644
--- a/kernel/debug/kdb/kdb_io.c
+++ b/kernel/debug/kdb/kdb_io.c
@@ -545,18 +545,18 @@ static int kdb_search_string(char *searched, char *searchfor)
static void kdb_msg_write(const char *msg, int msg_len)
{
struct console *c;
+ const char *cp;
+ int len;
if (msg_len == 0)
return;
- if (dbg_io_ops) {
- const char *cp = msg;
- int len = msg_len;
+ cp = msg;
+ len = msg_len;
- while (len--) {
- dbg_io_ops->write_char(*cp);
- cp++;
- }
+ while (len--) {
+ dbg_io_ops->write_char(*cp);
+ cp++;
}
for_each_console(c) {
--
2.27.0
On Tue, 30 Jun 2020 at 14:00, Cengiz Can <[email protected]> wrote:
>
> `kdb_msg_write` operates on a global `struct kgdb_io *` called
> `dbg_io_ops`.
>
> It's initialized in `debug_core.c` and checked throughout the debug
> flow.
>
> There's a null check in `kdb_msg_write` which triggers static analyzers
> and gives the (almost entirely wrong) impression that it can be null.
>
> Coverity scanner caught this as CID 1465042.
>
> I have removed the unnecessary null check and eliminated false-positive
> forward null dereference warning.
>
> Signed-off-by: Cengiz Can <[email protected]>
> ---
> kernel/debug/kdb/kdb_io.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
Reviewed-by: Sumit Garg <[email protected]>
> diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> index 683a799618ad..81783ecaec58 100644
> --- a/kernel/debug/kdb/kdb_io.c
> +++ b/kernel/debug/kdb/kdb_io.c
> @@ -545,18 +545,18 @@ static int kdb_search_string(char *searched, char *searchfor)
> static void kdb_msg_write(const char *msg, int msg_len)
> {
> struct console *c;
> + const char *cp;
> + int len;
>
> if (msg_len == 0)
> return;
>
> - if (dbg_io_ops) {
> - const char *cp = msg;
> - int len = msg_len;
> + cp = msg;
> + len = msg_len;
>
> - while (len--) {
> - dbg_io_ops->write_char(*cp);
> - cp++;
> - }
> + while (len--) {
> + dbg_io_ops->write_char(*cp);
> + cp++;
> }
>
> for_each_console(c) {
> --
> 2.27.0
>
On Tue, Jun 30, 2020 at 05:06:31PM +0530, Sumit Garg wrote:
> On Tue, 30 Jun 2020 at 14:00, Cengiz Can <[email protected]> wrote:
> >
> > `kdb_msg_write` operates on a global `struct kgdb_io *` called
> > `dbg_io_ops`.
> >
> > It's initialized in `debug_core.c` and checked throughout the debug
> > flow.
> >
> > There's a null check in `kdb_msg_write` which triggers static analyzers
> > and gives the (almost entirely wrong) impression that it can be null.
> >
> > Coverity scanner caught this as CID 1465042.
> >
> > I have removed the unnecessary null check and eliminated false-positive
> > forward null dereference warning.
> > - while (len--) {
> > - dbg_io_ops->write_char(*cp);
> > - cp++;
> > - }
> > + while (len--) {
> > + dbg_io_ops->write_char(*cp);
> > + cp++;
> > }
Despite being in the original code this can be simple done in two lines:
while (len--)
dbg_io_ops->write_char(*cp++);
--
With Best Regards,
Andy Shevchenko
Hi,
On Tue, Jun 30, 2020 at 1:30 AM Cengiz Can <[email protected]> wrote:
>
> `kdb_msg_write` operates on a global `struct kgdb_io *` called
> `dbg_io_ops`.
>
> It's initialized in `debug_core.c` and checked throughout the debug
> flow.
>
> There's a null check in `kdb_msg_write` which triggers static analyzers
> and gives the (almost entirely wrong) impression that it can be null.
>
> Coverity scanner caught this as CID 1465042.
>
> I have removed the unnecessary null check and eliminated false-positive
> forward null dereference warning.
>
> Signed-off-by: Cengiz Can <[email protected]>
> ---
> kernel/debug/kdb/kdb_io.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
Reviewed-by: Douglas Anderson <[email protected]>
Tested-by: Douglas Anderson <[email protected]>
Hello Daniel,
On 2020-07-01 01:32, Doug Anderson wrote:
>
> Reviewed-by: Douglas Anderson <[email protected]>
> Tested-by: Douglas Anderson <[email protected]>
Wanted to ask about the status of this proposed patch.
I have checked your tree in git.kernel.org but you might be
collecting them somewhere else perhaps.
Thank you for your time
--
Cengiz Can
@cengiz_io
On Fri, Jul 10, 2020 at 03:15:55PM +0300, Cengiz Can wrote:
> Hello Daniel,
>
> On 2020-07-01 01:32, Doug Anderson wrote:
> >
> > Reviewed-by: Douglas Anderson <[email protected]>
> > Tested-by: Douglas Anderson <[email protected]>
>
> Wanted to ask about the status of this proposed patch.
>
> I have checked your tree in git.kernel.org but you might be
> collecting them somewhere else perhaps.
It's applied... but then holiday happened. Should be pushed out soon.
Daniel.