Re: [PATCH] block: protect debugfs attributes using q->elevator_lock

[Christoph Hellwig <hch@lst.de>]

On Wed, Mar 12, 2025 at 03:58:38PM +0530, Nilay Shroff wrote:
> Additionally, debugfs attribute "busy" is currently unprotected. This
> attribute iterates over all started requests in a tagset and prints them. 
> However, the tags can be updated simultaneously via the sysfs attribute 
> "nr_requests" or "scheduler" (elevator switch), leading to potential race 
> conditions. Since the sysfs attributes "nr_requests" and "scheduler" are 
> already protected using q->elevator_lock, extend this protection to the 
> debugfs "busy" attribute as well.

I'd split that into a separate patch for bisectability.

>  	struct show_busy_params params = { .m = m, .hctx = hctx };
> +	int res;
>  
> +	res = mutex_lock_interruptible(&hctx->queue->elevator_lock);
> +	if (res)
> +		goto out;

Is mutex_lock_interruptible really the right primitive here?  We don't
really want this read system call interrupted by random signals, do
we?  I guess this should be mutex_lock_killable.

(and the same for the existing methods this is copy and pasted from).

>  	blk_mq_tagset_busy_iter(hctx->queue->tag_set, hctx_show_busy_rq,
>  				&params);
> -
> -	return 0;
> +	mutex_unlock(&hctx->queue->elevator_lock);
> +out:
> +	return res;

And as Damien already said, no need for the labels here, including for
the existing code.  That should probably be alsot changed in an extra
patch for the existing code while you're at it.