Re: [PATCH] fbnic: close fw_log race between users and teardown

[Lee Trager]

On 2/9/26 8:38 AM, Chengfeng Ye wrote:

> Fixes a theoretical race on fw_log between the teardown path and fw_log
> show/write functions.
>
> The fw_log null check was performed outside the lock in both
> fbnic_dbg_fw_log_show() and fbnic_fw_log_write() (called from debugfs
> paths or mailbox threaded IRQ handler).
This was done intentionally. Initially the log buffer would only be 
allocated if DebugFS was enabled. If a bug in firmware caused a log to 
be sent the driver can reject it without the lock.
> Concurrent teardown in
> fbnic_fw_log_free() could clear and free the log buffer after the check
> because there is no proper synchronization, leading to list traversal or
> buffer access on freed memory.

fbnic_fw_log_free() is only called when the driver is removed, after 
DebugFS has been disabled. Before freeing the buffer the driver sends an 
explicit message to firmware to stop sending new message.

> fbnic_fw_log_write() can be reached from the mailbox handler
> fbnic_fw_msix_intr(), but fbnic_fw_log_free() runs before IRQ/MBX
> teardown. fbnic_dbg_fw_log_show() runs via debugfs and seems to be
> not synchronized with removal either.
>
> Possible Interleaving scenario:
>    CPU0: fbnic_dbg_fw_log_show() or fbnic_fw_log_write()
>            if (fbnic_fw_log_ready())   // true
>            ... preempt ...
>    CPU1: fbnic_fw_log_free()
>            vfree(log->data_start);
>            log->data_start = NULL;
>    CPU0: continues, walks log->entries or writes to log->data_start
>
> Move readiness checks under the fw_log spinlock, and clear the log state
> under the same lock in fbnic_fw_log_free() before freeing the buffer.
> This makes readers/writers mutually exclusive with teardown.
I can see how this makes things safer but not sure the extra locking is 
truly needed. So far we have not seen issues in test or prod during 
driver removal.

@Alex - Up to you if you think we should always lock when touching the 
buffer.

Lee