Linux-Security-Module Archive on lore.kernel.org
 help / color / Atom feed
From: Linus Torvalds <torvalds@linux-foundation.org>
To: David Howells <dhowells@redhat.com>
Cc: Ray Strode <rstrode@redhat.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Steven Whitehouse <swhiteho@redhat.com>,
	Nicolas Dichtel <nicolas.dichtel@6wind.com>,
	raven@themaw.net, keyrings@vger.kernel.org,
	linux-usb@vger.kernel.org,
	linux-block <linux-block@vger.kernel.org>,
	Christian Brauner <christian@brauner.io>,
	LSM List <linux-security-module@vger.kernel.org>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	Linux API <linux-api@vger.kernel.org>,
	Linux List Kernel Mailing <linux-kernel@vger.kernel.org>,
	Al Viro <viro@zeniv.linux.org.uk>,
	"Ray, Debarshi" <debarshi.ray@gmail.com>,
	Robbie Harwood <rharwood@redhat.com>
Subject: Re: Why add the general notification queue and its sources
Date: Fri, 6 Sep 2019 08:35:46 -0700
Message-ID: <CAHk-=wiR1fpahgKuxSOQY6OfgjWD+MKz8UF6qUQ6V_y2TC_V6w@mail.gmail.com> (raw)
In-Reply-To: <27732.1567764557@warthog.procyon.org.uk>

On Fri, Sep 6, 2019 at 3:09 AM David Howells <dhowells@redhat.com> wrote:
>
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
> > But it's *literally* just finding the places that work with
> > pipe->curbuf/nrbufs and making them use atomic updates.
>
> No.  It really isn't.  That's two variables that describe the occupied section
> of the buffer.  Unless you have something like a 68020 with CAS2, or put them
> next to each other so you can use CMPXCHG8, you can't do that.
>
> They need converting to head/tail pointers first.

You misunderstand - because I phrased it badly. I meant "atomic" in
the traditional kernel sense, as in "usable in not thread context" (eg
GFP_ATOMIC etc).

I'd start out just using a spinlock.

I do agree that we could try to be fancy and do it entirely locklessly
too, and I mentioned that in another part:

 "[..] it should not
  be all that hard to just make the whole "curbuf/nrbufs" handling use
  its own locking (maybe even some lockless atomics and cmpxchg)"

but I also very much agree that it's much more complex.

The main complexity of a lockless thing is actually almost certainly
not in curbuf/nrbufs, because those could easily be packed as two
16-bit values in a 32-bit entity and then regular cmpxchg works fine.

No, the complexity in the lockless model is that then you have to be
very careful with the "buf[]" array update too.  Maybe that's trivial
(just make sure that they are NULL when not used), but it just looks
less than wonderfully easy.

So a lockless update I'm sure is _doable_ with some cleverness, but is
probably not really worth it.

That's particularly true since we already *have* a spinlock that we
would take anyway: the we could strive to use the waitqueue spinlock
in pipe->wait, and not even really add any new locking. That would
require a bit of cleverness too and re-ordering things more, but we do
that in other places (eg completions, but the fs_pin code does it too,
and a few other cases.

Look for "wake_up_locked()" and friends, which is a sure-fire sign
that somebody is playing games and taking the wait-queue lock manually
for their own nefarious reasons.

> > They really would work with almost anything. You could even mix-and-match
> > "data generated by kernel" and "data done by 'write()' or 'splice()' by a
> > user process".
>
> Imagine that userspace writes a large message and takes the mutex.  At the
> same time something in softirq context decides *it* wants to write a message -
> it can't take the mutex and it can't wait, so the userspace write would have
> to cause the kernel message to be dropped.

No. You're missing the point entirely.

The mutex is entirely immaterial for the "insert a message". It is
only used for user-space synchronization. The "add message to the pipe
buffers" would only do the low-level buffer updates (whether using a
new spinlock, re-using the pipe waitqueue lock, or entirely
locklessly, ends up being then just an implementation detail).

Note that user-space writes are defined to be atomic, but they are (a)
not ordered and (b) only atomic up to a single buffer entry (which is
that PIPE_BUF limit). So you can always put in a new buffer entry at
any time.

Obviously if a user space write just fills up the whole queue (or
_other_ messages fill up the whole queue) you'd have to drop the
notification. But that's always true. That's true even in your thing.
The only difference is that we _allow_ other user spaces to write to
the notification queue too.

But if you don't want to allow that, then don't give out the write
side of the pipe to any untrusted user space.

But in *general*, allowing user space to write to the pipe is a great
feature: it means that your notification source *can* be a user space
daemon that you gave the write side of the pipe to (possibly using fd
passing, possibly by just forking your own user-space child or cloning
a thread).

