[F/Unstable,1/1] UBUNTU: SAUCE: blk/core: Gracefully handle unset make_request_fn

BugLink: https://bugs.launchpad.net/bugs/1860231

When device-mapper adapted for multi-queue functionality, they
also re-organized the way the make-request function was set.
Before, this happened when the device-mapper logical device was
created. Now it is done once the mapping table gets loaded the
first time (this also decides whether the block device is request
or bio based).

However in generic_make_request(), the request function gets used
without further checks and this happens if one tries to mount such
a partially set up device.

This can easily be reproduced with the following steps:
 - dmsetup create -n test
 - mount /dev/dm-<#> /mnt

This maybe is something which also should be fixed up in device-
mapper. But given there is already a check for an unset queue
pointer and potentially there could be other drivers which do or
might do the same, it sounds like a good move to add another check
to generic_make_request_checks() and to bail out if the request
function has not been set, yet.

Fixes: ff36ab34583a ("dm: remove request-based logic from make_request_fn wrapper")
Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
---
 block/blk-core.c | 7 +++++++
 1 file changed, 7 insertions(+)

On 2020-01-23 10:33:03, Stefan Bader wrote:
> BugLink: https://bugs.launchpad.net/bugs/1860231
> 
> When device-mapper adapted for multi-queue functionality, they
> also re-organized the way the make-request function was set.
> Before, this happened when the device-mapper logical device was
> created. Now it is done once the mapping table gets loaded the
> first time (this also decides whether the block device is request
> or bio based).
> 
> However in generic_make_request(), the request function gets used
> without further checks and this happens if one tries to mount such
> a partially set up device.
> 
> This can easily be reproduced with the following steps:
>  - dmsetup create -n test
>  - mount /dev/dm-<#> /mnt
> 
> This maybe is something which also should be fixed up in device-
> mapper. But given there is already a check for an unset queue
> pointer and potentially there could be other drivers which do or
> might do the same, it sounds like a good move to add another check
> to generic_make_request_checks() and to bail out if the request
> function has not been set, yet.
> 
> Fixes: ff36ab34583a ("dm: remove request-based logic from make_request_fn wrapper")
> Signed-off-by: Stefan Bader <stefan.bader@canonical.com>

Acked-by: Tyler Hicks <tyhicks@canonical.com>

Tyler

