[PATCH v2] block: Fix elv_iosched_local_module handling of "none" scheduler

[PATCH v2] block: Fix elv_iosched_local_module handling of "none" scheduler

[Damien Le Moal]

Commit 734e1a860312 ("block: Prevent deadlocks when switching
elevators") introduced the function elv_iosched_load_module() to allow
loading an elevator module outside of elv_iosched_store() with the
target device queue not frozen, to avoid deadlocks. However, the "none"
scheduler does not have a module and as a result,
elv_iosched_load_module() always returns an error when trying to switch
to this valid scheduler.

Fix this by ignoring the return value of the request_module() call
done by elv_iosched_load_module(). This restores the behavior before
commit 734e1a860312, which was to ignore the request_module() result and
instead rely on elevator_change() to handle the "none" scheduler case.

Reported-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
Fixes: 734e1a860312 ("block: Prevent deadlocks when switching elevators")
Cc: stable@vger.kernel.org
Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
Changes from v1:
 - Switch to ignoring the return value of request_module() instead of
   doing nothing if the scheduler name is "none".

 block/elevator.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/block/elevator.c b/block/elevator.c
index c355b55d0107..4122026b11f1 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -715,7 +715,9 @@ int elv_iosched_load_module(struct gendisk *disk, const char *buf,
 
 	strscpy(elevator_name, buf, sizeof(elevator_name));
 
-	return request_module("%s-iosched", strstrip(elevator_name));
+	request_module("%s-iosched", strstrip(elevator_name));
+
+	return 0;
 }
 
 ssize_t elv_iosched_store(struct gendisk *disk, const char *buf,
-- 
2.46.0

Re: [PATCH v2] block: Fix elv_iosched_local_module handling of "none" scheduler

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2] block: Fix elv_iosched_local_module handling of "none" scheduler

[Jens Axboe]

On Tue, 17 Sep 2024 22:32:31 +0900, Damien Le Moal wrote:
> Commit 734e1a860312 ("block: Prevent deadlocks when switching
> elevators") introduced the function elv_iosched_load_module() to allow
> loading an elevator module outside of elv_iosched_store() with the
> target device queue not frozen, to avoid deadlocks. However, the "none"
> scheduler does not have a module and as a result,
> elv_iosched_load_module() always returns an error when trying to switch
> to this valid scheduler.
> 
> [...]

Applied, thanks!

[1/1] block: Fix elv_iosched_local_module handling of "none" scheduler
      commit: e3accac1a976e65491a9b9fba82ce8ddbd3d2389

Best regards,
-- 
Jens Axboe

Re: [PATCH v2] block: Fix elv_iosched_local_module handling of "none" scheduler

[Andreas Hindborg]

Hi Damien,

Damien Le Moal <dlemoal@kernel.org> writes:

> Commit 734e1a860312 ("block: Prevent deadlocks when switching
> elevators") introduced the function elv_iosched_load_module() to allow
> loading an elevator module outside of elv_iosched_store() with the
> target device queue not frozen, to avoid deadlocks. However, the "none"
> scheduler does not have a module and as a result,
> elv_iosched_load_module() always returns an error when trying to switch
> to this valid scheduler.

The commit message here is a bit misleading. The problem is not that
`request_module` can fail, the problem is that some failure modes cause
this function to return a positive integer. This is not caught by
callers and it ends up causing all kinds of problems in user space.

Perhaps it makes sense to check for a positive return value at the call
site of the `load_module` pointer in `queue_attr_store`, so this does
not repeat at some point in the future?

Or maybe document that `load_module` implementations should not return a
positive value unless it actually wants to send this to user space as
the result of a write to the `scheduler` sysfs file?

Best regards,
Andreas

Re: [PATCH v2] block: Fix elv_iosched_local_module handling of "none" scheduler

[Damien Le Moal]

On 10/7/24 19:51, Andreas Hindborg wrote:
> Hi Damien,
> 
> Damien Le Moal <dlemoal@kernel.org> writes:
> 
>> Commit 734e1a860312 ("block: Prevent deadlocks when switching
>> elevators") introduced the function elv_iosched_load_module() to allow
>> loading an elevator module outside of elv_iosched_store() with the
>> target device queue not frozen, to avoid deadlocks. However, the "none"
>> scheduler does not have a module and as a result,
>> elv_iosched_load_module() always returns an error when trying to switch
>> to this valid scheduler.
> 
> The commit message here is a bit misleading. The problem is not that
> `request_module` can fail, the problem is that some failure modes cause
> this function to return a positive integer. This is not caught by
> callers and it ends up causing all kinds of problems in user space.
> 
> Perhaps it makes sense to check for a positive return value at the call
> site of the `load_module` pointer in `queue_attr_store`, so this does
> not repeat at some point in the future?

Well, the patch version that went upstream totally ignores the return value of
request_module(), as it did before. So I do not think there are any issues
anymore... ? Might be good to add a comment about it above that request_module()
call in elv_iosched_load_module().

> 
> Or maybe document that `load_module` implementations should not return a
> positive value unless it actually wants to send this to user space as
> the result of a write to the `scheduler` sysfs file?

Yes, ->load_module() should return 0 on success and negative error code on
error. Otherwise, a positive error may be interpreted by user space as a success
write to the sysfs attribute. Adding a comment for that will be good too.

Care to send a patch ?

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2] block: Fix elv_iosched_local_module handling of "none" scheduler

[Christoph Hellwig]

On Tue, Oct 08, 2024 at 07:48:10AM +0900, Damien Le Moal wrote:
> Yes, ->load_module() should return 0 on success and negative error code on
> error. Otherwise, a positive error may be interpreted by user space as a success
> write to the sysfs attribute. Adding a comment for that will be good too.
> 
> Care to send a patch ?

It should not return anything at all as alreaday said last round.
Jens just asked for that to be sent layer, so I guess I'll do it now.

Re: [PATCH v2] block: Fix elv_iosched_local_module handling of "none" scheduler

[Damien Le Moal]

On 10/8/24 13:20, Christoph Hellwig wrote:
> On Tue, Oct 08, 2024 at 07:48:10AM +0900, Damien Le Moal wrote:
>> Yes, ->load_module() should return 0 on success and negative error code on
>> error. Otherwise, a positive error may be interpreted by user space as a success
>> write to the sysfs attribute. Adding a comment for that will be good too.
>>
>> Care to send a patch ?
> 
> It should not return anything at all as alreaday said last round.
> Jens just asked for that to be sent layer, so I guess I'll do it now.

Ah, yes, that is even better.

-- 
Damien Le Moal
Western Digital Research