Re: [PATCH] drbd: Fix local_cnt refcount leak on ascw allocation failure in _drbd_set_state

[Christoph Böhmwalder]

On Thu, Jun 25, 2026 at 11:16:36PM +0800, Wentao Liang wrote:
> In _drbd_set_state(), when transitioning a device to D_FAILED or
> D_DISKLESS, an extra reference on local_cnt is taken via
> atomic_inc(&device->local_cnt) to prevent premature destruction of
> the local disk. This reference is normally released by put_ldev()
> in after_state_ch(), which is called asynchronously through the
> after_state_chg_work (ascw) work item.
>
> If the GFP_ATOMIC allocation of the ascw work item fails, the work
> is never queued, after_state_ch() never runs, and the extra
> local_cnt reference is permanently leaked. Additionally, the
> state_change object allocated by remember_old_state() is also
> leaked, along with the krefs it acquired on the resource,
> connections, and devices.
>
> Fix both leaks in the ascw allocation failure path:
> - Call put_ldev() to release the extra local_cnt reference when
>   the transition matches the same conditions used for the
>   atomic_inc.
> - Call forget_state_change() to free the state_change object and
>   release the krefs it holds.
>
> Cc: stable@xxxxxxxxxxxxxxx
> Fixes: d01801710265 ("drbd: Remove the terrible DEV hack")
> Signed-off-by: Wentao Liang <vulab@xxxxxxxxxxx>
> ---
> drivers/block/drbd/drbd_state.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/block/drbd/drbd_state.c b/drivers/block/drbd/drbd_state.c
> index adcba7f1d8ea..68e273c6d5be 100644
> --- a/drivers/block/drbd/drbd_state.c
> +++ b/drivers/block/drbd/drbd_state.c
> @@ -1480,7 +1480,13 @@ _drbd_set_state(struct drbd_device *device, union drbd_state ns,
> 		drbd_queue_work(&connection->sender_work,
> 				&ascw->w);
> 	} else {
> -		drbd_err(device, "Could not kmalloc an ascw\n");
> +		if ((os.disk != D_FAILED && ns.disk == D_FAILED) ||
> +		    (os.disk != D_DISKLESS && ns.disk == D_DISKLESS))
> +			put_ldev(device);

Thanks for the patch. The logic itself looks correct to me.

> +
> +		forget_state_change(state_change);
> +		drbd_err(device, "Could not kmalloc an ascw, state change %p -> %p leaked\n",
> +			 &os, &ns);

However, this error message is nonsensical. If anything, we should print 
some halfway human-readable identifier for the state values here, not 
the pointer.

Also, the state change is precisely *not* leaked at the point this
message triggers, since we free it here. What actually gets lost is the 
effects of the state change, so if anything we should point that out 
here. But I think just keeping the original message is fine.

> 	}
>
> 	return rv;
> --
> 2.39.5 (Apple Git-154)

Also, the Fixes tag points to the wrong commit, that was just a 
mechanical change. The actual breakage was introduced in commit
82f59cc63538 ("drbd: fix potential deadlock on detach").