Re: [PATCH -mm 3/5] fault-inject: make fail-nth read/write interface symmetric

[Akinobu Mita]

2017-07-13 5:49 GMT+09:00 Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>:
> On Fri, 7 Apr 2017 22:37:01 +0200 Dmitry Vyukov <dvyukov@xxxxxxxxxx> wrote:
>
>> On Thu, Apr 6, 2017 at 4:55 PM, Akinobu Mita <akinobu.mita@xxxxxxxxx> wrote:
>> > 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 <dvyukov@xxxxxxxxxx>
>> > Signed-off-by: Akinobu Mita <akinobu.mita@xxxxxxxxx>
>> > ---
>> >  Documentation/fault-injection/fault-injection.txt | 13 +++++++------
>> >  fs/proc/base.c                                    | 14 ++++++--------
>> >  2 files changed, 13 insertions(+), 14 deletions(-)
>> >
>> > diff --git a/Documentation/fault-injection/fault-injection.txt b/Documentation/fault-injection/fault-injection.txt
>> > index a321905..370ddcb 100644
>> > --- a/Documentation/fault-injection/fault-injection.txt
>> > +++ b/Documentation/fault-injection/fault-injection.txt
>> > @@ -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;
>> > diff --git a/fs/proc/base.c b/fs/proc/base.c
>> > index 42c52e2..9d14215 100644
>> > --- a/fs/proc/base.c
>> > +++ b/fs/proc/base.c
>> > @@ -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;
>> > -       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?

I have just send a proposed fix. (Subject: [PATCH -mm] fault-inject: avoid
unwanted data race to task->fail_nth)