Skip to contentSkip to conversation

Add SCSI generic test group

PATCH In review Johannes Thumshirn 3 participants 12 messages May 19, 2017 – Jun 7, 2017

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

v2 cover Add SCSI generic test group

Add a test group for the SCSI generic driver and one syzcaller reproducer for this group.

The reprodcuer is distributed as a C program, so the makefile is amended to build C files to be used in the test.

Changes to v1:
* Stripped left over TODO comment
* Modified reproducer to accept a device name
* Fixed Makefile so it can build more than one target

Johannes Thumshirn (3):
Add ability to build test-cases
tests/sg: add SCSI generic test grouop
sg/001: add regression test for syzcaller generated GPF in sg_read
path

 Makefile         |  26 +++-
 common/sg        |  41 ++++++
 src/.gitignore   |   1 +
 src/Makefile     |  16 ++
 src/sg-001.c     | 438 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/sg/001     |  47 ++++++
 tests/sg/001.out |   2 +
 tests/sg/group   |  28 ++++
 8 files changed, 598 insertions(+), 1 deletion(-)
 create mode 100644 common/sg
 create mode 100644 src/.gitignore
 create mode 100644 src/Makefile
 create mode 100644 src/sg-001.c
 create mode 100755 tests/sg/001
 create mode 100644 tests/sg/001.out
 create mode 100644 tests/sg/group
Signature · 1 line
2.12.0
#1 lore raw

PATCH v2 1/3 Add ability to build test-cases

Add the ability to build test cases from C files. This is handy for things like syzcaller reproducers and all other kinds of test binaries.

---
 Makefile       |  26 +++-
 src/.gitignore |   1 +
 src/Makefile   |  16 +++
 src/sg-001.c   | 438 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 480 insertions(+), 1 deletion(-)
 create mode 100644 src/.gitignore
 create mode 100644 src/Makefile
 create mode 100644 src/sg-001.c
Makefile +25 -1
index 3a0f0100232c..a70623dd4a52 100644
@@ -1,7 +1,31 @@
+ifeq ("$(origin V)", "command line")
+	BUILD_VERBOSE = $(V)
+else
+	BUILD_VERBOSE = 0
+endif
+ifndef BUILD_VERBOSE
+	BUILD_VERBOSE = 0
+endif
+
+ifeq ($(BUILD_VERBOSE),1)
+	Q =
+else
+	Q = @
+endif
+
+MAKEOPTS = --no-print-directory Q=$(Q)
+
+SUBDIRS = src
+default:
+	$(Q)$(MAKE) $(MAKEOPTS) -C $(SUBDIRS)
+
+clean:
+	$(Q)$(MAKE) $(MAKEOPTS) -C $(SUBDIRS) clean
+
 all:
 	@echo "Please read README.md"
 
 shellcheck:
 	shellcheck -x -f gcc check new common/* tests/*/[0-9]*[0-9]
 
-.PHONY: all shellcheck
+.PHONY: all shellcheck clean
src/.gitignore new file +1 -0
new file mode 100644
index 000000000000..f543ddb9280f
@@ -0,0 +1 @@
+sg-001
src/Makefile new file +16 -0
new file mode 100644
index 000000000000..d05b625cc7f8
@@ -0,0 +1,16 @@
+CC = gcc
+CFLAGS = -O2
+
+TARGETS = sg-001
+
+FILES = $(TARGETS:=.c)
+
+all: $(TARGETS)
+
+$(TARGETS): $(FILES)
+	@echo "    [CC]    $@"
+	$(Q)$(CC) [email protected] -o $@ $(CFLAGS)
+
+clean:
+	@echo "    [CLEAN]  $(notdir $(CURDIR))"
+	$(Q)rm -f $(TARGETS) *.o
src/sg-001.c new file +438 -0

438 lines not on this page · raw message

Signature · 1 line
2.12.0
#2 lore raw

PATCH v2 2/3 tests/sg: add SCSI generic test grouop

In reply to Johannes Thumshirn

Add a test group for tests of the SCSI generic driver and and functions common to the SCSI generic driver and it's test cases.

---
 common/sg      | 41 +++++++++++++++++++++++++++++++++++++++++
 tests/sg/group | 28 ++++++++++++++++++++++++++++
 2 files changed, 69 insertions(+)
 create mode 100644 common/sg
 create mode 100644 tests/sg/group
common/sg new file +41 -0
new file mode 100644
index 000000000000..123735178d96
@@ -0,0 +1,41 @@
+#!/bin/bash
+#
+# SCSI generic helper functions.
+#
+# Copyright (C) 2017 Johannes Thumshirn
+#
+# 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/>.
+
+_have_scsi_generic() {
+	_have_module sg
+}
+
+_test_dev_is_scsi() {
+
+	local DEV="$1"
+
+	if [[ -d "/sys/block/$DEV/device/scsi_device" ]]; then
+		return 0
+	else
+		return 1
+	fi
+}
+
+_get_sg_from_blockdev()
+{
+	local sg_path="/sys/block/$1/device/scsi_generic/"
+	local sg_dev=$(ls $sg_path | grep -E 'sg[0-9]+') 
+
+	echo "$sg_dev"
+}
tests/sg/group new file +28 -0
new file mode 100644
index 000000000000..3121d799fe25
@@ -0,0 +1,28 @@
+#!/bin/bash
+#
+# Regression tests for SCSI generic device
+#
+# Copyright (C) 2017 Johannes Thumshirn <[email protected]>
+#
+# 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
+
+group_requires() {
+	_have_scsi_generic
+}
+
+group_device_requires() {
+	_test_dev_is_scsi
+}
Signature · 1 line
2.12.0
#3 lore raw

