DM Regression in 4.16-rc1 - read() returns data when it shouldn't

DM Regression in 4.16-rc1 - read() returns data when it shouldn't

[Milan Broz]

Hi,

the commit (found by bisect)

  commit 18a25da84354c6bb655320de6072c00eda6eb602
  Author: NeilBrown <neilb@suse.com>
  Date:   Wed Sep 6 09:43:28 2017 +1000

    dm: ensure bio submission follows a depth-first tree walk

cause serious regression while reading from DM device.

The reproducer is below, basically it tries to simulate failure we see in cryptsetup
regression test: we have DM device with error and zero target and try to read
"passphrase" from it (it is test for 64 bit offset error path):

Test device:
# dmsetup table test
0 10000000 error 
10000000 1000000 zero 

We try to run this operation:
  lseek64(fd, 5119999988, SEEK_CUR); // this should seek to error target sector
  read(fd, buf, 13); // this should fail, if we seek to error part of the device

While on 4.15 the read properly fails:
  Seek returned 5119999988.
  Read returned -1.

for 4.16 it actually succeeds returning some random data
(perhaps kernel memory, so this bug is even more dangerous):
  Seek returned 5119999988.
  Read returned 13.

Full reproducer below:

#define _GNU_SOURCE
#define _LARGEFILE64_SOURCE
#include <stdio.h>
#include <stddef.h>
#include <stdint.h>
#include <stdlib.h>
#include <unistd.h>
#include <fcntl.h>
#include <inttypes.h>

int main (int argc, char *argv[])
{
        char buf[13];
        int fd;
        //uint64_t offset64 = 5119999999;
        uint64_t offset64 =   5119999988;
        off64_t r;
        ssize_t bytes;

        system("echo -e \'0 10000000 error\'\\\\n\'10000000 1000000 zero\' | dmsetup create test");

        fd = open("/dev/mapper/test", O_RDONLY);
        if (fd == -1) {
                printf("open fail\n");
                return 1;
        }

        r = lseek64(fd, offset64, SEEK_CUR);
        printf("Seek returned %" PRIu64 ".\n", r);
        if (r < 0) {
                printf("seek fail\n");
                close(fd);
                return 2;
        }

        bytes = read(fd, buf, 13);
        printf("Read returned %d.\n", (int)bytes);

        close(fd);
        return 0;
}


Please let me know if you need more info to reproduce it.

Milan

Re: DM Regression in 4.16-rc1 - read() returns data when it shouldn't

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 3299 bytes --]

On Wed, Feb 14 2018, Milan Broz wrote:

> Hi,
>
> the commit (found by bisect)
>
>   commit 18a25da84354c6bb655320de6072c00eda6eb602
>   Author: NeilBrown <neilb@suse.com>
>   Date:   Wed Sep 6 09:43:28 2017 +1000
>
>     dm: ensure bio submission follows a depth-first tree walk
>
> cause serious regression while reading from DM device.
>
> The reproducer is below, basically it tries to simulate failure we see in cryptsetup
> regression test: we have DM device with error and zero target and try to read
> "passphrase" from it (it is test for 64 bit offset error path):
>
> Test device:
> # dmsetup table test
> 0 10000000 error 
> 10000000 1000000 zero 
>
> We try to run this operation:
>   lseek64(fd, 5119999988, SEEK_CUR); // this should seek to error target sector
>   read(fd, buf, 13); // this should fail, if we seek to error part of the device
>
> While on 4.15 the read properly fails:
>   Seek returned 5119999988.
>   Read returned -1.
>
> for 4.16 it actually succeeds returning some random data
> (perhaps kernel memory, so this bug is even more dangerous):
>   Seek returned 5119999988.
>   Read returned 13.
>
> Full reproducer below:
>
> #define _GNU_SOURCE
> #define _LARGEFILE64_SOURCE
> #include <stdio.h>
> #include <stddef.h>
> #include <stdint.h>
> #include <stdlib.h>
> #include <unistd.h>
> #include <fcntl.h>
> #include <inttypes.h>
>
> int main (int argc, char *argv[])
> {
>         char buf[13];
>         int fd;
>         //uint64_t offset64 = 5119999999;
>         uint64_t offset64 =   5119999988;
>         off64_t r;
>         ssize_t bytes;
>
>         system("echo -e \'0 10000000 error\'\\\\n\'10000000 1000000 zero\' | dmsetup create test");
>
>         fd = open("/dev/mapper/test", O_RDONLY);
>         if (fd == -1) {
>                 printf("open fail\n");
>                 return 1;
>         }
>
>         r = lseek64(fd, offset64, SEEK_CUR);
>         printf("Seek returned %" PRIu64 ".\n", r);
>         if (r < 0) {
>                 printf("seek fail\n");
>                 close(fd);
>                 return 2;
>         }
>
>         bytes = read(fd, buf, 13);
>         printf("Read returned %d.\n", (int)bytes);
>
>         close(fd);
>         return 0;
> }
>
>
> Please let me know if you need more info to reproduce it.

Thanks for the detailed report.  I haven't tried to reproduce, but the
code looks very weird.
The patch I posted "Date: Wed, 06 Sep 2017 09:43:28 +1000" had:
      +				struct bio *b = bio_clone_bioset(bio, GFP_NOIO,
      +								 md->queue->bio_split);
      +				bio_advance(b, (bio_sectors(b) - ci.sector_count) << 9);
      +				bio_chain(b, bio);
      +				generic_make_request(b);
      +				break;

The code in Linux has:

				struct bio *b = bio_clone_bioset(bio, GFP_NOIO,
								 md->queue->bio_split);
				ci.io->orig_bio = b;
				bio_advance(bio, (bio_sectors(bio) - ci.sector_count) << 9);
				bio_chain(b, bio);
				ret = generic_make_request(bio);
				break;

So the wrong bio is sent to generic_make_request().
Mike: do you remember how that change happened?  I think there were
discussions following the patch, but I cannot find anything about making
that change.

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: DM Regression in 4.16-rc1 - read() returns data when it shouldn't

[Mike Snitzer]

On Wed, Feb 14 2018 at  3:39pm -0500,
NeilBrown <neilb@suse.com> wrote:

