Message-ID: <CACT4Y+bD0Yak9FzFig+DYcU305MHfcCT46jqZO=di9unsGK2rQ@mail.gmail.com>
Date:   Wed, 24 May 2017 12:22:26 +0200
From:   Dmitry Vyukov <dvyukov@...gle.com>
To:     Akinobu Mita <akinobu.mita@...il.com>
Cc:     Michal Hocko <mhocko@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        LKML <linux-kernel@...r.kernel.org>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>,
        "open list:ABI/API" <linux-api@...r.kernel.org>
Subject: Re: [PATCH v2] fault-inject: support systematic fault injection

Hi,

I've made the current interface work with all types of our sandboxes.
For setuid the secret souse was prctl(PR_SET_DUMPABLE, 1, 0, 0, 0) to
make /proc entries non-root owned. So I am fine with the current
version of the code.

Andrew,

I see that this is already in linux-next. Please proceed with pushing
it further.

Thanks


On Sun, Apr 9, 2017 at 12:39 PM, Akinobu Mita <akinobu.mita@...il.com> wrote:
> 2017-04-09 2:40 GMT+09:00 Dmitry Vyukov <dvyukov@...gle.com>:
>> On Fri, Apr 7, 2017 at 6:47 PM, Akinobu Mita <akinobu.mita@...il.com> wrote:
>>> 2017-04-07 3:33 GMT+09:00 Michal Hocko <mhocko@...nel.org>:
>>>> [Let's add linux-api - please always cc this list when adding/modifying
>>>> user visible interfaces]
>>>>
>>>> On Tue 28-03-17 15:01:28, Dmitry Vyukov wrote:
>>>>> Add /proc/self/task/<current-tid>/fail-nth file that allows failing
>>>>> 0-th, 1-st, 2-nd and so on calls systematically.
>>>>> Excerpt from the added documentation:
>>>>
>>>> I didn't really get to read through details here but it just feels wrong
>>>> to add this debugging only feature into proc. It also smells like one
>>>> off thing as well.
>>>
>>> We have 'sched' (CONFIG_SCHED_DEBUG), 'latency' (CONFIG_LATENCYTOP)
>>> and 'make-it-fail' as debugging per-process proc files.  So it doesn't
>>> look very wrong to me.  But I would like to avoid per-process proc
>>> directory becoming messy. Do you think introducing /proc/<pid>/debug/
>>> directory for debugging stuff makes sense?
>>>
>>> Side note: 'fail-nth' was originally a single debugfs file
>>> /sys/kernel/debug/fail_once.  But it actually read/write current task's
>>> fail_nth field, so I suggested changing per process procfs file.i
>>> This change enables to inject N-th fail to kernel threads, too.
>>
>>
>> /sys/kernel/debug/fail_once (or fail_nth) looks more appropriate to me
>> for a optional testing feature. The fact that it currently
>> reads/writes a task_struct field is merely an implementation detail.
>> I would also prefer ioctl's. Then we don't need to preserve "symmetry"
>> for no useful reason and deal with nonsensical uses like setting it
>> for a non-current task and running cat on it.
>
> Sounds reasonable for adding ioctl interface as it can work with your
> setuid sandbox test.  But could you keep the procfs interface, too?
> Because both ioctl debugfs interface and procfs interface can co-exist
> and I would like to play it with procfs for a while.
>
>>>>> ===
>>>>> Write to this file of integer N makes N-th call in the current task fail
>>>>> (N is 0-based). 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.
>>>>> Note that this file enables all types of faults (slab, futex, etc).
>>>>> This setting takes precedence over all other generic settings like
>>>>> probability, interval, times, etc. But per-capability settings
>>>>> (e.g. fail_futex/ignore-private) take precedence over it.
>>>>> This feature is intended for systematic testing of faults in a single
>>>>> system call. See an example below.
>>>>> ===
>>>>>
>>>>> Why adding new setting:
>>>>> 1. Existing settings are global rather than per-task.
>>>>>    So parallel testing is not possible.
>>>>> 2. attr->interval is close but it depends on attr->count
>>>>>    which is non reset to 0, so interval does not work as expected.
>>>>> 3. Trying to model this with existing settings requires manipulations
>>>>>    of all of probability, interval, times, space, task-filter and
>>>>>    unexposed count and per-task make-it-fail files.
>>>>> 4. Existing settings are per-failure-type, and the set of failure
>>>>>    types is potentially expanding.
>>>>> 5. make-it-fail can't be changed by unprivileged user and aggressive
>>>>>    stress testing better be done from an unprivileged user.
>>>>>    Similarly, this would require opening the debugfs files to the
>>>>>    unprivileged user, as he would need to reopen at least times file
>>>>>    (not possible to pre-open before dropping privs).
>>>>>
>>>>> The proposed interface solves all of the above (see the example).
>>>>>
>>>>> Signed-off-by: Dmitry Vyukov <dvyukov@...gle.com>
>>>>> Cc: Akinobu Mita <akinobu.mita@...il.com>
>>>>> Cc: Andrew Morton <akpm@...ux-foundation.org>
>>>>> Cc: linux-kernel@...r.kernel.org
>>>>> Cc: linux-mm@...ck.org
>>>>>
>>>>> ---
>>>>> We want to integrate this into syzkaller fuzzer.
>>>>> A prototype has found 10 bugs in kernel in first day of usage:
>>>>> https://groups.google.com/forum/#!searchin/syzkaller/%22FAULT_INJECTION%22%7Csort:relevance
>>>>>
>>>>> Changes since v1:
>>>>>  - change file name from /sys/kernel/debug/fail_once
>>>>>    to /proc/self/task/<current-tid>/fail-nth as per
>>>>>    Akinobu suggestion
>>>>>
>>>>> ---
>>>>>  Documentation/fault-injection/fault-injection.txt | 78 +++++++++++++++++++++++
>>>>>  fs/proc/base.c                                    | 52 +++++++++++++++
>>>>>  include/linux/sched.h                             |  1 +
>>>>>  kernel/fork.c                                     |  4 ++
>>>>>  lib/fault-inject.c                                |  7 ++
>>>>>  5 files changed, 142 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/fault-injection/fault-injection.txt b/Documentation/fault-injection/fault-injection.txt
>>>>> index 415484f3d59a..192d8cbcc5f9 100644
>>>>> --- a/Documentation/fault-injection/fault-injection.txt
>>>>> +++ b/Documentation/fault-injection/fault-injection.txt
>>>>> @@ -134,6 +134,22 @@ use the boot option:
>>>>>       fail_futex=
>>>>>       mmc_core.fail_request=<interval>,<probability>,<space>,<times>
>>>>>
>>>>> +o proc entries
>>>>> +
>>>>> +- /proc/self/task/<current-tid>/fail-nth:
>>>>> +
>>>>> +     Write to this file of integer N makes N-th call in the current task fail
>>>>> +     (N is 0-based). 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.
>>>>> +     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
>>>>> +     (e.g. fail_futex/ignore-private) take precedence over it.
>>>>> +
>>>>> +     This feature is intended for systematic testing of faults in a single
>>>>> +     system call. See an example below.
>>>>> +
>>>>>  How to add new fault injection capability
>>>>>  -----------------------------------------
>>>>>
>>>>> @@ -278,3 +294,65 @@ allocation failure.
>>>>>       # env FAILCMD_TYPE=fail_page_alloc \
>>>>>               ./tools/testing/fault-injection/failcmd.sh --times=100 \
>>>>>                  -- make -C tools/testing/selftests/ run_tests
>>>>> +
>>>>> +Systematic faults using fail-nth
>>>>> +---------------------------------
>>>>> +
>>>>> +The following code systematically faults 0-th, 1-st, 2-nd and so on
>>>>> +capabilities in the socketpair() system call.
>>>>> +
>>>>> +#include <sys/types.h>
>>>>> +#include <sys/stat.h>
>>>>> +#include <sys/socket.h>
>>>>> +#include <sys/syscall.h>
>>>>> +#include <fcntl.h>
>>>>> +#include <unistd.h>
>>>>> +#include <string.h>
>>>>> +#include <stdlib.h>
>>>>> +#include <stdio.h>
>>>>> +#include <errno.h>
>>>>> +
>>>>> +int main()
>>>>> +{
>>>>> +     int i, err, res, fail_nth, fds[2];
>>>>> +     char buf[128];
>>>>> +
>>>>> +     system("echo N > /sys/kernel/debug/failslab/ignore-gfp-wait");
>>>>> +     sprintf(buf, "/proc/self/task/%ld/fail-nth", syscall(SYS_gettid));
>>>>> +     fail_nth = open(buf, O_RDWR);
>>>>> +     for (i = 0;; i++) {
>>>>> +             sprintf(buf, "%d", i);
>>>>> +             write(fail_nth, buf, strlen(buf));
>>>>> +             res = socketpair(AF_LOCAL, SOCK_STREAM, 0, fds);
>>>>> +             err = errno;
>>>>> +             read(fail_nth, buf, 1);
>>>>> +             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')
>>>>> +                     break;
>>>>> +     }
>>>>> +     return 0;
>>>>> +}
>>>>> +
>>>>> +An example output:
>>>>> +
>>>>> +0-th fault Y: res=-1/23
>>>>> +1-th fault Y: res=-1/23
>>>>> +2-th fault Y: res=-1/23
>>>>> +3-th fault Y: res=-1/12
>>>>> +4-th fault Y: res=-1/12
>>>>> +5-th fault Y: res=-1/23
>>>>> +6-th fault Y: res=-1/23
>>>>> +7-th fault Y: res=-1/23
>>>>> +8-th fault Y: res=-1/12
>>>>> +9-th fault Y: res=-1/12
>>>>> +10-th fault Y: res=-1/12
>>>>> +11-th fault Y: res=-1/12
>>>>> +12-th fault Y: res=-1/12
>>>>> +13-th fault Y: res=-1/12
>>>>> +14-th fault Y: res=-1/12
>>>>> +15-th fault Y: res=-1/12
>>>>> +16-th fault N: res=0/12
>>>>> diff --git a/fs/proc/base.c b/fs/proc/base.c
>>>>> index 6e8655845830..66001172249b 100644
>>>>> --- a/fs/proc/base.c
>>>>> +++ b/fs/proc/base.c
>>>>> @@ -1353,6 +1353,53 @@ static const struct file_operations proc_fault_inject_operations = {
>>>>>       .write          = proc_fault_inject_write,
>>>>>       .llseek         = generic_file_llseek,
>>>>>  };
>>>>> +
>>>>> +static ssize_t proc_fail_nth_write(struct file *file, const char __user *buf,
>>>>> +                                size_t count, loff_t *ppos)
>>>>> +{
>>>>> +     struct task_struct *task;
>>>>> +     int err, n;
>>>>> +
>>>>> +     task = get_proc_task(file_inode(file));
>>>>> +     if (!task)
>>>>> +             return -ESRCH;
>>>>> +     put_task_struct(task);
>>>>> +     if (task != current)
>>>>> +             return -EPERM;
>>>>> +     err = kstrtoint_from_user(buf, count, 10, &n);
>>>>> +     if (err)
>>>>> +             return err;
>>>>> +     if (n < 0 || n == INT_MAX)
>>>>> +             return -EINVAL;
>>>>> +     current->fail_nth = n + 1;
>>>>> +     return len;
>>>>> +}
>>>>> +
>>>>> +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;
>>>>> +
>>>>> +     task = get_proc_task(file_inode(file));
>>>>> +     if (!task)
>>>>> +             return -ESRCH;
>>>>> +     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;
>>>>> +     current->fail_nth = 0;
>>>>> +     return 1;
>>>>> +}
>>>>> +
>>>>> +static const struct file_operations proc_fail_nth_operations = {
>>>>> +     .read           = proc_fail_nth_read,
>>>>> +     .write          = proc_fail_nth_write,
>>>>> +};
>>>>>  #endif
>>>>>
>>>>>
>>>>> @@ -3296,6 +3343,11 @@ 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),
>>>>>  #endif
>>>>>  #ifdef CONFIG_TASK_IO_ACCOUNTING
>>>>>       ONE("io",       S_IRUSR, proc_tid_io_accounting),
>>>>> diff --git a/include/linux/sched.h b/include/linux/sched.h
>>>>> index 543e0ea82684..7b50221fea51 100644
>>>>> --- a/include/linux/sched.h
>>>>> +++ b/include/linux/sched.h
>>>>> @@ -1897,6 +1897,7 @@ struct task_struct {
>>>>>  #endif
>>>>>  #ifdef CONFIG_FAULT_INJECTION
>>>>>       int make_it_fail;
>>>>> +     int fail_nth;
>>>>>  #endif
>>>>>       /*
>>>>>        * when (nr_dirtied >= nr_dirtied_pause), it's time to call
>>>>> diff --git a/kernel/fork.c b/kernel/fork.c
>>>>> index 61284d8122fa..869c97a0a930 100644
>>>>> --- a/kernel/fork.c
>>>>> +++ b/kernel/fork.c
>>>>> @@ -545,6 +545,10 @@ static struct task_struct *dup_task_struct(struct task_struct *orig, int node)
>>>>>
>>>>>       kcov_task_init(tsk);
>>>>>
>>>>> +#ifdef CONFIG_FAULT_INJECTION
>>>>> +     tsk->fail_nth = 0;
>>>>> +#endif
>>>>> +
>>>>>       return tsk;
>>>>>
>>>>>  free_stack:
>>>>> diff --git a/lib/fault-inject.c b/lib/fault-inject.c
>>>>> index 6a823a53e357..d6516ba64d33 100644
>>>>> --- a/lib/fault-inject.c
>>>>> +++ b/lib/fault-inject.c
>>>>> @@ -107,6 +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)
>>>>> +                     goto fail;
>>>>> +             return false;
>>>>> +     }
>>>>> +
>>>>>       /* No need to check any other properties if the probability is 0 */
>>>>>       if (attr->probability == 0)
>>>>>               return false;
>>>>> @@ -134,6 +140,7 @@ bool should_fail(struct fault_attr *attr, ssize_t size)
>>>>>       if (!fail_stacktrace(attr))
>>>>>               return false;
>>>>>
>>>>> +fail:
>>>>>       fail_dump(attr);
>>>>>
>>>>>       if (atomic_read(&attr->times) != -1)
>>>>> --
>>>>> 2.12.2.564.g063fe858b8-goog
>>>>>
>>>>> --
>>>>> To unsubscribe, send a message with 'unsubscribe linux-mm' in
>>>>> the body to majordomo@...ck.org.  For more info on Linux MM,
>>>>> see: http://www.linux-mm.org/ .
>>>>> Don't email: <a href=mailto:"dont@...ck.org"> email@...ck.org </a>
>>>>
>>>> --
>>>> Michal Hocko
>>>> SUSE Labs

Powered by blists - more mailing lists