linux-kernel - Re: [PATCH v1] add binder genl for txn report

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CANBPYPjCobjbtyKoFJHAciE3=m0Z_QeDTdD9C=ggCcvaWy0daA@mail.gmail.com>
Date: Mon, 12 Aug 2024 23:16:27 -0700
From: Li Li <dualli@...omium.org>
To: Greg KH <gregkh@...uxfoundation.org>
Cc: dualli@...gle.com, arve@...roid.com, tkjos@...roid.com, maco@...roid.com, 
	joel@...lfernandes.org, brauner@...nel.org, cmllamas@...gle.com, 
	surenb@...gle.com, arnd@...db.de, masahiroy@...nel.org, 
	devel@...verdev.osuosl.org, linux-kernel@...r.kernel.org, hridya@...gle.com, 
	smoreland@...gle.com, kernel-team@...roid.com
Subject: Re: [PATCH v1] add binder genl for txn report

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.

I'm happy to rename it to binder_netlink.c if that's easier to remember.

>
>
> >
> > Usage example (user space pseudo code):
> >
> >       // enable binder report from the interested binder context(s)
> >       struct binder_report_info info = {0, BINDER_REPORT_ALL};
> >       ioctl(binder1, BINDER_ENABLE_REPORT, &info);
> >       ioctl(binder2, BINDER_ENABLE_REPORT, &info);
> >
> >       // set optional per-process report, overriding the global one
> >       struct binder_report_info info2;
> >       info2.pid = getpid();
> >       info2.flags = BINDER_REPORT_FAILED | BINDER_REPORT_OVERRIDE;
> >       ioctl(binder2, BINDER_ENABLE_REPORT, &info2);
> >
> >       // open netlink socket
> >       int fd = socket(AF_NETLINK, SOCK_RAW, NETLINK_GENERIC);
> >
> >       // bind netlink socket
> >       bind(fd, struct socketaddr);
> >
> >       // get the family id of binder generic netlink
> >       send(fd, CTRL_CMD_GETFAMILY, CTRL_ATTR_FAMILY_NAME);
> >       int id = recv(CTRL_ATTR_FAMILY_ID);
> >
> >       // register the current process to receive binder reports
> >       send(fd, id, BINDER_GENL_CMD_REGISTER);
> >
> >       // verify registration by reading back the registered pid
> >       recv(fd, BINDER_GENL_ATTR_PID, &pid);
> >
> >       // wait and read all binder reports
> >       while (running) {
> >               struct binder_report report;
> >               recv(fd, BINDER_GENL_ATTR_REPORT, &report);
> >
> >               // process struct binder_report
> >               do_something(&report);
> >       }
> >
> >       // clean up
> >       close(fd);
>
> 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.

>
> 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.

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?

>
> 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.

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

Thanks,
Li