linux-kernel - Re: [PATCH net-next v10 2/2] binder: report txn errors via generic netlink

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: <CANBPYPhZ-_5=VMRoBxbfVb+AFb_qu49QH_hKOiSjX93E1GQA8A@mail.gmail.com>
Date: Mon, 16 Dec 2024 10:58:10 -0800
From: Li Li <dualli@...omium.org>
To: Carlos Llamas <cmllamas@...gle.com>
Cc: dualli@...gle.com, corbet@....net, davem@...emloft.net, 
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com, 
	donald.hunter@...il.com, gregkh@...uxfoundation.org, arve@...roid.com, 
	tkjos@...roid.com, maco@...roid.com, joel@...lfernandes.org, 
	brauner@...nel.org, surenb@...gle.com, arnd@...db.de, masahiroy@...nel.org, 
	bagasdotme@...il.com, horms@...nel.org, linux-kernel@...r.kernel.org, 
	linux-doc@...r.kernel.org, netdev@...r.kernel.org, hridya@...gle.com, 
	smoreland@...gle.com, kernel-team@...roid.com
Subject: Re: [PATCH net-next v10 2/2] binder: report txn errors via generic netlink

On Mon, Dec 16, 2024 at 10:12 AM Carlos Llamas <cmllamas@...gle.com> wrote:
>
> On Thu, Dec 12, 2024 at 02:41:14PM -0800, Li Li wrote:
> > ---
> >  Documentation/admin-guide/binder_genl.rst     | 110 ++++++++
>
> Thanks for renaming to "Binder Netlink" this seems much better IMO.
> Also, I belive the documentation should also be binder_netlink.rst in
> such case?
>

Will also change it.

> > +++ b/Documentation/netlink/specs/binder_netlink.yaml
> > @@ -0,0 +1,108 @@
> > +# SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause)
>
> I think you need a Copyright for this. I'm not sure if it would also be
> needed for the Documentation though.
>

Hmm, all other yaml files follow the same code style.

Netlink experts, can you please clarify this?

> > +/**
> > + * binder_find_proc() - set binder report flags
>
> the description of "binder report flags" is no longer accurate here.

Good catch! Will fix that.

>
> > + * @pid:     the target process
> > + */
> > +static struct binder_proc *binder_find_proc(int pid)
> > +{
> > +     struct binder_proc *proc;
> > +
> > +     mutex_lock(&binder_procs_lock);
> > +     hlist_for_each_entry(proc, &binder_procs, proc_node) {
> > +             if (proc->pid == pid) {
> > +                     mutex_unlock(&binder_procs_lock);
> > +                     return proc;
>
> fwiw, the for_each stops when proc is NULL, so you can just break and
> return proc everytime. e.g.:
>
>         mutex_lock(&binder_procs_lock);
>         hlist_for_each_entry(proc, &binder_procs, proc_node) {
>                 if (proc->pid == pid)
>                         break;
>         }
>         mutex_unlock(&binder_procs_lock);
>
>         return proc;
>

Got it. Thx

> >  /**
> >   * struct binder_device - information about a binder device node
> > - * @hlist:          list of binder devices (only used for devices requested via
> > - *                  CONFIG_ANDROID_BINDER_DEVICES)
> > + * @hlist:          list of binder devices
>
> This is the hunk that needs to go on the first 1/2 patch.

Sure.

> >  /**
> > diff --git a/drivers/android/binder_netlink.c b/drivers/android/binder_netlink.c
> > new file mode 100644
> > index 000000000000..2081b4319268
> > --- /dev/null
> > +++ b/drivers/android/binder_netlink.c
> > @@ -0,0 +1,39 @@
> > +// SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause)
> > +/* Do not edit directly, auto-generated from: */
> > +/*   Documentation/netlink/specs/binder_netlink.yaml */
> > +/* YNL-GEN kernel source */
> > +
> > +#include <net/netlink.h>
> > +#include <net/genetlink.h>
> > +
> > +#include "binder_netlink.h"
> > +
> > +#include <uapi/linux/android/binder_netlink.h>
> > +
> > +/* BINDER_NETLINK_CMD_REPORT_SETUP - do */
> > +static const struct nla_policy binder_netlink_report_setup_nl_policy[BINDER_NETLINK_A_CMD_FLAGS + 1] = {
> > +     [BINDER_NETLINK_A_CMD_CONTEXT] = { .type = NLA_NUL_STRING, },
> > +     [BINDER_NETLINK_A_CMD_PID] = { .type = NLA_U32, },
> > +     [BINDER_NETLINK_A_CMD_FLAGS] = NLA_POLICY_MASK(NLA_U32, 0xf),
> > +};
> > +
> > +/* Ops table for binder_netlink */
> > +static const struct genl_split_ops binder_netlink_nl_ops[] = {
>
> not: There are several places where you have "netlink_nl" which seems
> kind of redundant to me. wdyt? IMO you should drop the "nl" in all of
> these cases.
>

These are automatically generated from the yaml file. So let's just
keep them as is.
But it's a good suggestion to the owner of yaml parser.