Hello.
While searching for races in Linux kernel I've come across
drivers/block/loop.ko module. Here is the question that I came up with
while analyzing results. Lines are given using the info from Linux v4.12.
In loop_init function additional initialization happens after a
successful call to misc_register() (loop.c: line 1961). Consider the
following case:
Thread 1: Thread 2:
loop_init()
misc_register() loop_control_ioctl
part_shift = 0 -> loop_add
if (max_part > 0) { alloc_disk(1 << part_shift)
part_shift =
<greater than 0>
...
}
In this case alloc_disk() will be called with 1 as a parameter although
part_shift should have been greater than 0. Maybe it would be better to
move the call to a misc_register() function a bit further down (at least
so it could be after the part_shift initialization)?
Thank you for your time.
-- Anton Volkov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: [email protected]
On Fri, Jul 28, 2017 at 11:55 PM, Anton Volkov <[email protected]> wrote:
> Hello.
> While searching for races in Linux kernel I've come across
> drivers/block/loop.ko module. Here is the question that I came up with while
> analyzing results. Lines are given using the info from Linux v4.12.
>
> In loop_init function additional initialization happens after a successful
> call to misc_register() (loop.c: line 1961). Consider the following case:
>
> Thread 1: Thread 2:
> loop_init()
> misc_register() loop_control_ioctl
> part_shift = 0 -> loop_add
> if (max_part > 0) { alloc_disk(1 << part_shift)
> part_shift =
> <greater than 0>
> ...
> }
>
> In this case alloc_disk() will be called with 1 as a parameter although
> part_shift should have been greater than 0. Maybe it would be better to move
> the call to a misc_register() function a bit further down (at least so it
> could be after the part_shift initialization)?
That looks a good idea, could you cook a patch to do it?
--
Ming Lei
The early device registration made possible a race leading to allocations
of disks with wrong minors.
This patch moves the device registration further down the loop_init
function to make the race infeasible.
Found by Linux Driver Verification project (linuxtesting.org).
Signed-off-by: Anton Volkov <[email protected]>
---
drivers/block/loop.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index ef83349..2fbd4089 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1996,10 +1996,6 @@ static int __init loop_init(void)
struct loop_device *lo;
int err;
- err = misc_register(&loop_misc);
- if (err < 0)
- return err;
-
part_shift = 0;
if (max_part > 0) {
part_shift = fls(max_part);
@@ -2017,12 +2013,12 @@ static int __init loop_init(void)
if ((1UL << part_shift) > DISK_MAX_PARTS) {
err = -EINVAL;
- goto misc_out;
+ goto err_out;
}
if (max_loop > 1UL << (MINORBITS - part_shift)) {
err = -EINVAL;
- goto misc_out;
+ goto err_out;
}
/*
@@ -2041,6 +2037,11 @@ static int __init loop_init(void)
range = 1UL << MINORBITS;
}
+ err = misc_register(&loop_misc);
+ if (err < 0)
+ goto err_out;
+
+
if (register_blkdev(LOOP_MAJOR, "loop")) {
err = -EIO;
goto misc_out;
@@ -2060,6 +2061,7 @@ static int __init loop_init(void)
misc_out:
misc_deregister(&loop_misc);
+err_out:
return err;
}
--
2.7.4
On Thu, Aug 3, 2017 at 11:01 PM, Anton Volkov <[email protected]> wrote:
> The early device registration made possible a race leading to allocations
> of disks with wrong minors.
>
> This patch moves the device registration further down the loop_init
> function to make the race infeasible.
>
> Found by Linux Driver Verification project (linuxtesting.org).
>
> Signed-off-by: Anton Volkov <[email protected]>
> ---
> drivers/block/loop.c | 14 ++++++++------
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index ef83349..2fbd4089 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -1996,10 +1996,6 @@ static int __init loop_init(void)
> struct loop_device *lo;
> int err;
>
> - err = misc_register(&loop_misc);
> - if (err < 0)
> - return err;
> -
> part_shift = 0;
> if (max_part > 0) {
> part_shift = fls(max_part);
> @@ -2017,12 +2013,12 @@ static int __init loop_init(void)
>
> if ((1UL << part_shift) > DISK_MAX_PARTS) {
> err = -EINVAL;
> - goto misc_out;
> + goto err_out;
> }
>
> if (max_loop > 1UL << (MINORBITS - part_shift)) {
> err = -EINVAL;
> - goto misc_out;
> + goto err_out;
> }
>
> /*
> @@ -2041,6 +2037,11 @@ static int __init loop_init(void)
> range = 1UL << MINORBITS;
> }
>
> + err = misc_register(&loop_misc);
> + if (err < 0)
> + goto err_out;
> +
> +
> if (register_blkdev(LOOP_MAJOR, "loop")) {
> err = -EIO;
> goto misc_out;
> @@ -2060,6 +2061,7 @@ static int __init loop_init(void)
>
> misc_out:
> misc_deregister(&loop_misc);
> +err_out:
> return err;
> }
>
> --
> 2.7.4
>
Looks fine:
Reviewed-by: Ming Lei <[email protected]>
BTW, this patch should have been CCed to linux-block mail list.
Thanks,
Ming Lei
The early device registration made possible a race leading to allocations
of disks with wrong minors.
This patch moves the device registration further down the loop_init
function to make the race infeasible.
Found by Linux Driver Verification project (linuxtesting.org).
Signed-off-by: Anton Volkov <[email protected]>
Reviewed-by: Ming Lei <[email protected]>
---
drivers/block/loop.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index ef83349..2fbd4089 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1996,10 +1996,6 @@ static int __init loop_init(void)
struct loop_device *lo;
int err;
- err = misc_register(&loop_misc);
- if (err < 0)
- return err;
-
part_shift = 0;
if (max_part > 0) {
part_shift = fls(max_part);
@@ -2017,12 +2013,12 @@ static int __init loop_init(void)
if ((1UL << part_shift) > DISK_MAX_PARTS) {
err = -EINVAL;
- goto misc_out;
+ goto err_out;
}
if (max_loop > 1UL << (MINORBITS - part_shift)) {
err = -EINVAL;
- goto misc_out;
+ goto err_out;
}
/*
@@ -2041,6 +2037,11 @@ static int __init loop_init(void)
range = 1UL << MINORBITS;
}
+ err = misc_register(&loop_misc);
+ if (err < 0)
+ goto err_out;
+
+
if (register_blkdev(LOOP_MAJOR, "loop")) {
err = -EIO;
goto misc_out;
@@ -2060,6 +2061,7 @@ static int __init loop_init(void)
misc_out:
misc_deregister(&loop_misc);
+err_out:
return err;
}
--
2.7.4
On Mon, Aug 07, 2017 at 03:37:50PM +0300, Anton Volkov wrote:
> +err_out:
> return err;
Any reason you can't just use return err; at the respective callsites?
Thanks,
Johannes
--
Johannes Thumshirn Storage
[email protected] +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
This is more of a style-oriented suggestion. This kind of template is
commonly used in other modules.
Regards,
Anton
On 07.08.2017 15:54, Johannes Thumshirn wrote:
> On Mon, Aug 07, 2017 at 03:37:50PM +0300, Anton Volkov wrote:
>> +err_out:
>> return err;
>
> Any reason you can't just use return err; at the respective callsites?
>
> Thanks,
> Johannes
>
-- Anton Volkov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: [email protected]
On Mon, Aug 07, 2017 at 04:09:12PM +0300, Anton Volkov wrote:
> This is more of a style-oriented suggestion. This kind of template is
> commonly used in other modules.
Yes but there is no point in using gotos here (i.e. cleanup to be done), you
an just return directly.
And yes it is a minor nit.
Anyways,
Reviewed-by: Johannes Thumshirn <[email protected]>
--
Johannes Thumshirn Storage
[email protected] +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
On Mon, Aug 07, 2017 at 03:37:50PM +0300, Anton Volkov wrote:
> The early device registration made possible a race leading to allocations
> of disks with wrong minors.
>
> This patch moves the device registration further down the loop_init
> function to make the race infeasible.
>
> Found by Linux Driver Verification project (linuxtesting.org).
>
> Signed-off-by: Anton Volkov <[email protected]>
> Reviewed-by: Ming Lei <[email protected]>
Hi, Anton,
Were you able to reproduce this issue or was it purely theoretical? If
the former, it'd be nice if you could add a test case to blktests [1].
1: https://github.com/osandov/blktests
Thanks!
Omar
Hello, Omar.
It was a purely theoretical race that had been considered to be possible
in real-life.
Regards,
Anton
On 09.08.2017 01:00, Omar Sandoval wrote:
> On Mon, Aug 07, 2017 at 03:37:50PM +0300, Anton Volkov wrote:
>> The early device registration made possible a race leading to allocations
>> of disks with wrong minors.
>>
>> This patch moves the device registration further down the loop_init
>> function to make the race infeasible.
>>
>> Found by Linux Driver Verification project (linuxtesting.org).
>>
>> Signed-off-by: Anton Volkov <[email protected]>
>> Reviewed-by: Ming Lei <[email protected]>
>
> Hi, Anton,
>
> Were you able to reproduce this issue or was it purely theoretical? If
> the former, it'd be nice if you could add a test case to blktests [1].
>
> 1: https://github.com/osandov/blktests
>
> Thanks!
> Omar
>
-- Anton Volkov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: [email protected]
On 08/07/2017 06:37 AM, Anton Volkov wrote:
> The early device registration made possible a race leading to allocations
> of disks with wrong minors.
>
> This patch moves the device registration further down the loop_init
> function to make the race infeasible.
>
> Found by Linux Driver Verification project (linuxtesting.org).
Added for 4.14, thanks.
--
Jens Axboe