> On Wed, Feb 14 2018, Milan Broz wrote:
> 
> > Hi,
> >
> > the commit (found by bisect)
> >
> >   commit 18a25da84354c6bb655320de6072c00eda6eb602
> >   Author: NeilBrown <neilb@suse.com>
> >   Date:   Wed Sep 6 09:43:28 2017 +1000
> >
> >     dm: ensure bio submission follows a depth-first tree walk
> >
> > cause serious regression while reading from DM device.
> >
> > The reproducer is below, basically it tries to simulate failure we see in cryptsetup
> > regression test: we have DM device with error and zero target and try to read
> > "passphrase" from it (it is test for 64 bit offset error path):
> >
> > Test device:
> > # dmsetup table test
> > 0 10000000 error 
> > 10000000 1000000 zero 
> >
> > We try to run this operation:
> >   lseek64(fd, 5119999988, SEEK_CUR); // this should seek to error target sector
> >   read(fd, buf, 13); // this should fail, if we seek to error part of the device
> >
> > While on 4.15 the read properly fails:
> >   Seek returned 5119999988.
> >   Read returned -1.
> >
> > for 4.16 it actually succeeds returning some random data
> > (perhaps kernel memory, so this bug is even more dangerous):
> >   Seek returned 5119999988.
> >   Read returned 13.
> >
> > Full reproducer below:
> >
> > #define _GNU_SOURCE
> > #define _LARGEFILE64_SOURCE
> > #include <stdio.h>
> > #include <stddef.h>
> > #include <stdint.h>
> > #include <stdlib.h>
> > #include <unistd.h>
> > #include <fcntl.h>
> > #include <inttypes.h>
> >
> > int main (int argc, char *argv[])
> > {
> >         char buf[13];
> >         int fd;
> >         //uint64_t offset64 = 5119999999;
> >         uint64_t offset64 =   5119999988;
> >         off64_t r;
> >         ssize_t bytes;
> >
> >         system("echo -e \'0 10000000 error\'\\\\n\'10000000 1000000 zero\' | dmsetup create test");
> >
> >         fd = open("/dev/mapper/test", O_RDONLY);
> >         if (fd == -1) {
> >                 printf("open fail\n");
> >                 return 1;
> >         }
> >
> >         r = lseek64(fd, offset64, SEEK_CUR);
> >         printf("Seek returned %" PRIu64 ".\n", r);
> >         if (r < 0) {
> >                 printf("seek fail\n");
> >                 close(fd);
> >                 return 2;
> >         }
> >
> >         bytes = read(fd, buf, 13);
> >         printf("Read returned %d.\n", (int)bytes);
> >
> >         close(fd);
> >         return 0;
> > }
> >
> >
> > Please let me know if you need more info to reproduce it.
> 
> Thanks for the detailed report.  I haven't tried to reproduce, but the
> code looks very weird.
> The patch I posted "Date: Wed, 06 Sep 2017 09:43:28 +1000" had:
>       +				struct bio *b = bio_clone_bioset(bio, GFP_NOIO,
>       +								 md->queue->bio_split);
>       +				bio_advance(b, (bio_sectors(b) - ci.sector_count) << 9);
>       +				bio_chain(b, bio);
>       +				generic_make_request(b);
>       +				break;
> 
> The code in Linux has:
> 
> 				struct bio *b = bio_clone_bioset(bio, GFP_NOIO,
> 								 md->queue->bio_split);
> 				ci.io->orig_bio = b;
> 				bio_advance(bio, (bio_sectors(bio) - ci.sector_count) << 9);
> 				bio_chain(b, bio);
> 				ret = generic_make_request(bio);
> 				break;
> 
> So the wrong bio is sent to generic_make_request().
> Mike: do you remember how that change happened?  I think there were
> discussions following the patch, but I cannot find anything about making
> that change.

Mikulas had this feedback:
https://www.redhat.com/archives/dm-devel/2017-November/msg00159.html

You replied with:
https://www.redhat.com/archives/dm-devel/2017-November/msg00165.html

Where you said "Yes, you are right something like that would be better."

And you then provided a follow-up patch:
https://www.redhat.com/archives/dm-devel/2017-November/msg00175.html

That we discussed and I said I'd just fold it into the original, and you
agreed and thanked me for checking with you ;)
https://www.redhat.com/archives/dm-devel/2017-November/msg00208.html

Anyway, I'm just about to switch to Daddy Daycare mode (need to get my
daughter up from her nap, feed her dinner, etc) so: I'll circle back to
this tomorrow morning.

Thanks,
Mike

Re: [dm-devel] DM Regression in 4.16-rc1 - read() returns data when it shouldn't

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 6581 bytes --]

On Wed, Feb 14 2018, Mike Snitzer wrote:

> On Wed, Feb 14 2018 at  3:39pm -0500,
> NeilBrown <neilb@suse.com> wrote:
>
>> On Wed, Feb 14 2018, Milan Broz wrote:
>> 
>> > Hi,
>> >
>> > the commit (found by bisect)
>> >
>> >   commit 18a25da84354c6bb655320de6072c00eda6eb602
>> >   Author: NeilBrown <neilb@suse.com>
>> >   Date:   Wed Sep 6 09:43:28 2017 +1000
>> >
>> >     dm: ensure bio submission follows a depth-first tree walk
>> >
>> > cause serious regression while reading from DM device.
>> >
>> > The reproducer is below, basically it tries to simulate failure we see in cryptsetup
>> > regression test: we have DM device with error and zero target and try to read
>> > "passphrase" from it (it is test for 64 bit offset error path):
>> >
>> > Test device:
>> > # dmsetup table test
>> > 0 10000000 error 
>> > 10000000 1000000 zero 
>> >
>> > We try to run this operation:
>> >   lseek64(fd, 5119999988, SEEK_CUR); // this should seek to error target sector
>> >   read(fd, buf, 13); // this should fail, if we seek to error part of the device
>> >
>> > While on 4.15 the read properly fails:
>> >   Seek returned 5119999988.
>> >   Read returned -1.
>> >
>> > for 4.16 it actually succeeds returning some random data
>> > (perhaps kernel memory, so this bug is even more dangerous):
>> >   Seek returned 5119999988.
>> >   Read returned 13.
>> >
>> > Full reproducer below:
>> >
>> > #define _GNU_SOURCE
>> > #define _LARGEFILE64_SOURCE
>> > #include <stdio.h>
>> > #include <stddef.h>
>> > #include <stdint.h>
>> > #include <stdlib.h>
>> > #include <unistd.h>
>> > #include <fcntl.h>
>> > #include <inttypes.h>
>> >
>> > int main (int argc, char *argv[])
>> > {
>> >         char buf[13];
>> >         int fd;
>> >         //uint64_t offset64 = 5119999999;
>> >         uint64_t offset64 =   5119999988;
>> >         off64_t r;
>> >         ssize_t bytes;
>> >
>> >         system("echo -e \'0 10000000 error\'\\\\n\'10000000 1000000 zero\' | dmsetup create test");
>> >
>> >         fd = open("/dev/mapper/test", O_RDONLY);
>> >         if (fd == -1) {
>> >                 printf("open fail\n");
>> >                 return 1;
>> >         }
>> >
>> >         r = lseek64(fd, offset64, SEEK_CUR);
>> >         printf("Seek returned %" PRIu64 ".\n", r);
>> >         if (r < 0) {
>> >                 printf("seek fail\n");
>> >                 close(fd);
>> >                 return 2;
>> >         }
>> >
>> >         bytes = read(fd, buf, 13);
>> >         printf("Read returned %d.\n", (int)bytes);
>> >
>> >         close(fd);
>> >         return 0;
>> > }
>> >
>> >
>> > Please let me know if you need more info to reproduce it.
>> 
>> Thanks for the detailed report.  I haven't tried to reproduce, but the
>> code looks very weird.
>> The patch I posted "Date: Wed, 06 Sep 2017 09:43:28 +1000" had:
>>       +				struct bio *b = bio_clone_bioset(bio, GFP_NOIO,
>>       +								 md->queue->bio_split);
>>       +				bio_advance(b, (bio_sectors(b) - ci.sector_count) << 9);
>>       +				bio_chain(b, bio);
>>       +				generic_make_request(b);
>>       +				break;
>> 
>> The code in Linux has:
>> 
>> 				struct bio *b = bio_clone_bioset(bio, GFP_NOIO,
>> 								 md->queue->bio_split);
>> 				ci.io->orig_bio = b;
>> 				bio_advance(bio, (bio_sectors(bio) - ci.sector_count) << 9);
>> 				bio_chain(b, bio);
>> 				ret = generic_make_request(bio);
>> 				break;
>> 
>> So the wrong bio is sent to generic_make_request().
>> Mike: do you remember how that change happened?  I think there were
>> discussions following the patch, but I cannot find anything about making
>> that change.
>
> Mikulas had this feedback:
> https://www.redhat.com/archives/dm-devel/2017-November/msg00159.html
>
> You replied with:
> https://www.redhat.com/archives/dm-devel/2017-November/msg00165.html
>
> Where you said "Yes, you are right something like that would be better."
>
> And you then provided a follow-up patch:
> https://www.redhat.com/archives/dm-devel/2017-November/msg00175.html
>
> That we discussed and I said I'd just fold it into the original, and you
> agreed and thanked me for checking with you ;)
> https://www.redhat.com/archives/dm-devel/2017-November/msg00208.html
>
> Anyway, I'm just about to switch to Daddy Daycare mode (need to get my
> daughter up from her nap, feed her dinner, etc) so: I'll circle back to
> this tomorrow morning.
>

