[PATCH blktests 0/3] Add SCSI generic test group

[PATCH blktests 0/3] Add SCSI generic test group

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

I didn't get the TIMEOUT to work (not even with block/001) so I
decided to just require the 'timeout' program in my testcase.

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

 Makefile         |  26 +++-
 common/sg        |  22 +++
 src/.gitignore   |   1 +
 src/Makefile     |  14 ++
 src/sg-001.c     | 430 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/sg/001     |  48 +++++++
 tests/sg/001.out |   2 +
 tests/sg/group   |  40 ++++++
 8 files changed, 582 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

-- 
2.12.0

[PATCH blktests 1/3] Add ability to build test-cases

[Johannes Thumshirn]

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.

Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
---
 Makefile       |  26 +++-
 src/.gitignore |   1 +
 src/Makefile   |  14 ++
 src/sg-001.c   | 430 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 470 insertions(+), 1 deletion(-)
 create mode 100644 src/.gitignore
 create mode 100644 src/Makefile
 create mode 100644 src/sg-001.c

diff --git a/Makefile b/Makefile
index 3a0f0100232c..a70623dd4a52 100644
--- a/Makefile
+++ b/Makefile
@@ -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
diff --git a/src/.gitignore b/src/.gitignore
new file mode 100644
index 000000000000..f543ddb9280f
--- /dev/null
+++ b/src/.gitignore
@@ -0,0 +1 @@
+sg-001
diff --git a/src/Makefile b/src/Makefile
new file mode 100644
index 000000000000..8012c2e542c0
--- /dev/null
+++ b/src/Makefile
@@ -0,0 +1,14 @@
+CC = gcc
+CFLAGS = -O2
+
+TARGETS = sg-001
+
+FILES = $(TARGETS:=.c)
+
+$(TARGETS):
+	@echo "    [CC]    $@"
+	$(Q)$(CC) $@.c -o $@ $(CFLAGS)
+
+clean:
+	@echo "    [CLEAN]  $(notdir $(CURDIR))"
+	$(Q)rm -f $(TARGETS) *.o
diff --git a/src/sg-001.c b/src/sg-001.c
new file mode 100644
index 000000000000..ace85af523b3
--- /dev/null
+++ b/src/sg-001.c
@@ -0,0 +1,430 @@
+// autogenerated by syzkaller (http://github.com/google/syzkaller)
+
+#ifndef __NR_read
+#define __NR_read 0
+#endif
+#ifndef __NR_mmap
+#define __NR_mmap 9
+#endif
+#ifndef __NR_syz_open_dev
+#define __NR_syz_open_dev 1000002
+#endif
+#ifndef __NR_write
+#define __NR_write 1
+#endif
+
+#define _GNU_SOURCE
+
+#include <sys/ioctl.h>
+#include <sys/mman.h>
+#include <sys/mount.h>
+#include <sys/prctl.h>
+#include <sys/resource.h>
+#include <sys/socket.h>
+#include <sys/stat.h>
+#include <sys/syscall.h>
+#include <sys/time.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+
+#include <arpa/inet.h>
+#include <linux/capability.h>
+#include <linux/if.h>
+#include <linux/if_ether.h>
+#include <linux/if_tun.h>
+#include <linux/ip.h>
+#include <linux/kvm.h>
+#include <linux/sched.h>
+#include <linux/tcp.h>
+#include <net/if_arp.h>
+
+#include <assert.h>
+#include <dirent.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <grp.h>
+#include <pthread.h>
+#include <setjmp.h>
+#include <signal.h>
+#include <stdarg.h>
+#include <stdbool.h>
+#include <stddef.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+
+const int kFailStatus = 67;
+const int kErrorStatus = 68;
+const int kRetryStatus = 69;
+
+__attribute__((noreturn)) void doexit(int status)
+{
+  volatile unsigned i;
+  syscall(__NR_exit_group, status);
+  for (i = 0;; i++) {
+  }
+}
+
+__attribute__((noreturn)) void fail(const char* msg, ...)
+{
+  int e = errno;
+  fflush(stdout);
+  va_list args;
+  va_start(args, msg);
+  vfprintf(stderr, msg, args);
+  va_end(args);
+  fprintf(stderr, " (errno %d)\n", e);
+  doexit((e == ENOMEM || e == EAGAIN) ? kRetryStatus : kFailStatus);
+}
+
+__attribute__((noreturn)) void exitf(const char* msg, ...)
+{
+  int e = errno;
+  fflush(stdout);
+  va_list args;
+  va_start(args, msg);
+  vfprintf(stderr, msg, args);
+  va_end(args);
+  fprintf(stderr, " (errno %d)\n", e);
+  doexit(kRetryStatus);
+}
+
+static int flag_debug;
+
+void debug(const char* msg, ...)
+{
+  if (!flag_debug)
+    return;
+  va_list args;
+  va_start(args, msg);
+  vfprintf(stdout, msg, args);
+  va_end(args);
+  fflush(stdout);
+}
+
+__thread int skip_segv;
+__thread jmp_buf segv_env;
+
+static void segv_handler(int sig, siginfo_t* info, void* uctx)
+{
+  uintptr_t addr = (uintptr_t)info->si_addr;
+  const uintptr_t prog_start = 1 << 20;
+  const uintptr_t prog_end = 100 << 20;
+  if (__atomic_load_n(&skip_segv, __ATOMIC_RELAXED) &&
+      (addr < prog_start || addr > prog_end)) {
+    debug("SIGSEGV on %p, skipping\n", addr);
+    _longjmp(segv_env, 1);
+  }
+  debug("SIGSEGV on %p, exiting\n", addr);
+  doexit(sig);
+  for (;;) {
+  }
+}
+
+static void install_segv_handler()
+{
+  struct sigaction sa;
+  memset(&sa, 0, sizeof(sa));
+  sa.sa_sigaction = segv_handler;
+  sa.sa_flags = SA_NODEFER | SA_SIGINFO;
+  sigaction(SIGSEGV, &sa, NULL);
+  sigaction(SIGBUS, &sa, NULL);
+}
+
+#define NONFAILING(...)                                                \
+  {                                                                    \
+    __atomic_fetch_add(&skip_segv, 1, __ATOMIC_SEQ_CST);               \
+    if (_setjmp(segv_env) == 0) {                                      \
+      __VA_ARGS__;                                                     \
+    }                                                                  \
+    __atomic_fetch_sub(&skip_segv, 1, __ATOMIC_SEQ_CST);               \
+  }
+
+#define BITMASK_LEN(type, bf_len) (type)((1ull << (bf_len)) - 1)
+
+#define BITMASK_LEN_OFF(type, bf_off, bf_len)                          \
+  (type)(BITMASK_LEN(type, (bf_len)) << (bf_off))
+
+#define STORE_BY_BITMASK(type, addr, val, bf_off, bf_len)              \
+  if ((bf_off) == 0 && (bf_len) == 0) {                                \
+    *(type*)(addr) = (type)(val);                                      \
+  } else {                                                             \
+    type new_val = *(type*)(addr);                                     \
+    new_val &= ~BITMASK_LEN_OFF(type, (bf_off), (bf_len));             \
+    new_val |= ((type)(val)&BITMASK_LEN(type, (bf_len))) << (bf_off);  \
+    *(type*)(addr) = new_val;                                          \
+  }
+
+struct csum_inet {
+  uint32_t acc;
+};
+
+void csum_inet_init(struct csum_inet* csum)
+{
+  csum->acc = 0;
+}
+
+void csum_inet_update(struct csum_inet* csum, const uint8_t* data,
+                      size_t length)
+{
+  if (length == 0)
+    return;
+
+  size_t i;
+  for (i = 0; i < length - 1; i += 2)
+    csum->acc += *(uint16_t*)&data[i];
+
+  if (length & 1)
+    csum->acc += (uint16_t)data[length - 1];
+
+  while (csum->acc > 0xffff)
+    csum->acc = (csum->acc & 0xffff) + (csum->acc >> 16);
+}
+
+uint16_t csum_inet_digest(struct csum_inet* csum)
+{
+  return ~csum->acc;
+}
+
+static uintptr_t syz_open_dev(uintptr_t a0, uintptr_t a1, uintptr_t a2)
+{
+  if (a0 == 0xc || a0 == 0xb) {
+    char buf[128];
+    sprintf(buf, "/dev/%s/%d:%d", a0 == 0xc ? "char" : "block",
+            (uint8_t)a1, (uint8_t)a2);
+    return open(buf, O_RDWR, 0);
+  } else {
+    char buf[1024];
+    char* hash;
+    NONFAILING(strncpy(buf, (char*)a0, sizeof(buf)));
+    buf[sizeof(buf) - 1] = 0;
+    while ((hash = strchr(buf, '#'))) {
+      *hash = '0' + (char)(a1 % 10);
+      a1 /= 10;
+    }
+    return open(buf, a2, 0);
+  }
+}
+
+static uintptr_t execute_syscall(int nr, uintptr_t a0, uintptr_t a1,
+                                 uintptr_t a2, uintptr_t a3,
+                                 uintptr_t a4, uintptr_t a5,
+                                 uintptr_t a6, uintptr_t a7,
+                                 uintptr_t a8)
+{
+  switch (nr) {
+  default:
+    return syscall(nr, a0, a1, a2, a3, a4, a5);
+  case __NR_syz_open_dev:
+    return syz_open_dev(a0, a1, a2);
+  }
+}
+
+static void setup_main_process()
+{
+  struct sigaction sa;
+  memset(&sa, 0, sizeof(sa));
+  sa.sa_handler = SIG_IGN;
+  syscall(SYS_rt_sigaction, 0x20, &sa, NULL, 8);
+  syscall(SYS_rt_sigaction, 0x21, &sa, NULL, 8);
+  install_segv_handler();
+
+  char tmpdir_template[] = "./syzkaller.XXXXXX";
+  char* tmpdir = mkdtemp(tmpdir_template);
+  if (!tmpdir)
+    fail("failed to mkdtemp");
+  if (chmod(tmpdir, 0777))
+    fail("failed to chmod");
+  if (chdir(tmpdir))
+    fail("failed to chdir");
+}
+
+static void loop();
+
+static void sandbox_common()
+{
+  prctl(PR_SET_PDEATHSIG, SIGKILL, 0, 0, 0);
+  setpgrp();
+  setsid();
+
+  struct rlimit rlim;
+  rlim.rlim_cur = rlim.rlim_max = 128 << 20;
+  setrlimit(RLIMIT_AS, &rlim);
+  rlim.rlim_cur = rlim.rlim_max = 1 << 20;
+  setrlimit(RLIMIT_FSIZE, &rlim);
+  rlim.rlim_cur = rlim.rlim_max = 1 << 20;
+  setrlimit(RLIMIT_STACK, &rlim);
+  rlim.rlim_cur = rlim.rlim_max = 0;
+  setrlimit(RLIMIT_CORE, &rlim);
+
+  unshare(CLONE_NEWNS);
+  unshare(CLONE_NEWIPC);
+  unshare(CLONE_IO);
+}
+
+static int do_sandbox_none(int executor_pid, bool enable_tun)
+{
+  int pid = fork();
+  if (pid)
+    return pid;
+
+  sandbox_common();
+
+  loop();
+  doexit(1);
+}
+
+static void remove_dir(const char* dir)
+{
+  DIR* dp;
+  struct dirent* ep;
+  int iter = 0;
+retry:
+  dp = opendir(dir);
+  if (dp == NULL) {
+    if (errno == EMFILE) {
+      exitf("opendir(%s) failed due to NOFILE, exiting");
+    }
+    exitf("opendir(%s) failed", dir);
+  }
+  while ((ep = readdir(dp))) {
+    if (strcmp(ep->d_name, ".") == 0 || strcmp(ep->d_name, "..") == 0)
+      continue;
+    char filename[FILENAME_MAX];
+    snprintf(filename, sizeof(filename), "%s/%s", dir, ep->d_name);
+    struct stat st;
+    if (lstat(filename, &st))
+      exitf("lstat(%s) failed", filename);
+    if (S_ISDIR(st.st_mode)) {
+      remove_dir(filename);
+      continue;
+    }
+    int i;
+    for (i = 0;; i++) {
+      debug("unlink(%s)\n", filename);
+      if (unlink(filename) == 0)
+        break;
+      if (errno == EROFS) {
+        debug("ignoring EROFS\n");
+        break;
+      }
+      if (errno != EBUSY || i > 100)
+        exitf("unlink(%s) failed", filename);
+      debug("umount(%s)\n", filename);
+      if (umount2(filename, MNT_DETACH))
+        exitf("umount(%s) failed", filename);
+    }
+  }
+  closedir(dp);
+  int i;
+  for (i = 0;; i++) {
+    debug("rmdir(%s)\n", dir);
+    if (rmdir(dir) == 0)
+      break;
+    if (i < 100) {
+      if (errno == EROFS) {
+        debug("ignoring EROFS\n");
+        break;
+      }
+      if (errno == EBUSY) {
+        debug("umount(%s)\n", dir);
+        if (umount2(dir, MNT_DETACH))
+          exitf("umount(%s) failed", dir);
+        continue;
+      }
+      if (errno == ENOTEMPTY) {
+        if (iter < 100) {
+          iter++;
+          goto retry;
+        }
+      }
+    }
+    exitf("rmdir(%s) failed", dir);
+  }
+}
+
+static uint64_t current_time_ms()
+{
+  struct timespec ts;
+
+  if (clock_gettime(CLOCK_MONOTONIC, &ts))
+    fail("clock_gettime failed");
+  return (uint64_t)ts.tv_sec * 1000 + (uint64_t)ts.tv_nsec / 1000000;
+}
+
+static void test();
+
+void loop()
+{
+  int iter;
+  for (iter = 0;; iter++) {
+    char cwdbuf[256];
+    sprintf(cwdbuf, "./%d", iter);
+    if (mkdir(cwdbuf, 0777))
+      fail("failed to mkdir");
+    int pid = fork();
+    if (pid < 0)
+      fail("clone failed");
+    if (pid == 0) {
+      prctl(PR_SET_PDEATHSIG, SIGKILL, 0, 0, 0);
+      setpgrp();
+      if (chdir(cwdbuf))
+        fail("failed to chdir");
+      test();
+      doexit(0);
+    }
+    int status = 0;
+    uint64_t start = current_time_ms();
+    for (;;) {
+      int res = waitpid(-1, &status, __WALL | WNOHANG);
+      if (res == pid)
+        break;
+      usleep(1000);
+      if (current_time_ms() - start > 5 * 1000) {
+        kill(-pid, SIGKILL);
+        kill(pid, SIGKILL);
+        while (waitpid(-1, &status, __WALL) != pid) {
+        }
+        break;
+      }
+    }
+    remove_dir(cwdbuf);
+  }
+}
+
+long r[15];
+void test()
+{
+  memset(r, -1, sizeof(r));
+  r[0] = execute_syscall(__NR_mmap, 0x20000000ul, 0x5000ul, 0x3ul,
+                         0x32ul, 0xfffffffffffffffful, 0x0ul, 0, 0, 0);
+  NONFAILING(memcpy((void*)0x20000000,
+                    "\x2f\x64\x65\x76\x2f\x73\x67\x23\x00", 9));
+  r[2] = execute_syscall(__NR_syz_open_dev, 0x20000000ul, 0x0ul, 0x2ul,
+                         0, 0, 0, 0, 0, 0);
+  NONFAILING(*(uint64_t*)0x20001fdc = (uint64_t)0x0);
+  NONFAILING(*(uint64_t*)0x20001fe4 = (uint64_t)0x0);
+  NONFAILING(*(uint16_t*)0x20001fec = (uint16_t)0x3);
+  NONFAILING(*(uint16_t*)0x20001fee = (uint16_t)0x4);
+  NONFAILING(*(uint32_t*)0x20001ff0 = (uint32_t)0x9);
+  NONFAILING(*(uint64_t*)0x20001ff4 = (uint64_t)0x0);
+  NONFAILING(*(uint64_t*)0x20001ffc = (uint64_t)0x2710);
+  NONFAILING(*(uint16_t*)0x20002004 = (uint16_t)0x7);
+  NONFAILING(*(uint16_t*)0x20002006 = (uint16_t)0x7);
+  NONFAILING(*(uint32_t*)0x20002008 = (uint32_t)0x8);
+  r[13] = execute_syscall(__NR_write, r[2], 0x20001fdcul, 0x30ul, 0, 0,
+                          0, 0, 0, 0);
+  r[14] = execute_syscall(__NR_read, r[2], 0x20002f36ul, 0x0ul, 0, 0, 0,
+                          0, 0, 0);
+}
+int main()
+{
+  setup_main_process();
+  int pid = do_sandbox_none(0, false);
+  int status = 0;
+  while (waitpid(pid, &status, __WALL) != pid) {
+  }
+  return 0;
+}
-- 
2.12.0

