lkml.org 
[lkml]   [2018]   [Feb]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: DM Regression in 4.16-rc1 - read() returns data when it shouldn't
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
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2018-02-14 21:40    [W:0.101 / U:0.052 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site