Thanks for the reminder - it's coming back to me now.

And looking again at the code, it doesn't seem so bad.  I was worried
about reassigning ci.io->orig_bio after calling
__split_and_process_non_flush(), but the important thing is to set it
before the last dec_pending() call, and the code gets that right.

I think I've found the problem though:

			/* done with normal IO or empty flush */
			bio->bi_status = io_error;

When we have chained bios, there are multiple sources for error which
need to be merged into bi_status:  all bios that were chained to this
one, and this bio itself.

__bio_chain_endio uses:

	if (!parent->bi_status)
		parent->bi_status = bio->bi_status;

so the new status won't over-ride the old status (unless there are
races....), but dm doesn't do that - I guess it didn't have to worry
about chains before.

So this:

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index d6de00f367ef..68136806d365 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -903,7 +903,8 @@ static void dec_pending(struct dm_io *io, blk_status_t error)
 			queue_io(md, bio);
 		} else {
 			/* done with normal IO or empty flush */
-			bio->bi_status = io_error;
+			if (io_error)
+				bio->bi_status = io_error;
 			bio_endio(bio);
 		}
 	}

should fix it.  And __bio_chain_endio should probably be

diff --git a/block/bio.c b/block/bio.c
index e1708db48258..ad77140edc6f 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -312,7 +312,7 @@ static struct bio *__bio_chain_endio(struct bio *bio)
 {
 	struct bio *parent = bio->bi_private;
 
-	if (!parent->bi_status)
+	if (bio->bi_status)
 		parent->bi_status = bio->bi_status;
 	bio_put(bio);
 	return parent;

to remove the chance that a race will hide a real error.

Milan: could you please check that the patch fixes the dm-crypt bug?
I've confirmed that it fixes your test case.
When you confirm, I'll send a proper patch.

I guess I should audit all code that assigns to ->bi_status and make
sure nothing ever assigns 0 to it.

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [dm-devel] DM Regression in 4.16-rc1 - read() returns data when it shouldn't

[Milan Broz]

On 02/15/2018 01:07 AM, NeilBrown wrote:
> 
> And looking again at the code, it doesn't seem so bad.  I was worried
> about reassigning ci.io->orig_bio after calling
> __split_and_process_non_flush(), but the important thing is to set it
> before the last dec_pending() call, and the code gets that right.
> 
> I think I've found the problem though:
> 
> 			/* done with normal IO or empty flush */
> 			bio->bi_status = io_error;
> 
> When we have chained bios, there are multiple sources for error which
> need to be merged into bi_status:  all bios that were chained to this
> one, and this bio itself.
> 
> __bio_chain_endio uses:
> 
> 	if (!parent->bi_status)
> 		parent->bi_status = bio->bi_status;
> 
> so the new status won't over-ride the old status (unless there are
> races....), but dm doesn't do that - I guess it didn't have to worry
> about chains before.
> 
> So this:
> 
> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index d6de00f367ef..68136806d365 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -903,7 +903,8 @@ static void dec_pending(struct dm_io *io, blk_status_t error)
>  			queue_io(md, bio);
>  		} else {
>  			/* done with normal IO or empty flush */
> -			bio->bi_status = io_error;
> +			if (io_error)
> +				bio->bi_status = io_error;
>  			bio_endio(bio);
>  		}
>  	}
> 

Hi,

well, it indeed fixes the reported problem, thanks.
But I just tested the first (dm.c) change above.

The important part is also that if the error is hits bio later, it will produce
short read (and not fail of the whole operation), this can be easily tested if we switch
the error and the zero target in that reproducer
  //system("echo -e \'0 10000000 error\'\\\\n\'10000000 1000000 zero\' | dmsetup create test");
  system("echo -e \'0 10000000 zero\'\\\\n\'10000000 1000000 error\' | dmsetup create test");

So it seems your patch works.

I am not sure about this part though:

> should fix it.  And __bio_chain_endio should probably be
> 
> diff --git a/block/bio.c b/block/bio.c
> index e1708db48258..ad77140edc6f 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -312,7 +312,7 @@ static struct bio *__bio_chain_endio(struct bio *bio)
>  {
>  	struct bio *parent = bio->bi_private;
>  
> -	if (!parent->bi_status)
> +	if (bio->bi_status)
>  		parent->bi_status = bio->bi_status;

Shouldn't it be
	if (!parent->bi_status && bio->bi_status)
  		parent->bi_status = bio->bi_status;
?

Maybe one rephrased question: what about priorities for errors (bio statuses)?

For example, part of a bio fails with EILSEQ (data integrity check error, BLK_STS_PROTECTION),
part with EIO (media error, BLK_STS_IOERR). What should be reported for parent bio?
(I would expect I always get EIO here because this is hard, uncorrectable error.)

Anyway, thanks for the fix!

Milan


>  	bio_put(bio);
>  	return parent;
> 
> to remove the chance that a race will hide a real error.
> 
> Milan: could you please check that the patch fixes the dm-crypt bug?
> I've confirmed that it fixes your test case.
> When you confirm, I'll send a proper patch.
> 
> I guess I should audit all code that assigns to ->bi_status and make
> sure nothing ever assigns 0 to it.
> 
> Thanks,
> NeilBrown
> 

Re: [dm-devel] DM Regression in 4.16-rc1 - read() returns data when it shouldn't

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 4249 bytes --]

On Thu, Feb 15 2018, Milan Broz wrote:

