| Message ID | 1676294058-136786-5-git-send-email-moshe@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | devlink: cleanups and move devlink health functionality to separate file | expand |
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? Same story with adding a note why this is harmless as patch 2
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
diff --git a/net/devlink/leftover.c b/net/devlink/leftover.c index 0b1c5e0122f3..bc72d80141cf 100644 --- a/net/devlink/leftover.c +++ b/net/devlink/leftover.c @@ -6077,8 +6077,8 @@ int devlink_health_report(struct devlink_health_reporter *reporter, int ret; /* write a log message of the current error */ - WARN_ON(!msg); - trace_devlink_health_report(devlink, reporter->ops->name, msg); + if (!WARN_ON(!msg)) + trace_devlink_health_report(devlink, reporter->ops->name, msg); reporter->error_count++; prev_health_state = reporter->health_state; reporter->health_state = DEVLINK_HEALTH_REPORTER_STATE_ERROR;
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. Signed-off-by: Moshe Shemesh <moshe@nvidia.com> --- net/devlink/leftover.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)