fault-inject: improve fail-nth interface

The read interface for fail-nth looks a bit odd. Read from this file returns "NYYYY..." or "YYYYY..." (this makes me surprise when cat this file). Because there is no EOF condition. The first character indicates current->fail_nth is zero or not, and then current->fail_nth is reset to zero.

Just returning task->fail_nth value is more natural to understand.

Cc Dmitry Vyukov <[email protected]>

Signed-off-by Akinobu Mita <[email protected]>

---
 Documentation/fault-injection/fault-injection.txt | 13 +++++++------
 fs/proc/base.c                                    | 14 ++++++--------
 2 files changed, 13 insertions(+), 14 deletions(-)

Documentation/fault-injection/fault-injection.txt +7 -6

index a321905..370ddcb 100644
@@ -139,9 +139,9 @@ o proc entries
 - /proc/self/task/<current-tid>/fail-nth:
 
        Write to this file of integer N makes N-th call in the task fail.
-       Read from this file returns a single char 'Y' or 'N'
-       that says if the fault setup with a previous write to this file was
-       injected or not, and disables the fault if it wasn't yet injected.
+       Read from this file returns a integer value. A value of '0' indicates
+       that the fault setup with a previous write to this file was injected.
+       A positive integer N indicates that the fault wasn't yet injected.
        Note that this file enables all types of faults (slab, futex, etc).
        This setting takes precedence over all other generic debugfs settings
        like probability, interval, times, etc. But per-capability settings

@@ -325,13 +325,14 @@ int main()
                write(fail_nth, buf, strlen(buf));
                res = socketpair(AF_LOCAL, SOCK_STREAM, 0, fds);
                err = errno;
-               read(fail_nth, buf, 1);
+               pread(fail_nth, buf, sizeof(buf), 0);
                if (res == 0) {
                        close(fds[0]);
                        close(fds[1]);
                }
-               printf("%d-th fault %c: res=%d/%d\n", i, buf[0], res, err);
-               if (buf[0] != 'Y')
+               printf("%d-th fault %c: res=%d/%d\n", i, atoi(buf) ? 'N' : 'Y',
+                       res, err);
+               if (atoi(buf))
                        break;
        }
        return 0;

fs/proc/base.c +2 -1