> On 02/15/2018 01:07 AM, NeilBrown wrote:
>> 
>> And looking again at the code, it doesn't seem so bad.  I was worried
>> about reassigning ci.io->orig_bio after calling
>> __split_and_process_non_flush(), but the important thing is to set it
>> before the last dec_pending() call, and the code gets that right.
>> 
>> I think I've found the problem though:
>> 
>> 			/* done with normal IO or empty flush */
>> 			bio->bi_status = io_error;
>> 
>> When we have chained bios, there are multiple sources for error which
>> need to be merged into bi_status:  all bios that were chained to this
>> one, and this bio itself.
>> 
>> __bio_chain_endio uses:
>> 
>> 	if (!parent->bi_status)
>> 		parent->bi_status = bio->bi_status;
>> 
>> so the new status won't over-ride the old status (unless there are
>> races....), but dm doesn't do that - I guess it didn't have to worry
>> about chains before.
>> 
>> So this:
>> 
>> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
>> index d6de00f367ef..68136806d365 100644
>> --- a/drivers/md/dm.c
>> +++ b/drivers/md/dm.c
>> @@ -903,7 +903,8 @@ static void dec_pending(struct dm_io *io, blk_status_t error)
>>  			queue_io(md, bio);
>>  		} else {
>>  			/* done with normal IO or empty flush */
>> -			bio->bi_status = io_error;
>> +			if (io_error)
>> +				bio->bi_status = io_error;
>>  			bio_endio(bio);
>>  		}
>>  	}
>> 
>
> Hi,
>
> well, it indeed fixes the reported problem, thanks.
> But I just tested the first (dm.c) change above.
>
> The important part is also that if the error is hits bio later, it will produce
> short read (and not fail of the whole operation), this can be easily tested if we switch
> the error and the zero target in that reproducer
>   //system("echo -e \'0 10000000 error\'\\\\n\'10000000 1000000 zero\' | dmsetup create test");
>   system("echo -e \'0 10000000 zero\'\\\\n\'10000000 1000000 error\' | dmsetup create test");
>
> So it seems your patch works.

Excellent - thanks.

>
> I am not sure about this part though:
>
>> should fix it.  And __bio_chain_endio should probably be
>> 
>> diff --git a/block/bio.c b/block/bio.c
>> index e1708db48258..ad77140edc6f 100644
>> --- a/block/bio.c
>> +++ b/block/bio.c
>> @@ -312,7 +312,7 @@ static struct bio *__bio_chain_endio(struct bio *bio)
>>  {
>>  	struct bio *parent = bio->bi_private;
>>  
>> -	if (!parent->bi_status)
>> +	if (bio->bi_status)
>>  		parent->bi_status = bio->bi_status;
>
> Shouldn't it be
> 	if (!parent->bi_status && bio->bi_status)
>   		parent->bi_status = bio->bi_status;
> ?

That wouldn't hurt, but I don't see how it would help.
It would still be racy as two chained bios could complete at
the same time, so ->bi_status much change between test and set.

>
> Maybe one rephrased question: what about priorities for errors (bio statuses)?
>
> For example, part of a bio fails with EILSEQ (data integrity check error, BLK_STS_PROTECTION),
> part with EIO (media error, BLK_STS_IOERR). What should be reported for parent bio?
> (I would expect I always get EIO here because this is hard, uncorrectable error.)

If there was a well defined ordering, then we could easily write to code
ensure the highest priority status wins, but I don't think there is any
such ordering.
Certainly dec_pending() in dm.c already faces the possibility that it will
be called multiple times with different errors, but doesn't consider any
ordering (except relating to BLK_STS_DM_REQUEUE), when assigning a
status code to io->status.

>
> Anyway, thanks for the fix!

And thanks for the report.

NeilBrown

>
> Milan
>
>
>>  	bio_put(bio);
>>  	return parent;
>> 
>> to remove the chance that a race will hide a real error.
>> 
>> Milan: could you please check that the patch fixes the dm-crypt bug?
>> I've confirmed that it fixes your test case.
>> When you confirm, I'll send a proper patch.
>> 
>> I guess I should audit all code that assigns to ->bi_status and make
>> sure nothing ever assigns 0 to it.
>> 
>> Thanks,
>> NeilBrown
>> 
>
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[PATCH] dm: correctly handle chained bios in dec_pending()

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1810 bytes --]


dec_pending() is given an error status (possibly 0)
to be recorded against a bio.
It can be called several times on the one 'struct dm_io',
and it is careful to only assign a non-zero error to
io->status.
However when it then assigned io->status to bio->bi_status,
it is not careful and could overwrite a genuine error status with
zero.

This can happen when chained bios are in use.  If a bio is chained
beneath the bio that this dm_io is handling, the child bio might
complete and set bio->bi_status before the dm_io completes.

This has been possible since chained bios were introduced in 3.14, and
become a lot easier to trigger with commit 18a25da84354 ("dm: ensure bio
submission follows a depth-first tree walk") as that commit caused dm to
start using chained bios itself.

A particular failure mode is that if a bio spans an 'error' target and
a working target, the 'error' fragment will complete instantly and set
the ->bi_status, and the other fragment will normally complete a
little later, and will clear ->bi_status.

The fix is simply to only assign io_error to bio->bi_status when
io_error is not zero.

Reported-and-tested-by: Milan Broz <gmazyland@gmail.com>
Cc: stable@vger.kernel.org (v3.14+)
Signed-off-by: NeilBrown <neilb@suse.com>
---
 drivers/md/dm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index d6de00f367ef..68136806d365 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -903,7 +903,8 @@ static void dec_pending(struct dm_io *io, blk_status_t error)
 			queue_io(md, bio);
 		} else {
 			/* done with normal IO or empty flush */
-			bio->bi_status = io_error;
+			if (io_error)
+				bio->bi_status = io_error;
 			bio_endio(bio);
 		}
 	}
-- 
2.14.0.rc0.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[RFC PATCH] dm: don't assign zero to ->bi_status of an active bio.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 4354 bytes --]


Between the moment when generic_make_request() is first
called on a bio, and when bio_endio() finally gets past
bio_remaining_done(), a bio might have chained children, and might
get ->bi_status set asynchronously.

So during this time it is not safe to set it to zero.
It *is* safe to set it to any other error status as for most
purposes, one error is as good as another.

This patch is untested and RFC only.  It identifies a number of
possible problem areas in dm and often suggests fixes.

Please don't apply without careful review.

Reviewed-by: Nobody yet.
Signed-off-by: NeilBrown <neilb@suse.com>
---
 drivers/md/dm-bio-prison-v1.c | 3 ++-
 drivers/md/dm-bufio.c         | 6 ++++--
 drivers/md/dm-crypt.c         | 3 ++-
 drivers/md/dm-mpath.c         | 3 +++
 drivers/md/dm-thin.c          | 3 ++-
 drivers/md/dm-verity-target.c | 3 ++-
 drivers/md/dm-zoned-target.c  | 3 ++-
 7 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/drivers/md/dm-bio-prison-v1.c b/drivers/md/dm-bio-prison-v1.c
index 874841f0fc83..68cb4fe98e9f 100644
--- a/drivers/md/dm-bio-prison-v1.c
+++ b/drivers/md/dm-bio-prison-v1.c
@@ -238,7 +238,8 @@ void dm_cell_error(struct dm_bio_prison *prison,
 	dm_cell_release(prison, cell, &bios);
 
 	while ((bio = bio_list_pop(&bios))) {
-		bio->bi_status = error;
+		if (error)
+			bio->bi_status = error;
 		bio_endio(bio);
 	}
 }
