Message-ID: <CANBPYPgU9uL9jdxqsri=NwLTJcFpzdB313QsYjSQAuopRppTDw@mail.gmail.com>
Date: Mon, 9 Dec 2024 17:26:14 -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 v9 1/1] binder: report txn errors via generic netlink

On Mon, Dec 9, 2024 at 4:44 PM Carlos Llamas <cmllamas@...gle.com> wrote:
>
> On Mon, Dec 09, 2024 at 11:22:47AM -0800, Li Li wrote:
> > From: Li Li <dualli@...gle.com>
> >
> > Frozen tasks can't process binder transactions, so sync binder
> > transactions will fail with BR_FROZEN_REPLY and async binder
> > transactions will be queued in the kernel async binder buffer.
> > As these queued async transactions accumulates over time, the async
> > buffer will eventually be running out, denying all new transactions
> > after that with BR_FAILED_REPLY.
> >
> > In addition to the above cases, different kinds of binder error codes
> > might be returned to the sender. However, the core Linux, or Android,
> > system administration process never knows what's actually happening.
>
> I don't think the previous two paragraphs provide anything meaninful
> and the explanation below looks enough IMO. I would just drop the noise.

That makes sense. I'll remove them. Thanks!

>
> >
> > Introduce generic netlink messages into the binder driver so that the
> > Linux/Android system administration process can listen to important
> > events and take corresponding actions, like stopping a broken app from
> > attacking the OS by sending huge amount of spamming binder transactions.
> >
> > The new binder genl sources and headers are automatically generated from
> > the corresponding binder_genl YAML spec. Don't modify them directly.
>
> I assume "genl" comes from "generic netlink". Did you think about using
> just "netlink". IMO it provides better context about what this is about.
>

Yes, "genl" has been widely used in the Linux kernel. But I'm fine to rename
it to just "netlink". I'll change it in v10 unless there's other opinions.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/?qt=grep&q=genl
https://man7.org/linux/man-pages/man8/genl.8.html

> >
> > Signed-off-by: Li Li <dualli@...gle.com>
> > ---
> >  Documentation/admin-guide/binder_genl.rst    |  96 +++++++
>
> We already have a "binderfs" entry. Perhaps, we should just merge your
> Documentation with that one and call it "binder" instead?
> You might want to run this by Christian Brauner though.
>

I'm happy to merge it if the doc maintainers like this idea. Or we can do
it in a separate patch later.

> > +===========================================================
> > +Generic Netlink for the Android Binder Driver (Binder Genl)
> > +===========================================================
> > +
> > +The Generic Netlink subsystem in the Linux kernel provides a generic way for
> > +the Linux kernel to communicate to the user space applications via binder
>
> nit: s/communicate to/communicate with/

Would fix it. Thanks!

>
> > +driver. It is used to report various kinds of binder transactions to user
> > +space administration process. The driver allows multiple binder devices and
>
> The transactions types that I'm familiar with are sync/async. I think
> you want to say "report transaction errors" or something like that
> instead?
>

Yes, it means the transaction error code. I'll make it clearer.

> > +their corresponding binder contexts. Each context has an independent Generic
> > +Netlink for security reason. To prevent untrusted user applications from
> > +accessing the netlink data, the kernel driver uses unicast mode instead of
> > +multicast.
> > +
> > +Basically, the user space code uses the "set" command to request what kind
>
> Can you use the actual command? e.g. BINDER_GENL_CMD_SET
>
> BTW, why set? what are we setting? Would *_REPORT_SETUP be more
> appropriate?
>

Hmm, I intentionally make them short in the netlink YAML file.
Otherwise the generated code/name is quite long. But if this is
causing confusion, I'm happy to use a more descriptive (and longer)
name.

> > +of binder transactions should be reported by the kernel binder driver. The
> > +driver then echoes the attributes in a reply message to acknowledge the
> > +request. The "set" command also registers the current user space process to
> > +receive the reports. When the user space process exits, the previous request
> > +will be reset to prevent any potential leaks.
> > +
> > +Currently the driver can report binder transactions that "failed" to reach
> > +the target process, or that are "delayed" due to the target process being
>
> "Delayed" transaction is an entirely new concept. I suppose it means
> async + frozen. Why not use that?
>
> Also, per this logic it seems that a "delayed" transaction could also be
> "spam" correct? e.g. the flags are not mutually exclusive.

It depends on the actual implementation. Currently each binder
transaction only returns one single error code. A "spam" one also
indicates it's a "delayed" one.

>
> > +frozen by cgroup freezer, or that are considered "spam" according to existing
> > +logic in binder_alloc.c.
> > +
> > +When the specified binder transactions happen, the driver uses the "report"
> > +command to send a generic netlink message to the registered process,
> > +containing the payload struct binder_report.
> > +
> > +More details about the flags, attributes and operations can be found at the
> > +the doc sections in Documentations/netlink/specs/binder_genl.yaml and the
> > +kernel-doc comments of the new source code in binder.{h|c}.
> > +
> > +Using Binder Genl
> > +-----------------
> > +
> > +The Binder Genl can be used in the same way as any other generic netlink
> > +drivers. Userspace application uses a raw netlink socket to send commands
> > +to and receive packets from the kernel driver.
> > +
> > +.. note::
> > +    If the userspace application that talks to the driver exits, the kernel
> > +    driver will automatically reset the configuration to the default and
> > +    stop sending more reports to prevent leaking memory.
>
> I'm not sure what you mean by preventing memory leaks. What happens when
> userspace setups the report and doesn't call "recv()"? Is that what we
> are worried about?
>