[PATCH blktests 2/3] tests/sg: add SCSI generic test grouop

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

Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
---
 common/sg      | 22 ++++++++++++++++++++++
 tests/sg/group | 40 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+)
 create mode 100644 common/sg
 create mode 100644 tests/sg/group

diff --git a/common/sg b/common/sg
new file mode 100644
index 000000000000..30b5089c68f7
--- /dev/null
+++ b/common/sg
@@ -0,0 +1,22 @@
+#!/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
+}
diff --git a/tests/sg/group b/tests/sg/group
new file mode 100644
index 000000000000..55af9a2730eb
--- /dev/null
+++ b/tests/sg/group
@@ -0,0 +1,40 @@
+#!/bin/bash
+#
+# Regression tests for SCSI generic device
+#
+# 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
+
+group_requires() {
+	_have_scsi_generic
+}
+
+# TODO: if this test group has extra requirements for what devices it can be
+# run on, it should define a group_device_requires() function. If tests in this
+# group can be run on the test device, it should return zero. Otherwise, it
+# should return non-zero and set the $SKIP_REASON variable. $TEST_DEV is the
+# full path of the block device (e.g., /dev/nvme0n1 or /dev/sda1), and
+# $TEST_DEV_SYSFS is the sysfs path of the disk (not the partition, e.g.,
+# /sys/block/nvme0n1 or /sys/block/sda).
+#
+# Usually, group_device_requires() just needs to check that the test device is
+# the right type of hardware or supports any necessary features using the
+# _test_dev_foo helpers. If group_device_requires() returns non-zero, all tests
+# in this group will be skipped on that device.
+# group_device_requires() {
+# 	_test_dev_is_foo && _test_dev_supports_bar
+# }
-- 
2.12.0

