On Mon, Dec 9, 2024 at 4:44 PM Carlos Llamas <cmllamas@xxxxxxxxxx> wrote: > > On Mon, Dec 09, 2024 at 11:22:47AM -0800, Li Li wrote: > > From: Li Li <dualli@xxxxxxxxxx> > > > > 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@xxxxxxxxxx> > > --- > > 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
