netdev - Re: [PATCH net-next 04/10] devlink: health: Don't try to add trace with NULL msg

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <afc5c0af-9c40-3b69-d0ad-1418d3714c43@nvidia.com>
Date:   Tue, 14 Feb 2023 15:48:52 +0200
From:   Moshe Shemesh <moshe@...dia.com>
To:     Jakub Kicinski <kuba@...nel.org>
CC:     "David S. Miller" <davem@...emloft.net>,
        Jiri Pirko <jiri@...dia.com>, <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 04/10] devlink: health: Don't try to add trace
 with NULL msg


On 14/02/2023 8:22, Jakub Kicinski wrote:
> On Mon, 13 Feb 2023 15:14:12 +0200 Moshe Shemesh wrote:
>> In case devlink_health_report() msg argument is NULL a warning is
>> triggered, but then continue and try to print a trace with NULL pointer.
>>
>> Fix it to skip trace call if msg pointer is NULL.
> The trace macros take NULLs, can you double check?

Just tested it, so basically the trace will log "(null)" instead of msg.

But in this case, msg=NULL influence also reporter_name in the trace 
which led me to another fix :

diff --git a/include/trace/events/devlink.h b/include/trace/events/devlink.h
index 24969184c534..77ff7cfc6049 100644
--- a/include/trace/events/devlink.h
+++ b/include/trace/events/devlink.h
@@ -88,7 +88,7 @@ TRACE_EVENT(devlink_health_report,
                 __string(bus_name, devlink_to_dev(devlink)->bus->name)
                 __string(dev_name, dev_name(devlink_to_dev(devlink)))
                 __string(driver_name, 
devlink_to_dev(devlink)->driver->name)
-               __string(reporter_name, msg)
+               __string(reporter_name, reporter_name)
                 __string(msg, msg)
         ),

Here too, the trace logs part of reporter name and "(null)", not really 
harmful, but wrong, as we do have the reporter_name.

I will drop this patch as I think having a log with most data 
(timestamp, device, reporter name), but "(null)" instead of msg along 
with the warning is better then no log.

I will add a patch to fix the wrong trace point struct entry for 
reporter_name.

> Same story with adding a note why this is harmless as patch 2