[PATCH] md/raid10: initialize r10_bio->read_slot before use.

[PATCH] md/raid10: initialize r10_bio->read_slot before use.

[Kevin Vigor]

In __make_request() a new r10bio is allocated and passed to
raid10_read_request(). The read_slot member of the bio is not
initialized, and the raid10_read_request() uses it to index an
array. This leads to occasional panics.

Fix by initializing the field to invalid value and checking for
valid value in raid10_read_request().

Signed-off-by: Kevin Vigor <kvigor@gmail.com>
---
 drivers/md/raid10.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 8a1354a08a1a..64b1306b0c4a 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -1143,7 +1143,7 @@ static void raid10_read_request(struct mddev *mddev, struct bio *bio,
 	struct md_rdev *err_rdev = NULL;
 	gfp_t gfp = GFP_NOIO;
 
-	if (r10_bio->devs[slot].rdev) {
+	if (slot >= 0 && r10_bio->devs[slot].rdev) {
 		/*
 		 * This is an error retry, but we cannot
 		 * safely dereference the rdev in the r10_bio,
@@ -1508,6 +1508,7 @@ static void __make_request(struct mddev *mddev, struct bio *bio, int sectors)
 	r10_bio->mddev = mddev;
 	r10_bio->sector = bio->bi_iter.bi_sector;
 	r10_bio->state = 0;
+	r10_bio->read_slot = -1;
 	memset(r10_bio->devs, 0, sizeof(r10_bio->devs[0]) * conf->copies);
 
 	if (bio_data_dir(bio) == READ)
-- 
2.26.2

Re: [PATCH] md/raid10: initialize r10_bio->read_slot before use.

[Song Liu]

On Thu, Nov 5, 2020 at 10:08 AM Kevin Vigor <kvigor@gmail.com> wrote:
>
> In __make_request() a new r10bio is allocated and passed to
> raid10_read_request(). The read_slot member of the bio is not
> initialized, and the raid10_read_request() uses it to index an
> array. This leads to occasional panics.
>
> Fix by initializing the field to invalid value and checking for
> valid value in raid10_read_request().
>
> Signed-off-by: Kevin Vigor <kvigor@gmail.com>

Thanks for the fix!

I am having problem apply this patch, could you please rebase against

   git://git.kernel.org/pub/scm/linux/kernel/git/song/md.git  md-next

> ---
>  drivers/md/raid10.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index 8a1354a08a1a..64b1306b0c4a 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -1143,7 +1143,7 @@ static void raid10_read_request(struct mddev *mddev, struct bio *bio,
>         struct md_rdev *err_rdev = NULL;
>         gfp_t gfp = GFP_NOIO;
>
> -       if (r10_bio->devs[slot].rdev) {
> +       if (slot >= 0 && r10_bio->devs[slot].rdev) {

If we set default read_slot = 0, we should not need this change, right?

>                 /*
>                  * This is an error retry, but we cannot
>                  * safely dereference the rdev in the r10_bio,
> @@ -1508,6 +1508,7 @@ static void __make_request(struct mddev *mddev, struct bio *bio, int sectors)
>         r10_bio->mddev = mddev;
>         r10_bio->sector = bio->bi_iter.bi_sector;
>         r10_bio->state = 0;
> +       r10_bio->read_slot = -1;
>         memset(r10_bio->devs, 0, sizeof(r10_bio->devs[0]) * conf->copies);
>
>         if (bio_data_dir(bio) == READ)
> --
> 2.26.2
>

[PATCH v2] md/raid10: initialize r10_bio->read_slot before use.

[Kevin Vigor]

In __make_request() a new r10bio is allocated and passed to
raid10_read_request(). The read_slot member of the bio is not
initialized, and the raid10_read_request() uses it to index an
array. This leads to occasional panics.

Fix by initializing the field to invalid value and checking for
valid value in raid10_read_request().

Signed-off-by: Kevin Vigor <kvigor@gmail.com>
---
v2:
- rebase onto md-next
---
 drivers/md/raid10.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index b7bca6703df8..3153183b7772 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -1127,7 +1127,7 @@ static void raid10_read_request(struct mddev *mddev, struct bio *bio,
 	struct md_rdev *err_rdev = NULL;
 	gfp_t gfp = GFP_NOIO;
 
-	if (r10_bio->devs[slot].rdev) {
+	if (slot >= 0 && r10_bio->devs[slot].rdev) {
 		/*
 		 * This is an error retry, but we cannot
 		 * safely dereference the rdev in the r10_bio,
@@ -1508,6 +1508,7 @@ static void __make_request(struct mddev *mddev, struct bio *bio, int sectors)
 	r10_bio->mddev = mddev;
 	r10_bio->sector = bio->bi_iter.bi_sector;
 	r10_bio->state = 0;
+	r10_bio->read_slot = -1;
 	memset(r10_bio->devs, 0, sizeof(r10_bio->devs[0]) * conf->geo.raid_disks);
 
 	if (bio_data_dir(bio) == READ)
-- 
2.26.2

Re: [PATCH v2] md/raid10: initialize r10_bio->read_slot before use.

[Song Liu]

On Fri, Nov 6, 2020 at 2:21 PM Kevin Vigor <kvigor@gmail.com> wrote:
>
> In __make_request() a new r10bio is allocated and passed to
> raid10_read_request(). The read_slot member of the bio is not
> initialized, and the raid10_read_request() uses it to index an
> array. This leads to occasional panics.
>
> Fix by initializing the field to invalid value and checking for
> valid value in raid10_read_request().
>
> Signed-off-by: Kevin Vigor <kvigor@gmail.com>
> ---
> v2:
> - rebase onto md-next
> ---
>  drivers/md/raid10.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index b7bca6703df8..3153183b7772 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -1127,7 +1127,7 @@ static void raid10_read_request(struct mddev *mddev, struct bio *bio,
>         struct md_rdev *err_rdev = NULL;
>         gfp_t gfp = GFP_NOIO;
>
> -       if (r10_bio->devs[slot].rdev) {
> +       if (slot >= 0 && r10_bio->devs[slot].rdev) {

How about we initialize read_slot to 0, and get rid of this check?

Thanks,
Song

Re: [PATCH v2] md/raid10: initialize r10_bio->read_slot before use.

[Song Liu]

On Fri, Nov 6, 2020 at 5:25 PM Kevin Vigor <kvigor@gmail.com> wrote:
>
> > How about we initialize read_slot to 0, and get rid of this check?
>
> That will definitely work properly since we memset the rdev to null in __make_request. But the read_slot is truly invalid until we call read_balance() successfully and it feels wrong to set it to a valid value even if it os harmless by accident. So I prefer to give it an explicitly invalid value.
>
> But your solution saves one test in the issue path and a line of diff, so I will happily change if you prefer the zero solution.

I think initialized to -1 is safer in long run.

Thanks for the fix! Applied to md-next.