[PATCH blktests 3/3] sg/001: add regression test for syzcaller generated GPF

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

Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
---
 tests/sg/001     | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 tests/sg/001.out |  2 ++
 2 files changed, 50 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..3a72931d5748
--- /dev/null
+++ b/tests/sg/001
@@ -0,0 +1,48 @@
+#!/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="Regression test for commit 48ae8484e9fc (\"scsi: sg: don't return bogus Sg_requests\")"
+TIMED=1
+QUICK=1
+
+requires() {
+	_have_program src/sg-001 \
+	    _have_program timeout \
+	    && _have_scsi_debug \
+	    && _have_scsi_generic
+}
+
+
+test() {
+	echo "Running ${TEST_NAME}"
+
+	if ! _get_scsi_debug_dev; then
+	    return 1
+	fi
+
+	_divide_timeout 2
+	timeout -s INT 10s ./src/sg-001
+
+	_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
-- 
2.12.0

Re: [PATCH blktests 0/3] Add SCSI generic test group

[Christoph Hellwig]

All SG_IO test should also apply to block device nodes that support
the ioctl..

Re: [PATCH blktests 0/3] Add SCSI generic test group

[Johannes Thumshirn]

On 05/18/2017 03:19 PM, Christoph Hellwig wrote:
> All SG_IO test should also apply to block device nodes that support
> the ioctl..
> 

But these are not necessarily SG_IO tests, are they?

The test included is doesn't hit the SG_IO path in the sg driver, but
the sg_read path.

Of cause we can make a generic sg_io tool but IMHO it's quite convenient
to just throw in the reproducers we get.

And as it exercises the /dev/sg character device I decided to make a new
group.

	Johannes

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +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

Re: [PATCH blktests 2/3] tests/sg: add SCSI generic test grouop

[Omar Sandoval]

On Thu, May 18, 2017 at 02:13:07PM +0200, Johannes Thumshirn wrote:
> 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.
> 
> Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
> ---
>  common/sg      | 22 ++++++++++++++++++++++
>  tests/sg/group | 40 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 62 insertions(+)
>  create mode 100644 common/sg
>  create mode 100644 tests/sg/group
> 
> diff --git a/common/sg b/common/sg
> new file mode 100644
> index 000000000000..30b5089c68f7
> --- /dev/null
> +++ b/common/sg
> +# TODO: if this test group has extra requirements for what devices it can be
> +# run on, it should define a group_device_requires() function. If tests in this
> +# group can be run on the test device, it should return zero. Otherwise, it
> +# should return non-zero and set the $SKIP_REASON variable. $TEST_DEV is the
> +# full path of the block device (e.g., /dev/nvme0n1 or /dev/sda1), and
> +# $TEST_DEV_SYSFS is the sysfs path of the disk (not the partition, e.g.,
> +# /sys/block/nvme0n1 or /sys/block/sda).
> +#
> +# Usually, group_device_requires() just needs to check that the test device is
> +# the right type of hardware or supports any necessary features using the
> +# _test_dev_foo helpers. If group_device_requires() returns non-zero, all tests
> +# in this group will be skipped on that device.
> +# group_device_requires() {
> +# 	_test_dev_is_foo && _test_dev_supports_bar
> +# }

Leftover TODO, I'll remove it when applying. If we add an sg test that
runs on an actual device, we can define group_device_requires().

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

[Omar Sandoval]

On Thu, May 18, 2017 at 02:13:08PM +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.
> 
> Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
> ---
>  tests/sg/001     | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/sg/001.out |  2 ++
>  2 files changed, 50 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..3a72931d5748
> --- /dev/null
> +++ b/tests/sg/001
> @@ -0,0 +1,48 @@
> +#!/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="Regression test for commit 48ae8484e9fc (\"scsi: sg: don't return bogus Sg_requests\")"

This description doesn't really say what the test does. If this test
fails, the person running it should be able to know what the test is
doing without having to decode the syzkaller reproducer. I'd prefer
something like:

DESCRIPTION="do bogus sg reads and writes"

Or something like that, that's my best attempt at understanding the
reproducer :)