index 42c52e2..9d14215 100644
@@ -1383,7 +1383,8 @@ static ssize_t proc_fail_nth_read(struct file *file, char __user *buf,
                                  size_t count, loff_t *ppos)
 {
        struct task_struct *task;
-       int err;
+       char numbuf[PROC_NUMBUF];
+       ssize_t len;
 
        task = get_proc_task(file_inode(file));
        if (!task)

@@ -1391,13 +1392,10 @@ static ssize_t proc_fail_nth_read(struct file *file, char __user *buf,
        put_task_struct(task);

fs/proc/base.c +3 -1

+       len = simple_read_from_buffer(buf, count, ppos, numbuf, len);
+
+       return len;
 }
 
 static const struct file_operations proc_fail_nth_operations = {
--

2.7.4

Quoted 73 lines · Akinobu Mita

The read interface for fail-nth looks a bit odd. Read from this file returns "NYYYY..." or "YYYYY..." (this makes me surprise when cat this file). Because there is no EOF condition. The first character indicates current->fail_nth is zero or not, and then current->fail_nth is reset to zero.

Just returning task->fail_nth value is more natural to understand.

Cc Dmitry Vyukov <[email protected]>

Signed-off-by Akinobu Mita <[email protected]>

---
 Documentation/fault-injection/fault-injection.txt | 13 +++++++------
 fs/proc/base.c                                    | 14 ++++++--------
 2 files changed, 13 insertions(+), 14 deletions(-)

Documentation/fault-injection/fault-injection.txt +7 -6

index a321905..370ddcb 100644
@@ -139,9 +139,9 @@ o proc entries
 - /proc/self/task/<current-tid>/fail-nth:
 
        Write to this file of integer N makes N-th call in the task fail.
-       Read from this file returns a single char 'Y' or 'N'
-       that says if the fault setup with a previous write to this file was
-       injected or not, and disables the fault if it wasn't yet injected.
+       Read from this file returns a integer value. A value of '0' indicates
+       that the fault setup with a previous write to this file was injected.
+       A positive integer N indicates that the fault wasn't yet injected.
        Note that this file enables all types of faults (slab, futex, etc).
        This setting takes precedence over all other generic debugfs settings
        like probability, interval, times, etc. But per-capability settings

@@ -325,13 +325,14 @@ int main()
                write(fail_nth, buf, strlen(buf));
                res = socketpair(AF_LOCAL, SOCK_STREAM, 0, fds);
                err = errno;
-               read(fail_nth, buf, 1);
+               pread(fail_nth, buf, sizeof(buf), 0);
                if (res == 0) {
                        close(fds[0]);
                        close(fds[1]);
                }
-               printf("%d-th fault %c: res=%d/%d\n", i, buf[0], res, err);
-               if (buf[0] != 'Y')
+               printf("%d-th fault %c: res=%d/%d\n", i, atoi(buf) ? 'N' : 'Y',
+                       res, err);
+               if (atoi(buf))
                        break;
        }
        return 0;

fs/proc/base.c +2 -6

index 42c52e2..9d14215 100644
@@ -1383,7 +1383,8 @@ static ssize_t proc_fail_nth_read(struct file *file, char __user *buf,
                                  size_t count, loff_t *ppos)
 {
        struct task_struct *task;
-       int err;
+       char numbuf[PROC_NUMBUF];
+       ssize_t len;
 
        task = get_proc_task(file_inode(file));
        if (!task)

@@ -1391,13 +1392,10 @@ static ssize_t proc_fail_nth_read(struct file *file, char __user *buf,
        put_task_struct(task);
        if (task != current)
                return -EPERM;
-       if (count < 1)
-               return -EINVAL;
-       err = put_user((char)(current->fail_nth ? 'N' : 'Y'), buf);
-       if (err)
-               return err;

fs/proc/base.c static ssize_t proc_fail_nth_read(struct file *file, char __user *buf, · line 1395

Quoted 3 lines

fs/proc/base.c +1 -2

-       current->fail_nth = 0;
-       return 1;
+       len = snprintf(numbuf, sizeof(numbuf), "%u\n", task->fail_nth);

If we allow setting this for non current task, then we need to prevent data races as the task uses task->fail_nth concurrently. Reads then should use READ_ONCE and writes in fault-inject.c should use WRITE_ONCE.

Quoted 75 lines · Akinobu Mita

Quoted 73 lines

The read interface for fail-nth looks a bit odd. Read from this file returns "NYYYY..." or "YYYYY..." (this makes me surprise when cat this file). Because there is no EOF condition. The first character indicates current->fail_nth is zero or not, and then current->fail_nth is reset to zero.

Just returning task->fail_nth value is more natural to understand.

Cc Dmitry Vyukov <[email protected]>

Signed-off-by Akinobu Mita <[email protected]>

---
 Documentation/fault-injection/fault-injection.txt | 13 +++++++------
 fs/proc/base.c                                    | 14 ++++++--------
 2 files changed, 13 insertions(+), 14 deletions(-)

Documentation/fault-injection/fault-injection.txt +7 -6

index a321905..370ddcb 100644
@@ -139,9 +139,9 @@ o proc entries
 - /proc/self/task/<current-tid>/fail-nth:
 
        Write to this file of integer N makes N-th call in the task fail.
-       Read from this file returns a single char 'Y' or 'N'
-       that says if the fault setup with a previous write to this file was
-       injected or not, and disables the fault if it wasn't yet injected.
+       Read from this file returns a integer value. A value of '0' indicates
+       that the fault setup with a previous write to this file was injected.
+       A positive integer N indicates that the fault wasn't yet injected.
        Note that this file enables all types of faults (slab, futex, etc).
        This setting takes precedence over all other generic debugfs settings
        like probability, interval, times, etc. But per-capability settings

@@ -325,13 +325,14 @@ int main()
                write(fail_nth, buf, strlen(buf));
                res = socketpair(AF_LOCAL, SOCK_STREAM, 0, fds);
                err = errno;
-               read(fail_nth, buf, 1);
+               pread(fail_nth, buf, sizeof(buf), 0);
                if (res == 0) {
                        close(fds[0]);
                        close(fds[1]);
                }
-               printf("%d-th fault %c: res=%d/%d\n", i, buf[0], res, err);
-               if (buf[0] != 'Y')
+               printf("%d-th fault %c: res=%d/%d\n", i, atoi(buf) ? 'N' : 'Y',
+                       res, err);
+               if (atoi(buf))
                        break;
        }
        return 0;

fs/proc/base.c +2 -6

index 42c52e2..9d14215 100644
@@ -1383,7 +1383,8 @@ static ssize_t proc_fail_nth_read(struct file *file, char __user *buf,
                                  size_t count, loff_t *ppos)
 {
        struct task_struct *task;
-       int err;
+       char numbuf[PROC_NUMBUF];
+       ssize_t len;
 
        task = get_proc_task(file_inode(file));
        if (!task)

@@ -1391,13 +1392,10 @@ static ssize_t proc_fail_nth_read(struct file *file, char __user *buf,
        put_task_struct(task);
        if (task != current)
                return -EPERM;
-       if (count < 1)
-               return -EINVAL;
-       err = put_user((char)(current->fail_nth ? 'N' : 'Y'), buf);
-       if (err)
-               return err;

Quoted 8 lines

fs/proc/base.c static ssize_t proc_fail_nth_read(struct file *file, char __user *buf, · line 1395

Quoted 3 lines

fs/proc/base.c +1 -2

-       current->fail_nth = 0;
-       return 1;
+       len = snprintf(numbuf, sizeof(numbuf), "%u\n", task->fail_nth);

If we allow setting this for non current task, then we need to prevent data races as the task uses task->fail_nth concurrently. Reads then should use READ_ONCE and writes in fault-inject.c should use WRITE_ONCE.

This remains unresolved?

The fail-nth file is created with 0666 and the access is permitted if and only if the task is current.

This file is owned by the currnet user. So we can create it with 0644 and allow the owner to write it. This enables to watch the status of task->fail_nth from another processes.

Cc Dmitry Vyukov <[email protected]>

Signed-off-by Akinobu Mita <[email protected]>

---
 fs/proc/base.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

fs/proc/base.c +0 -0

index 9d14215..14e7b73 100644

fs/proc/base.c +7 -15

+       if (err)
+               return err;
+
        task = get_proc_task(file_inode(file));
        if (!task)
                return -ESRCH;
+       task->fail_nth = n;
        put_task_struct(task);
-       if (task != current)
-               return -EPERM;
-       err = kstrtouint_from_user(buf, count, 0, &n);
-       if (err)
-               return err;
-       current->fail_nth = n;
+
        return count;
 }
 

@@ -1389,11 +1389,9 @@ static ssize_t proc_fail_nth_read(struct file *file, char __user *buf,
        task = get_proc_task(file_inode(file));
        if (!task)
                return -ESRCH;
-       put_task_struct(task);
-       if (task != current)
-               return -EPERM;
        len = snprintf(numbuf, sizeof(numbuf), "%u\n", task->fail_nth);
        len = simple_read_from_buffer(buf, count, ppos, numbuf, len);
+       put_task_struct(task);
 
        return len;
 }

@@ -3358,11 +3356,7 @@ static const struct pid_entry tid_base_stuff[] = {
 #endif
 #ifdef CONFIG_FAULT_INJECTION
        REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),
-       /*
-        * Operations on the file check that the task is current,
-        * so we create it with 0666 to support testing under unprivileged user.
-        */
-       REG("fail-nth", 0666, proc_fail_nth_operations),
+       REG("fail-nth", 0644, proc_fail_nth_operations),
 #endif
 #ifdef CONFIG_TASK_IO_ACCOUNTING
        ONE("io",       S_IRUSR, proc_tid_io_accounting),
--

2.7.4

The fail-nth file is created with 0666 and the access is permitted if and only if the task is current.

This file is owned by the currnet user. So we can create it with 0644 and allow the owner to write it. This enables to watch the status of task->fail_nth from another processes.

Cc Dmitry Vyukov <[email protected]>

Signed-off-by Akinobu Mita <[email protected]>

---
 fs/proc/base.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

fs/proc/base.c +7 -9

index 9d14215..14e7b73 100644
@@ -1366,16 +1366,16 @@ static ssize_t proc_fail_nth_write(struct file *file, const char __user *buf,
        int err;
        unsigned int n;
 
+       err = kstrtoint_from_user(buf, count, 0, &n);
+       if (err)
+               return err;
+
        task = get_proc_task(file_inode(file));
        if (!task)
                return -ESRCH;
+       task->fail_nth = n;
        put_task_struct(task);
-       if (task != current)
-               return -EPERM;
-       err = kstrtouint_from_user(buf, count, 0, &n);
-       if (err)
-               return err;
-       current->fail_nth = n;
+
        return count;
 }
 

@@ -1389,11 +1389,9 @@ static ssize_t proc_fail_nth_read(struct file *file, char __user *buf,
        task = get_proc_task(file_inode(file));
        if (!task)
                return -ESRCH;
-       put_task_struct(task);
-       if (task != current)
-               return -EPERM;
        len = snprintf(numbuf, sizeof(numbuf), "%u\n", task->fail_nth);
        len = simple_read_from_buffer(buf, count, ppos, numbuf, len);
+       put_task_struct(task);
 
        return len;
 }

fs/proc/base.c +0 -1

 #endif
 #ifdef CONFIG_TASK_IO_ACCOUNTING
        ONE("io",       S_IRUSR, proc_tid_io_accounting),
--

2.7.4

Quoted 54 lines

The fail-nth file is created with 0666 and the access is permitted if and only if the task is current.

This file is owned by the currnet user. So we can create it with 0644 and allow the owner to write it. This enables to watch the status of task->fail_nth from another processes.

Cc Dmitry Vyukov <[email protected]>

Signed-off-by Akinobu Mita <[email protected]>

---
 fs/proc/base.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

fs/proc/base.c +7 -9

index 9d14215..14e7b73 100644
@@ -1366,16 +1366,16 @@ static ssize_t proc_fail_nth_write(struct file *file, const char __user *buf,
        int err;
        unsigned int n;
 
+       err = kstrtoint_from_user(buf, count, 0, &n);
+       if (err)
+               return err;
+
        task = get_proc_task(file_inode(file));
        if (!task)
                return -ESRCH;
+       task->fail_nth = n;
        put_task_struct(task);
-       if (task != current)
-               return -EPERM;
-       err = kstrtouint_from_user(buf, count, 0, &n);
-       if (err)
-               return err;
-       current->fail_nth = n;
+
        return count;
 }
 

@@ -1389,11 +1389,9 @@ static ssize_t proc_fail_nth_read(struct file *file, char __user *buf,
        task = get_proc_task(file_inode(file));
        if (!task)
                return -ESRCH;
-       put_task_struct(task);
-       if (task != current)
-               return -EPERM;
        len = snprintf(numbuf, sizeof(numbuf), "%u\n", task->fail_nth);
        len = simple_read_from_buffer(buf, count, ppos, numbuf, len);
+       put_task_struct(task);
 
        return len;
 }

fs/proc/base.c static const struct pid_entry tid_base_stuff[] = { · line 3359

Quoted 9 lines

fs/proc/base.c +1 -5

@@ -3358,11 +3356,7 @@ static const struct pid_entry tid_base_stuff[] = {
 #endif
 #ifdef CONFIG_FAULT_INJECTION
        REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),
-       /*
-        * Operations on the file check that the task is current,
-        * so we create it with 0666 to support testing under unprivileged user.
-        */
-       REG("fail-nth", 0666, proc_fail_nth_operations),
+       REG("fail-nth", 0644, proc_fail_nth_operations),

/\/\/\/\/\/\

This breaks us.
Under setuid sandbox test threads can't open the file anymore. And we can't pre-open the files before dropping privs as new threads can be created afterwards.

I think the root cause of all these problems (permissions, parsing, serialization, broken cat, symmetry) is that we are trying to fit a programmatic API into reads and writes on textual files. We don't need symmetry, we don't need read+write to reset injection, we don't need parsing and serialization, it does not make sense to do this of non-current task, it definitely does not make sense to cat this, etc.

What do you think of 2 ioctls on /sys/kernel/debug/fail_nth?

Quoted 6 lines

fs/proc/base.c +0 -1

 #endif
 #ifdef CONFIG_TASK_IO_ACCOUNTING
        ONE("io",       S_IRUSR, proc_tid_io_accounting),
--

2.7.4

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

Quoted 67 lines · Akinobu Mita

Quoted 54 lines

The fail-nth file is created with 0666 and the access is permitted if and only if the task is current.

This file is owned by the currnet user. So we can create it with 0644 and allow the owner to write it. This enables to watch the status of task->fail_nth from another processes.

Cc Dmitry Vyukov <[email protected]>

Signed-off-by Akinobu Mita <[email protected]>

---
 fs/proc/base.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

fs/proc/base.c +7 -9

index 9d14215..14e7b73 100644
@@ -1366,16 +1366,16 @@ static ssize_t proc_fail_nth_write(struct file *file, const char __user *buf,
        int err;
        unsigned int n;
 
+       err = kstrtoint_from_user(buf, count, 0, &n);
+       if (err)
+               return err;
+
        task = get_proc_task(file_inode(file));
        if (!task)
                return -ESRCH;
+       task->fail_nth = n;
        put_task_struct(task);
-       if (task != current)
-               return -EPERM;
-       err = kstrtouint_from_user(buf, count, 0, &n);
-       if (err)
-               return err;
-       current->fail_nth = n;
+
        return count;
 }
 

@@ -1389,11 +1389,9 @@ static ssize_t proc_fail_nth_read(struct file *file, char __user *buf,
        task = get_proc_task(file_inode(file));
        if (!task)
                return -ESRCH;
-       put_task_struct(task);
-       if (task != current)
-               return -EPERM;
        len = snprintf(numbuf, sizeof(numbuf), "%u\n", task->fail_nth);
        len = simple_read_from_buffer(buf, count, ppos, numbuf, len);
+       put_task_struct(task);
 
        return len;
 }

fs/proc/base.c static const struct pid_entry tid_base_stuff[] = { · line 3359

Quoted 9 lines

fs/proc/base.c +1 -5

@@ -3358,11 +3356,7 @@ static const struct pid_entry tid_base_stuff[] = {
 #endif
 #ifdef CONFIG_FAULT_INJECTION
        REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),
-       /*
-        * Operations on the file check that the task is current,
-        * so we create it with 0666 to support testing under unprivileged user.
-        */
-       REG("fail-nth", 0666, proc_fail_nth_operations),
+       REG("fail-nth", 0644, proc_fail_nth_operations),

/\/\/\/\/\/\

Quoted 4 lines

This breaks us.
Under setuid sandbox test threads can't open the file anymore. And we can't pre-open the files before dropping privs as new threads can be created afterwards.

Could you provide a working example for this? Because I'm not sure I understand the problem you described here.

If we omit resetting tsk->fail_nth in dup_task_struct(), tsk->fail_nth is inherited from parent to child process. So the parent process can pre-open and set fail-nth file and reset parent's own ->fail_nth after fork by writing 0 to fail-nth file. Does that fix your problem?

Quoted 8 lines

I think the root cause of all these problems (permissions, parsing, serialization, broken cat, symmetry) is that we are trying to fit a programmatic API into reads and writes on textual files. We don't need symmetry, we don't need read+write to reset injection, we don't need parsing and serialization, it does not make sense to do this of non-current task, it definitely does not make sense to cat this, etc.

What do you think of 2 ioctls on /sys/kernel/debug/fail_nth?

I think the misc device is suitable than debugfs file for ioctl only knob. But I prefer read/write interface than ioctl if possible.

Quoted 6 lines

Quoted 6 lines

fs/proc/base.c +0 -1

 #endif
 #ifdef CONFIG_TASK_IO_ACCOUNTING
        ONE("io",       S_IRUSR, proc_tid_io_accounting),
--

2.7.4