> ---
>  block/blk-core.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 1075aaff606d..adcd042edd2d 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -884,6 +884,13 @@ generic_make_request_checks(struct bio *bio)
>  			bio_devname(bio, b), (long long)bio->bi_iter.bi_sector);
>  		goto end_io;
>  	}
> +	if (unlikely(!q->make_request_fn)) {
> +		printk(KERN_ERR
> +		       "generic_make_request: Trying to access "
> +		       "block-device without request function: %s\n",
> +		       bio_devname(bio, b));
> +		goto end_io;
> +	}
>  
>  	/*
>  	 * Non-mq queues do not honor REQ_NOWAIT, so complete a bio
> -- 
> 2.17.1
>

On Thu, Jan 23, 2020 at 10:33:03AM +0200, Stefan Bader wrote:
> BugLink: https://bugs.launchpad.net/bugs/1860231
> 
> When device-mapper adapted for multi-queue functionality, they
> also re-organized the way the make-request function was set.
> Before, this happened when the device-mapper logical device was
> created. Now it is done once the mapping table gets loaded the
> first time (this also decides whether the block device is request
> or bio based).
> 
> However in generic_make_request(), the request function gets used
> without further checks and this happens if one tries to mount such
> a partially set up device.
> 
> This can easily be reproduced with the following steps:
>  - dmsetup create -n test
>  - mount /dev/dm-<#> /mnt
> 
> This maybe is something which also should be fixed up in device-
> mapper. But given there is already a check for an unset queue
> pointer and potentially there could be other drivers which do or
> might do the same, it sounds like a good move to add another check
> to generic_make_request_checks() and to bail out if the request
> function has not been set, yet.
> 
> Fixes: ff36ab34583a ("dm: remove request-based logic from make_request_fn wrapper")
> Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
> ---
>  block/blk-core.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 1075aaff606d..adcd042edd2d 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -884,6 +884,13 @@ generic_make_request_checks(struct bio *bio)
>  			bio_devname(bio, b), (long long)bio->bi_iter.bi_sector);
>  		goto end_io;
>  	}
> +	if (unlikely(!q->make_request_fn)) {
> +		printk(KERN_ERR
> +		       "generic_make_request: Trying to access "
> +		       "block-device without request function: %s\n",
> +		       bio_devname(bio, b));
> +		goto end_io;
> +	}

Instead of returning an I/O error, I was wondering if it would be better
to give a chance to the caller to repeat the I/O request gracefully,
returning -EAGAIN. Something like this:

	if (unlikely(!q->make_request_fn)) {
		printk(KERN_WARNING
		       "generic_make_request: Trying to access "
		       "block-device without request function: %s\n",
		       bio_devname(bio, b));
		status = BLK_STS_AGAIN;
		goto end_io;
	}

>  
>  	/*
>  	 * Non-mq queues do not honor REQ_NOWAIT, so complete a bio
> -- 
> 2.17.1

However, this change also looks safe as it is (definitely better than
crashing the kernel when q->make_request_fn is NULL), therefore:

Acked-by: Andrea Righi <andrea.righi@canonical.com>

On 23/01/2020 08:33, Stefan Bader wrote:
> BugLink: https://bugs.launchpad.net/bugs/1860231
> 
> When device-mapper adapted for multi-queue functionality, they
> also re-organized the way the make-request function was set.
> Before, this happened when the device-mapper logical device was
> created. Now it is done once the mapping table gets loaded the
> first time (this also decides whether the block device is request
> or bio based).
> 
> However in generic_make_request(), the request function gets used
> without further checks and this happens if one tries to mount such
> a partially set up device.
> 
> This can easily be reproduced with the following steps:
>  - dmsetup create -n test
>  - mount /dev/dm-<#> /mnt
> 
> This maybe is something which also should be fixed up in device-
> mapper. But given there is already a check for an unset queue
> pointer and potentially there could be other drivers which do or
> might do the same, it sounds like a good move to add another check
> to generic_make_request_checks() and to bail out if the request
> function has not been set, yet.
> 
> Fixes: ff36ab34583a ("dm: remove request-based logic from make_request_fn wrapper")
> Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
> ---
>  block/blk-core.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 1075aaff606d..adcd042edd2d 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -884,6 +884,13 @@ generic_make_request_checks(struct bio *bio)
>  			bio_devname(bio, b), (long long)bio->bi_iter.bi_sector);
>  		goto end_io;
>  	}
> +	if (unlikely(!q->make_request_fn)) {
> +		printk(KERN_ERR
> +		       "generic_make_request: Trying to access "
> +		       "block-device without request function: %s\n",
> +		       bio_devname(bio, b));
> +		goto end_io;
> +	}
>  
>  	/*
>  	 * Non-mq queues do not honor REQ_NOWAIT, so complete a bio
> 

Seems very reasonable to me. Thanks smb

Acked by Colin Ian King <colin.king@canonical.com>

On 23.01.20 11:13, Andrea Righi wrote:
> On Thu, Jan 23, 2020 at 10:33:03AM +0200, Stefan Bader wrote:
>> BugLink: https://bugs.launchpad.net/bugs/1860231
>>
>> When device-mapper adapted for multi-queue functionality, they
>> also re-organized the way the make-request function was set.
>> Before, this happened when the device-mapper logical device was
>> created. Now it is done once the mapping table gets loaded the
>> first time (this also decides whether the block device is request
>> or bio based).
>>
>> However in generic_make_request(), the request function gets used
>> without further checks and this happens if one tries to mount such
>> a partially set up device.
>>
>> This can easily be reproduced with the following steps:
>>  - dmsetup create -n test
>>  - mount /dev/dm-<#> /mnt
>>
>> This maybe is something which also should be fixed up in device-
>> mapper. But given there is already a check for an unset queue
>> pointer and potentially there could be other drivers which do or
>> might do the same, it sounds like a good move to add another check
>> to generic_make_request_checks() and to bail out if the request
>> function has not been set, yet.
>>
>> Fixes: ff36ab34583a ("dm: remove request-based logic from make_request_fn wrapper")
>> Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
>> ---
>>  block/blk-core.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/block/blk-core.c b/block/blk-core.c
>> index 1075aaff606d..adcd042edd2d 100644
>> --- a/block/blk-core.c
>> +++ b/block/blk-core.c
>> @@ -884,6 +884,13 @@ generic_make_request_checks(struct bio *bio)
>>  			bio_devname(bio, b), (long long)bio->bi_iter.bi_sector);
>>  		goto end_io;
>>  	}
>> +	if (unlikely(!q->make_request_fn)) {
>> +		printk(KERN_ERR
>> +		       "generic_make_request: Trying to access "
>> +		       "block-device without request function: %s\n",
>> +		       bio_devname(bio, b));
>> +		goto end_io;
>> +	}
> 
> Instead of returning an I/O error, I was wondering if it would be better
> to give a chance to the caller to repeat the I/O request gracefully,
> returning -EAGAIN. Something like this:

This potentially can take some time (potentially).Like in the example given you
create a device without that mapping function until you load the table. That
might be much later and maybe by that time there are already a lot of I/O
requests in flight. Plus I basically repeated the handling which is done when
you submit I/O to a device which has no request queue set up at all.

> 
> 	if (unlikely(!q->make_request_fn)) {
> 		printk(KERN_WARNING
> 		       "generic_make_request: Trying to access "
> 		       "block-device without request function: %s\n",
> 		       bio_devname(bio, b));
> 		status = BLK_STS_AGAIN;
> 		goto end_io;
> 	}
> 
>>  
>>  	/*
>>  	 * Non-mq queues do not honor REQ_NOWAIT, so complete a bio
>> -- 
>> 2.17.1
> 
> However, this change also looks safe as it is (definitely better than
> crashing the kernel when q->make_request_fn is NULL), therefore:
> 
> Acked-by: Andrea Righi <andrea.righi@canonical.com>
>

On Thu, Jan 23, 2020 at 11:24:54AM +0200, Stefan Bader wrote:
> On 23.01.20 11:13, Andrea Righi wrote:
> > On Thu, Jan 23, 2020 at 10:33:03AM +0200, Stefan Bader wrote:
> >> BugLink: https://bugs.launchpad.net/bugs/1860231
> >>
> >> When device-mapper adapted for multi-queue functionality, they
> >> also re-organized the way the make-request function was set.
> >> Before, this happened when the device-mapper logical device was
> >> created. Now it is done once the mapping table gets loaded the
> >> first time (this also decides whether the block device is request
> >> or bio based).
> >>
> >> However in generic_make_request(), the request function gets used
> >> without further checks and this happens if one tries to mount such
> >> a partially set up device.
> >>
> >> This can easily be reproduced with the following steps:
> >>  - dmsetup create -n test
> >>  - mount /dev/dm-<#> /mnt
> >>
> >> This maybe is something which also should be fixed up in device-
> >> mapper. But given there is already a check for an unset queue
> >> pointer and potentially there could be other drivers which do or
> >> might do the same, it sounds like a good move to add another check
> >> to generic_make_request_checks() and to bail out if the request
> >> function has not been set, yet.
> >>
> >> Fixes: ff36ab34583a ("dm: remove request-based logic from make_request_fn wrapper")
> >> Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
> >> ---
> >>  block/blk-core.c | 7 +++++++
> >>  1 file changed, 7 insertions(+)
> >>
> >> diff --git a/block/blk-core.c b/block/blk-core.c
> >> index 1075aaff606d..adcd042edd2d 100644
> >> --- a/block/blk-core.c
> >> +++ b/block/blk-core.c
> >> @@ -884,6 +884,13 @@ generic_make_request_checks(struct bio *bio)
> >>  			bio_devname(bio, b), (long long)bio->bi_iter.bi_sector);
> >>  		goto end_io;
> >>  	}
> >> +	if (unlikely(!q->make_request_fn)) {
> >> +		printk(KERN_ERR
> >> +		       "generic_make_request: Trying to access "
> >> +		       "block-device without request function: %s\n",
> >> +		       bio_devname(bio, b));
> >> +		goto end_io;
> >> +	}
> > 
> > Instead of returning an I/O error, I was wondering if it would be better
> > to give a chance to the caller to repeat the I/O request gracefully,
> > returning -EAGAIN. Something like this:
> 
> This potentially can take some time (potentially).Like in the example given you
> create a device without that mapping function until you load the table. That
> might be much later and maybe by that time there are already a lot of I/O
> requests in flight. Plus I basically repeated the handling which is done when
> you submit I/O to a device which has no request queue set up at all.

Hm... yeah, makes sense, then failing here is probably a safer action.

Thanks,
-Andrea