Probably "leaking memory" isn't accurate here. If the user app
doesn't call recv(), the netlink message would just fail to send.
There's no memleak. But I think it's a good idea to reset the
configuration. Let me describe it in a better way.

> > +
> > +Usage example (user space pseudo code):
> > +
> > +::
> > +
> > +    // open netlink socket
> > +    int fd = socket(AF_NETLINK, SOCK_RAW, NETLINK_GENERIC);
> > +
> > +    // bind netlink socket
> > +    bind(fd, struct socketaddr);
> > +
> > +    // get the family id of the binder genl
> > +    send(fd, CTRL_CMD_GETFAMILY, CTRL_ATTR_FAMILY_NAME,
> > +            BINDER_GENL_FAMILY_NAME);
>
> ok, what is happening here? this is not a regular send(). Is this
> somehow an overloaded send()? If so, I had a really hard time trying to
> figuring that out so might be best to rename this.
>

This pseudo code means a few attributes are sent by a single send().

> > +     if (flags != (flags & (BINDER_GENL_FLAG_OVERRIDE
> > +                     | BINDER_GENL_FLAG_FAILED
> > +                     | BINDER_GENL_FLAG_DELAYED
> > +                     | BINDER_GENL_FLAG_SPAM))) {
> > +             pr_err("Invalid binder report flags: %u\n", flags);
> > +             return -EINVAL;
> > +     }
>
> didn't Jakub mentioned this part wasn't needed?
>

Good catch! I removed them but somehow didn't commit the change.


> > +int binder_genl_nl_set_doit(struct sk_buff *skb, struct genl_info *info)
> > +{
> > +     int portid;
> > +     u32 pid;
> > +     u32 flags;
> > +     void *hdr;
> > +     struct binder_device *device;
> > +     struct binder_context *context = NULL;
>
> nit: would you mind using reverse christmas tree for this variables
> and also in other functions too?
>

Sure.

> > +
> > +     hlist_for_each_entry(device, &binder_devices, hlist) {
> > +             if (!nla_strcmp(info->attrs[BINDER_GENL_A_CMD_CONTEXT],
> > +                             device->context.name)) {
> > +                     context = &device->context;
> > +                     break;
> > +             }
> > +     }
> > +
> > +     if (!context) {
> > +             NL_SET_ERR_MSG(info->extack, "Unknown binder context\n");
> > +             return -EINVAL;
> > +     }
> > +
> > +     portid = nlmsg_hdr(skb)->nlmsg_pid;
> > +     pid = nla_get_u32(info->attrs[BINDER_GENL_A_CMD_PID]);
> > +     flags = nla_get_u32(info->attrs[BINDER_GENL_A_CMD_FLAGS]);
> > +
> > +     if (context->report_portid && context->report_portid != portid) {
> > +             NL_SET_ERR_MSG_FMT(info->extack,
> > +                                "No permission to set flags from %d\n",
> > +                                portid);
> > +             return -EPERM;
> > +     }
> > +
> > +     if (binder_genl_set_report(context, pid, flags) < 0) {
> > +             pr_err("Failed to set report flags %u for %u\n", flags, pid);
> > +             return -EINVAL;
> > +     }
>
> With the flags check being unnecessary you probably want to fold
> binder_genl_set_report() here instead.
>

Sorry, I don't quite understand this request. Can you please explain it?

> > +/**
> > + * Add a binder device to binder_devices
> > + * @device: the new binder device to add to the global list
> > + *
> > + * Not reentrant as the list is not protected by any locks
> > + */
> > +void binder_add_device(struct binder_device *device)
> > +{
> > +     hlist_add_head(&device->hlist, &binder_devices);
> > +}
>
> nit: would you mind separating the binder_add_device() logic into a
> separate "prep" commit?
>

Sure.

> > +
> >  static int __init init_binder_device(const char *name)
> >  {
> >       int ret;
> > @@ -6953,6 +7217,7 @@ static int __init init_binder_device(const char *name)
> >       }
> >
> >       hlist_add_head(&binder_device->hlist, &binder_devices);
> > +     binder_device->context.report_seq = (atomic_t)ATOMIC_INIT(0);
>
> I don't think this is meant to be used like this.
>
> Also, binder_device is kzalloc'ed so no need to init report_seq at all.
>

I'll remove this unnecessary code.

> >
>
>
> Also, how is userspace going to determine that this new interface is
> available? Do we need a new entry under binder features? Or is this not
> a problem?

It's not a problem. The generic netlink command "getfamily" will fail.

>
> Regards,
> --
> Carlos Llamas

Powered by blists - more mailing lists