| Message ID | 79600648b662bc8b9e701ff6627986887384585b.1495201975.git.jthumshirn@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, May 19, 2017 at 03:55:31PM +0200, Johannes Thumshirn wrote: > Add a regression test for commit 48ae8484e9fc ("scsi: sg: don't return > bogus Sg_requests"). This is a general protection fault triggered by > syzcaller via issuing bogus read(2)s on the /dev/sg devices. > > Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de> > --- > tests/sg/001 | 47 +++++++++++++++++++++++++++++++++++++++++++++++ > tests/sg/001.out | 2 ++ > 2 files changed, 49 insertions(+) > create mode 100755 tests/sg/001 > create mode 100644 tests/sg/001.out > > diff --git a/tests/sg/001 b/tests/sg/001 > new file mode 100755 > index 000000000000..86430409b6a3 > --- /dev/null > +++ b/tests/sg/001 > @@ -0,0 +1,47 @@ > +#!/bin/bash > +# > +# Regression test for commit 48ae8484e9fc ("scsi: sg: don't return bogus > +# Sg_requests") > +# > +# Copyright (C) 2017 Johannes Thumshirn <jthumshirn@suse.de> > +# > +# This program is free software: you can redistribute it and/or modify > +# it under the terms of the GNU General Public License as published by > +# the Free Software Foundation, either version 3 of the License, or > +# (at your option) any later version. > +# > +# This program is distributed in the hope that it will be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > +# > +# You should have received a copy of the GNU General Public License > +# along with this program. If not, see <http://www.gnu.org/licenses/>. > + > +. common/sg > +. common/scsi_debug > + > +DESCRIPTION="try triggering a kernel GPF with 0 byte SG reads" > +QUICK=1 > + > +requires() { > + _have_program src/sg-001 \ > + && _have_scsi_debug \ > + && _have_scsi_generic > +} > + > + > +test() { > + echo "Running ${TEST_NAME}" > + > + if ! _get_scsi_debug_dev; then > + return 1 > + fi > + > + SG_DEV=$(_get_sg_from_blockdev "$SCSI_DEBUG_NAME") > + timeout -s INT 10s ./src/sg-001 "$SG_DEV" > + > + _put_scsi_debug_dev > + > + echo "Test complete" > +} This looks much better, thanks! One question for you: is there any value in running this on specific test devices (i.e., changing test() to test_device() and using "$TEST_DEV" instead of a scsi-debug device), or would it be a waste of time since it's just exercising generic code?
On 05/22/2017 07:59 PM, Omar Sandoval wrote: > This looks much better, thanks! One question for you: is there any value > in running this on specific test devices (i.e., changing test() to > test_device() and using "$TEST_DEV" instead of a scsi-debug device), or > would it be a waste of time since it's just exercising generic code? That's just generic code. All I need is a SCSI device so I get a /dev/sg device node. One could do a check if $TEST_DEV is a SCSI device and have a fall-back to scsi_debug if it isn't, but I'm not sure if this isn't just a waste of time. Byte, Johannes
On 05/23/2017 12:58 AM, Johannes Thumshirn wrote: > On 05/22/2017 07:59 PM, Omar Sandoval wrote: >> This looks much better, thanks! One question for you: is there any value >> in running this on specific test devices (i.e., changing test() to >> test_device() and using "$TEST_DEV" instead of a scsi-debug device), or >> would it be a waste of time since it's just exercising generic code? > > That's just generic code. All I need is a SCSI device so I get a /dev/sg > device node. > > One could do a check if $TEST_DEV is a SCSI device and have a fall-back > to scsi_debug if it isn't, but I'm not sure if this isn't just a waste > of time. Add some code to the framework that allows you to get the corresponding SG device for a SCSI block device? Make that part of the prepare, skip the test if the block device isn't a SCSI dev.
On 05/23/2017 04:15 PM, Jens Axboe wrote: > Add some code to the framework that allows you to get the corresponding > SG device for a SCSI block device? Make that part of the prepare, skip > the test if the block device isn't a SCSI dev. Well the code is already there (in patch 2/3). I'll pack it into the prepare stage.
On 05/23/2017 08:25 AM, Johannes Thumshirn wrote: > On 05/23/2017 04:15 PM, Jens Axboe wrote: >> Add some code to the framework that allows you to get the corresponding >> SG device for a SCSI block device? Make that part of the prepare, skip >> the test if the block device isn't a SCSI dev. > > Well the code is already there (in patch 2/3). > > I'll pack it into the prepare stage. I tried to look up that commit: 48ae8484e9fc ("scsi: sg: don't return bogus Sg_requests") but that isn't in Linus' tree. Even searched for just the title, still didn't find anything. I'm assuming this is a bug in the sg.c driver, in which case the 2/3 prep and real test case looks fine. For generic device testing, we should just use SG_IO and not bother with sg.c at all. The world would be a better place if we could just get rid of sg.c...
On 05/23/2017 04:39 PM, Jens Axboe wrote: > I tried to look up that commit: > > 48ae8484e9fc ("scsi: sg: don't return bogus Sg_requests") > > but that isn't in Linus' tree. Even searched for just the title, still > didn't find anything. It's queued up in Martin's tree [1]. > > I'm assuming this is a bug in the sg.c driver, in which case the 2/3 > prep and real test case looks fine. For generic device testing, we > should just use SG_IO and not bother with sg.c at all. The world would > be a better place if we could just get rid of sg.c... Agreed. Yes the bug is in the sg.c driver and we did have quite some of these lately thanks to the syzcaller folks. My intention with these tests was to have a place where we can throw in the syzcaller reproducers and run it in nicely Qemu. [1] https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git/commit/?h=4.12/scsi-fixes&id=48ae8484e9fc324b4968d33c585e54bc98e44d61
diff --git a/tests/sg/001 b/tests/sg/001 new file mode 100755 index 000000000000..86430409b6a3 --- /dev/null +++ b/tests/sg/001 @@ -0,0 +1,47 @@ +#!/bin/bash +# +# Regression test for commit 48ae8484e9fc ("scsi: sg: don't return bogus +# Sg_requests") +# +# Copyright (C) 2017 Johannes Thumshirn <jthumshirn@suse.de> +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + +. common/sg +. common/scsi_debug + +DESCRIPTION="try triggering a kernel GPF with 0 byte SG reads" +QUICK=1 + +requires() { + _have_program src/sg-001 \ + && _have_scsi_debug \ + && _have_scsi_generic +} + + +test() { + echo "Running ${TEST_NAME}" + + if ! _get_scsi_debug_dev; then + return 1 + fi + + SG_DEV=$(_get_sg_from_blockdev "$SCSI_DEBUG_NAME") + timeout -s INT 10s ./src/sg-001 "$SG_DEV" + + _put_scsi_debug_dev + + echo "Test complete" +} diff --git a/tests/sg/001.out b/tests/sg/001.out new file mode 100644 index 000000000000..beb4c437dd28 --- /dev/null +++ b/tests/sg/001.out @@ -0,0 +1,2 @@ +Running sg/001 +Test complete
Add a regression test for commit 48ae8484e9fc ("scsi: sg: don't return bogus Sg_requests"). This is a general protection fault triggered by syzcaller via issuing bogus read(2)s on the /dev/sg devices. Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de> --- tests/sg/001 | 47 +++++++++++++++++++++++++++++++++++++++++++++++ tests/sg/001.out | 2 ++ 2 files changed, 49 insertions(+) create mode 100755 tests/sg/001 create mode 100644 tests/sg/001.out