Re: [PATCH -mm 4/5] fault-inject: simplify access check for fail-nth

[Dmitry Vyukov]

On Sat, Apr 8, 2017 at 10:25 AM, Akinobu Mita <akinobu.mita@xxxxxxxxx> wrote:
> 2017-04-08 5:45 GMT+09:00 Dmitry Vyukov <dvyukov@xxxxxxxxxx>:
>> On Thu, Apr 6, 2017 at 4:56 PM, Akinobu Mita <akinobu.mita@xxxxxxxxx> wrote:
>>> 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 <dvyukov@xxxxxxxxxx>
>>> Signed-off-by: Akinobu Mita <akinobu.mita@xxxxxxxxx>
>>> ---
>>>  fs/proc/base.c | 22 ++++++++--------------
>>>  1 file changed, 8 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/fs/proc/base.c b/fs/proc/base.c
>>> index 9d14215..14e7b73 100644
>>> --- a/fs/proc/base.c
>>> +++ b/fs/proc/base.c
>>> @@ -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;
>>>  }
>>> @@ -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.
>
> Could you provide a working example for this?  Because I'm not sure
> I understand the problem you described here.

The actual use case is syzkaller executor:
https://github.com/google/syzkaller/blob/fault_inject/executor/executor.cc
(search for "fail-nth"). When sandbox_setuid is enabled, open of the
file fails with EPERM.

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

I don't think so.
First clone does unknown number of allocations. Second I don't
need/want the thread to start failing start away. At some point in
future it _may_ need to inject failures into a particular system call.
There are multiple threads, and we don't know apriori which one of
them will need to inject failures, since thread assignment is dynamic
and depends on which system calls will block and which will not block.

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

Why do you prefer read/write?
This interface is not meant to be used from command line, like lots of
other system calls.
We did /sys/kernel/debug/kcov with ioctls and it works perfect.


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