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

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

[Hidayath Khan]

dibs->lock is initialised by dibs_dev_add(), but a dibs device can
already take interrupts before that call: ism_probe() runs
ism_dev_init(), and hence request_irq(), before it calls
dibs_dev_add(). No client can have registered a dmb at that point, so
no dmb interrupt can occur, but a GID event interrupt can, and
ism_handle_irq() takes dibs->lock unconditionally on entry, before it
inspects anything else.

Initialise the lock in dibs_dev_alloc() instead, so that it is valid as
soon as a driver can publish the device to its interrupt handler.

Fixes: cc21191b584c ("dibs: Move data path to dibs layer")
Cc: stable@vger.kernel.org
Reviewed-by: Alexandra Winter <wintera@linux.ibm.com>
Signed-off-by: Hidayath Khan <hidayath@linux.ibm.com>
---
 drivers/dibs/dibs_main.c | 2 +-
 include/linux/dibs.h     | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/dibs/dibs_main.c b/drivers/dibs/dibs_main.c
index 14c3e2d84902..4c26fd06973f 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);
@@ -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;
diff --git a/include/linux/dibs.h b/include/linux/dibs.h
index c75607f8a5cf..d3e0777f25ae 100644
--- a/include/linux/dibs.h
+++ b/include/linux/dibs.h
@@ -439,7 +439,7 @@ static inline void *dibs_get_priv(struct dibs_dev *dev,
 /**
  * dibs_dev_alloc() - allocate and reference device structure
  *
- * The following fields will be valid upon successful return: dev
+ * The following fields will be valid upon successful return: dev, lock
  * NOTE: Use put_device(dibs_get_dev(@dibs)) to give up your reference instead
  * of freeing @dibs @dev directly once you have successfully called this
  * function.

base-commit: 10019318fb9fd64fda6ed43f9b34d0c9077d5802
-- 
2.52.0

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

[Simon Horman]

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?

[ ... ]

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

[Hidayathulla Khan I]

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@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?
>
> [ ... ]
>

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu, 30 Jul 2026 14:42:27 +0200 you wrote:
> dibs->lock is initialised by dibs_dev_add(), but a dibs device can
> already take interrupts before that call: ism_probe() runs
> ism_dev_init(), and hence request_irq(), before it calls
> dibs_dev_add(). No client can have registered a dmb at that point, so
> no dmb interrupt can occur, but a GID event interrupt can, and
> ism_handle_irq() takes dibs->lock unconditionally on entry, before it
> inspects anything else.
> 
> [...]

Here is the summary with links:
  - [net] dibs: initialise dibs->lock in dibs_dev_alloc()
    https://git.kernel.org/netdev/net/c/c27e36054537

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html