Skip to contentSkip to conversation

fault-inject: avoid unwanted data race to task->fail_nth

PATCH In review Akinobu Mita 3 participants 5 messages Jul 13, 2017 – Aug 1, 2017

Subsystems (3)
Library code Proc filesystem Fault injection support
lore
Share & tools
lore

Akinobu Mita

author maintainer
#0 lore raw

PATCH 1/1 fault-inject: avoid unwanted data race to task->fail_nth

The fault-inject-make-fail-nth-read-write-interface-symmetric.patch in -mm tree allows users to set task->fail_nth for non current task by procfs. On the other hand, the current task's fail_nth is decreased to zero in fault-injection path without any specific locks.

So we need to prevent the task->fail_nth from being unexpected value by data races (for example, setting task->fail_nth to zero while decreasing the current->fail_nth). In this fix, we use READ_ONCE() and WRITE_ONCE() to prevent the compiler from creating unsolicited accesses.

Cc Dmitry Vyukov <[email protected]>
Reported-by Dmitry Vyukov <[email protected]>
Signed-off-by Akinobu Mita <[email protected]>
---
 fs/proc/base.c     | 5 +++--
 lib/fault-inject.c | 7 +++++--
 2 files changed, 8 insertions(+), 4 deletions(-)
fs/proc/base.c +3 -2
index ecc8a25..719c2e9 100644
@@ -1370,7 +1370,7 @@ static ssize_t proc_fail_nth_write(struct file *file, const char __user *buf,
 	task = get_proc_task(file_inode(file));
 	if (!task)
 		return -ESRCH;
-	task->fail_nth = n;
+	WRITE_ONCE(task->fail_nth, n);
 	put_task_struct(task);
 
 	return count;

@@ -1386,7 +1386,8 @@ static ssize_t proc_fail_nth_read(struct file *file, char __user *buf,
 	task = get_proc_task(file_inode(file));
 	if (!task)
 		return -ESRCH;
-	len = snprintf(numbuf, sizeof(numbuf), "%u\n", task->fail_nth);
+	len = snprintf(numbuf, sizeof(numbuf), "%u\n",
+			READ_ONCE(task->fail_nth));
 	len = simple_read_from_buffer(buf, count, ppos, numbuf, len);
 	put_task_struct(task);
lib/fault-inject.c +5 -2
index 09ac73c1..7d315fd 100644
@@ -107,9 +107,12 @@ static inline bool fail_stacktrace(struct fault_attr *attr)
 
 bool should_fail(struct fault_attr *attr, ssize_t size)
 {
-	if (in_task() && current->fail_nth) {
-		if (--current->fail_nth == 0)
+	if (in_task()) {
+		unsigned int fail_nth = READ_ONCE(current->fail_nth);
+
+		if (fail_nth && !WRITE_ONCE(current->fail_nth, fail_nth - 1))
 			goto fail;
+
 		return false;
 	}
Signature · 1 line
2.7.4
#1 lore raw
Quoted 65 lines · Akinobu Mita

The fault-inject-make-fail-nth-read-write-interface-symmetric.patch in -mm tree allows users to set task->fail_nth for non current task by procfs. On the other hand, the current task's fail_nth is decreased to zero in fault-injection path without any specific locks.

So we need to prevent the task->fail_nth from being unexpected value by data races (for example, setting task->fail_nth to zero while decreasing the current->fail_nth). In this fix, we use READ_ONCE() and WRITE_ONCE() to prevent the compiler from creating unsolicited accesses.

Cc Dmitry Vyukov <[email protected]>
Reported-by Dmitry Vyukov <[email protected]>
Signed-off-by Akinobu Mita <[email protected]>

---
fs/proc/base.c | 5 +++--
lib/fault-inject.c | 7 +++++--

2 files changed, 8 insertions(+), 4 deletions(-)
fs/proc/base.c +3 -2
index ecc8a25..719c2e9 100644
@@ -1370,7 +1370,7 @@ static ssize_t proc_fail_nth_write(struct file *file, const char __user *buf,
 	task = get_proc_task(file_inode(file));
 	if (!task)
 		return -ESRCH;
-	task->fail_nth = n;
+	WRITE_ONCE(task->fail_nth, n);
 	put_task_struct(task);
 
 	return count;

@@ -1386,7 +1386,8 @@ static ssize_t proc_fail_nth_read(struct file *file, char __user *buf,
 	task = get_proc_task(file_inode(file));
 	if (!task)
 		return -ESRCH;
-	len = snprintf(numbuf, sizeof(numbuf), "%u\n", task->fail_nth);
+	len = snprintf(numbuf, sizeof(numbuf), "%u\n",
+			READ_ONCE(task->fail_nth));
 	len = simple_read_from_buffer(buf, count, ppos, numbuf, len);
 	put_task_struct(task);
lib/fault-inject.c +5 -2
index 09ac73c1..7d315fd 100644
@@ -107,9 +107,12 @@ static inline bool fail_stacktrace(struct fault_attr *attr)
 
 bool should_fail(struct fault_attr *attr, ssize_t size)
 {
-	if (in_task() && current->fail_nth) {
-		if (--current->fail_nth == 0)
+	if (in_task()) {
+		unsigned int fail_nth = READ_ONCE(current->fail_nth);
+
+		if (fail_nth && !WRITE_ONCE(current->fail_nth, fail_nth - 1))
 			goto fail;
+
 		return false;
 	}
Signature · 1 line
2.7.4

hi

I'm a btrfs developer. I found that fail_make_request didn't produce the expected IO ERROR when running xfstests on linux 4.13-rc1.

That testcase enable fail_make_request by the following commands:

# echo 100 > /sys/kernel/debug/fail_make_request/probability
# echo 2 > /sys/kernel/debug/fail_make_request/times
# echo 0 > /sys/kernel/debug/fail_make_request/verbose
# echo 1 > /sys/block/sda/sda1/make-it-fail
# dd if=/dev/zero of=/dev/sda1 bs=128K count=1 oflag=direct

As I understand it, after applying this patch, I have to write /proc/<dd pid>/file-nth firstly so that dd process can catch the IO ERROR. However, the dd process is so fast that I can't write file-nth.

So, could you tell me how to produce IO ERROR under these circumstances?

A response would be very much appreciated. Thank you for your time.

Signature · 2 lines
Thanks,
Lu
#2 lore raw
Quoted 78 lines · Lu Fengqi
Quoted 61 lines · Akinobu Mita

The fault-inject-make-fail-nth-read-write-interface-symmetric.patch in -mm tree allows users to set task->fail_nth for non current task by procfs. On the other hand, the current task's fail_nth is decreased to zero in fault-injection path without any specific locks.

So we need to prevent the task->fail_nth from being unexpected value by data races (for example, setting task->fail_nth to zero while decreasing the current->fail_nth). In this fix, we use READ_ONCE() and WRITE_ONCE() to prevent the compiler from creating unsolicited accesses.

Cc Dmitry Vyukov <[email protected]>
Reported-by Dmitry Vyukov <[email protected]>
Signed-off-by Akinobu Mita <[email protected]>

---
fs/proc/base.c | 5 +++--
lib/fault-inject.c | 7 +++++--

2 files changed, 8 insertions(+), 4 deletions(-)
fs/proc/base.c +3 -2
index ecc8a25..719c2e9 100644
@@ -1370,7 +1370,7 @@ static ssize_t proc_fail_nth_write(struct file *file, const char __user *buf,
      task = get_proc_task(file_inode(file));
      if (!task)
              return -ESRCH;
-      task->fail_nth = n;
+      WRITE_ONCE(task->fail_nth, n);
      put_task_struct(task);
 
      return count;

@@ -1386,7 +1386,8 @@ static ssize_t proc_fail_nth_read(struct file *file, char __user *buf,
      task = get_proc_task(file_inode(file));
      if (!task)
              return -ESRCH;
-      len = snprintf(numbuf, sizeof(numbuf), "%u\n", task->fail_nth);
+      len = snprintf(numbuf, sizeof(numbuf), "%u\n",
+                      READ_ONCE(task->fail_nth));
      len = simple_read_from_buffer(buf, count, ppos, numbuf, len);
      put_task_struct(task);
lib/fault-inject.c +5 -3
index 09ac73c1..7d315fd 100644
@@ -107,9 +107,12 @@ static inline bool fail_stacktrace(struct fault_attr *attr)
 
 bool should_fail(struct fault_attr *attr, ssize_t size)
 {
-      if (in_task() && current->fail_nth) {
-              if (--current->fail_nth == 0)
+      if (in_task()) {
+              unsigned int fail_nth = READ_ONCE(current->fail_nth);
+
+              if (fail_nth && !WRITE_ONCE(current->fail_nth, fail_nth - 1))
                      goto fail;
+
              return false;
      }
 
--
Quoted 4 lines

2.7.4

hi

I'm a btrfs developer. I found that fail_make_request didn't produce the expected IO ERROR when running xfstests on linux 4.13-rc1.

That testcase enable fail_make_request by the following commands:

# echo 100 > /sys/kernel/debug/fail_make_request/probability
# echo 2 > /sys/kernel/debug/fail_make_request/times
# echo 0 > /sys/kernel/debug/fail_make_request/verbose
# echo 1 > /sys/block/sda/sda1/make-it-fail
# dd if=/dev/zero of=/dev/sda1 bs=128K count=1 oflag=direct
Quoted 5 lines

As I understand it, after applying this patch, I have to write /proc/<dd pid>/file-nth firstly so that dd process can catch the IO ERROR. However, the dd process is so fast that I can't write file-nth.

So, could you tell me how to produce IO ERROR under these circumstances?

Hi,

fail-nth is orthogonal to the existing mechanisms, so if you have a setup that fails all sites with certain probability, that should continue to work.

If you are writing a new facility and want to use fail-nth, then the test process itself needs to cooperate and write fail-nth accordingly. See the original patch for an example of how to do it:
https://groups.google.com/d/msg/syzkaller/DbB4rjYd82s/3MHDwtcqCAAJ

author maintainer
#3 lore raw

2017-08-01 22:45 GMT+09:00 Dmitry Vyukov <[email protected]>:

Quoted 83 lines · Lu Fengqi
Quoted 82 lines · Akinobu Mita
Quoted 61 lines

The fault-inject-make-fail-nth-read-write-interface-symmetric.patch in -mm tree allows users to set task->fail_nth for non current task by procfs. On the other hand, the current task's fail_nth is decreased to zero in fault-injection path without any specific locks.

So we need to prevent the task->fail_nth from being unexpected value by data races (for example, setting task->fail_nth to zero while decreasing the current->fail_nth). In this fix, we use READ_ONCE() and WRITE_ONCE() to prevent the compiler from creating unsolicited accesses.

Cc Dmitry Vyukov <[email protected]>
Reported-by Dmitry Vyukov <[email protected]>
Signed-off-by Akinobu Mita <[email protected]>

---
fs/proc/base.c | 5 +++--
lib/fault-inject.c | 7 +++++--

2 files changed, 8 insertions(+), 4 deletions(-)
fs/proc/base.c +3 -2
index ecc8a25..719c2e9 100644
@@ -1370,7 +1370,7 @@ static ssize_t proc_fail_nth_write(struct file *file, const char __user *buf,
      task = get_proc_task(file_inode(file));
      if (!task)
              return -ESRCH;
-      task->fail_nth = n;
+      WRITE_ONCE(task->fail_nth, n);
      put_task_struct(task);
 
      return count;

@@ -1386,7 +1386,8 @@ static ssize_t proc_fail_nth_read(struct file *file, char __user *buf,
      task = get_proc_task(file_inode(file));
      if (!task)
              return -ESRCH;
-      len = snprintf(numbuf, sizeof(numbuf), "%u\n", task->fail_nth);
+      len = snprintf(numbuf, sizeof(numbuf), "%u\n",
+                      READ_ONCE(task->fail_nth));
      len = simple_read_from_buffer(buf, count, ppos, numbuf, len);
      put_task_struct(task);
lib/fault-inject.c +5 -3
index 09ac73c1..7d315fd 100644
@@ -107,9 +107,12 @@ static inline bool fail_stacktrace(struct fault_attr *attr)
 
 bool should_fail(struct fault_attr *attr, ssize_t size)
 {
-      if (in_task() && current->fail_nth) {
-              if (--current->fail_nth == 0)
+      if (in_task()) {
+              unsigned int fail_nth = READ_ONCE(current->fail_nth);
+
+              if (fail_nth && !WRITE_ONCE(current->fail_nth, fail_nth - 1))
                      goto fail;
+
              return false;
      }
 
--
Quoted 4 lines

2.7.4

hi

I'm a btrfs developer. I found that fail_make_request didn't produce the expected IO ERROR when running xfstests on linux 4.13-rc1.

That testcase enable fail_make_request by the following commands:

# echo 100 > /sys/kernel/debug/fail_make_request/probability
# echo 2 > /sys/kernel/debug/fail_make_request/times
# echo 0 > /sys/kernel/debug/fail_make_request/verbose
# echo 1 > /sys/block/sda/sda1/make-it-fail
# dd if=/dev/zero of=/dev/sda1 bs=128K count=1 oflag=direct

As I understand it, after applying this patch, I have to write /proc/<dd pid>/file-nth firstly so that dd process can catch the IO ERROR. However, the dd process is so fast that I can't write file-nth.

Quoted 7 lines
Quoted 1 line

So, could you tell me how to produce IO ERROR under these circumstances?

Hi,

fail-nth is orthogonal to the existing mechanisms, so if you have a setup that fails all sites with certain probability, that should continue to work.

Lu's setting for fail_make_request is fine before introducing systematic fault injection and they want to inject fail_make_request only.

So I think we need a global parameter to turn on/off the systematic fault injection. (e.g. /sys/kernel/debug/systematic-fault-inject/enable)

Quoted 4 lines

If you are writing a new facility and want to use fail-nth, then the test process itself needs to cooperate and write fail-nth accordingly. See the original patch for an example of how to do it:
https://groups.google.com/d/msg/syzkaller/DbB4rjYd82s/3MHDwtcqCAAJ

author maintainer
#4 lore raw

2017-08-02 0:54 GMT+09:00 Akinobu Mita <[email protected]>:

Quoted 92 lines

2017-08-01 22:45 GMT+09:00 Dmitry Vyukov <[email protected]>:

Quoted 83 lines · Lu Fengqi
Quoted 82 lines · Akinobu Mita
Quoted 61 lines

The fault-inject-make-fail-nth-read-write-interface-symmetric.patch in -mm tree allows users to set task->fail_nth for non current task by procfs. On the other hand, the current task's fail_nth is decreased to zero in fault-injection path without any specific locks.

So we need to prevent the task->fail_nth from being unexpected value by data races (for example, setting task->fail_nth to zero while decreasing the current->fail_nth). In this fix, we use READ_ONCE() and WRITE_ONCE() to prevent the compiler from creating unsolicited accesses.

Cc Dmitry Vyukov <[email protected]>
Reported-by Dmitry Vyukov <[email protected]>
Signed-off-by Akinobu Mita <[email protected]>

---
fs/proc/base.c | 5 +++--
lib/fault-inject.c | 7 +++++--

2 files changed, 8 insertions(+), 4 deletions(-)
fs/proc/base.c +3 -2
index ecc8a25..719c2e9 100644
@@ -1370,7 +1370,7 @@ static ssize_t proc_fail_nth_write(struct file *file, const char __user *buf,
      task = get_proc_task(file_inode(file));
      if (!task)
              return -ESRCH;
-      task->fail_nth = n;
+      WRITE_ONCE(task->fail_nth, n);
      put_task_struct(task);
 
      return count;

@@ -1386,7 +1386,8 @@ static ssize_t proc_fail_nth_read(struct file *file, char __user *buf,
      task = get_proc_task(file_inode(file));
      if (!task)
              return -ESRCH;
-      len = snprintf(numbuf, sizeof(numbuf), "%u\n", task->fail_nth);
+      len = snprintf(numbuf, sizeof(numbuf), "%u\n",
+                      READ_ONCE(task->fail_nth));
      len = simple_read_from_buffer(buf, count, ppos, numbuf, len);
      put_task_struct(task);
lib/fault-inject.c +5 -3
index 09ac73c1..7d315fd 100644
@@ -107,9 +107,12 @@ static inline bool fail_stacktrace(struct fault_attr *attr)
 
 bool should_fail(struct fault_attr *attr, ssize_t size)
 {
-      if (in_task() && current->fail_nth) {
-              if (--current->fail_nth == 0)
+      if (in_task()) {
+              unsigned int fail_nth = READ_ONCE(current->fail_nth);
+
+              if (fail_nth && !WRITE_ONCE(current->fail_nth, fail_nth - 1))
                      goto fail;
+
              return false;
      }
 
--
Quoted 4 lines

2.7.4

hi

I'm a btrfs developer. I found that fail_make_request didn't produce the expected IO ERROR when running xfstests on linux 4.13-rc1.

That testcase enable fail_make_request by the following commands:

# echo 100 > /sys/kernel/debug/fail_make_request/probability
# echo 2 > /sys/kernel/debug/fail_make_request/times
# echo 0 > /sys/kernel/debug/fail_make_request/verbose
# echo 1 > /sys/block/sda/sda1/make-it-fail
# dd if=/dev/zero of=/dev/sda1 bs=128K count=1 oflag=direct

As I understand it, after applying this patch, I have to write /proc/<dd pid>/file-nth firstly so that dd process can catch the IO ERROR. However, the dd process is so fast that I can't write file-nth.

Quoted 7 lines
Quoted 1 line

So, could you tell me how to produce IO ERROR under these circumstances?

Hi,

fail-nth is orthogonal to the existing mechanisms, so if you have a setup that fails all sites with certain probability, that should continue to work.

Quoted 5 lines

Lu's setting for fail_make_request is fine before introducing systematic fault injection and they want to inject fail_make_request only.

So I think we need a global parameter to turn on/off the systematic fault injection. (e.g. /sys/kernel/debug/systematic-fault-inject/enable)

Oops. That is simply a bug in my patch. Correct should_fail() is below.

bool should_fail(struct fault_attr *attr, ssize_t size)

{
        if (in_task()) {
                unsigned int fail_nth = READ_ONCE(current->fail_nth);

                if (fail_nth) {
                        if (!WRITE_ONCE(current->fail_nth, fail_nth - 1))
                                goto fail;

                        return false;
                }
        }

...

Quoted 4 lines
Quoted 4 lines

If you are writing a new facility and want to use fail-nth, then the test process itself needs to cooperate and write fail-nth accordingly. See the original patch for an example of how to do it:
https://groups.google.com/d/msg/syzkaller/DbB4rjYd82s/3MHDwtcqCAAJ