So for example, from a consumer standpoint, you can start off doing
these things in user space with a helper thread that feeds the pipe
(for example, polling /proc/mounts every second), and then when you've
prototyped it and are happy with it, you can add the system call (or
ioctl or whatever) to make the kernel generate the messages so that
you don't have to poll.

But now, once you have the kernel patch, you already have a proven
user, and you can show numbers ("My user-space thing works, but it
uses up 0.1% CPU time and has that nasty up-to-one-second latency
because of polling"). Ta-daa!

End result: it's backwards compatible, it's prototypable, and it's
fairly easily extensible. Want to add a new source of events? Just
pass the pipe to any random piece of code you want. It needs kernel
support only when you've proven the concept _and_ you can show that
"yeah, this user space polling model is a real performance or
complexity problem" or whatever.

This is why I like pipes. You can use them today. They are simple, and
extensible, and you don't need to come up with a new subsystem and
some untested ad-hoc thing that nobody has actually used.

And they work automatically with all the existing infrastructure. They
work with whatever perl or shell scripts, they work with poll/select
loops, they work with user-space sources of events, they are just very
flexible.

                     Linus

  reply index

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-04 22:15 [PATCH 00/11] Keyrings, Block and USB notifications [ver #8] David Howells
2019-09-04 22:15 ` [PATCH 01/11] uapi: General notification ring definitions " David Howells
2019-09-04 22:16 ` [PATCH 02/11] security: Add hooks to rule on setting a watch " David Howells
2019-09-04 22:16 ` [PATCH 03/11] security: Add a hook for the point of notification insertion " David Howells
2019-09-04 22:16 ` [PATCH 04/11] General notification queue with user mmap()'able ring buffer " David Howells
2019-09-04 22:16 ` [PATCH 05/11] keys: Add a notification facility " David Howells
2019-09-04 22:16 ` [PATCH 06/11] Add a general, global device notification watch list " David Howells
2019-09-04 22:16 ` [PATCH 07/11] block: Add block layer notifications " David Howells
2019-09-04 22:16 ` [PATCH 08/11] usb: Add USB subsystem " David Howells
2019-09-04 22:17 ` [PATCH 09/11] Add sample notification program " David Howells
2019-09-04 22:17 ` [PATCH 10/11] selinux: Implement the watch_key security hook " David Howells
2019-09-04 22:17 ` [PATCH 11/11] smack: Implement the watch_key and post_notification hooks " David Howells
2019-09-04 22:28 ` [PATCH 00/11] Keyrings, Block and USB notifications " Linus Torvalds
2019-09-05 17:01 ` Why add the general notification queue and its sources David Howells
2019-09-05 17:19   ` Linus Torvalds
2019-09-05 18:32     ` Ray Strode
2019-09-05 20:39       ` Linus Torvalds
2019-09-06 19:32         ` Ray Strode
2019-09-06 19:41           ` Ray Strode
2019-09-06 19:53           ` Robbie Harwood
2019-09-05 21:32       ` David Howells
2019-09-05 22:08         ` Linus Torvalds
2019-09-05 23:18         ` David Howells
2019-09-06  0:07           ` Linus Torvalds
2019-09-06 10:09           ` David Howells
2019-09-06 15:35             ` Linus Torvalds [this message]
2019-09-06 15:53               ` Linus Torvalds
2019-09-06 16:12                 ` Steven Whitehouse
2019-09-06 17:07                   ` Linus Torvalds
2019-09-06 17:14                     ` Linus Torvalds
2019-09-06 21:19                       ` David Howells
2019-09-06 17:14                   ` Andy Lutomirski
2019-09-05 18:37     ` Steven Whitehouse
2019-09-05 18:51       ` Ray Strode
2019-09-05 20:09         ` David Lehman
2019-09-05 18:33   ` Greg Kroah-Hartman

Reply instructions:

You may reply publically 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='CAHk-=wiR1fpahgKuxSOQY6OfgjWD+MKz8UF6qUQ6V_y2TC_V6w@mail.gmail.com' \
    --to=torvalds@linux-foundation.org \
    --cc=christian@brauner.io \
    --cc=debarshi.ray@gmail.com \
    --cc=dhowells@redhat.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=keyrings@vger.kernel.org \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-block@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=nicolas.dichtel@6wind.com \
    --cc=raven@themaw.net \
    --cc=rharwood@redhat.com \
    --cc=rstrode@redhat.com \
    --cc=swhiteho@redhat.com \
    --cc=viro@zeniv.linux.org.uk \
    /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

Linux-Security-Module Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-security-module/0 linux-security-module/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-security-module linux-security-module/ https://lore.kernel.org/linux-security-module \
		linux-security-module@vger.kernel.org linux-security-module@archiver.kernel.org
	public-inbox-index linux-security-module

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-security-module


AGPL code for this site: git clone https://public-inbox.org/ public-inbox