Date:	Thu, 23 Apr 2015 10:34:32 -0700
From:	Andy Lutomirski <luto@...capital.net>
To:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Arnd Bergmann <arnd@...db.de>,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	One Thousand Gnomes <gnomes@...rguk.ukuu.org.uk>,
	Tom Gundersen <teg@...m.no>, Jiri Kosina <jkosina@...e.cz>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Daniel Mack <daniel@...que.org>,
	David Herrmann <dh.herrmann@...il.com>,
	Djalal Harouni <tixxdz@...ndz.org>
Subject: Re: [GIT PULL] kdbus for 4.1-rc1

On Thu, Apr 23, 2015 at 10:16 AM, Greg Kroah-Hartman
<gregkh@...uxfoundation.org> wrote:
> On Thu, Apr 23, 2015 at 09:46:22AM -0700, Andy Lutomirski wrote:
>> On Thu, Apr 23, 2015 at 9:36 AM, Greg Kroah-Hartman
>> <gregkh@...uxfoundation.org> wrote:
>> > On Thu, Apr 23, 2015 at 03:05:48PM +0200, Greg Kroah-Hartman wrote:
>> >>
>> >> Andy's concerns about the capability stuff has been hashed out in
>> >> multiple threads here.  The kernel code isn't buggy as-designed or
>> >> implemented from what we can all tell, it's just that the new
>> >> functionality isn't liked by everyone, which is totally fair, but not a
>> >> reason to declare that the function isn't useful.
>> >
>> > Andy, did I capture your existing position correctly?  If we drop the
>> > caps metadata, I'm guessing that you are ok with the code as you have
>> > reviewed it and tested it out.  So should I just add a small patch that
>> > removes this for now?  After that, we can discuss the addition of
>> > capabilities to the metadata as an add-on feature with a future patch
>> > and not hold up this larger merge request?
>>
>> No.  I can fish out lists I've posted of what I personally dislike.
>> To repeat from my not-yet-awake memory, briefly:
>>
>>  - starttime, cmdline, and possibly other pieces of metadata are also
>> problematic.  I think starttime is especially bad because it both
>> breaks CRIU and is IMO completely unnecessary -- I sent out draft
>> "highpid" patches a while ago to give a much better alternative that
>> isn't racy and won't break CRIU.  But cmdline is also IMO ridiculous.
>
> starttime was removed a while ago, are you sure you are looking at the
> latest code?

No, I'm sure I haven't.  I looked at the latest code just long enough
to see that caps were still there.  So the latest code is unreviewed
by me or, as far as I can tell, by anyone else who should review it.

>
> cmdline has been discussed and it really helps with debugging.
> Decisions aren't being made based on it.

This might be addressed by the module parameter.  Haven't checked
recent versions.

None of this addresses the fact that metadata is captured both at send
and connect time.  I still think that this is asking for tons of
security problems down the line.

>
>>  - There's still an open performance question.  Namely: is kdbus performant?
>
> Yes, I thought that was already answered.  Tizen posted some numbers
> with a much older version of the code, before David fixed a bunch of
> issues that he and you found, and that averaged between 25-50% faster.
> Details are in this presentation:
>         http://download.tizen.org/misc/media/conference2014/slides/tdc2014-kdbus-in-tizen3.pdf
>

AFAICS no one has ever even tried to address whether the kdbus design
(shmem pools, send-time metadata, plus optional memfd) gives as good
performance as plain ol' sockets.  A lot of the complexity of kdbus is
due to its novel buffering scheme, and that scheme AFAICS has only
been seriously benchmarked against userspace dbus, which is a poor
reference.  I neither see any compelling a priori reason to think that
the buffering scheme is a performance win, nor do I see good numbers.
Instead, I've seen numbers suggesting that it's much slower than
AF_UNIX peer to peer.

I realize that it looks like I'm comparing apples (peer to peer) to
oranges (bus), but that's just because AF_UNIX really is the best
comparison in the absence of a serious attempt at a socket-like bus
with benchmarks.

> The Tizen and GENIVI developers are off running numbers with the latest
> code, or so they told me through emails, but I don't know when/if that
> will ever happen, so I can't promise more than what is already here.
>
>>  - The policy system still sucks.  Now, if we give up on the idea of
>> anyone ever using it for anything other than dbus as it currently
>> works, maybe this isn't a real problem.
>
> As designed, it's for D-Bus, so there's not much I can suggest here,
> this isn't a "generic IPC" :)

Move it to userspace with a daemon that answers policy questions and
makes introductions?

>
>>  - Someone should probably convince someone who understands memory
>> accounting that the pool mechanism accounts memory acceptably.  I
>> don't know much about mm stuff, but I think it's subject to all kinds
>> of nasty latency and accounting abuses, some of which might even be
>> exploited by accident.
>
> Michal and David agree that this all works properly.  I don't know of
> anyone else to ask about it, do you?

I thought Michal wasn't a little less convinced.  I really don't see
why pages allocated due to sends would be charged to the receiver, nor
do I see why, even if that were fixed, it wouldn't be a serious
performance problem with memcgs and memory pressure in play.

I'm really surprised that GENIVI is okay with this.  The latency seems
like it will be highly unpredictable.

>
>> I haven't reviewed most of it.  I've reviewed the metadata code (and
>> not recently) and the pool *docs*.
>>
>> Shouldn't the bulk of this code have actual review before it gets
>> merged?  I've only reviewed some of it, and I didn't like what I found
>> in that small fraction, hence my objections to caps.
>
> I'd love more review, and we have been asking for it since last October.
> You provided a lot of it a while ago, and that helped immensely.
>
> I can't force anyone to read the code, I can only go on what people
> offer to do.  We have 3 signed-off-bys on the main kdbus patches, and
> numerous other different developers have provided fixes / tweaks that
> are in this tree, so it's not like this is unread/unposted code here at
> all.

I think it doesn't help that reviewing the code can be a painful
exercise when threads about a single review point drag on for hundreds
of posts.  Also, it's discouraging that, after a single review point
results in hundreds of posts, reviewers get asked whether everything's
okay now.

--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists