Page MenuHomeFreeBSD

if_bnxt: log: fix missing lock coverage in bnxt_log_live()
ClosedPublic

Authored by sumit.saxena_broadcom.com on Aug 21 2026, 3:30 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Oct 7, 9:57 AM
Unknown Object (File)
Sun, Oct 4, 9:56 PM
Unknown Object (File)
Sat, Oct 3, 5:23 PM
Unknown Object (File)
Sat, Oct 3, 6:11 AM
Unknown Object (File)
Sat, Oct 3, 3:36 AM
Unknown Object (File)
Fri, Oct 2, 5:13 AM
Unknown Object (File)
Wed, Sep 30, 9:33 AM
Unknown Object (File)
Wed, Sep 30, 9:33 AM
Subscribers

Details

Summary

bnxt_log_live() walked the loggers TAILQ without holding log_lock,
unlike every other bnxt_log.c function, racing against concurrent
bnxt_register_logger()/bnxt_unregister_logger() calls. Take log_lock
around the traversal, and bail out early (with the lock dropped) if
live_msgs_len has already reached max_live_buff_size, which would
otherwise underflow the length passed to bnxt_log_info().

bnxt_start_logging_driver_coredump() now drops log_lock before
invoking logger->log_live_op() (which calls back into
bnxt_log_live()) and re-acquires it afterward, resetting live_msgs
to NULL so a later bnxt_log_live() call (e.g. from a VF async event
handler) can't write into a coredump buffer the caller has already
freed.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

AI scan fixes:

  • bnxt_start_logging_driver_coredump()'s TAILQ_FOREACH_SAFE captures its next-node pointer under log_lock, but the loop body then drops the lock (to call log_live_op()) before advancing to it. A concurrent bnxt_unregister_logger() can free either that pre-fetched next node or the current node itself during that window (both are dereferenced again after the lock is re-acquired), a use-after-free reachable the moment a second logger exists (BNXT_LOGGER_ROCE is already defined alongside BNXT_LOGGER_L2). Add a refcnt/ unregister_pending pin on struct bnxt_logger: the coredump loop pins the current logger before dropping the lock and only chases its successor by logger_id (a stable value), re-deriving the pointer under the lock afterward rather than trusting one captured before the unlock; bnxt_unregister_logger() still unlinks immediately so no new traversal can find the node, but defers the actual free() until the pin count reaches zero.
  • bnxt_log_info() truncated with text_len > max_len, but the following memcpy() always writes text_len + 1 bytes; the text_len == max_len case slipped through uncut and wrote one byte past the max_len-byte destination (e.g. logger->live_msgs_len could reach max_live_buff_size + 1, one byte past the space bnxt_get_loggers_coredump_size() actually reserved). Truncate at text_len >= max_len instead, reserving the trailing '\n' within the max_len budget.
  • bnxt_log_live() now holds log_lock for its entire traversal, so TAILQ_FOREACH_SAFE's protection against concurrent removal is unnecessary there (unlike the coredump loop, which still drops the lock mid-body); switched to plain TAILQ_FOREACH to say so directly.
  • bnxt_start_logging_driver_coredump() sized the segment length and advanced offset past the live-message region using logger->buffer_size (the historical ring buffer's capacity) instead of the number of bytes log_live_op() actually wrote into live_msgs. bnxt_get_loggers_coredump_size() reserves max_live_buff_size per logger for exactly this region, so using buffer_size instead could either advance offset past where a later logger's segment header actually needs to start (if buffer_size < max_live_buff_size, overlapping/corrupting it) or misreport the segment's declared length to a coredump parser (if buffer_size > max_live_buff_size). Capture live_msgs_len before resetting it and use that instead.

Maybe tone down those comments a bit?

This revision is now accepted and ready to land.Thu, Sep 10, 4:08 PM
This revision now requires review to proceed.Tue, Sep 29, 10:26 AM
This revision is now accepted and ready to land.Wed, Oct 7, 3:50 PM