2017-07-13 5:49 GMT+09:00 Andrew Morton <a...@linux-foundation.org>: > 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?
I have just send a proposed fix. (Subject: [PATCH -mm] fault-inject: avoid unwanted data race to task->fail_nth)