On Fri, 7 Apr 2017 22:37:01 +0200 Dmitry Vyukov <dvyu...@google.com> wrote:
> On Thu, Apr 6, 2017 at 4:55 PM, Akinobu Mita <akinobu.m...@gmail.com> 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 <dvyu...@google.com>
> > Signed-off-by: Akinobu Mita <akinobu.m...@gmail.com>
> > ---
> > 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?
