Re: [PATCH net] dibs: initialise dibs->lock in dibs_dev_alloc()

[Simon Horman <horms@kernel.org>]

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@linux.ibm.com?part=1
---
--- Patch [1]: [PATCH net] dibs: initialise dibs->lock in dibs_dev_alloc() ---
commit d5daf0b54be512c77adffebc7ed60dee0c5e8e74
Author: Hidayath Khan <hidayath@linux.ibm.com>

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
> 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);

[Severity: High]
[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)
>  	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;

[Severity: High]
[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?

[ ... ]