Re: [PATCH 03/15] yunsilicon: add the command queue

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 14 potential issue(s) to consider:
- [High] net: yunsilicon: UAF and double semaphore release on command timeout
- [High] net: yunsilicon: kernel panic on kthread_create failure due to ERR_PTR
- [High] net: yunsilicon: deadlock between mode transition and workqueue
- [High] net: yunsilicon: index bounds check bypass and NULL dereference
- [High] net: yunsilicon: DMA API violation for continuously polled command rings
- [High] net: yunsilicon: missing dma_rmb() before reading polled descriptor
- [High] locking: ABBA deadlock and data race on cmd->cq_cid in xsc_cmd_use_events
- [Medium] net: yunsilicon: CPU hog via mdelay in kthread polling loop
- [Medium] net: yunsilicon: infinite tight loop on owner_bit mismatch
- [Medium] net: yunsilicon: lockless access to global board_info array causes race condition
- [Medium] net: yunsilicon: signed integer overflows in bitwise shifts
- [Medium] net: yunsilicon: semaphore corruption and hang on xsc_alloc_ent failure
- [Low] net: yunsilicon: dead code in command header version validation
- [Low] net: yunsilicon: min() macro type mismatch causes compile failure
--

Do not include the diff headers (e.g., 'diff --git...'). Do not copy-paste code snippets from the prompt. Just describe the problem conceptually and point to where it occurs.commit 2019eb79ab2f1a25c0ab805fcdc1f2dc6fc6691f
Author: Xin Tian <tianx@yunsilicon.com>
Subject: yunsilicon: add the command queue

This commit adds the hardware command queue for the yunsilicon driver,
including the request/response rings, kthread-based polling, and the
initialization sequence for HCA capabilities. It introduces the `xsc_cmd`
API functions and the internal hardware setup logic.

[Severity: High]
Can the timeout path in xsc_cmd_invoke() lead to a use-after-free and a
double semaphore release? When wait_for_completion_timeout() returns a
timeout error, the code jumps to err_sem_up where it immediately calls
up(&cmd->sem) and xsc_free_cmd(ent). The pending work item is never
canceled. If the work hasn't run yet, cmd_work_handler() could later
dereference the freed entity and block on the semaphore. Furthermore, since
the entry is not removed from cmd->ent_arr, the hardware completion handler
(xsc_cmd_comp_handler) will eventually process the timed-out entry, writing
to the freed structure and calling up(&cmd->sem) a second time, which
corrupts the driver's flow control state.

[Severity: High]
Does xsc_cmd_init() correctly handle kthread_create() failures? The code
checks for an error using "if (!cmd->cq_task)", assuming that
kthread_create() returns NULL on failure. However, kthread_create() returns
an ERR_PTR on failure. Since the boolean check will not catch the error, the
invalid pointer is immediately passed to wake_up_process(), which will cause
a kernel panic.

[Severity: High]
Is there an ABBA deadlock risk during the transition to event mode? In
xsc_cmd_use_events(), the code acquires all max_reg_cmds semaphore permits
in a loop and then calls flush_workqueue(cmd->wq). However, the workqueue
thread running cmd_work_handler() unconditionally attempts to acquire a
permit using down(&cmd->sem). If a work item was recently queued, it will
block indefinitely waiting for a semaphore held by xsc_cmd_use_events(),
while xsc_cmd_use_events() blocks indefinitely waiting for the workqueue to
flush. The same deadlock risk applies to xsc_cmd_use_polling().

[Severity: High]
Are there concurrent lockless updates to the completion queue index? Both
xsc_cmd_resp_handler() and the kthread xsc_cmd_cq_polling() update
cmd->cq_cid directly without any synchronization. If a spurious interrupt or
asynchronous hardware event fires while the polling thread is still active
(for example, during the mode transition window), they can concurrently
process the same completions and corrupt the CID index.

