| Message ID | 20240826120205.5506-1-liwang@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ioctl_loop06: no validate block size | expand |
On Mon, Aug 26, 2024 at 2:02 PM Li Wang <liwang@redhat.com> wrote: > > Since commit 9423c653fe6110 ("loop: Don't bother validating blocksize") kernel This commit above says "The block queue limits validation does this for us now.", shouldn't that still catch the errors? > drop validating blocksize for both loop_configure and loop_set_block_size so > that set large block size succeeds. > > Error log: > 12 ioctl_loop06.c:76: TINFO: Using LOOP_SET_BLOCK_SIZE with arg > PAGE_SIZE > 13 ioctl_loop06.c:59: TFAIL: Set block size succeed unexpectedly > ... > 18 ioctl_loop06.c:76: TINFO: Using LOOP_CONFIGURE with block_size > PAGE_SIZE > 19 ioctl_loop06.c:59: TFAIL: Set block size succeed unexpectedly > > Signed-off-by: Li Wang <liwang@redhat.com> > --- > testcases/kernel/syscalls/ioctl/ioctl_loop06.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/testcases/kernel/syscalls/ioctl/ioctl_loop06.c b/testcases/kernel/syscalls/ioctl/ioctl_loop06.c > index 317f693a0..4aacd284a 100644 > --- a/testcases/kernel/syscalls/ioctl/ioctl_loop06.c > +++ b/testcases/kernel/syscalls/ioctl/ioctl_loop06.c > @@ -23,6 +23,7 @@ static char dev_path[1024]; > static int dev_num, dev_fd, file_fd, attach_flag, loop_configure_sup = 1; > static unsigned int invalid_value, half_value, unalign_value; > static struct loop_config loopconfig; > +static int novalidate_blocksize = 0; > > static struct tcase { > unsigned int *setvalue; > @@ -74,6 +75,11 @@ static void run(unsigned int n) > struct tcase *tc = &tcases[n]; > > tst_res(TINFO, "%s", tc->message); > + if ((*(tc->setvalue) == invalid_value) && novalidate_blocksize) { > + tst_res(TCONF, "Kernel doesn't validate block size, skip invalid value test"); > + return; > + } > + > if (tc->ioctl_flag == LOOP_SET_BLOCK_SIZE) { > if (!attach_flag) { > tst_attach_device(dev_path, "test.img"); > @@ -126,6 +132,9 @@ static void setup(void) > return; > } > loopconfig.fd = file_fd; > + > + if ((tst_kvercmp(6, 11, 0)) >= 0) > + novalidate_blocksize = 1; > } > > static void cleanup(void) > -- > 2.46.0 >
Hi All, On Mon, Aug 26, 2024 at 8:02 PM Li Wang <liwang@redhat.com> wrote: > Since commit 9423c653fe6110 ("loop: Don't bother validating blocksize") > kernel > drop validating blocksize for both loop_configure and loop_set_block_size > so > that set large block size succeeds. > > Error log: > 12 ioctl_loop06.c:76: TINFO: Using LOOP_SET_BLOCK_SIZE with arg > > PAGE_SIZE > 13 ioctl_loop06.c:59: TFAIL: Set block size succeed unexpectedly > ... > 18 ioctl_loop06.c:76: TINFO: Using LOOP_CONFIGURE with block_size > > PAGE_SIZE > 19 ioctl_loop06.c:59: TFAIL: Set block size succeed unexpectedly > Hmm, maybe I was wrong here, the commit says "The block queue limits validation does this for us now." which indicates the validation is still on. So the test failure probably means a kernel bug but not a test problem. CC block devs to give some advice. > > Signed-off-by: Li Wang <liwang@redhat.com> > --- > testcases/kernel/syscalls/ioctl/ioctl_loop06.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/testcases/kernel/syscalls/ioctl/ioctl_loop06.c > b/testcases/kernel/syscalls/ioctl/ioctl_loop06.c > index 317f693a0..4aacd284a 100644 > --- a/testcases/kernel/syscalls/ioctl/ioctl_loop06.c > +++ b/testcases/kernel/syscalls/ioctl/ioctl_loop06.c > @@ -23,6 +23,7 @@ static char dev_path[1024]; > static int dev_num, dev_fd, file_fd, attach_flag, loop_configure_sup = 1; > static unsigned int invalid_value, half_value, unalign_value; > static struct loop_config loopconfig; > +static int novalidate_blocksize = 0; > > static struct tcase { > unsigned int *setvalue; > @@ -74,6 +75,11 @@ static void run(unsigned int n) > struct tcase *tc = &tcases[n]; > > tst_res(TINFO, "%s", tc->message); > + if ((*(tc->setvalue) == invalid_value) && novalidate_blocksize) { > + tst_res(TCONF, "Kernel doesn't validate block size, skip > invalid value test"); > + return; > + } > + > if (tc->ioctl_flag == LOOP_SET_BLOCK_SIZE) { > if (!attach_flag) { > tst_attach_device(dev_path, "test.img"); > @@ -126,6 +132,9 @@ static void setup(void) > return; > } > loopconfig.fd = file_fd; > + > + if ((tst_kvercmp(6, 11, 0)) >= 0) > + novalidate_blocksize = 1; > } > > static void cleanup(void) > -- > 2.46.0 > > > -- > Mailing list info: https://lists.linux.it/listinfo/ltp > >
On Mon, Aug 26, 2024 at 8:45 PM Jan Stancek <jstancek@redhat.com> wrote: > On Mon, Aug 26, 2024 at 2:02 PM Li Wang <liwang@redhat.com> wrote: > > > > Since commit 9423c653fe6110 ("loop: Don't bother validating blocksize") > kernel > > This commit above says "The block queue limits validation does this > for us now.", > shouldn't that still catch the errors? > Yes, I overlooked that before, after removing the blk_validate_block_size() from kernel side, test ioctl_loop06 always failed on ppc64le/aarch64. Weirdly, x86_64 and s390x work well. I guess it's probably a kernel new bug. > > drop validating blocksize for both loop_configure and > loop_set_block_size so > > that set large block size succeeds. > > > > Error log: > > 12 ioctl_loop06.c:76: TINFO: Using LOOP_SET_BLOCK_SIZE with arg > > PAGE_SIZE > > 13 ioctl_loop06.c:59: TFAIL: Set block size succeed unexpectedly > > ... > > 18 ioctl_loop06.c:76: TINFO: Using LOOP_CONFIGURE with block_size > > PAGE_SIZE > > 19 ioctl_loop06.c:59: TFAIL: Set block size succeed unexpectedly > > > > Signed-off-by: Li Wang <liwang@redhat.com> > > --- > > testcases/kernel/syscalls/ioctl/ioctl_loop06.c | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/testcases/kernel/syscalls/ioctl/ioctl_loop06.c > b/testcases/kernel/syscalls/ioctl/ioctl_loop06.c > > index 317f693a0..4aacd284a 100644 > > --- a/testcases/kernel/syscalls/ioctl/ioctl_loop06.c > > +++ b/testcases/kernel/syscalls/ioctl/ioctl_loop06.c > > @@ -23,6 +23,7 @@ static char dev_path[1024]; > > static int dev_num, dev_fd, file_fd, attach_flag, loop_configure_sup = > 1; > > static unsigned int invalid_value, half_value, unalign_value; > > static struct loop_config loopconfig; > > +static int novalidate_blocksize = 0; > > > > static struct tcase { > > unsigned int *setvalue; > > @@ -74,6 +75,11 @@ static void run(unsigned int n) > > struct tcase *tc = &tcases[n]; > > > > tst_res(TINFO, "%s", tc->message); > > + if ((*(tc->setvalue) == invalid_value) && novalidate_blocksize) { > > + tst_res(TCONF, "Kernel doesn't validate block size, skip > invalid value test"); > > + return; > > + } > > + > > if (tc->ioctl_flag == LOOP_SET_BLOCK_SIZE) { > > if (!attach_flag) { > > tst_attach_device(dev_path, "test.img"); > > @@ -126,6 +132,9 @@ static void setup(void) > > return; > > } > > loopconfig.fd = file_fd; > > + > > + if ((tst_kvercmp(6, 11, 0)) >= 0) > > + novalidate_blocksize = 1; > > } > > > > static void cleanup(void) > > -- > > 2.46.0 > > > >
On Mon, Aug 26, 2024 at 2:46 PM Li Wang <liwang@redhat.com> wrote: > > Hi All, > > On Mon, Aug 26, 2024 at 8:02 PM Li Wang <liwang@redhat.com> wrote: > > > Since commit 9423c653fe6110 ("loop: Don't bother validating blocksize") > > kernel > > drop validating blocksize for both loop_configure and loop_set_block_size > > so > > that set large block size succeeds. > > > > Error log: > > 12 ioctl_loop06.c:76: TINFO: Using LOOP_SET_BLOCK_SIZE with arg > > > PAGE_SIZE > > 13 ioctl_loop06.c:59: TFAIL: Set block size succeed unexpectedly > > ... > > 18 ioctl_loop06.c:76: TINFO: Using LOOP_CONFIGURE with block_size > > > PAGE_SIZE > > 19 ioctl_loop06.c:59: TFAIL: Set block size succeed unexpectedly > > > > Hmm, maybe I was wrong here, the commit says > > "The block queue limits validation does this for us now." > > which indicates the validation is still on. > > So the test failure probably means a kernel bug but not a test problem. Before the patch, blk_validate_block_size() did validate original value as unsigned long, after patch it's validated after cast to unsigned short. In LTP thread you mentioned it failed on ppc64le/aarch64 and worked on x86_64 and s390x. Is it by chance now failing only on kernels with 64k page size? (Test attempts to set block size to 2*page size.) > > CC block devs to give some advice. > > > > > > > Signed-off-by: Li Wang <liwang@redhat.com> > > --- > > testcases/kernel/syscalls/ioctl/ioctl_loop06.c | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/testcases/kernel/syscalls/ioctl/ioctl_loop06.c > > b/testcases/kernel/syscalls/ioctl/ioctl_loop06.c > > index 317f693a0..4aacd284a 100644 > > --- a/testcases/kernel/syscalls/ioctl/ioctl_loop06.c > > +++ b/testcases/kernel/syscalls/ioctl/ioctl_loop06.c > > @@ -23,6 +23,7 @@ static char dev_path[1024]; > > static int dev_num, dev_fd, file_fd, attach_flag, loop_configure_sup = 1; > > static unsigned int invalid_value, half_value, unalign_value; > > static struct loop_config loopconfig; > > +static int novalidate_blocksize = 0; > > > > static struct tcase { > > unsigned int *setvalue; > > @@ -74,6 +75,11 @@ static void run(unsigned int n) > > struct tcase *tc = &tcases[n]; > > > > tst_res(TINFO, "%s", tc->message); > > + if ((*(tc->setvalue) == invalid_value) && novalidate_blocksize) { > > + tst_res(TCONF, "Kernel doesn't validate block size, skip > > invalid value test"); > > + return; > > + } > > + > > if (tc->ioctl_flag == LOOP_SET_BLOCK_SIZE) { > > if (!attach_flag) { > > tst_attach_device(dev_path, "test.img"); > > @@ -126,6 +132,9 @@ static void setup(void) > > return; > > } > > loopconfig.fd = file_fd; > > + > > + if ((tst_kvercmp(6, 11, 0)) >= 0) > > + novalidate_blocksize = 1; > > } > > > > static void cleanup(void) > > -- > > 2.46.0 > > > > > > -- > > Mailing list info: https://lists.linux.it/listinfo/ltp > > > > > > -- > Regards, > Li Wang > > -- > Mailing list info: https://lists.linux.it/listinfo/ltp
On Mon, Aug 26, 2024 at 9:15 PM Jan Stancek <jstancek@redhat.com> wrote: > On Mon, Aug 26, 2024 at 2:46 PM Li Wang <liwang@redhat.com> wrote: > > > > Hi All, > > > > On Mon, Aug 26, 2024 at 8:02 PM Li Wang <liwang@redhat.com> wrote: > > > > > Since commit 9423c653fe6110 ("loop: Don't bother validating blocksize") > > > kernel > > > drop validating blocksize for both loop_configure and > loop_set_block_size > > > so > > > that set large block size succeeds. > > > > > > Error log: > > > 12 ioctl_loop06.c:76: TINFO: Using LOOP_SET_BLOCK_SIZE with arg > > > > PAGE_SIZE > > > 13 ioctl_loop06.c:59: TFAIL: Set block size succeed unexpectedly > > > ... > > > 18 ioctl_loop06.c:76: TINFO: Using LOOP_CONFIGURE with block_size > > > > PAGE_SIZE > > > 19 ioctl_loop06.c:59: TFAIL: Set block size succeed unexpectedly > > > > > > > Hmm, maybe I was wrong here, the commit says > > > > "The block queue limits validation does this for us now." > > > > which indicates the validation is still on. > > > > So the test failure probably means a kernel bug but not a test problem. > > Before the patch, blk_validate_block_size() did validate original > value as unsigned long, > after patch it's validated after cast to unsigned short. > > In LTP thread you mentioned it failed on ppc64le/aarch64 and worked on > x86_64 and s390x. > Is it by chance now failing only on kernels with 64k page size? > Right, I checked the automation jobs, all recent (v6.11) aarch64-kernel-64k reports that fail. > (Test attempts to set block size to 2*page size.) > > > > > CC block devs to give some advice. > > > > > > > > > > > > Signed-off-by: Li Wang <liwang@redhat.com> > > > --- > > > testcases/kernel/syscalls/ioctl/ioctl_loop06.c | 9 +++++++++ > > > 1 file changed, 9 insertions(+) > > > > > > diff --git a/testcases/kernel/syscalls/ioctl/ioctl_loop06.c > > > b/testcases/kernel/syscalls/ioctl/ioctl_loop06.c > > > index 317f693a0..4aacd284a 100644 > > > --- a/testcases/kernel/syscalls/ioctl/ioctl_loop06.c > > > +++ b/testcases/kernel/syscalls/ioctl/ioctl_loop06.c > > > @@ -23,6 +23,7 @@ static char dev_path[1024]; > > > static int dev_num, dev_fd, file_fd, attach_flag, loop_configure_sup > = 1; > > > static unsigned int invalid_value, half_value, unalign_value; > > > static struct loop_config loopconfig; > > > +static int novalidate_blocksize = 0; > > > > > > static struct tcase { > > > unsigned int *setvalue; > > > @@ -74,6 +75,11 @@ static void run(unsigned int n) > > > struct tcase *tc = &tcases[n]; > > > > > > tst_res(TINFO, "%s", tc->message); > > > + if ((*(tc->setvalue) == invalid_value) && > novalidate_blocksize) { > > > + tst_res(TCONF, "Kernel doesn't validate block size, > skip > > > invalid value test"); > > > + return; > > > + } > > > + > > > if (tc->ioctl_flag == LOOP_SET_BLOCK_SIZE) { > > > if (!attach_flag) { > > > tst_attach_device(dev_path, "test.img"); > > > @@ -126,6 +132,9 @@ static void setup(void) > > > return; > > > } > > > loopconfig.fd = file_fd; > > > + > > > + if ((tst_kvercmp(6, 11, 0)) >= 0) > > > + novalidate_blocksize = 1; > > > } > > > > > > static void cleanup(void) > > > -- > > > 2.46.0 > > > > > > > > > -- > > > Mailing list info: https://lists.linux.it/listinfo/ltp > > > > > > > > > > -- > > Regards, > > Li Wang > > > > -- > > Mailing list info: https://lists.linux.it/listinfo/ltp > >
On Mon, Aug 26, 2024 at 9:21 PM Li Wang <liwang@redhat.com> wrote: > > > On Mon, Aug 26, 2024 at 9:15 PM Jan Stancek <jstancek@redhat.com> wrote: > >> On Mon, Aug 26, 2024 at 2:46 PM Li Wang <liwang@redhat.com> wrote: >> > >> > Hi All, >> > >> > On Mon, Aug 26, 2024 at 8:02 PM Li Wang <liwang@redhat.com> wrote: >> > >> > > Since commit 9423c653fe6110 ("loop: Don't bother validating >> blocksize") >> > > kernel >> > > drop validating blocksize for both loop_configure and >> loop_set_block_size >> > > so >> > > that set large block size succeeds. >> > > >> > > Error log: >> > > 12 ioctl_loop06.c:76: TINFO: Using LOOP_SET_BLOCK_SIZE with arg > >> > > PAGE_SIZE >> > > 13 ioctl_loop06.c:59: TFAIL: Set block size succeed unexpectedly >> > > ... >> > > 18 ioctl_loop06.c:76: TINFO: Using LOOP_CONFIGURE with block_size > >> > > PAGE_SIZE >> > > 19 ioctl_loop06.c:59: TFAIL: Set block size succeed unexpectedly >> > > >> > >> > Hmm, maybe I was wrong here, the commit says >> > >> > "The block queue limits validation does this for us now." >> > >> > which indicates the validation is still on. >> > >> > So the test failure probably means a kernel bug but not a test problem. >> >> Before the patch, blk_validate_block_size() did validate original >> value as unsigned long, >> after patch it's validated after cast to unsigned short. >> >> In LTP thread you mentioned it failed on ppc64le/aarch64 and worked on >> x86_64 and s390x. >> Is it by chance now failing only on kernels with 64k page size? >> > > Right, I checked the automation jobs, all recent (v6.11) > aarch64-kernel-64k reports that fail. > And I verified your inference. Once I lower the invalid_value to 'pg_size + 1' the test passed on my ppc64le platform. And looking at the code, seems from loop_reconfigure_limits() cast to 'unsined short' caused that. That's why another patch John submitted fe3d508ba95bc63a ("block: Validate logical block size in blk_validate_limits()") doesn't make the behavior consistent on all arches (with different page size). > > >> (Test attempts to set block size to 2*page size.) >> >> > >> > CC block devs to give some advice. >> > >> > >> > >> > > >> > > Signed-off-by: Li Wang <liwang@redhat.com> >> > > --- >> > > testcases/kernel/syscalls/ioctl/ioctl_loop06.c | 9 +++++++++ >> > > 1 file changed, 9 insertions(+) >> > > >> > > diff --git a/testcases/kernel/syscalls/ioctl/ioctl_loop06.c >> > > b/testcases/kernel/syscalls/ioctl/ioctl_loop06.c >> > > index 317f693a0..4aacd284a 100644 >> > > --- a/testcases/kernel/syscalls/ioctl/ioctl_loop06.c >> > > +++ b/testcases/kernel/syscalls/ioctl/ioctl_loop06.c >> > > @@ -23,6 +23,7 @@ static char dev_path[1024]; >> > > static int dev_num, dev_fd, file_fd, attach_flag, loop_configure_sup >> = 1; >> > > static unsigned int invalid_value, half_value, unalign_value; >> > > static struct loop_config loopconfig; >> > > +static int novalidate_blocksize = 0; >> > > >> > > static struct tcase { >> > > unsigned int *setvalue; >> > > @@ -74,6 +75,11 @@ static void run(unsigned int n) >> > > struct tcase *tc = &tcases[n]; >> > > >> > > tst_res(TINFO, "%s", tc->message); >> > > + if ((*(tc->setvalue) == invalid_value) && >> novalidate_blocksize) { >> > > + tst_res(TCONF, "Kernel doesn't validate block size, >> skip >> > > invalid value test"); >> > > + return; >> > > + } >> > > + >> > > if (tc->ioctl_flag == LOOP_SET_BLOCK_SIZE) { >> > > if (!attach_flag) { >> > > tst_attach_device(dev_path, "test.img"); >> > > @@ -126,6 +132,9 @@ static void setup(void) >> > > return; >> > > } >> > > loopconfig.fd = file_fd; >> > > + >> > > + if ((tst_kvercmp(6, 11, 0)) >= 0) >> > > + novalidate_blocksize = 1; >> > > } >> > > >> > > static void cleanup(void) >> > > -- >> > > 2.46.0 >> > > >> > > >> > > -- >> > > Mailing list info: https://lists.linux.it/listinfo/ltp >> > > >> > > >> > >> > -- >> > Regards, >> > Li Wang >> > >> > -- >> > Mailing list info: https://lists.linux.it/listinfo/ltp >> >> > > -- > Regards, > Li Wang >
diff --git a/testcases/kernel/syscalls/ioctl/ioctl_loop06.c b/testcases/kernel/syscalls/ioctl/ioctl_loop06.c index 317f693a0..4aacd284a 100644 --- a/testcases/kernel/syscalls/ioctl/ioctl_loop06.c +++ b/testcases/kernel/syscalls/ioctl/ioctl_loop06.c @@ -23,6 +23,7 @@ static char dev_path[1024]; static int dev_num, dev_fd, file_fd, attach_flag, loop_configure_sup = 1; static unsigned int invalid_value, half_value, unalign_value; static struct loop_config loopconfig; +static int novalidate_blocksize = 0; static struct tcase { unsigned int *setvalue; @@ -74,6 +75,11 @@ static void run(unsigned int n) struct tcase *tc = &tcases[n]; tst_res(TINFO, "%s", tc->message); + if ((*(tc->setvalue) == invalid_value) && novalidate_blocksize) { + tst_res(TCONF, "Kernel doesn't validate block size, skip invalid value test"); + return; + } + if (tc->ioctl_flag == LOOP_SET_BLOCK_SIZE) { if (!attach_flag) { tst_attach_device(dev_path, "test.img"); @@ -126,6 +132,9 @@ static void setup(void) return; } loopconfig.fd = file_fd; + + if ((tst_kvercmp(6, 11, 0)) >= 0) + novalidate_blocksize = 1; } static void cleanup(void)
Since commit 9423c653fe6110 ("loop: Don't bother validating blocksize") kernel drop validating blocksize for both loop_configure and loop_set_block_size so that set large block size succeeds. Error log: 12 ioctl_loop06.c:76: TINFO: Using LOOP_SET_BLOCK_SIZE with arg > PAGE_SIZE 13 ioctl_loop06.c:59: TFAIL: Set block size succeed unexpectedly ... 18 ioctl_loop06.c:76: TINFO: Using LOOP_CONFIGURE with block_size > PAGE_SIZE 19 ioctl_loop06.c:59: TFAIL: Set block size succeed unexpectedly Signed-off-by: Li Wang <liwang@redhat.com> --- testcases/kernel/syscalls/ioctl/ioctl_loop06.c | 9 +++++++++ 1 file changed, 9 insertions(+)