Re: [GIT PULL] kdbus for 4.1-rc1

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Wed, Apr 22, 2015 at 10:58:28AM +0200, Borislav Petkov wrote:
> On Mon, Apr 13, 2015 at 02:29:35PM -0500, Eric W. Biederman wrote:
> > And the code that transfers the meta-data is wrong.
> > 
> > It is generally not something that userspace requires today, certainly
> > userspace is not using it.
> > 
> > You are exporting a weird set of information in a unique way that makes
> > it race free enough to make ``security'' decisions upon but the data
> > in general is not appropriate to make those decisions.
> > 
> > I remain opposed to this half thought out trash of an ABI for the
> > meta-data.
> > 
> > Just because something happens to be exported in a DEBUG api today does
> > not make it appropriate for userspace to run around making security
> > decisions with that information.
> > 
> > Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>
> > 
> > I think it is premature to be merging kdbus.  You have fuddamental
> > issues that can not be fixed once the ABI is frozen.
> > 
> > The semantics of the meta-data you export are extremely poorly defined.
> 
> Not only that - it looks like a serious amount of work on each sent
> packet. So I did some staring, correct me if I missed something:
> 
> kdbus_cmd_send	- KDBUS_CMD_SEND, ioctl cmd, copy stuff from userspace
> |-> kdbus_kmsg_new_from_cmd(), kmalloc+memset + prepare a *lot* of stuff like:
>     |-> m->proc_meta = kdbus_meta_proc_new();
> 	m->conn_meta = kdbus_meta_conn_new();
> 	...
>     |-> kdbus_bus_broadcast(conn->ep->bus, conn, kmsg); let's look at the broadcast mode
>         |-> hash_for_each(bus->conn_hash, i, conn_dst, hentry) { 	iterate over hash buckets, O(256)

I don't know what O(256) means here, O notation usually is used to
show the complexity of a function, so this really is almost always the
same amount of time, based on using the hash function.  I've never seen
a number in O() before, but I went to school a long time ago, and
probably forgot something...

Or am I misunderstanding your note here?

> 	    |-> kdbus_meta_proc_collect(kmsg->proc_meta, attach_flags);	collect a *lot* of stuff from current etc
> 	    |-> kdbus_meta_conn_collect(kmsg->conn_meta, kmsg, conn_src, attach_flags); collect more stuff
> 
> and this happens on *every* send. A *lot* of work.

Yes, these looks like a lot of stuff but it's still really fast.  And we
need it.

> Now multiply that by the amount of messages this thing is going to send
> per second. It piles up. So you have the overhead right then and there
> in the design without even being able to fix it. Or at least pretty damn
> hard to fix.

It's way faster than what we have today, and David has found a few
areas that can go faster, so I don't really understand the objection.
If you can come up with a faster way to do this, that would be great and
most appreciated.

> So unless I'm missing something, this right there is a design problem.
> 
> Why can't this messaging be done with a nifty O(1) scheme like sending
> parties issuing auth tokens and whatever and the kernel doing the
> arbitration and distribution of those tokens?

Hm, this seems to be to be O(1), pretty constant, we do the same amount
of work all the time.  Then we send the message to the people listening
to it (so that is O(n) depending on the number of listeners, really the
best that I think you can get).

Or am I misunderstanding what you are asking for here?

> That gets you sandboxing, dropping privileges and whatever else fancy
> containers people wanna do for free. Token recipient has the token -
> that's all that counts.

I don't understand what a token provides that is different from what is
happening here, please explain.  How can that be faster than what we do
today?

confused,

greg k-h