linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
To: oleg@redhat.com
Cc: dserrg@gmail.com, snanda@chromium.org, rientjes@google.com,
	linux-kernel@vger.kernel.org
Subject: Question about replacing while_each_thread().
Date: Wed, 1 Feb 2017 19:47:32 +0900	[thread overview]
Message-ID: <201702011947.DBD56740.OMVHOLOtSJFFFQ@I-love.SAKURA.ne.jp> (raw)

Hello.

I have a question about commit 0c740d0afc3bff0a ("introduce
for_each_thread() to replace the buggy while_each_thread()").

IOPRIO_WHO_USER case in sys_ioprio_set()/sys_ioprio_get() in block/ioprio.c
are using

  rcu_read_lock();
  do_each_thread(g, p) {
    (...snipped...)
  } while_each_thread(g, p);
  rcu_read_unlock();

sequence which is unsafe according to that commit, but
I'm not sure what the correct fix is.

That commit says

  The new for_each_thread(g, t) helper is always safe under
  rcu_read_lock() as long as this task_struct can't go away.

but what is the requirement for "can't go away" ?

Is rcu_read_lock() sufficient (i.e.

  rcu_read_lock();
  for_each_process_thread(g, p) {
    (...snipped...)
  }
  rcu_read_unlock();

is OK) for "can't go away" ?
Is tasklist_lock held for read or write required (i.e.

  read_lock(&tasklist_lock);
  for_each_process_thread(g, p) {
    (...snipped...)
  }
  read_unlock(&tasklist_lock);

is needed) for "can't go away" ?

I hope rcu_read_lock() is sufficient according to usage in
show_state_filter() and check_hung_uninterruptible_tasks().

Likewise, IOPRIO_WHO_PGRP case are using

  rcu_read_lock();
  do {
    if ((pgrp) != NULL)
      hlist_for_each_entry_rcu((p), &(pgrp)->tasks[PIDTYPE_PGID], pids[PIDTYPE_PGID].node) {
        {
          struct task_struct *tg___ = p;
          do {
            (...snipped...)
          } while_each_thread(tg___, p);
          p = tg___;
        }
        if (PIDTYPE_PGID == PIDTYPE_PID)
          break;
      }
  } while (0);
  rcu_read_unlock();

sequence which I guess it is unsafe as well.
In this case updating do_each_pid_thread() to use for_each_thread() and
updating while_each_pid_thread() not to use while_each_thread() is
the correct fix?

             reply	other threads:[~2017-02-01 10:47 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-02-01 10:47 Tetsuo Handa [this message]
2017-02-01 17:19 ` Question about replacing while_each_thread() Oleg Nesterov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=201702011947.DBD56740.OMVHOLOtSJFFFQ@I-love.SAKURA.ne.jp \
    --to=penguin-kernel@i-love.sakura.ne.jp \
    --cc=dserrg@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=oleg@redhat.com \
    --cc=rientjes@google.com \
    --cc=snanda@chromium.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).