diff --git a/drivers/md/dm-bufio.c b/drivers/md/dm-bufio.c
index 414c9af54ded..80131e098650 100644
--- a/drivers/md/dm-bufio.c
+++ b/drivers/md/dm-bufio.c
@@ -565,7 +565,8 @@ static void dmio_complete(unsigned long error, void *context)
 {
 	struct dm_buffer *b = context;
 
-	b->bio.bi_status = error ? BLK_STS_IOERR : 0;
+	if (error)
+		b->bio.bi_status = BLK_STS_IOERR;
 	b->bio.bi_end_io(&b->bio);
 }
 
@@ -614,7 +615,8 @@ static void inline_endio(struct bio *bio)
 	 */
 	bio_reset(bio);
 
-	bio->bi_status = status;
+	if (status)
+		bio->bi_status = status;
 	end_fn(bio);
 }
 
diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 8168f737590e..3e7db4a1093e 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -1488,7 +1488,8 @@ static void crypt_dec_pending(struct dm_crypt_io *io)
 	else
 		kfree(io->integrity_metadata);
 
-	base_bio->bi_status = error;
+	if (error)
+		base_bio->bi_status = error;
 	bio_endio(base_bio);
 }
 
diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index 7d3e572072f5..6f793d7d29f0 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -651,6 +651,9 @@ static int __multipath_map_bio(struct multipath *m, struct bio *bio,
 
 	mpio->pgpath = pgpath;
 
+	/* FIXME If the bio was chained, this could
+	 * over-write an error... is that possible?
+	 */
 	bio->bi_status = 0;
 	bio_set_dev(bio, pgpath->path.dev->bdev);
 	bio->bi_opf |= REQ_FAILFAST_TRANSPORT;
diff --git a/drivers/md/dm-thin.c b/drivers/md/dm-thin.c
index 629c555890c1..16d4c386cc56 100644
--- a/drivers/md/dm-thin.c
+++ b/drivers/md/dm-thin.c
@@ -563,7 +563,8 @@ static void error_bio_list(struct bio_list *bios, blk_status_t error)
 	struct bio *bio;
 
 	while ((bio = bio_list_pop(bios))) {
-		bio->bi_status = error;
+		if (error)
+			bio->bi_status = error;
 		bio_endio(bio);
 	}
 }
diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
index aedb8222836b..86a1d501d915 100644
--- a/drivers/md/dm-verity-target.c
+++ b/drivers/md/dm-verity-target.c
@@ -503,7 +503,8 @@ static void verity_finish_io(struct dm_verity_io *io, blk_status_t status)
 	struct bio *bio = dm_bio_from_per_bio_data(io, v->ti->per_io_data_size);
 
 	bio->bi_end_io = io->orig_bi_end_io;
-	bio->bi_status = status;
+	if (status)
+		bio->bi_status = status;
 
 	verity_fec_finish_io(io);
 
