Message-ID: <2024081350-establish-direness-38ee@gregkh>
Date: Tue, 13 Aug 2024 08:28:08 +0200
From: Greg KH <gregkh@...uxfoundation.org>
To: Li Li <dualli@...omium.org>
Cc: dualli@...gle.com, devel@...verdev.osuosl.org, brauner@...nel.org,
	kernel-team@...roid.com, tkjos@...roid.com, arnd@...db.de,
	surenb@...gle.com, masahiroy@...nel.org, cmllamas@...gle.com,
	linux-kernel@...r.kernel.org, hridya@...gle.com, arve@...roid.com,
	smoreland@...gle.com, joel@...lfernandes.org, maco@...roid.com
Subject: Re: [PATCH v1] add binder genl for txn report

On Mon, Aug 12, 2024 at 11:16:27PM -0700, Li Li wrote:
> On Mon, Aug 12, 2024 at 10:13 PM Greg KH <gregkh@...uxfoundation.org> wrote:
> >
> > On Mon, Aug 12, 2024 at 02:18:44PM -0700, Li Li wrote:
> > > From: Li Li <dualli@...gle.com>
> >
> > Sorry, but I can not parse your Subject: line at all.  Please use
> > vowels, we don't have a lack of them :)
> >
> > Also look at how other patches are formatted for these files to get an
> > idea of how to create a good subject line.
> 
> Thank you for reviewing the patch!
> 
> Sure, I'll find a more meaningful subject in v2.
> 
> > > To prevent making the already bloated binder.c even bigger, a new source
> > > file binder_genl.c is created to host those generic netlink code.
> >
> > "genl" is a rough abbreviation that is not going to be easy to remember,
> > what's wrong with "binder_netlink.c"?
> 
> It's just because genl has already been used in both of generic netlink
> kernel code (e.g. in linux/include/net/genetlink.h) and user space libraries
> https://man7.org/linux/man-pages/man8/genl.8.html.

Ah, I wasn't aware of the existing names, so that's fine if it is what
the networking world is used to.

Which reminds me, why aren't you asking for their review here as well to
ensure that you are doing things with netlink correctly?

> > What userspace code is now going to use this and require it?  How was it
> > tested?  Where is the test code?  Where is the new user/kernel api that
> > you created here documented?
> 
> As mentioned in the commit message, binder is used in Android OS. But the
> user space administration process can do little to deal with binder transaction
> errors. This is tested with Android. I'll upload the user space code to AOSP.
> If there's a better option to host the test code, e.g. a smaller and
> simpler project
> that uses binder, please let me know.

It needs to be somewhere, otherwise we don't know how any of this is
being used at all.  And there was some binder "test code" somewhere, is
this new functionality added to that also?

> > You added a new ioctl here as well, why not mention that?  Why is it
> > needed?  Why not always emit netlink messages?  How do you turn them
> > off?
> 
> The generic netlink message is a convenient way for the kernel driver to send
> information to user space. Technically it's possible to replace this
> new ioctl with
> a netlink message. But that requires much more unnecessary code parsing the
> raw byte streams and accessing internal binder_proc and other structures from
> netlink layer. It's much more elegant to use an ioctl with only a
> couple lines of
> source code.

Then you need to document that somewhere.

> To turn them off, just call the same ioctl, resetting the flags to 0.
> That said, the
> name of this new ioctl (BINDER_ENABLE_REPORT) isn't good enough.
> What do you think if I change it to BINDER_SET_NETLINK_REPORT?

Yes, the name needs to change if you can both enable and disable reports
from it.

> > And how does this deal with binder namespaces?  Are these messages all
> > now "global" somehow?  Or are they separated properly per namespace?
> 
> The new ioctl BINDER_ENABLE_REPORT (again, it deserves a better name)
> sets the report_flags for its associated binder context. Each binder context has
> its own settings. The messages from all binder contexts are sent to user space
> via the same netlink socket. The user space code can enable the reports for
> each binder context separately and distinguish the netlink messages by the
> name of the binder context.

So userspace will now get all reports and has to do the parsing to
determine what is, and is not, relevant for what they want?  That feels
like a big security hole to me, has this been audited by the Android
security team yet?

> It's also possible to have one netlink socket for each binder context.
> But it seems
> like overkill to me.

Again, think of security issues please.  Do you want all binder
processes in a system to be able to see all other messages?

thanks,

greg k-h

Powered by blists - more mailing lists