> +TIMED=1
> +QUICK=1
> +
> +requires() {
> +	_have_program src/sg-001 \
> +	    _have_program timeout \

timeout is part of coreutils so this isn't necessary.

> +	    && _have_scsi_debug \
> +	    && _have_scsi_generic
> +}
> +
> +
> +test() {
> +	echo "Running ${TEST_NAME}"
> +
> +	if ! _get_scsi_debug_dev; then
> +	    return 1
> +	fi
> +
> +	_divide_timeout 2

You don't use $TIMEOUT in this test, so remove this and the TIMED=1. The
arbitrary timeout is fine for this kind of test, I think.

> +	timeout -s INT 10s ./src/sg-001
> +
> +	_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
> -- 
> 2.12.0
> 

Re: [PATCH blktests 2/3] tests/sg: add SCSI generic test grouop

[Omar Sandoval]

On Thu, May 18, 2017 at 02:06:20PM -0700, Omar Sandoval wrote:
> On Thu, May 18, 2017 at 02:13:07PM +0200, Johannes Thumshirn wrote:
> > 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.
> > 
> > Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
> > ---
> >  common/sg      | 22 ++++++++++++++++++++++
> >  tests/sg/group | 40 ++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 62 insertions(+)
> >  create mode 100644 common/sg
> >  create mode 100644 tests/sg/group
> > 
> > diff --git a/common/sg b/common/sg
> > new file mode 100644
> > index 000000000000..30b5089c68f7
> > --- /dev/null
> > +++ b/common/sg
> > +# TODO: if this test group has extra requirements for what devices it can be
> > +# run on, it should define a group_device_requires() function. If tests in this
> > +# group can be run on the test device, it should return zero. Otherwise, it
> > +# should return non-zero and set the $SKIP_REASON variable. $TEST_DEV is the
> > +# full path of the block device (e.g., /dev/nvme0n1 or /dev/sda1), and
> > +# $TEST_DEV_SYSFS is the sysfs path of the disk (not the partition, e.g.,
> > +# /sys/block/nvme0n1 or /sys/block/sda).
> > +#
> > +# Usually, group_device_requires() just needs to check that the test device is
> > +# the right type of hardware or supports any necessary features using the
> > +# _test_dev_foo helpers. If group_device_requires() returns non-zero, all tests
> > +# in this group will be skipped on that device.
> > +# group_device_requires() {
> > +# 	_test_dev_is_foo && _test_dev_supports_bar
> > +# }
> 
> Leftover TODO, I'll remove it when applying. If we add an sg test that
> runs on an actual device, we can define group_device_requires().

On second though, since I had some comments for patch 3, just fix this
up when you resend.

Re: [PATCH blktests 0/3] Add SCSI generic test group

[Omar Sandoval]

On Thu, May 18, 2017 at 03:29:45PM +0200, Johannes Thumshirn wrote:
> On 05/18/2017 03:19 PM, Christoph Hellwig wrote:
> > All SG_IO test should also apply to block device nodes that support
> > the ioctl..
> > 
> 
> But these are not necessarily SG_IO tests, are they?
> 
> The test included is doesn't hit the SG_IO path in the sg driver, but
> the sg_read path.
> 
> Of cause we can make a generic sg_io tool but IMHO it's quite convenient
> to just throw in the reproducers we get.
> 
> And as it exercises the /dev/sg character device I decided to make a new
> group.
> 
> 	Johannes

Looking at this some more, it seems like the syzkaller reproducer always
bangs on /dev/sg0. How hard would it be to adapt it to run on the sg
device for every test device instead?

Re: [PATCH blktests 0/3] Add SCSI generic test group

[Johannes Thumshirn]

On 05/19/2017 12:46 AM, Omar Sandoval wrote:
> Looking at this some more, it seems like the syzkaller reproducer always
> bangs on /dev/sg0. How hard would it be to adapt it to run on the sg
> device for every test device instead?

Can't be too hard I guess ;-).

Maybe I can even clean it up a bit so it compiles with the "-Wall
-Wextra -Werror" trinity.

I'll send you a v2 once I'm done with it (and all other comments).

Byte,
	Johannes
-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +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

Re: [PATCH blktests 0/3] Add SCSI generic test group

[Christoph Hellwig]

On Thu, May 18, 2017 at 03:29:45PM +0200, Johannes Thumshirn wrote:
> On 05/18/2017 03:19 PM, Christoph Hellwig wrote:
> > All SG_IO test should also apply to block device nodes that support
> > the ioctl..
> > 
> 
> But these are not necessarily SG_IO tests, are they?
> 
> The test included is doesn't hit the SG_IO path in the sg driver, but
> the sg_read path.

Oh, you're right.

> 
> Of cause we can make a generic sg_io tool but IMHO it's quite convenient
> to just throw in the reproducers we get.
> 
> And as it exercises the /dev/sg character device I decided to make a new
> group.

Yeah, makes sense.  And any SG_IO test should be able to run on
both block device nodes and sg nodes.