Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.
Critical: 0 · High: 1 · Medium: 2 (1) · Low: 0
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [High] Incomplete fix: the cc2520_probe() error path
(drivers/net/ieee802154/cc2520.c:1155-1158) still uses…
- [Medium] The commit message's quoted evidence does not match the defect
it describes or the change it makes.
- [Medium] Incorrect Fixes: tag / wrong stable backport range.
Pre-existing issues:
- [Medium] Pre-existing: the cc2520_probe() error path does not roll back
the hardware power/reset ramp.
--
ieee802154: cc2520: flush fifop_irqwork before destroying buffer_mutex in probe
This swaps two statements on the err_hw_init path of cc2520_probe() so that
flush_work(&priv->fifop_irqwork) runs before mutex_destroy(&priv->buffer_mutex),
with the stated goal of matching the teardown order used by cc2520_remove().
A few questions about the change and the commit log below.
>From the commit message:
> In cc2520_probe()'s err_hw_init path, mutex_destroy(&priv->buffer_mutex)
> is called before flush_work(&priv->fifop_irqwork). If fifop_irq fires
> after devm_request_irq() and schedules priv->fifop_irqwork before probe
> fails, cc2520_fifop_irqwork() can lock priv->buffer_mutex after it has
> already been destroyed:
>
> BUG: KASAN: slab-use-after-free in assign_work+0x2f1/0x340
> Read of size 8 at addr ffff88800246b0b0 by task kworker/1:1/43
> Workqueue: 0x0 (events)
> Call Trace:
> <TASK>
> dump_stack_lvl+0x70/0xa0
> print_report+0x153/0x4c6
> kasan_report+0xf1/0x120
> assign_work+0x2f1/0x340
> worker_thread+0x2b8/0xb70
[Severity: Medium]
Does this trace actually correspond to the ordering problem being described?
For !CONFIG_DEBUG_MUTEXES, include/linux/mutex.h has:
static inline void mutex_destroy(struct mutex *lock) {}
so on production kernels the reorder is a no-op. With
CONFIG_DEBUG_MUTEXES, kernel/locking/mutex-debug.c does:
void mutex_destroy(struct mutex *lock)
{
DEBUG_LOCKS_WARN_ON(mutex_is_locked(lock));
lock->magic = NULL;
}
which can produce a DEBUG_LOCKS warning on a subsequent mutex_lock(), but
does not free any memory.
The pasted report is a use-after-free on the work_struct inside the
workqueue core (assign_work()/worker_thread()), i.e. priv freed while the
work was still queued, which is a different failure from
cc2520_fifop_irqwork() locking a destroyed buffer_mutex. Could the commit
message be adjusted so the quoted symptom matches the described defect,
given that the log carries Cc: [email protected]?
Similarly, does "Tested in QEMU with KASAN enabled by scheduling
fifop_irqwork on the cc2520_probe() error path" demonstrate the described
mutex ordering issue, or the queued-work-after-free one?
> Fixes: ff5891b266a7 ("ieee802154: cc2520: fix FIFOP work use-after-free")
[Severity: Medium]
Is this the right commit to blame? ff5891b266a7 changes only
cc2520_remove(), where it replaced the flush with:
disable_work_sync(&priv->fifop_irqwork);
git blame of the err_hw_init sequence being reordered here points at
0da6bc8cc341 ("ieee802154: cc2520: adds driver for TI CC2520 radio") from
2014, not ff5891b266a7. With the current tag, stable trees that predate
ff5891b266a7 would not pick this up.
> diff --git a/drivers/net/ieee802154/cc2520.c b/drivers/net/ieee802154/cc2520.c
> index 545487223838..d2c8484d8fea 100644
> --- a/drivers/net/ieee802154/cc2520.c
> +++ b/drivers/net/ieee802154/cc2520.c
> @@ -1153,8 +1153,8 @@ static int cc2520_probe(struct spi_device *spi)
> return 0;
>
> err_hw_init:
> - mutex_destroy(&priv->buffer_mutex);
> flush_work(&priv->fifop_irqwork);
> + mutex_destroy(&priv->buffer_mutex);
> return ret;
> }
[Severity: High]
Can flush_work() close this window at all while the FIFOP interrupt is
still armed?
The FIFOP handler re-queues unconditionally on every edge:
static irqreturn_t cc2520_fifop_isr(int irq, void *data)
{
struct cc2520_private *priv = data;
schedule_work(&priv->fifop_irqwork);
return IRQ_HANDLED;
}
flush_work() waits for a queued or running instance but does not prevent a
new submission, and the irq was requested with devm_request_irq(), so it
stays registered until the driver core unwinds devres after probe returns:
cc2520_probe() [err_hw_init]
flush_work(&priv->fifop_irqwork); /* CPU0 */
/* CPU1: FIFOP edge ->
cc2520_fifop_isr() ->
schedule_work() re-queues */
mutex_destroy(&priv->buffer_mutex);
return ret;
really_probe()
device_unbind_cleanup()
devres_release_all() /* frees the irq and the
devm_kzalloc'ed priv */
priv embeds both fifop_irqwork and buffer_mutex, so the worker pool then
touches a freed work_struct, and cc2520_fifop_irqwork() can dereference
freed priv:
if (gpiod_get_value(priv->fifo_pin))
cc2520_rx(priv);
...
cc2520_cmd_strobe(priv, CC2520_CMD_SFLUSHRX);
reaching cc2520_cmd_strobe(), which locks the destroyed and freed
buffer_mutex. That is the same assign_work()/worker_thread() signature
quoted in the commit message.
Would disable_work_sync() be appropriate here, the way ff5891b266a7 handled
cc2520_remove() for exactly this devm-irq-outlives-teardown case?
Alternatively, releasing or masking the FIFOP irq before draining the work.
There is also the cc2520_register() failure case, where priv->hw is freed
but not cleared:
err_free_device:
ieee802154_free_hw(priv->hw);
so a surviving work item can reach a freed hw through
cc2520_rx()->ieee802154_rx_irqsafe().
For reference, no later patch in this series changes this path; err_hw_init
still reads flush_work() followed by mutex_destroy() at the end of the
series.
[Severity: Medium]
This isn't a bug introduced by this patch, but while the err_hw_init path is
being touched: should it also unwind the power and reset ramp done earlier in
cc2520_probe()?
gpiod_set_value(vreg, HIGH);
usleep_range(100, 150);
gpiod_set_value(reset, HIGH);
usleep_range(200, 250);
err_hw_init only flushes the work and destroys the mutex. Releasing a gpio
descriptor through devres does not restore its output level, so after a
failure in cc2520_hw_init(), either devm_request_irq(), or cc2520_register(),
the CC2520 stays powered and out of reset with no driver bound. On a later
re-bind, reset is already HIGH, so does cc2520_hw_init() then program a chip
that never saw a reset pulse? This dates back to the original driver
(0da6bc8cc341).
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921074207.2289391-1-benquike%40gmail.com
