From: Peter Zijlstra <peterz@infradead.org>
To: Jann Horn <jannh@google.com>
Cc: Peter Oskolkov <posk@google.com>, Ingo Molnar <mingo@redhat.com>,
Thomas Gleixner <tglx@linutronix.de>,
kernel list <linux-kernel@vger.kernel.org>,
Linux API <linux-api@vger.kernel.org>,
Paul Turner <pjt@google.com>, Ben Segall <bsegall@google.com>,
Peter Oskolkov <posk@posk.io>,
Joel Fernandes <joel@joelfernandes.org>,
Andrew Morton <akpm@linux-foundation.org>,
Andrei Vagin <avagin@google.com>,
Jim Newsome <jnewsome@torproject.org>
Subject: Re: [RFC PATCH v0.1 4/9] sched/umcg: implement core UMCG API
Date: Wed, 9 Jun 2021 15:01:03 +0200 [thread overview]
Message-ID: <20210609130103.GB68187@worktop.programming.kicks-ass.net> (raw)
In-Reply-To: <CAG48ez3Ur61rpOZduQRFabB9R=RbSin9Th+=0=z9FUpcZ21C=w@mail.gmail.com>
On Fri, May 21, 2021 at 11:33:14PM +0200, Jann Horn wrote:
> > SYSCALL_DEFINE2(umcg_wake, u32, flags, u32, next_tid)
> > {
> > - return -ENOSYS;
> > + struct umcg_task_data *next_utd;
> > + struct task_struct *next;
> > + int ret = -EINVAL;
> > +
> > + if (!next_tid)
> > + return -EINVAL;
> > + if (flags)
> > + return -EINVAL;
> > +
> > + next = find_get_task_by_vpid(next_tid);
> > + if (!next)
> > + return -ESRCH;
> > + rcu_read_lock();
>
> Wouldn't it be more efficient to replace the last 4 lines with the following?
>
> rcu_read_lock();
> next = find_task_by_vpid(next_tid);
> if (!next) {
> err = -ESRCH;
> goto out;
> }
This wakeup crud needs to modify the umcg->state, which is a user
variable. That can't be done under RCU. Weirdly the proposed code
doesn't actually do any of that for undocumented raisins :/
> Then you don't need to use refcounting here...
>
> > + next_utd = rcu_dereference(next->umcg_task_data);
> > + if (!next_utd)
> > + goto out;
> > +
> > + if (!READ_ONCE(next_utd->in_wait)) {
> > + ret = -EAGAIN;
> > + goto out;
> > + }
> > +
> > + ret = wake_up_process(next);
> > + put_task_struct(next);
>
> ... and you'd be able to drop this put_task_struct(), too.
>
> > + if (ret)
> > + ret = 0;
> > + else
> > + ret = -EAGAIN;
> > +
> > +out:
> > + rcu_read_unlock();
> > + return ret;
> > }
> >
> > /**
> > @@ -139,5 +325,44 @@ SYSCALL_DEFINE2(umcg_wake, u32, flags, u32, next_tid)
> > SYSCALL_DEFINE4(umcg_swap, u32, wake_flags, u32, next_tid, u32, wait_flags,
> > const struct __kernel_timespec __user *, timeout)
> > {
> > - return -ENOSYS;
> > + struct umcg_task_data *curr_utd;
> > + struct umcg_task_data *next_utd;
> > + struct task_struct *next;
> > + int ret = -EINVAL;
> > +
> > + rcu_read_lock();
> > + curr_utd = rcu_dereference(current->umcg_task_data);
> > +
> > + if (!next_tid || wake_flags || wait_flags || !curr_utd)
> > + goto out;
> > +
> > + if (timeout) {
> > + ret = -EOPNOTSUPP;
> > + goto out;
> > + }
> > +
> > + next = find_get_task_by_vpid(next_tid);
> > + if (!next) {
> > + ret = -ESRCH;
> > + goto out;
> > + }
>
> There isn't any type of access check here, right? Any task can wake up
> any other task? That feels a bit weird to me - and if you want to keep
> it as-is, it should probably at least be documented that any task on
> the system can send you spurious wakeups if you opt in to umcg.
You can only send wakeups to other UMCG thingies, per the
next->umcg_task_data check below. That said..
> In contrast, shared futexes can avoid this because they get their
> access control implicitly from the VMA.
Every task must expect spurious wakups at all times, always (for
TASK_NORMAL wakeups that is). There's plenty ways to generate them.
> > + next_utd = rcu_dereference(next->umcg_task_data);
> > + if (!next_utd) {
> > + ret = -EINVAL;
> > + goto out;
> > + }
next prev parent reply other threads:[~2021-06-09 13:02 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-20 18:36 [RFC PATCH v0.1 0/9] UMCG early preview/RFC patchset Peter Oskolkov
2021-05-20 18:36 ` [RFC PATCH v0.1 1/9] sched/umcg: add UMCG syscall stubs and CONFIG_UMCG Peter Oskolkov
2021-05-22 18:40 ` kernel test robot
2021-05-22 21:49 ` kernel test robot
2021-05-20 18:36 ` [RFC PATCH v0.1 2/9] sched/umcg: add uapi/linux/umcg.h and sched/umcg.c Peter Oskolkov
2021-05-20 18:36 ` [RFC PATCH v0.1 3/9] sched: add WF_CURRENT_CPU and externise ttwu Peter Oskolkov
2021-05-20 18:36 ` [RFC PATCH v0.1 4/9] sched/umcg: implement core UMCG API Peter Oskolkov
2021-05-21 19:06 ` Andrei Vagin
2021-05-21 21:31 ` Jann Horn
2021-05-21 22:03 ` Peter Oskolkov
2021-05-21 19:32 ` Andy Lutomirski
2021-05-21 22:01 ` Peter Oskolkov
2021-05-21 21:33 ` Jann Horn
2021-06-09 13:01 ` Peter Zijlstra [this message]
2021-05-20 18:36 ` [RFC PATCH v0.1 5/9] lib/umcg: implement UMCG core API for userspace Peter Oskolkov
2021-05-20 18:36 ` [RFC PATCH v0.1 6/9] selftests/umcg: add UMCG core API selftest Peter Oskolkov
2021-05-20 18:36 ` [RFC PATCH v0.1 7/9] sched/umcg: add UMCG server/worker API (early RFC) Peter Oskolkov
2021-05-21 20:17 ` Andrei Vagin
2021-05-22 18:29 ` kernel test robot
2021-05-22 19:34 ` kernel test robot
2021-05-22 20:19 ` kernel test robot
2021-05-20 18:36 ` [RFC PATCH v0.1 8/9] lib/umcg: " Peter Oskolkov
2021-05-20 18:36 ` [RFC PATCH v0.1 9/9] selftests/umcg: add UMCG server/worker API selftest Peter Oskolkov
2021-05-20 21:17 ` [RFC PATCH v0.1 0/9] UMCG early preview/RFC patchset Jonathan Corbet
2021-05-20 21:38 ` Peter Oskolkov
2021-05-21 0:15 ` Randy Dunlap
2021-05-21 8:04 ` Peter Zijlstra
2021-05-21 15:08 ` Jonathan Corbet
2021-05-21 16:03 ` Peter Oskolkov
2021-05-21 19:17 ` Jonathan Corbet
2021-05-27 0:06 ` Peter Oskolkov
2021-05-27 15:41 ` Jonathan Corbet
[not found] ` <CAEWA0a72SvpcuN4ov=98T3uWtExPCr7BQePOgjkqD1ofWKEASw@mail.gmail.com>
2021-05-21 19:13 ` Peter Oskolkov
2021-05-21 23:08 ` Jann Horn
2021-06-09 12:54 ` Peter Zijlstra
2021-06-09 20:18 ` Peter Oskolkov
2021-06-10 18:02 ` Peter Zijlstra
2021-06-10 20:06 ` Peter Oskolkov
2021-07-07 17:45 ` Thierry Delisle
2021-07-08 21:44 ` Peter Oskolkov
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=20210609130103.GB68187@worktop.programming.kicks-ass.net \
--to=peterz@infradead.org \
--cc=akpm@linux-foundation.org \
--cc=avagin@google.com \
--cc=bsegall@google.com \
--cc=jannh@google.com \
--cc=jnewsome@torproject.org \
--cc=joel@joelfernandes.org \
--cc=linux-api@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@redhat.com \
--cc=pjt@google.com \
--cc=posk@google.com \
--cc=posk@posk.io \
--cc=tglx@linutronix.de \
/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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.