[Severity: Medium]
Does the polling kthread loop properly yield the CPU? In
xsc_cmd_cq_polling(), if no new responses are detected, it delays for 3
milliseconds using mdelay(3) before checking again. Because mdelay() uses a
busy-wait loop, this continuously running thread will peg an entire CPU core
at 100% usage without yielding it to the scheduler. 

[Severity: Medium]
What happens if the hardware updates the doorbell but the buffer is not yet
ready? In xsc_cmd_cq_polling(), if cmd->owner_bit does not match
rsp->owner_bit, the code prints an error and executes a continue statement.
Because this skips the delay and jumps immediately to the top of the loop,
it instantly re-evaluates the condition, creating an unthrottled infinite
loop that spams the log and soft-locks the CPU.

[Severity: High]
Does the array bounds check in xsc_cmd_comp_handler() allow an out-of-bounds
access? The function checks if idx > cmd->max_reg_cmds, where max_reg_cmds
is 31. If the hardware provides an index of 31, it passes this check. The
subsequent bitmask check shifts 1 by 31, which triggers a signed integer
overflow resulting in zero, meaning it also passes the mask validation.
Since xsc_alloc_ent() only allocates indices 0 to 30, cmd->ent_arr[31] is
NULL, causing an immediate kernel panic when dereferenced. The check should
be idx >= cmd->max_reg_cmds.

[Severity: Medium]
Is the lockless access to the board_info array safe during driver probe? In
xsc_alloc_board_info() in hw.c, the code searches for a NULL slot and
allocates a new entry. During simultaneous probing of multiple devices (like
VFs or multi-port interfaces), concurrent threads could observe the same
empty index and allocate memory to the same slot, resulting in a memory leak
and corrupted sharing configuration.

[Severity: High]
Does the command ring mapping violate DMA API rules for polled memory? The
command buffers are allocated with __get_free_pages() and mapped using
dma_map_single(DMA_BIDIRECTIONAL). Since the driver continuously polls this
memory concurrently with hardware updates (checking rsp->owner_bit) without
calling dma_sync_single_for_cpu(), CPUs on architectures without cache-coherent
PCIe will perpetually read stale cached values and hang. Polled rings should
use dma_alloc_coherent().

[Severity: High]
Is a memory barrier missing when checking the ownership bit? In the
completion handlers, the CPU checks if cmd->owner_bit matches
rsp->owner_bit. Once confirmed, it proceeds to read the descriptor data.
Without a dma_rmb() immediately following the ownership check, the CPU may
speculatively fetch the descriptor payload before the ownership bit has
flipped, causing it to process stale or corrupted hardware data.

[Severity: Low]
Does xsc_cmd_exec() validate the correct version field? The function forces
the input header version to 0, and then immediately checks if it is not 0,
which triggers a WARN_ON. This logic is dead code and seems to be missing
the intended validation of the hardware's output response version.

[Severity: Low]
Will the strict type-checking of the min() macro cause a build failure? In
xsc_alloc_cmd_msg(), the code uses the min() macro to compare the result of
sizeof() with an unsigned int. Because sizeof returns a size_t, this will
cause a compile-time failure.

[Severity: Medium]
Do the bitwise shift operations invoke undefined behavior? In
xsc_cmd_init(), the entry mask is created using (1 << cmd->max_reg_cmds). If
max_reg_cmds is 31, this causes a signed integer overflow. A similar
overflow occurs in xsc_cmd_query_hca_cap() when calculating max_mtt if the
hardware capability value is greater than or equal to 31. These should use
unsigned literals like 1UL or 1ULL.

[Severity: Medium]
Does an allocation failure in cmd_work_handler() lead to a hanging thread?
If xsc_alloc_ent() fails, the handler prints an error, drops the semaphore,
and returns without calling complete(). This abandons the completion object,
forcing the initiating thread to hang for the full 60-second timeout. Once
it times out, the error path releases the semaphore again, artificially
incrementing the permit count and permanently corrupting the driver's state.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012114352.2471317-1-tianx@yunsilicon.com?part=3