Re: [PATCH net] dibs: initialise dibs->lock in dibs_dev_alloc()
From: Hidayathulla Khan I
Date: Wed Aug 05 2026 - 02:26:14 EST
Thanks for the review.
Finding: main-3-1: Confirmed. This issue is already being addressed by Alexandra Winter in a separate patch.
Finding: main-3-0: Already addressed by the following patches on the netdev mailing list.
[PATCH net] dibs: fix double free of dmb_clientid_arr
[PATCH net] net/dibs: Fix UAF of dmb_clientid_arr after dibs_dev_del()
Neither issue is introduced by this patch.
On 04/08/26 10:32 pm, Simon Horman wrote:
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260730124227.167829-1-hidayath@xxxxxxxxxxxxx?part=1
---
--- Patch [1]: [PATCH net] dibs: initialise dibs->lock in dibs_dev_alloc() ---
commit d5daf0b54be512c77adffebc7ed60dee0c5e8e74
Author: Hidayath Khan <hidayath@xxxxxxxxxxxxx>
dibs: initialise dibs->lock in dibs_dev_alloc()
This moves spin_lock_init(&dibs->lock) from dibs_dev_add() into
dibs_dev_alloc(), so the lock is valid as soon as a driver can hand the
device to its interrupt handler, and updates the dibs_dev_alloc()
kerneldoc accordingly.
While looking at the surrounding lifetime rules for struct dibs_dev, two
questions came up about pre-existing behaviour in the same area.
diff --git a/drivers/dibs/dibs_main.c b/drivers/dibs/dibs_main.c[Severity: High]
index 14c3e2d84902e..4c26fd06973f7 100644
--- a/drivers/dibs/dibs_main.c
+++ b/drivers/dibs/dibs_main.c
@@ -138,6 +138,7 @@ struct dibs_dev *dibs_dev_alloc(void)
dibs = kzalloc_obj(*dibs);
if (!dibs)
return dibs;
+ spin_lock_init(&dibs->lock);
dibs->dev.release = dibs_dev_release;
dibs->dev.class = &dibs_class;
device_initialize(&dibs->dev);
[Finding: main-3-1]
[Sources: opus-5]
This isn't a bug introduced by this patch, but since the commit message
argues about ism_handle_irq() running before dibs_dev_add(), does the
mirror-image window at teardown need the same attention?
ism_remove() tears things down in this order:
drivers/s390/net/ism_drv.c:ism_remove() {
...
dibs_dev_del(dibs);
ism_dev_exit(ism);
/* pairs with dibs_dev_alloc() */
put_device(&dibs->dev);
}
and ism_dev_exit() only disarms the interrupt as its last step:
drivers/s390/net/ism_drv.c:ism_dev_exit() {
...
unregister_ieq(ism);
unregister_sba(ism);
free_irq(pci_irq_vector(pdev, 0), ism);
}
unregister_sba() frees the SBA and sets ism->sba = NULL, but the handler
writes through it unconditionally on entry:
drivers/s390/net/ism_drv.c:ism_handle_irq() {
spin_lock(&dibs->lock);
ism->sba->s = 0;
...
}
Likewise unregister_ieq() frees the IEQ buffer and NULLs ism->ieq, while
ism_handle_event() dereferences it in the loop condition:
while ((ism->ieq_idx + 1) != READ_ONCE(ism->ieq->header.idx)) {
Is there anything that serializes ism_handle_irq() against those frees?
dibs->lock is the only lock the handler takes, and unregister_ieq() /
unregister_sba() never take it, so an MSI serviced on another CPU during
ism_remove() looks like it can write to a freed/NULL sba and read a freed
ieq in hardirq context. Would calling free_irq() (or synchronize_irq())
before releasing the structures the handler consumes close this?
The same ordering shows up on the probe error path: dibs_dev_add() failure
frees dmb_clientid_arr and returns to ism_probe(), which only reaches
ism_dev_exit() (and thus free_irq()) afterwards.
@@ -186,7 +187,6 @@ int dibs_dev_add(struct dibs_dev *dibs)[Severity: High]
int i, ret;
max_dmbs = dibs->ops->max_dmbs();
- spin_lock_init(&dibs->lock);
dibs->dmb_clientid_arr = kzalloc(max_dmbs, GFP_KERNEL);
if (!dibs->dmb_clientid_arr)
return -ENOMEM;
[Finding: main-3-0]
[Sources: opus-5]
This is a pre-existing issue and not something this patch changes, but
while the ownership rules of dibs_dev_add() are in view: can
dmb_clientid_arr be freed twice on the loopback probe error path?
dibs_dev_add() frees the array on every failure after the allocation and
leaves the pointer in place:
drivers/dibs/dibs_main.c:dibs_dev_add() {
...
ret = device_add(&dibs->dev);
if (ret)
goto free_client_arr;
...
err_device_del:
device_del(&dibs->dev);
free_client_arr:
kfree(dibs->dmb_clientid_arr);
return ret;
}
and dibs_lo_dev_probe() frees it again:
drivers/dibs/dibs_loopback.c:dibs_lo_dev_probe() {
ret = dibs_dev_add(dibs);
if (ret)
goto err_reg;
...
err_reg:
kfree(dibs->dmb_clientid_arr);
/* pairs with dibs_dev_alloc() */
put_device(&dibs->dev);
kfree(ldev);
}
The early -ENOMEM return is harmless because the pointer is still NULL,
but a device_add() or sysfs_create_group() failure would reach the same
slab object twice. The two callers also disagree here: ism_probe()'s
err_ism / err_dibs paths do not repeat the kfree(). Would setting
dibs->dmb_clientid_arr = NULL after the kfree() in dibs_dev_add(), or
documenting who owns the array on failure, resolve the mismatch?
Also, dibs_dev_del() ends with kfree(dibs->dmb_clientid_arr) without
clearing the pointer, while ism_handle_irq() reads it for any set DMB bit:
drivers/s390/net/ism_drv.c:ism_handle_irq() {
...
client_id = dibs->dmb_clientid_arr[bit];
if (unlikely(client_id == NO_DIBS_CLIENT ||
!dibs->subs[client_id]))
...
}
Since client_id comes straight from that buffer and is used unchecked as
an index into the 8-entry subs[] array before an indirect call, does a
post-free interrupt here read freed memory and potentially index past the
end of struct dibs_dev?
[ ... ]