diff --git a/drivers/md/dm-zoned-target.c b/drivers/md/dm-zoned-target.c
index caff02caf083..cfdacaf79492 100644
--- a/drivers/md/dm-zoned-target.c
+++ b/drivers/md/dm-zoned-target.c
@@ -637,7 +637,8 @@ static int dmz_end_io(struct dm_target *ti, struct bio *bio, blk_status_t *error
 		return DM_ENDIO_INCOMPLETE;
 
 	/* Done */
-	bio->bi_status = bioctx->status;
+	if (bioctx->status)
+		bio->bi_status = bioctx->status;
 
 	if (bioctx->zone) {
 		struct dm_zone *zone = bioctx->zone;
-- 
2.14.0.rc0.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[PATCH] block: be more careful about status in __bio_chain_endio

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1427 bytes --]


If two bios are chained under the one parent (with bio_chain())
it is possible that one will succeed and the other will fail.
__bio_chain_endio must ensure that the failure error status
is reported for the whole, rather than the success.

It currently tries to be careful, but this test is racy.
If both children finish at the same time, they might both see that
parent->bi_status as zero, and so will assign their own status.
If the assignment to parent->bi_status by the successful bio happens
last, the error status will be lost which can lead to silent data
corruption.

Instead, __bio_chain_endio should only assign a non-zero status
to parent->bi_status.  There is then no need to test the current
value of parent->bi_status - a test that would be racy anyway.

Note that this bug hasn't been seen in practice.  It was only discovered
by examination after a similar bug was found in dm.c

Signed-off-by: NeilBrown <neilb@suse.com>
---
 block/bio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/bio.c b/block/bio.c
index e1708db48258..ad77140edc6f 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -312,7 +312,7 @@ static struct bio *__bio_chain_endio(struct bio *bio)
 {
 	struct bio *parent = bio->bi_private;
 
-	if (!parent->bi_status)
+	if (bio->bi_status)
 		parent->bi_status = bio->bi_status;
 	bio_put(bio);
 	return parent;
-- 
2.14.0.rc0.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: DM Regression in 4.16-rc1 - read() returns data when it shouldn't

[Thorsten Leemhuis]

JFYI: This issues is tracked in the regression reports for Linux 4.16
(http://bit.ly/lnxregrep416 ) with this id:

Linux-Regression-ID: lr#9e195f

Please include this line in the comment section of patches that are
supposed to fix the issue. Please also mention the string once in other
mailinglist threads or different bug tracking entries if you or someone
else start to discuss the issue there. By including that string you make
it a whole lot easier to track where an issue gets discussed and how far
patches to fix it have made it. For more details on this please see
here: http://bit.ly/lnxregtrackid

Thx for your help. Ciao, Thorsten

On 14.02.2018 14:02, Milan Broz wrote:
> Hi,
> 
> the commit (found by bisect)
> 
>   commit 18a25da84354c6bb655320de6072c00eda6eb602
>   Author: NeilBrown <neilb@suse.com>
>   Date:   Wed Sep 6 09:43:28 2017 +1000
> 
>     dm: ensure bio submission follows a depth-first tree walk
> 
> cause serious regression while reading from DM device.
> 
> The reproducer is below, basically it tries to simulate failure we see in cryptsetup
> regression test: we have DM device with error and zero target and try to read
> "passphrase" from it (it is test for 64 bit offset error path):
> 
> Test device:
> # dmsetup table test
> 0 10000000 error 
> 10000000 1000000 zero 
> 
> We try to run this operation:
>   lseek64(fd, 5119999988, SEEK_CUR); // this should seek to error target sector
>   read(fd, buf, 13); // this should fail, if we seek to error part of the device
> 
> While on 4.15 the read properly fails:
>   Seek returned 5119999988.
>   Read returned -1.
> 
> for 4.16 it actually succeeds returning some random data
> (perhaps kernel memory, so this bug is even more dangerous):
>   Seek returned 5119999988.
>   Read returned 13.
> 
> Full reproducer below:
> 
> #define _GNU_SOURCE
> #define _LARGEFILE64_SOURCE
> #include <stdio.h>
> #include <stddef.h>
> #include <stdint.h>
> #include <stdlib.h>
> #include <unistd.h>
> #include <fcntl.h>
> #include <inttypes.h>
> 
> int main (int argc, char *argv[])
> {
>         char buf[13];
>         int fd;
>         //uint64_t offset64 = 5119999999;
>         uint64_t offset64 =   5119999988;
>         off64_t r;
>         ssize_t bytes;
> 
>         system("echo -e \'0 10000000 error\'\\\\n\'10000000 1000000 zero\' | dmsetup create test");
> 
>         fd = open("/dev/mapper/test", O_RDONLY);
>         if (fd == -1) {
>                 printf("open fail\n");
>                 return 1;
>         }
> 
>         r = lseek64(fd, offset64, SEEK_CUR);
>         printf("Seek returned %" PRIu64 ".\n", r);
>         if (r < 0) {
>                 printf("seek fail\n");
>                 close(fd);
>                 return 2;
>         }
> 
>         bytes = read(fd, buf, 13);
>         printf("Read returned %d.\n", (int)bytes);
> 
>         close(fd);
>         return 0;
> }
> 
> 
> Please let me know if you need more info to reproduce it.
> 
> Milan
> 
> http://news.gmane.org/find-root.php?message_id=70cda2a3-f246-d45b-f600-1f9d15ba22ff%40gmail.com 
> http://mid.gmane.org/70cda2a3-f246-d45b-f600-1f9d15ba22ff%40gmail.com
> 

Re: DM Regression in 4.16-rc1 - read() returns data when it shouldn't

[Mike Snitzer]

On Mon, Feb 19 2018 at  8:44am -0500,
Thorsten Leemhuis <regressions@leemhuis.info> wrote:

> JFYI: This issues is tracked in the regression reports for Linux 4.16
> (http://bit.ly/lnxregrep416 ) with this id:
> 
> Linux-Regression-ID: lr#9e195f
> 
> Please include this line in the comment section of patches that are
> supposed to fix the issue. Please also mention the string once in other
> mailinglist threads or different bug tracking entries if you or someone
> else start to discuss the issue there. By including that string you make
> it a whole lot easier to track where an issue gets discussed and how far
> patches to fix it have made it. For more details on this please see
> here: http://bit.ly/lnxregtrackid
> 
> Thx for your help. Ciao, Thorsten

The fix was already merged by Linus on Friday, see:
git.kernel.org/linus/8dd601fa8317243be887458c49f6c29c2f3d719f

But moving forward I have no interest in sprinkling external metadata
references in Linux commit headers. 

Seems more like you're engineering something that gives you, and
possibly a select few others, meaning but that is make work for all
Linux maintainers.

Mike

Re: DM Regression in 4.16-rc1 - read() returns data when it shouldn't

[Thorsten Leemhuis]

Hi Mike! On 19.02.2018 18:15, Mike Snitzer wrote:
> On Mon, Feb 19 2018 at  8:44am -0500,
> Thorsten Leemhuis <regressions@leemhuis.info> wrote:
> 
>> JFYI: This issues is tracked in the regression reports for Linux 4.16
>> (http://bit.ly/lnxregrep416 ) with this id:
>> Linux-Regression-ID: lr#9e195f
>> Please include this line in the comment section of patches that are
> […]
> The fix was already merged by Linus on Friday, see:
> git.kernel.org/linus/8dd601fa8317243be887458c49f6c29c2f3d719f

Ohh, thx for the pointer. Could you please next time add a tag like

Fixes: 18a25da84354 ("dm: ensure bio submission follows a depth-first
tree walk")

to the commit? For details see
Documnetation/process/submitting-patches.rst section 2 and 13:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html

That would have saved both of us this conversation. In addition I head
that at least one big Linux distributor is using those tags when
backporting changes to make sure all relevant fixes for a particular
backported commit get backported as well.

> But moving forward I have no interest in sprinkling external metadata
> references in Linux commit headers. 
> 
> Seems more like you're engineering something that gives you, and
> possibly a select few others, meaning but that is make work for all
> Linux maintainers.

I just asked you to help me make regression tracking work a little bit
easier. I'm neither planing to enforce this nor in a position to do so,
thus feel free to not do what I asked for; that's totally fine for me.
Especially as those tags are only a interim solution afaics (one that I
don't like to much myself; but they sometimes help connecting the
various dots in the phase where the commit that introduced a regression
is not yet known).

Ciao, Thorsten

Re: DM Regression in 4.16-rc1 - read() returns data when it shouldn't

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2304 bytes --]

On Mon, Feb 26 2018, Thorsten Leemhuis wrote:

> Hi Mike! On 19.02.2018 18:15, Mike Snitzer wrote:
>> On Mon, Feb 19 2018 at  8:44am -0500,
>> Thorsten Leemhuis <regressions@leemhuis.info> wrote:
>> 
>>> JFYI: This issues is tracked in the regression reports for Linux 4.16
>>> (http://bit.ly/lnxregrep416 ) with this id:
>>> Linux-Regression-ID: lr#9e195f
>>> Please include this line in the comment section of patches that are
>> […]
>> The fix was already merged by Linus on Friday, see:
>> git.kernel.org/linus/8dd601fa8317243be887458c49f6c29c2f3d719f
>
> Ohh, thx for the pointer. Could you please next time add a tag like
>
> Fixes: 18a25da84354 ("dm: ensure bio submission follows a depth-first
> tree walk")

The thing is... it didn't fix that commit.  That commit was fine.
If fixed something else much further back, which that commit just made
more problematic.
That is why I added the Cc: stable with the earliest version that needed
fixing.

Unfortunately, reality isn't always neat and tidy :-(  when it is, I do
use Fixes:

Thanks,
NeilBrown

>
> to the commit? For details see
> Documnetation/process/submitting-patches.rst section 2 and 13:
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html
>
> That would have saved both of us this conversation. In addition I head
> that at least one big Linux distributor is using those tags when
> backporting changes to make sure all relevant fixes for a particular
> backported commit get backported as well.
>
>> But moving forward I have no interest in sprinkling external metadata
>> references in Linux commit headers. 
>> 
>> Seems more like you're engineering something that gives you, and
>> possibly a select few others, meaning but that is make work for all
>> Linux maintainers.
>
> I just asked you to help me make regression tracking work a little bit
> easier. I'm neither planing to enforce this nor in a position to do so,
> thus feel free to not do what I asked for; that's totally fine for me.
> Especially as those tags are only a interim solution afaics (one that I
> don't like to much myself; but they sometimes help connecting the
> various dots in the phase where the commit that introduced a regression
> is not yet known).
>
> Ciao, Thorsten

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: DM Regression in 4.16-rc1 - read() returns data when it shouldn't

[Thorsten Leemhuis]

On 26.02.2018 12:01, NeilBrown wrote:
> On Mon, Feb 26 2018, Thorsten Leemhuis wrote:
>> Hi Mike! On 19.02.2018 18:15, Mike Snitzer wrote:
>>> On Mon, Feb 19 2018 at  8:44am -0500,
>>> Thorsten Leemhuis <regressions@leemhuis.info> wrote:
>>>> JFYI: This issues is tracked in the regression reports for Linux 4.16
>>>> (http://bit.ly/lnxregrep416 ) with this id:
>>>> Linux-Regression-ID: lr#9e195f
>>>> Please include this line in the comment section of patches that are
>>> […]
>>> The fix was already merged by Linus on Friday, see:
>>> git.kernel.org/linus/8dd601fa8317243be887458c49f6c29c2f3d719f
>> Ohh, thx for the pointer. Could you please next time add a tag like
>> Fixes: 18a25da84354 ("dm: ensure bio submission follows a depth-first
>> tree walk")
> 
> The thing is... it didn't fix that commit.  That commit was fine.
> If fixed something else much further back, which that commit just made
> more problematic.
> That is why I added the Cc: stable with the earliest version that needed
> fixing.
> 
> Unfortunately, reality isn't always neat and tidy :-(  when it is, I do
> use Fixes:

Ha, okay, sorry for the noise then.

Side note:  At the same time this is might be a reason why a
"Linux-Regression-ID" or something like that might make sense, because
it shows that even a bisected commit sometimes isn't enough to build a
obvious connection between a regression report and the commit with the
fix...
 Ciao, Thorsten

Re: block: be more careful about status in __bio_chain_endio

[Mike Snitzer]

On Thu, Feb 15 2018 at  4:09am -0500,
NeilBrown <neilb@suse.com> wrote:

> 
> If two bios are chained under the one parent (with bio_chain())
> it is possible that one will succeed and the other will fail.
> __bio_chain_endio must ensure that the failure error status
> is reported for the whole, rather than the success.
> 
> It currently tries to be careful, but this test is racy.
> If both children finish at the same time, they might both see that
> parent->bi_status as zero, and so will assign their own status.
> If the assignment to parent->bi_status by the successful bio happens
> last, the error status will be lost which can lead to silent data
> corruption.
> 
> Instead, __bio_chain_endio should only assign a non-zero status
> to parent->bi_status.  There is then no need to test the current
> value of parent->bi_status - a test that would be racy anyway.
> 
> Note that this bug hasn't been seen in practice.  It was only discovered
> by examination after a similar bug was found in dm.c
> 
> Signed-off-by: NeilBrown <neilb@suse.com>
> ---
>  block/bio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/bio.c b/block/bio.c
> index e1708db48258..ad77140edc6f 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -312,7 +312,7 @@ static struct bio *__bio_chain_endio(struct bio *bio)
>  {
>  	struct bio *parent = bio->bi_private;
>  
> -	if (!parent->bi_status)
> +	if (bio->bi_status)
>  		parent->bi_status = bio->bi_status;
>  	bio_put(bio);
>  	return parent;
> -- 
> 2.14.0.rc0.dirty
> 

Reviewed-by: Mike Snitzer <snitzer@redhat.com>

Jens, this one slipped through the crack just over a year ago.
It is available in patchwork here:
https://patchwork.kernel.org/patch/10220727/

Re: block: be more careful about status in __bio_chain_endio

[Jens Axboe]

On 2/22/19 2:10 PM, Mike Snitzer wrote:
> On Thu, Feb 15 2018 at  4:09am -0500,
> NeilBrown <neilb@suse.com> wrote:
> 
>>
>> If two bios are chained under the one parent (with bio_chain())
>> it is possible that one will succeed and the other will fail.
>> __bio_chain_endio must ensure that the failure error status
>> is reported for the whole, rather than the success.
>>
>> It currently tries to be careful, but this test is racy.
>> If both children finish at the same time, they might both see that
>> parent->bi_status as zero, and so will assign their own status.
>> If the assignment to parent->bi_status by the successful bio happens
>> last, the error status will be lost which can lead to silent data
>> corruption.
>>
>> Instead, __bio_chain_endio should only assign a non-zero status
>> to parent->bi_status.  There is then no need to test the current
>> value of parent->bi_status - a test that would be racy anyway.
>>
>> Note that this bug hasn't been seen in practice.  It was only discovered
>> by examination after a similar bug was found in dm.c
>>
>> Signed-off-by: NeilBrown <neilb@suse.com>
>> ---
>>  block/bio.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/block/bio.c b/block/bio.c
>> index e1708db48258..ad77140edc6f 100644
>> --- a/block/bio.c
>> +++ b/block/bio.c
>> @@ -312,7 +312,7 @@ static struct bio *__bio_chain_endio(struct bio *bio)
>>  {
>>  	struct bio *parent = bio->bi_private;
>>  
>> -	if (!parent->bi_status)
>> +	if (bio->bi_status)
>>  		parent->bi_status = bio->bi_status;
>>  	bio_put(bio);
>>  	return parent;
>> -- 
>> 2.14.0.rc0.dirty
>>
> 
> Reviewed-by: Mike Snitzer <snitzer@redhat.com>
> 
> Jens, this one slipped through the crack just over a year ago.
> It is available in patchwork here:
> https://patchwork.kernel.org/patch/10220727/

Should this be:

	if (!parent->bi_status && bio->bi_status)
		parent->bi_status = bio->bi_status;

perhaps?

-- 
Jens Axboe

Re: block: be more careful about status in __bio_chain_endio

[Mike Snitzer]

On Fri, Feb 22 2019 at  5:46pm -0500,
Jens Axboe <axboe@kernel.dk> wrote:

> On 2/22/19 2:10 PM, Mike Snitzer wrote:
> > On Thu, Feb 15 2018 at  4:09am -0500,
> > NeilBrown <neilb@suse.com> wrote:
> > 
> >>
> >> If two bios are chained under the one parent (with bio_chain())
> >> it is possible that one will succeed and the other will fail.
> >> __bio_chain_endio must ensure that the failure error status
> >> is reported for the whole, rather than the success.
> >>
> >> It currently tries to be careful, but this test is racy.
> >> If both children finish at the same time, they might both see that
> >> parent->bi_status as zero, and so will assign their own status.
> >> If the assignment to parent->bi_status by the successful bio happens
> >> last, the error status will be lost which can lead to silent data
> >> corruption.
> >>
> >> Instead, __bio_chain_endio should only assign a non-zero status
> >> to parent->bi_status.  There is then no need to test the current
> >> value of parent->bi_status - a test that would be racy anyway.
> >>
> >> Note that this bug hasn't been seen in practice.  It was only discovered
> >> by examination after a similar bug was found in dm.c
> >>
> >> Signed-off-by: NeilBrown <neilb@suse.com>
> >> ---
> >>  block/bio.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/block/bio.c b/block/bio.c
> >> index e1708db48258..ad77140edc6f 100644
> >> --- a/block/bio.c
> >> +++ b/block/bio.c
> >> @@ -312,7 +312,7 @@ static struct bio *__bio_chain_endio(struct bio *bio)
> >>  {
> >>  	struct bio *parent = bio->bi_private;
> >>  
> >> -	if (!parent->bi_status)
> >> +	if (bio->bi_status)
> >>  		parent->bi_status = bio->bi_status;
> >>  	bio_put(bio);
> >>  	return parent;
> >> -- 
> >> 2.14.0.rc0.dirty
> >>
> > 
> > Reviewed-by: Mike Snitzer <snitzer@redhat.com>
> > 
> > Jens, this one slipped through the crack just over a year ago.
> > It is available in patchwork here:
> > https://patchwork.kernel.org/patch/10220727/
> 
> Should this be:
> 
> 	if (!parent->bi_status && bio->bi_status)
> 		parent->bi_status = bio->bi_status;
> 
> perhaps?

Yeap, even better.  Not seeing any reason to have the last error win,
the first in the chain is likely the most important.

Re: block: be more careful about status in __bio_chain_endio

[John Dorminy]

I am perhaps not understanding the intricacies here, or not seeing a
barrier protecting it, so forgive me if I'm off base. I think reading
parent->bi_status here is unsafe.
Consider the following sequence of events on two threads.

Thread 0                                 Thread 1
In __bio_chain_endio:                    In __bio_chain_endio:
[A] Child 0 reads parent->bi_status,
    no error.
                                         Child bio 1 reads parent, no error seen
                                         It sets parent->bi_status to an error
                                         It calls bio_put.
Child bio 0 calls bio_put
[end __bio_chain_endio]                  [end __bio_chain_endio]
                                         In bio_chain_endio(), bio_endio(parent)
                                         is called, calling bio_remaining_done()
                                         which decrements __bi_remaining to 1
                                         and returns false, so no further endio
                                         stuff is done.
In bio_chain_endio(), bio_endio(parent)
is called, calling bio_remaining_done(),
decrementing parent->__bi_remaining to
 0, and continuing to finish parent.
Either for block tracing or for parent's
bi_end_io(), this thread tries to read
parent->bi_status again.

The compiler or the CPU may cache the read from [A], and since there
are no intervening barriers, parent->bi_status is still believed on
thread 0 to be success. Thus the bio may still be falsely believed to
have completed successfully, even though child 1 set an error in it.

Am I missing a subtlety here?

Re: block: be more careful about status in __bio_chain_endio

[Mike Snitzer]

On Fri, Feb 22 2019 at  9:02pm -0500,
John Dorminy <jdorminy@redhat.com> wrote:

> I am perhaps not understanding the intricacies here, or not seeing a
> barrier protecting it, so forgive me if I'm off base. I think reading
> parent->bi_status here is unsafe.
> Consider the following sequence of events on two threads.
> 
> Thread 0                                 Thread 1
> In __bio_chain_endio:                    In __bio_chain_endio:
> [A] Child 0 reads parent->bi_status,
>     no error.
>                                          Child bio 1 reads parent, no error seen
>                                          It sets parent->bi_status to an error
>                                          It calls bio_put.
> Child bio 0 calls bio_put
> [end __bio_chain_endio]                  [end __bio_chain_endio]
>                                          In bio_chain_endio(), bio_endio(parent)
>                                          is called, calling bio_remaining_done()
>                                          which decrements __bi_remaining to 1
>                                          and returns false, so no further endio
>                                          stuff is done.
> In bio_chain_endio(), bio_endio(parent)
> is called, calling bio_remaining_done(),
> decrementing parent->__bi_remaining to
>  0, and continuing to finish parent.
> Either for block tracing or for parent's
> bi_end_io(), this thread tries to read
> parent->bi_status again.
> 
> The compiler or the CPU may cache the read from [A], and since there
> are no intervening barriers, parent->bi_status is still believed on
> thread 0 to be success. Thus the bio may still be falsely believed to
> have completed successfully, even though child 1 set an error in it.
> 
> Am I missing a subtlety here?

Either neilb's original or even Jens' suggestion would be fine though.

>       if (!parent->bi_status && bio->bi_status)
>               parent->bi_status = bio->bi_status;

Even if your scenario did play out (which I agree it looks possible)
it'd just degenerate to neilb's version:

>       if (bio->bi_status)
>               parent->bi_status = bio->bi_status;

Which also accomplishes fixing what Neil originally detailed in his
patch header.

Re: block: be more careful about status in __bio_chain_endio

[John Dorminy]

I'm also worried about the other two versions, though:

memory-barriers.txt#1724:

1724 (*) The compiler is within its rights to invent stores to a variable,

i.e. the compiler is free to decide __bio_chain_endio looks like this:

static struct bio *__bio_chain_endio(struct bio *bio)
{
  struct bio *parent = bio->bi_private;
  blk_status_t tmp = parent->bi_status;
  parent->bi_status = bio->bi_status;
  if (!bio->bi_status)
    parent->bi_status = tmp;
  bio_put(bio);
  return parent;
}

In which case, the read and later store on the two different threads
may overlap in such a way that bio_endio sometimes sees success, even
if one child had an error.

As a result, I believe the setting of parent->bi_status needs to be a
WRITE_ONCE() and the later reading needs to be a READ_ONCE()
[although, since the later reading happens in many different
functions, perhaps some other barrier to make sure all readers get the
correct value is in order.]

Re: block: be more careful about status in __bio_chain_endio

[John Dorminy]

Having studied the code in question more thoroughly, my first email's
scenario can't occur, I believe. bio_put() contains a
atomic_dec_and_test(), which (according to
Documentation/atomic_t.txt), having a return value, are fully ordered
and therefore impose a general memory barrier. A general memory
barrier means that no value set before bio_put() may be observed
afterwards, if I accurately understand
Documentation/memory_barriers.txt. Thus, the scenario I imagined, with
a load from inside __bio_chain_endio() being visible in bi_end_io(),
cannot occur.

However, the second email's scenario, of a compiler making two stores
out of a conditional store, is still accurate according to my
understanding. I continue to believe setting parent->bi_status needs
to be a WRITE_ONCE() and any reading of parent->bi_status before
bio_put() needs to be at least a READ_ONCE(). The last thing in that
email is wrong for the same reason that the first email couldn't
happen: the bio_put() general barrier means later accesses to the
field from a single thread will freshly read the field and thereby not
get an incorrectly cached value.

As a concrete proposal, I believe either of the following work and fix
the race NeilB described, as well as the compiler (or CPU) race I
described:

 -     if (!parent->bi_status)
 -               parent->bi_status = bio->bi_status;
 +     if (bio->bi_status)
 +               WRITE_ONCE(parent->bi_status, bio->bi_status);

--or--

 -     if (!parent->bi_status)
 -               parent->bi_status = bio->bi_status;
 +     if (!READ_ONCE(parent->bi_status) && bio->bi_status)
 +               WRITE_ONCE(parent->bi_status, bio->bi_status);

I believe the second of these might, but is not guaranteed to,
preserve the first error observed in a child; I believe if you want to
definitely save the first error you need an atomic.

Re: block: be more careful about status in __bio_chain_endio

[Christoph Hellwig]

On Tue, Jun 11, 2019 at 10:56:42PM -0400, John Dorminy wrote:
> I believe the second of these might, but is not guaranteed to,
> preserve the first error observed in a child; I believe if you want to
> definitely save the first error you need an atomic.

Is there any reason not to simply use a cmpxchg?  Yes, it is a
relatively expensive operation, but once we are chaining bios we are out
of the super hot path anyway.  We do something similar in xfs and iomap
already.

Re: block: be more careful about status in __bio_chain_endio

[Hannes Reinecke]

On 6/12/19 9:01 AM, Christoph Hellwig wrote:
> On Tue, Jun 11, 2019 at 10:56:42PM -0400, John Dorminy wrote:
>> I believe the second of these might, but is not guaranteed to,
>> preserve the first error observed in a child; I believe if you want to
>> definitely save the first error you need an atomic.
> 
> Is there any reason not to simply use a cmpxchg?  Yes, it is a
> relatively expensive operation, but once we are chaining bios we are out
> of the super hot path anyway.  We do something similar in xfs and iomap
> already.
> 
Agree.
Thing is, we need to check if the parent status is NULL, _and_ the
parent status might be modified asynchronously.
So even a READ_ONCE() wouldn't cut it, as it would tell us that the
parent status _was_ NULL, not that the parent status _is_ NULL by the
time we're setting it.
So cmpxchg() is it.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)