PATCH v2 3/3 sg/001: add regression test for syzcaller generated GPF in sg_read path

In reply to Johannes Thumshirn

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.

---
 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
tests/sg/001 new file +47 -0
new file mode 100755
index 000000000000..86430409b6a3
@@ -0,0 +1,47 @@
+#!/bin/bash
+#
+# Regression test for commit 48ae8484e9fc ("scsi: sg: don't return bogus
+# Sg_requests")
+#
+# Copyright (C) 2017 Johannes Thumshirn <[email protected]>
+#
+# 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"
+}
tests/sg/001.out new file +2 -0
new file mode 100644
index 000000000000..beb4c437dd28
@@ -0,0 +1,2 @@
+Running sg/001
+Test complete
Signature · 1 line
2.12.0
#4 lore raw

Re: [PATCH blktests v2 3/3] sg/001: add regression test for syzcaller generated GPF in sg_read path

Quoted 55 lines · Johannes Thumshirn

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.

---
 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
tests/sg/001 new file +37 -0
new file mode 100755
index 000000000000..86430409b6a3
@@ -0,0 +1,47 @@
+#!/bin/bash
+#
+# Regression test for commit 48ae8484e9fc ("scsi: sg: don't return bogus
+# Sg_requests")
+#
+# Copyright (C) 2017 Johannes Thumshirn <[email protected]>
+#
+# 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

tests/sg/001 line 47

Quoted 10 lines
tests/sg/001 new file +10 -0
+	    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?

#5 lore raw

Re: [PATCH blktests v2 3/3] sg/001: add regression test for syzcaller generated GPF in sg_read path

Quoted 4 lines · Omar Sandoval

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

Signature · 6 lines
Johannes Thumshirn                                          Storage
[email protected]                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
maintainer
#6 lore raw

Re: [PATCH blktests v2 3/3] sg/001: add regression test for syzcaller generated GPF in sg_read path

Quoted 6 lines · Johannes Thumshirn
Quoted 4 lines · Omar Sandoval

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?

Quoted 6 lines

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.

Signature · 1 line
Jens Axboe
#7 lore raw

Re: [PATCH blktests v2 3/3] sg/001: add regression test for syzcaller generated GPF in sg_read path

Quoted 3 lines · Jens Axboe

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.

Signature · 6 lines
Johannes Thumshirn                                          Storage
[email protected]                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
maintainer
#8 lore raw

Re: [PATCH blktests v2 3/3] sg/001: add regression test for syzcaller generated GPF in sg_read path

Quoted 8 lines · Johannes Thumshirn
Quoted 3 lines · Jens Axboe

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...

Signature · 1 line
Jens Axboe
#9 lore raw

Re: [PATCH blktests v2 3/3] sg/001: add regression test for syzcaller generated GPF in sg_read path

Quoted 6 lines · Jens Axboe

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].

Quoted 5 lines

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

Signature · 6 lines
Johannes Thumshirn                                          Storage
[email protected]                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
#10 lore raw
In reply to Johannes Thumshirn
Quoted 11 lines · Johannes Thumshirn

Add a test group for the SCSI generic driver and one syzcaller reproducer for this group.

The reprodcuer is distributed as a C program, so the makefile is amended to build C files to be used in the test.

Changes to v1:
* Stripped left over TODO comment
* Modified reproducer to accept a device name
* Fixed Makefile so it can build more than one target

Quoted 5 lines

Johannes Thumshirn (3):
Add ability to build test-cases
tests/sg: add SCSI generic test grouop
sg/001: add regression test for syzcaller generated GPF in sg_read
path

Hey, Johannes,

Sorry for the delay, I was chasing down a few Btrfs bugs for the last couple of weeks. I applied these with some small changes. I stuck with running syzkaller on the scsi-debug device like you had here, we can revisit that for future tests if it makes sense to run them on a real device. Thanks!

#11 lore raw
Quoted 5 lines · Omar Sandoval

Sorry for the delay, I was chasing down a few Btrfs bugs for the last couple of weeks. I applied these with some small changes. I stuck with running syzkaller on the scsi-debug device like you had here, we can revisit that for future tests if it makes sense to run them on a real device. Thanks!

Yes I wanted to go to it tomorrow and send you a v3, I got stuck in NVMe ;-). Thanks for applying it anyways, I'll send a follow up.

Johannes

Signature · 6 lines
Johannes Thumshirn                                          Storage
[email protected]                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850