* [PATCH net-next v9 0/1] binder: report txn errors via generic netlink
@ 2024-12-09 19:22 Li Li
2024-12-09 19:22 ` [PATCH net-next v9 1/1] " Li Li
0 siblings, 1 reply; 8+ messages in thread
From: Li Li @ 2024-12-09 19:22 UTC (permalink / raw)
To: dualli, corbet, davem, edumazet, kuba, pabeni, donald.hunter,
gregkh, arve, tkjos, maco, joel, brauner, cmllamas, surenb, arnd,
masahiroy, bagasdotme, horms, linux-kernel, linux-doc, netdev,
hridya, smoreland
Cc: kernel-team
From: Li Li <dualli@google.com>
It's a known issue that neither the frozen processes nor the system
administration process of the OS can correctly deal with failed binder
transactions. The reason is that there's no reliable way for the user
space administration process to fetch the binder errors from the kernel
binder driver.
Android is such an OS suffering from this issue. Since cgroup freezer
was used to freeze user applications to save battery, innocent frozen
apps have to be killed when they receive sync binder transactions or
when their async binder buffer is running out.
This patch introduces the Linux 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 transactiions.
The 1st version uses a global generic netlink for all binder contexts,
raising potential security concerns. There were a few other feedbacks
like request to kernel docs and test code. The thread can be found at
https://lore.kernel.org/lkml/20240812211844.4107494-1-dualli@chromium.org/
The 2nd version fixes those issues and has been tested on the latest
version of AOSP. See https://r.android.com/3305462 for how userspace is
going to use this feature and the test code. It can be found at
https://lore.kernel.org/lkml/20241011064427.1565287-1-dualli@chromium.org/
The 3rd version replaces the handcrafted netlink source code with the
netlink protocal specs in YAML. It also fixes the documentation issues.
https://lore.kernel.org/lkml/20241021182821.1259487-1-dualli@chromium.org/
The 4th version just containsi trivial fixes, making the subject of the
patch aligned with the subject of the cover letter.
https://lore.kernel.org/lkml/20241021191233.1334897-1-dualli@chromium.org/
The 5th version incorporates the suggested fixes to the kernel doc and
the init function. It also removes the unsupported uapi-header in YAML
that contains "/" for subdirectory.
https://lore.kernel.org/lkml/20241025075102.1785960-1-dualli@chromium.org/
The 6th version has some trivial kernel doc fixes, without modifying
any other source code.
https://lore.kernel.org/lkml/20241028101952.775731-1-dualli@chromium.org/
The 7th version breaks the binary struct netlink message into individual
attributes to better support automatic error checking. Thanks Jakub for
improving ynl-gen.
https://lore.kernel.org/all/20241031092504.840708-1-dualli@chromium.org/
The 8th version solves the multi-genl-family issue by demuxing the
messages based on a new context attribute. It also improves the YAML
spec to be consistent with netlink tradition. A Huge 'Thank You' to
Jakub who taught me a lot about the netlink protocol!
https://lore.kernel.org/all/20241113193239.2113577-1-dualli@chromium.org/
The 9th version only contains a few trivial fixes, removing a redundant
pr_err and unnecessary payload check. The ynl-gen patch to allow uapi
header in sub-dirs has been merged so it's no longer included in this
patch set.
v1: add a global binder genl socket for all contexts
v2: change to per-context binder genl for security reason
replace the new ioctl with a netlink command
add corresponding doc Documentation/admin-guide/binder_genl.rst
add user space test code in AOSP
v3: use YNL spec (./tools/net/ynl/ynl-regen.sh)
fix documentation index
v4: change the subject of the patch and remove unsed #if 0
v5: improve the kernel doc and the init function
remove unsupported uapi-header in YAML
v6: fix some trivial kernel doc issues
v7: break the binary struct binder_report into individual attributes
v8: use multiplex netlink message in a unified netlink family
improve the YAML spec to be consistent with netlink tradition
v9: remove unnecessary check to netlink flags and message payloads.
Li Li (1):
binder: report txn errors via generic netlink
Documentation/admin-guide/binder_genl.rst | 96 +++++++
Documentation/admin-guide/index.rst | 1 +
Documentation/netlink/specs/binder_genl.yaml | 108 ++++++++
drivers/android/Kconfig | 1 +
drivers/android/Makefile | 2 +-
drivers/android/binder.c | 277 ++++++++++++++++++-
drivers/android/binder_genl.c | 39 +++
drivers/android/binder_genl.h | 18 ++
drivers/android/binder_internal.h | 27 +-
drivers/android/binder_trace.h | 35 +++
drivers/android/binderfs.c | 2 +
include/uapi/linux/android/binder_genl.h | 55 ++++
12 files changed, 655 insertions(+), 6 deletions(-)
create mode 100644 Documentation/admin-guide/binder_genl.rst
create mode 100644 Documentation/netlink/specs/binder_genl.yaml
create mode 100644 drivers/android/binder_genl.c
create mode 100644 drivers/android/binder_genl.h
create mode 100644 include/uapi/linux/android/binder_genl.h
base-commit: 6145fefc1e42c1895c0c1c2c8593de2c085d8c56
--
2.47.0.338.g60cca15819-goog
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH net-next v9 1/1] binder: report txn errors via generic netlink 2024-12-09 19:22 [PATCH net-next v9 0/1] binder: report txn errors via generic netlink Li Li @ 2024-12-09 19:22 ` Li Li 2024-12-10 0:44 ` Carlos Llamas 2024-12-10 11:35 ` kernel test robot 0 siblings, 2 replies; 8+ messages in thread From: Li Li @ 2024-12-09 19:22 UTC (permalink / raw) To: dualli, corbet, davem, edumazet, kuba, pabeni, donald.hunter, gregkh, arve, tkjos, maco, joel, brauner, cmllamas, surenb, arnd, masahiroy, bagasdotme, horms, linux-kernel, linux-doc, netdev, hridya, smoreland Cc: kernel-team From: Li Li <dualli@google.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. 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. Signed-off-by: Li Li <dualli@google.com> --- Documentation/admin-guide/binder_genl.rst | 96 +++++++ Documentation/admin-guide/index.rst | 1 + Documentation/netlink/specs/binder_genl.yaml | 108 ++++++++ drivers/android/Kconfig | 1 + drivers/android/Makefile | 2 +- drivers/android/binder.c | 277 ++++++++++++++++++- drivers/android/binder_genl.c | 39 +++ drivers/android/binder_genl.h | 18 ++ drivers/android/binder_internal.h | 27 +- drivers/android/binder_trace.h | 35 +++ drivers/android/binderfs.c | 2 + include/uapi/linux/android/binder_genl.h | 55 ++++ 12 files changed, 655 insertions(+), 6 deletions(-) create mode 100644 Documentation/admin-guide/binder_genl.rst create mode 100644 Documentation/netlink/specs/binder_genl.yaml create mode 100644 drivers/android/binder_genl.c create mode 100644 drivers/android/binder_genl.h create mode 100644 include/uapi/linux/android/binder_genl.h diff --git a/Documentation/admin-guide/binder_genl.rst b/Documentation/admin-guide/binder_genl.rst new file mode 100644 index 000000000000..b395d7a9849b --- /dev/null +++ b/Documentation/admin-guide/binder_genl.rst @@ -0,0 +1,96 @@ +.. SPDX-License-Identifier: GPL-2.0 + +=========================================================== +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 +driver. It is used to report various kinds of binder transactions to user +space administration process. The driver allows multiple binder devices and +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 +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 +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. + +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); + void *data = recv(CTRL_CMD_NEWFAMILY); + __u16 id = nla(data)[CTRL_ATTR_FAMILY_ID]; + + // enable per-context binder report + send(fd, id, BINDER_GENL_CMD_SET, "binder", 0, BINDER_GENL_FLAG_FAILED | + BINDER_GENL_FLAG_DELAYED); + + // confirm the per-context configuration + data = recv(fd, BINDER_GENL_CMD_REPLY); + char *context = nla(data)[BINDER_GENL_A_CMD_CONTEXT]; + __u32 pid = nla(data)[BINDER_GENL_A_CMD_PID]; + __u32 flags = nla(data)[BINDER_GENL_A_CMD_FLAGS]; + + // set optional per-process report, overriding the per-context one + send(fd, id, BINDER_GENL_CMD_SET, "binder", getpid(), + BINDER_GENL_FLAG_SPAM | BINDER_REPORT_OVERRIDE); + + // confirm the optional per-process configuration + data = recv(fd, BINDER_GENL_CMD_REPLY); + context = nla(data)[BINDER_GENL_A_CMD_CONTEXT]; + pid = nla(data)[BINDER_GENL_A_CMD_PID]; + flags = nla(data)[BINDER_GENL_A_CMD_FLAGS]; + + // wait and read all binder reports + while (running) { + data = recv(fd, BINDER_GENL_CMD_REPORT); + auto *attr = nla(data)[BINDER_GENL_A_REPORT_XXX]; + + // process binder report + do_something(*attr); + } + + // clean up + send(fd, id, BINDER_GENL_CMD_SET, 0, 0); + send(fd, id, BINDER_GENL_CMD_SET, getpid(), 0); + close(fd); diff --git a/Documentation/admin-guide/index.rst b/Documentation/admin-guide/index.rst index e85b1adf5908..b3b5cfadffe5 100644 --- a/Documentation/admin-guide/index.rst +++ b/Documentation/admin-guide/index.rst @@ -79,6 +79,7 @@ configure specific aspects of kernel behavior to your liking. aoe/index auxdisplay/index bcache + binder_genl binderfs binfmt-misc blockdev/index diff --git a/Documentation/netlink/specs/binder_genl.yaml b/Documentation/netlink/specs/binder_genl.yaml new file mode 100644 index 000000000000..3c6024b9a689 --- /dev/null +++ b/Documentation/netlink/specs/binder_genl.yaml @@ -0,0 +1,108 @@ +# SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause) + +name: binder_genl +protocol: genetlink +uapi-header: linux/android/binder_genl.h +doc: Netlink protocol to report binder transaction errors and warnings. + +definitions: + - + type: flags + name: flag + doc: Define what kind of binder transactions should be reported. + entries: [ failed, delayed, spam, override ] + +attribute-sets: + - + name: cmd + doc: The supported attributes of "set" and "reply" commands + attributes: + - + name: context + type: string + doc: The binder context to enable binder genl report. + - + name: pid + type: u32 + doc: The binder proc to enable binder genl report. + - + name: flags + type: u32 + enum: flag + doc: What kind of binder transactions should be reported. + - + name: report + doc: The supported attributes of "report" command + attributes: + - + name: context + type: string + doc: The binder context where the binder genl report happens. + - + name: err + type: u32 + doc: Copy of binder_driver_return_protocol returned to the sender. + - + name: from_pid + type: u32 + doc: Sender pid of the corresponding binder transaction. + - + name: from_tid + type: u32 + doc: Sender tid of the corresponding binder transaction. + - + name: to_pid + type: u32 + doc: Target pid of the corresponding binder transaction. + - + name: to_tid + type: u32 + doc: Target tid of the corresponding binder transaction. + - + name: reply + type: u32 + doc: 1 means the transaction is a reply, 0 otherwise. + - + name: flags + type: u32 + doc: Copy of binder_transaction_data->flags. + - + name: code + type: u32 + doc: Copy of binder_transaction_data->code. + - + name: data_size + type: u32 + doc: Copy of binder_transaction_data->data_size. + +operations: + list: + - + name: set + doc: Set flags from user space. + attribute-set: cmd + + do: + request: ¶ms + attributes: + - context + - pid + - flags + reply: *params + - + name: report + doc: Send the requested reports to user space. + attribute-set: report + + event: + attributes: + - context + - err + - from_pid + - from_tid + - to_pid + - to_tid + - reply + - flags + - code + - data_size diff --git a/drivers/android/Kconfig b/drivers/android/Kconfig index 07aa8ae0a058..e2fa620934e2 100644 --- a/drivers/android/Kconfig +++ b/drivers/android/Kconfig @@ -4,6 +4,7 @@ menu "Android" config ANDROID_BINDER_IPC bool "Android Binder IPC Driver" depends on MMU + depends on NET default n help Binder is used in Android for both communication between processes, diff --git a/drivers/android/Makefile b/drivers/android/Makefile index c9d3d0c99c25..d818447fbc4c 100644 --- a/drivers/android/Makefile +++ b/drivers/android/Makefile @@ -2,5 +2,5 @@ ccflags-y += -I$(src) # needed for trace events obj-$(CONFIG_ANDROID_BINDERFS) += binderfs.o -obj-$(CONFIG_ANDROID_BINDER_IPC) += binder.o binder_alloc.o +obj-$(CONFIG_ANDROID_BINDER_IPC) += binder.o binder_alloc.o binder_genl.o obj-$(CONFIG_ANDROID_BINDER_IPC_SELFTEST) += binder_alloc_selftest.o diff --git a/drivers/android/binder.c b/drivers/android/binder.c index ef353ca13c35..eaa9b763ef20 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -72,6 +72,7 @@ #include <linux/cacheflush.h> +#include "binder_genl.h" #include "binder_internal.h" #include "binder_trace.h" @@ -2990,6 +2991,150 @@ static void binder_set_txn_from_error(struct binder_transaction *t, int id, binder_thread_dec_tmpref(from); } +/** + * binder_find_proc() - set binder report flags + * @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; + } + } + mutex_unlock(&binder_procs_lock); + + return NULL; +} + +/** + * binder_genl_set_report() - set binder report flags + * @context: the binder context to set the flags + * @pid: the target process + * @flags: the flags to set + * + * If pid is 0, the flags are applied to the whole binder context. + * Otherwise, the flags are applied to the specific process only. + */ +static int binder_genl_set_report(struct binder_context *context, u32 pid, + u32 flags) +{ + struct binder_proc *proc; + + 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; + } + + if (!pid) { + /* Set the global flags for the whole binder context */ + context->report_flags = flags; + } else { + /* Set the per-process flags */ + proc = binder_find_proc(pid); + if (!proc) { + pr_err("Invalid binder report pid %u\n", pid); + return -EINVAL; + } + + proc->report_flags = flags; + } + + return 0; +} + +/** + * binder_genl_report_enabled() - check if binder genl reports are enabled + * @proc: the binder_proc to check + * @mask: the categories of binder genl reports + * + * Returns true if certain binder genl reports are enabled for this binder + * proc (when per-process overriding takes effect) or context. + */ +static bool binder_genl_report_enabled(struct binder_proc *proc, u32 mask) +{ + struct binder_context *context = proc->context; + + if (!context->report_portid) + return false; + + if (proc->report_flags & BINDER_GENL_FLAG_OVERRIDE) + return (proc->report_flags & mask) != 0; + else + return (context->report_flags & mask) != 0; +} + +/** + * binder_genl_send_report() - send one binder genl report + * @context: the binder context + * @err: copy of binder_driver_return_protocol returned to the sender + * @pid: sender process + * @tid: sender thread + * @to_pid: target process + * @to_tid: target thread + * @reply: whether the binder transaction is a reply + * @tr: the binder transaction data + * + * Packs the report data into a "report" binder genl message and send it. + */ +static void binder_genl_send_report(struct binder_context *context, u32 err, + u32 pid, u32 tid, u32 to_pid, u32 to_tid, + u32 reply, + struct binder_transaction_data *tr) +{ + int ret; + struct sk_buff *skb; + void *hdr; + + trace_binder_send_report(context->name, err, pid, tid, to_pid, to_tid, + reply, tr); + + skb = genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_KERNEL); + if (!skb) { + pr_err("Failed to alloc binder genl message\n"); + return; + } + + hdr = genlmsg_put(skb, 0, atomic_inc_return(&context->report_seq), + &binder_genl_nl_family, 0, BINDER_GENL_CMD_REPORT); + if (!hdr) + goto free_skb; + + if (nla_put_string(skb, BINDER_GENL_A_REPORT_CONTEXT, context->name) || + nla_put_u32(skb, BINDER_GENL_A_REPORT_ERR, err) || + nla_put_u32(skb, BINDER_GENL_A_REPORT_FROM_PID, pid) || + nla_put_u32(skb, BINDER_GENL_A_REPORT_FROM_TID, tid) || + nla_put_u32(skb, BINDER_GENL_A_REPORT_TO_PID, to_pid) || + nla_put_u32(skb, BINDER_GENL_A_REPORT_TO_TID, to_tid) || + nla_put_u32(skb, BINDER_GENL_A_REPORT_REPLY, reply) || + nla_put_u32(skb, BINDER_GENL_A_REPORT_FLAGS, tr->flags) || + nla_put_u32(skb, BINDER_GENL_A_REPORT_CODE, tr->code) || + nla_put_u32(skb, BINDER_GENL_A_REPORT_DATA_SIZE, tr->data_size)) + goto cancel_skb; + + genlmsg_end(skb, hdr); + + ret = genlmsg_unicast(&init_net, skb, context->report_portid); + if (ret < 0) + pr_err("Failed to send binder genl message to %d: %d\n", + context->report_portid, ret); + return; + +cancel_skb: + pr_err("Failed to add report attributes to binder genl message\n"); + genlmsg_cancel(skb, hdr); +free_skb: + pr_err("Free binder genl report message on error\n"); + nlmsg_free(skb); +} + static void binder_transaction(struct binder_proc *proc, struct binder_thread *thread, struct binder_transaction_data *tr, int reply, @@ -3684,10 +3829,18 @@ static void binder_transaction(struct binder_proc *proc, return_error_line = __LINE__; goto err_copy_data_failed; } - if (t->buffer->oneway_spam_suspect) + if (t->buffer->oneway_spam_suspect) { tcomplete->type = BINDER_WORK_TRANSACTION_ONEWAY_SPAM_SUSPECT; - else + if (binder_genl_report_enabled(proc, BINDER_GENL_FLAG_SPAM)) + binder_genl_send_report(context, + BR_ONEWAY_SPAM_SUSPECT, + proc->pid, thread->pid, + target_proc ? target_proc->pid : 0, + target_thread ? target_thread->pid : 0, + reply, tr); + } else { tcomplete->type = BINDER_WORK_TRANSACTION_COMPLETE; + } t->work.type = BINDER_WORK_TRANSACTION; if (reply) { @@ -3737,8 +3890,15 @@ static void binder_transaction(struct binder_proc *proc, * process and is put in a pending queue, waiting for the target * process to be unfrozen. */ - if (return_error == BR_TRANSACTION_PENDING_FROZEN) + if (return_error == BR_TRANSACTION_PENDING_FROZEN) { tcomplete->type = BINDER_WORK_TRANSACTION_PENDING; + if (binder_genl_report_enabled(proc, BINDER_GENL_FLAG_DELAYED)) + binder_genl_send_report(context, return_error, + proc->pid, thread->pid, + target_proc ? target_proc->pid : 0, + target_thread ? target_thread->pid : 0, + reply, tr); + } binder_enqueue_thread_work(thread, tcomplete); if (return_error && return_error != BR_TRANSACTION_PENDING_FROZEN) @@ -3800,6 +3960,13 @@ static void binder_transaction(struct binder_proc *proc, binder_dec_node_tmpref(target_node); } + if (binder_genl_report_enabled(proc, BINDER_GENL_FLAG_FAILED)) + binder_genl_send_report(context, return_error, + proc->pid, thread->pid, + target_proc ? target_proc->pid : 0, + target_thread ? target_thread->pid : 0, + reply, tr); + binder_debug(BINDER_DEBUG_FAILED_TRANSACTION, "%d:%d transaction %s to %d:%d failed %d/%d/%d, size %lld-%lld line %d\n", proc->pid, thread->pid, reply ? "reply" : @@ -6137,6 +6304,11 @@ static int binder_release(struct inode *nodp, struct file *filp) binder_defer_work(proc, BINDER_DEFERRED_RELEASE); + if (proc->pid == proc->context->report_portid) { + proc->context->report_portid = 0; + proc->context->report_flags = 0; + } + return 0; } @@ -6335,6 +6507,87 @@ binder_defer_work(struct binder_proc *proc, enum binder_deferred_state defer) mutex_unlock(&binder_deferred_lock); } +/** + * binder_genl_nl_set_doit() - .doit handler for BINDER_GENL_CMD_SET + * @skb: the metadata struct passed from netlink driver + * @info: the generic netlink struct passed from netlink driver + * + * Implements the .doit function to process binder genl commands. + */ +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; + + 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; + } + + skb = genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_KERNEL); + if (!skb) { + pr_err("Failed to alloc binder genl reply message\n"); + return -ENOMEM; + } + + hdr = genlmsg_iput(skb, info); + if (!hdr) + goto free_skb; + + if (nla_put_string(skb, BINDER_GENL_A_CMD_CONTEXT, context->name) || + nla_put_u32(skb, BINDER_GENL_A_CMD_PID, pid) || + nla_put_u32(skb, BINDER_GENL_A_CMD_FLAGS, flags)) + goto cancel_skb; + + genlmsg_end(skb, hdr); + + if (genlmsg_reply(skb, info)) { + pr_err("Failed to send binder genl reply message\n"); + return -EFAULT; + } + + if (!context->report_portid) + context->report_portid = portid; + + return 0; + +cancel_skb: + pr_err("Failed to add reply attributes to binder genl message\n"); + genlmsg_cancel(skb, hdr); +free_skb: + pr_err("Free binder genl reply message on error\n"); + nlmsg_free(skb); + return -EMSGSIZE; +} + static void print_binder_transaction_ilocked(struct seq_file *m, struct binder_proc *proc, const char *prefix, @@ -6928,6 +7181,17 @@ const struct binder_debugfs_entry binder_debugfs_entries[] = { {} /* terminator */ }; +/** + * 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); +} + 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); return ret; } @@ -7009,6 +7274,12 @@ static int __init binder_init(void) if (ret) goto err_init_binder_device_failed; + ret = genl_register_family(&binder_genl_nl_family); + if (ret) { + pr_err("Failed to register binder genl family\n"); + goto err_init_binder_device_failed; + } + return ret; err_init_binder_device_failed: diff --git a/drivers/android/binder_genl.c b/drivers/android/binder_genl.c new file mode 100644 index 000000000000..29d79bdd9649 --- /dev/null +++ b/drivers/android/binder_genl.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_genl.yaml */ +/* YNL-GEN kernel source */ + +#include <net/netlink.h> +#include <net/genetlink.h> + +#include "binder_genl.h" + +#include <uapi/linux/android/binder_genl.h> + +/* BINDER_GENL_CMD_SET - do */ +static const struct nla_policy binder_genl_set_nl_policy[BINDER_GENL_A_CMD_FLAGS + 1] = { + [BINDER_GENL_A_CMD_CONTEXT] = { .type = NLA_NUL_STRING, }, + [BINDER_GENL_A_CMD_PID] = { .type = NLA_U32, }, + [BINDER_GENL_A_CMD_FLAGS] = NLA_POLICY_MASK(NLA_U32, 0xf), +}; + +/* Ops table for binder_genl */ +static const struct genl_split_ops binder_genl_nl_ops[] = { + { + .cmd = BINDER_GENL_CMD_SET, + .doit = binder_genl_nl_set_doit, + .policy = binder_genl_set_nl_policy, + .maxattr = BINDER_GENL_A_CMD_FLAGS, + .flags = GENL_CMD_CAP_DO, + }, +}; + +struct genl_family binder_genl_nl_family __ro_after_init = { + .name = BINDER_GENL_FAMILY_NAME, + .version = BINDER_GENL_FAMILY_VERSION, + .netnsok = true, + .parallel_ops = true, + .module = THIS_MODULE, + .split_ops = binder_genl_nl_ops, + .n_split_ops = ARRAY_SIZE(binder_genl_nl_ops), +}; diff --git a/drivers/android/binder_genl.h b/drivers/android/binder_genl.h new file mode 100644 index 000000000000..9d68c155b7c4 --- /dev/null +++ b/drivers/android/binder_genl.h @@ -0,0 +1,18 @@ +/* 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_genl.yaml */ +/* YNL-GEN kernel header */ + +#ifndef _LINUX_BINDER_GENL_GEN_H +#define _LINUX_BINDER_GENL_GEN_H + +#include <net/netlink.h> +#include <net/genetlink.h> + +#include <uapi/linux/android/binder_genl.h> + +int binder_genl_nl_set_doit(struct sk_buff *skb, struct genl_info *info); + +extern struct genl_family binder_genl_nl_family; + +#endif /* _LINUX_BINDER_GENL_GEN_H */ diff --git a/drivers/android/binder_internal.h b/drivers/android/binder_internal.h index f8d6be682f23..cc745367a519 100644 --- a/drivers/android/binder_internal.h +++ b/drivers/android/binder_internal.h @@ -12,21 +12,35 @@ #include <linux/stddef.h> #include <linux/types.h> #include <linux/uidgid.h> +#include <net/genetlink.h> #include <uapi/linux/android/binderfs.h> #include "binder_alloc.h" #include "dbitmap.h" +/** + * struct binder_context - information about a binder domain + * @binder_context_mgr_node: the context manager + * @context_mgr_node_lock: the lock protecting the above context manager node + * @binder_context_mgr_uid: the uid of the above context manager + * @name: the name of the binder device + * @report_portid: the netlink socket to receive binder reports + * @report_flags: the categories of binder transactions that would + * be reported (see enum binder_report_flag). + * @report_seq: the seq number of the generic netlink report + */ struct binder_context { struct binder_node *binder_context_mgr_node; struct mutex context_mgr_node_lock; kuid_t binder_context_mgr_uid; const char *name; + u32 report_portid; + u32 report_flags; + atomic_t report_seq; }; /** * 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 * @miscdev: information about a binder character device node * @context: binder context information * @binderfs_inode: This is the inode of the root dentry of the super block @@ -415,6 +429,8 @@ struct binder_ref { * @binderfs_entry: process-specific binderfs log file * @oneway_spam_detection_enabled: process enabled oneway spam detection * or not + * @report_flags: the categories of binder transactions that would + * be reported (see enum binder_genl_flag). * * Bookkeeping structure for binder processes */ @@ -453,6 +469,7 @@ struct binder_proc { spinlock_t outer_lock; struct dentry *binderfs_entry; bool oneway_spam_detection_enabled; + u32 report_flags; }; /** @@ -582,4 +599,10 @@ struct binder_object { }; }; +/** + * Add a binder device to binder_devices + * @device: the new binder device to add to the global list + */ +void binder_add_device(struct binder_device *device); + #endif /* _LINUX_BINDER_INTERNAL_H */ diff --git a/drivers/android/binder_trace.h b/drivers/android/binder_trace.h index fe38c6fc65d0..551b6c9d9f73 100644 --- a/drivers/android/binder_trace.h +++ b/drivers/android/binder_trace.h @@ -423,6 +423,41 @@ TRACE_EVENT(binder_return, "unknown") ); +TRACE_EVENT(binder_send_report, + TP_PROTO(const char *name, u32 err, u32 pid, u32 tid, u32 to_pid, + u32 to_tid, u32 reply, struct binder_transaction_data *tr), + TP_ARGS(name, err, pid, tid, to_pid, to_tid, reply, tr), + TP_STRUCT__entry( + __field(const char *, name) + __field(u32, err) + __field(u32, pid) + __field(u32, tid) + __field(u32, to_pid) + __field(u32, to_tid) + __field(u32, reply) + __field(u32, flags) + __field(u32, code) + __field(binder_size_t, data_size) + ), + TP_fast_assign( + __entry->name = name; + __entry->err = err; + __entry->pid = pid; + __entry->tid = tid; + __entry->to_pid = to_pid; + __entry->to_tid = to_tid; + __entry->reply = reply; + __entry->flags = tr->flags; + __entry->code = tr->code; + __entry->data_size = tr->data_size; + ), + TP_printk("%s: %d %d:%d -> %d:%d %s flags=0x08%x code=%d %llu", + __entry->name, __entry->err, __entry->pid, __entry->tid, + __entry->to_pid, __entry->to_tid, + __entry->reply ? "reply" : "", + __entry->flags, __entry->code, __entry->data_size) +); + #endif /* _BINDER_TRACE_H */ #undef TRACE_INCLUDE_PATH diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c index ad1fa7abc323..bc6bae76ccaf 100644 --- a/drivers/android/binderfs.c +++ b/drivers/android/binderfs.c @@ -207,6 +207,8 @@ static int binderfs_binder_device_create(struct inode *ref_inode, fsnotify_create(root->d_inode, dentry); inode_unlock(d_inode(root)); + binder_add_device(device); + return 0; err: diff --git a/include/uapi/linux/android/binder_genl.h b/include/uapi/linux/android/binder_genl.h new file mode 100644 index 000000000000..f39b5c81e477 --- /dev/null +++ b/include/uapi/linux/android/binder_genl.h @@ -0,0 +1,55 @@ +/* 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_genl.yaml */ +/* YNL-GEN uapi header */ + +#ifndef _UAPI_LINUX_ANDROID_BINDER_GENL_H +#define _UAPI_LINUX_ANDROID_BINDER_GENL_H + +#define BINDER_GENL_FAMILY_NAME "binder_genl" +#define BINDER_GENL_FAMILY_VERSION 1 + +/* + * Define what kind of binder transactions should be reported. + */ +enum binder_genl_flag { + BINDER_GENL_FLAG_FAILED = 1, + BINDER_GENL_FLAG_DELAYED = 2, + BINDER_GENL_FLAG_SPAM = 4, + BINDER_GENL_FLAG_OVERRIDE = 8, +}; + +enum { + BINDER_GENL_A_CMD_CONTEXT = 1, + BINDER_GENL_A_CMD_PID, + BINDER_GENL_A_CMD_FLAGS, + + __BINDER_GENL_A_CMD_MAX, + BINDER_GENL_A_CMD_MAX = (__BINDER_GENL_A_CMD_MAX - 1) +}; + +enum { + BINDER_GENL_A_REPORT_CONTEXT = 1, + BINDER_GENL_A_REPORT_ERR, + BINDER_GENL_A_REPORT_FROM_PID, + BINDER_GENL_A_REPORT_FROM_TID, + BINDER_GENL_A_REPORT_TO_PID, + BINDER_GENL_A_REPORT_TO_TID, + BINDER_GENL_A_REPORT_REPLY, + BINDER_GENL_A_REPORT_FLAGS, + BINDER_GENL_A_REPORT_CODE, + BINDER_GENL_A_REPORT_DATA_SIZE, + + __BINDER_GENL_A_REPORT_MAX, + BINDER_GENL_A_REPORT_MAX = (__BINDER_GENL_A_REPORT_MAX - 1) +}; + +enum { + BINDER_GENL_CMD_SET = 1, + BINDER_GENL_CMD_REPORT, + + __BINDER_GENL_CMD_MAX, + BINDER_GENL_CMD_MAX = (__BINDER_GENL_CMD_MAX - 1) +}; + +#endif /* _UAPI_LINUX_ANDROID_BINDER_GENL_H */ -- 2.47.0.338.g60cca15819-goog ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH net-next v9 1/1] binder: report txn errors via generic netlink 2024-12-09 19:22 ` [PATCH net-next v9 1/1] " Li Li @ 2024-12-10 0:44 ` Carlos Llamas 2024-12-10 1:26 ` Li Li 2024-12-10 11:35 ` kernel test robot 1 sibling, 1 reply; 8+ messages in thread From: Carlos Llamas @ 2024-12-10 0:44 UTC (permalink / raw) To: Li Li Cc: dualli, corbet, davem, edumazet, kuba, pabeni, donald.hunter, gregkh, arve, tkjos, maco, joel, brauner, surenb, arnd, masahiroy, bagasdotme, horms, linux-kernel, linux-doc, netdev, hridya, smoreland, kernel-team On Mon, Dec 09, 2024 at 11:22:47AM -0800, Li Li wrote: > From: Li Li <dualli@google.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. > > 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. > > Signed-off-by: Li Li <dualli@google.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. > Documentation/admin-guide/index.rst | 1 + > Documentation/netlink/specs/binder_genl.yaml | 108 ++++++++ > drivers/android/Kconfig | 1 + > drivers/android/Makefile | 2 +- > drivers/android/binder.c | 277 ++++++++++++++++++- > drivers/android/binder_genl.c | 39 +++ > drivers/android/binder_genl.h | 18 ++ > drivers/android/binder_internal.h | 27 +- > drivers/android/binder_trace.h | 35 +++ > drivers/android/binderfs.c | 2 + > include/uapi/linux/android/binder_genl.h | 55 ++++ > 12 files changed, 655 insertions(+), 6 deletions(-) > create mode 100644 Documentation/admin-guide/binder_genl.rst > create mode 100644 Documentation/netlink/specs/binder_genl.yaml > create mode 100644 drivers/android/binder_genl.c > create mode 100644 drivers/android/binder_genl.h > create mode 100644 include/uapi/linux/android/binder_genl.h > > diff --git a/Documentation/admin-guide/binder_genl.rst b/Documentation/admin-guide/binder_genl.rst > new file mode 100644 > index 000000000000..b395d7a9849b > --- /dev/null > +++ b/Documentation/admin-guide/binder_genl.rst > @@ -0,0 +1,96 @@ > +.. SPDX-License-Identifier: GPL-2.0 > + > +=========================================================== > +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/ > +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? > +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? > +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. > +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? > + > +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. > + void *data = recv(CTRL_CMD_NEWFAMILY); > + __u16 id = nla(data)[CTRL_ATTR_FAMILY_ID]; > + > + // enable per-context binder report > + send(fd, id, BINDER_GENL_CMD_SET, "binder", 0, BINDER_GENL_FLAG_FAILED | > + BINDER_GENL_FLAG_DELAYED); > + > + // confirm the per-context configuration > + data = recv(fd, BINDER_GENL_CMD_REPLY); > + char *context = nla(data)[BINDER_GENL_A_CMD_CONTEXT]; > + __u32 pid = nla(data)[BINDER_GENL_A_CMD_PID]; > + __u32 flags = nla(data)[BINDER_GENL_A_CMD_FLAGS]; > + > + // set optional per-process report, overriding the per-context one > + send(fd, id, BINDER_GENL_CMD_SET, "binder", getpid(), > + BINDER_GENL_FLAG_SPAM | BINDER_REPORT_OVERRIDE); > + > + // confirm the optional per-process configuration > + data = recv(fd, BINDER_GENL_CMD_REPLY); > + context = nla(data)[BINDER_GENL_A_CMD_CONTEXT]; > + pid = nla(data)[BINDER_GENL_A_CMD_PID]; > + flags = nla(data)[BINDER_GENL_A_CMD_FLAGS]; > + > + // wait and read all binder reports > + while (running) { > + data = recv(fd, BINDER_GENL_CMD_REPORT); > + auto *attr = nla(data)[BINDER_GENL_A_REPORT_XXX]; > + > + // process binder report > + do_something(*attr); > + } > + > + // clean up > + send(fd, id, BINDER_GENL_CMD_SET, 0, 0); > + send(fd, id, BINDER_GENL_CMD_SET, getpid(), 0); > + close(fd); > diff --git a/Documentation/admin-guide/index.rst b/Documentation/admin-guide/index.rst > index e85b1adf5908..b3b5cfadffe5 100644 > --- a/Documentation/admin-guide/index.rst > +++ b/Documentation/admin-guide/index.rst > @@ -79,6 +79,7 @@ configure specific aspects of kernel behavior to your liking. > aoe/index > auxdisplay/index > bcache > + binder_genl > binderfs > binfmt-misc > blockdev/index > diff --git a/Documentation/netlink/specs/binder_genl.yaml b/Documentation/netlink/specs/binder_genl.yaml > new file mode 100644 > index 000000000000..3c6024b9a689 > --- /dev/null > +++ b/Documentation/netlink/specs/binder_genl.yaml > @@ -0,0 +1,108 @@ > +# SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause) > + > +name: binder_genl > +protocol: genetlink > +uapi-header: linux/android/binder_genl.h > +doc: Netlink protocol to report binder transaction errors and warnings. > + > +definitions: > + - > + type: flags > + name: flag > + doc: Define what kind of binder transactions should be reported. > + entries: [ failed, delayed, spam, override ] > + > +attribute-sets: > + - > + name: cmd > + doc: The supported attributes of "set" and "reply" commands > + attributes: > + - > + name: context > + type: string > + doc: The binder context to enable binder genl report. > + - > + name: pid > + type: u32 > + doc: The binder proc to enable binder genl report. > + - > + name: flags > + type: u32 > + enum: flag > + doc: What kind of binder transactions should be reported. > + - > + name: report > + doc: The supported attributes of "report" command > + attributes: > + - > + name: context > + type: string > + doc: The binder context where the binder genl report happens. > + - > + name: err > + type: u32 > + doc: Copy of binder_driver_return_protocol returned to the sender. > + - > + name: from_pid > + type: u32 > + doc: Sender pid of the corresponding binder transaction. > + - > + name: from_tid > + type: u32 > + doc: Sender tid of the corresponding binder transaction. > + - > + name: to_pid > + type: u32 > + doc: Target pid of the corresponding binder transaction. > + - > + name: to_tid > + type: u32 > + doc: Target tid of the corresponding binder transaction. > + - > + name: reply > + type: u32 > + doc: 1 means the transaction is a reply, 0 otherwise. > + - > + name: flags > + type: u32 > + doc: Copy of binder_transaction_data->flags. > + - > + name: code > + type: u32 > + doc: Copy of binder_transaction_data->code. > + - > + name: data_size > + type: u32 > + doc: Copy of binder_transaction_data->data_size. > + > +operations: > + list: > + - > + name: set > + doc: Set flags from user space. > + attribute-set: cmd > + > + do: > + request: ¶ms > + attributes: > + - context > + - pid > + - flags > + reply: *params > + - > + name: report > + doc: Send the requested reports to user space. > + attribute-set: report > + > + event: > + attributes: > + - context > + - err > + - from_pid > + - from_tid > + - to_pid > + - to_tid > + - reply > + - flags > + - code > + - data_size > diff --git a/drivers/android/Kconfig b/drivers/android/Kconfig > index 07aa8ae0a058..e2fa620934e2 100644 > --- a/drivers/android/Kconfig > +++ b/drivers/android/Kconfig > @@ -4,6 +4,7 @@ menu "Android" > config ANDROID_BINDER_IPC > bool "Android Binder IPC Driver" > depends on MMU > + depends on NET > default n > help > Binder is used in Android for both communication between processes, > diff --git a/drivers/android/Makefile b/drivers/android/Makefile > index c9d3d0c99c25..d818447fbc4c 100644 > --- a/drivers/android/Makefile > +++ b/drivers/android/Makefile > @@ -2,5 +2,5 @@ > ccflags-y += -I$(src) # needed for trace events > > obj-$(CONFIG_ANDROID_BINDERFS) += binderfs.o > -obj-$(CONFIG_ANDROID_BINDER_IPC) += binder.o binder_alloc.o > +obj-$(CONFIG_ANDROID_BINDER_IPC) += binder.o binder_alloc.o binder_genl.o > obj-$(CONFIG_ANDROID_BINDER_IPC_SELFTEST) += binder_alloc_selftest.o > diff --git a/drivers/android/binder.c b/drivers/android/binder.c > index ef353ca13c35..eaa9b763ef20 100644 > --- a/drivers/android/binder.c > +++ b/drivers/android/binder.c > @@ -72,6 +72,7 @@ > > #include <linux/cacheflush.h> > > +#include "binder_genl.h" > #include "binder_internal.h" > #include "binder_trace.h" > > @@ -2990,6 +2991,150 @@ static void binder_set_txn_from_error(struct binder_transaction *t, int id, > binder_thread_dec_tmpref(from); > } > > +/** > + * binder_find_proc() - set binder report flags > + * @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; > + } > + } > + mutex_unlock(&binder_procs_lock); > + > + return NULL; > +} > + > +/** > + * binder_genl_set_report() - set binder report flags > + * @context: the binder context to set the flags > + * @pid: the target process > + * @flags: the flags to set > + * > + * If pid is 0, the flags are applied to the whole binder context. > + * Otherwise, the flags are applied to the specific process only. > + */ > +static int binder_genl_set_report(struct binder_context *context, u32 pid, > + u32 flags) > +{ > + struct binder_proc *proc; > + > + 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? > + > + if (!pid) { > + /* Set the global flags for the whole binder context */ > + context->report_flags = flags; > + } else { > + /* Set the per-process flags */ > + proc = binder_find_proc(pid); > + if (!proc) { > + pr_err("Invalid binder report pid %u\n", pid); > + return -EINVAL; > + } > + > + proc->report_flags = flags; > + } > + > + return 0; > +} > + > +/** > + * binder_genl_report_enabled() - check if binder genl reports are enabled > + * @proc: the binder_proc to check > + * @mask: the categories of binder genl reports > + * > + * Returns true if certain binder genl reports are enabled for this binder > + * proc (when per-process overriding takes effect) or context. > + */ > +static bool binder_genl_report_enabled(struct binder_proc *proc, u32 mask) > +{ > + struct binder_context *context = proc->context; > + > + if (!context->report_portid) > + return false; > + > + if (proc->report_flags & BINDER_GENL_FLAG_OVERRIDE) > + return (proc->report_flags & mask) != 0; > + else > + return (context->report_flags & mask) != 0; > +} > + > +/** > + * binder_genl_send_report() - send one binder genl report > + * @context: the binder context > + * @err: copy of binder_driver_return_protocol returned to the sender > + * @pid: sender process > + * @tid: sender thread > + * @to_pid: target process > + * @to_tid: target thread > + * @reply: whether the binder transaction is a reply > + * @tr: the binder transaction data > + * > + * Packs the report data into a "report" binder genl message and send it. > + */ > +static void binder_genl_send_report(struct binder_context *context, u32 err, > + u32 pid, u32 tid, u32 to_pid, u32 to_tid, > + u32 reply, > + struct binder_transaction_data *tr) > +{ > + int ret; > + struct sk_buff *skb; > + void *hdr; > + > + trace_binder_send_report(context->name, err, pid, tid, to_pid, to_tid, > + reply, tr); > + > + skb = genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_KERNEL); > + if (!skb) { > + pr_err("Failed to alloc binder genl message\n"); > + return; > + } > + > + hdr = genlmsg_put(skb, 0, atomic_inc_return(&context->report_seq), > + &binder_genl_nl_family, 0, BINDER_GENL_CMD_REPORT); > + if (!hdr) > + goto free_skb; > + > + if (nla_put_string(skb, BINDER_GENL_A_REPORT_CONTEXT, context->name) || > + nla_put_u32(skb, BINDER_GENL_A_REPORT_ERR, err) || > + nla_put_u32(skb, BINDER_GENL_A_REPORT_FROM_PID, pid) || > + nla_put_u32(skb, BINDER_GENL_A_REPORT_FROM_TID, tid) || > + nla_put_u32(skb, BINDER_GENL_A_REPORT_TO_PID, to_pid) || > + nla_put_u32(skb, BINDER_GENL_A_REPORT_TO_TID, to_tid) || > + nla_put_u32(skb, BINDER_GENL_A_REPORT_REPLY, reply) || > + nla_put_u32(skb, BINDER_GENL_A_REPORT_FLAGS, tr->flags) || > + nla_put_u32(skb, BINDER_GENL_A_REPORT_CODE, tr->code) || > + nla_put_u32(skb, BINDER_GENL_A_REPORT_DATA_SIZE, tr->data_size)) > + goto cancel_skb; > + > + genlmsg_end(skb, hdr); > + > + ret = genlmsg_unicast(&init_net, skb, context->report_portid); > + if (ret < 0) > + pr_err("Failed to send binder genl message to %d: %d\n", > + context->report_portid, ret); > + return; > + > +cancel_skb: > + pr_err("Failed to add report attributes to binder genl message\n"); > + genlmsg_cancel(skb, hdr); > +free_skb: > + pr_err("Free binder genl report message on error\n"); > + nlmsg_free(skb); > +} > + > static void binder_transaction(struct binder_proc *proc, > struct binder_thread *thread, > struct binder_transaction_data *tr, int reply, > @@ -3684,10 +3829,18 @@ static void binder_transaction(struct binder_proc *proc, > return_error_line = __LINE__; > goto err_copy_data_failed; > } > - if (t->buffer->oneway_spam_suspect) > + if (t->buffer->oneway_spam_suspect) { > tcomplete->type = BINDER_WORK_TRANSACTION_ONEWAY_SPAM_SUSPECT; > - else > + if (binder_genl_report_enabled(proc, BINDER_GENL_FLAG_SPAM)) > + binder_genl_send_report(context, > + BR_ONEWAY_SPAM_SUSPECT, > + proc->pid, thread->pid, > + target_proc ? target_proc->pid : 0, > + target_thread ? target_thread->pid : 0, > + reply, tr); > + } else { > tcomplete->type = BINDER_WORK_TRANSACTION_COMPLETE; > + } > t->work.type = BINDER_WORK_TRANSACTION; > > if (reply) { > @@ -3737,8 +3890,15 @@ static void binder_transaction(struct binder_proc *proc, > * process and is put in a pending queue, waiting for the target > * process to be unfrozen. > */ > - if (return_error == BR_TRANSACTION_PENDING_FROZEN) > + if (return_error == BR_TRANSACTION_PENDING_FROZEN) { > tcomplete->type = BINDER_WORK_TRANSACTION_PENDING; > + if (binder_genl_report_enabled(proc, BINDER_GENL_FLAG_DELAYED)) > + binder_genl_send_report(context, return_error, > + proc->pid, thread->pid, > + target_proc ? target_proc->pid : 0, > + target_thread ? target_thread->pid : 0, > + reply, tr); > + } > binder_enqueue_thread_work(thread, tcomplete); > if (return_error && > return_error != BR_TRANSACTION_PENDING_FROZEN) > @@ -3800,6 +3960,13 @@ static void binder_transaction(struct binder_proc *proc, > binder_dec_node_tmpref(target_node); > } > > + if (binder_genl_report_enabled(proc, BINDER_GENL_FLAG_FAILED)) > + binder_genl_send_report(context, return_error, > + proc->pid, thread->pid, > + target_proc ? target_proc->pid : 0, > + target_thread ? target_thread->pid : 0, > + reply, tr); > + > binder_debug(BINDER_DEBUG_FAILED_TRANSACTION, > "%d:%d transaction %s to %d:%d failed %d/%d/%d, size %lld-%lld line %d\n", > proc->pid, thread->pid, reply ? "reply" : > @@ -6137,6 +6304,11 @@ static int binder_release(struct inode *nodp, struct file *filp) > > binder_defer_work(proc, BINDER_DEFERRED_RELEASE); > > + if (proc->pid == proc->context->report_portid) { > + proc->context->report_portid = 0; > + proc->context->report_flags = 0; > + } > + > return 0; > } > > @@ -6335,6 +6507,87 @@ binder_defer_work(struct binder_proc *proc, enum binder_deferred_state defer) > mutex_unlock(&binder_deferred_lock); > } > > +/** > + * binder_genl_nl_set_doit() - .doit handler for BINDER_GENL_CMD_SET > + * @skb: the metadata struct passed from netlink driver > + * @info: the generic netlink struct passed from netlink driver > + * > + * Implements the .doit function to process binder genl commands. > + */ > +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? > + > + 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. > + > + skb = genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_KERNEL); > + if (!skb) { > + pr_err("Failed to alloc binder genl reply message\n"); > + return -ENOMEM; > + } > + > + hdr = genlmsg_iput(skb, info); > + if (!hdr) > + goto free_skb; > + > + if (nla_put_string(skb, BINDER_GENL_A_CMD_CONTEXT, context->name) || > + nla_put_u32(skb, BINDER_GENL_A_CMD_PID, pid) || > + nla_put_u32(skb, BINDER_GENL_A_CMD_FLAGS, flags)) > + goto cancel_skb; > + > + genlmsg_end(skb, hdr); > + > + if (genlmsg_reply(skb, info)) { > + pr_err("Failed to send binder genl reply message\n"); > + return -EFAULT; > + } > + > + if (!context->report_portid) > + context->report_portid = portid; > + > + return 0; > + > +cancel_skb: > + pr_err("Failed to add reply attributes to binder genl message\n"); > + genlmsg_cancel(skb, hdr); > +free_skb: > + pr_err("Free binder genl reply message on error\n"); > + nlmsg_free(skb); > + return -EMSGSIZE; > +} > + > static void print_binder_transaction_ilocked(struct seq_file *m, > struct binder_proc *proc, > const char *prefix, > @@ -6928,6 +7181,17 @@ const struct binder_debugfs_entry binder_debugfs_entries[] = { > {} /* terminator */ > }; > > +/** > + * 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? > + > 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. > > return ret; > } > @@ -7009,6 +7274,12 @@ static int __init binder_init(void) > if (ret) > goto err_init_binder_device_failed; > > + ret = genl_register_family(&binder_genl_nl_family); > + if (ret) { > + pr_err("Failed to register binder genl family\n"); > + goto err_init_binder_device_failed; > + } > + > return ret; > > err_init_binder_device_failed: > diff --git a/drivers/android/binder_genl.c b/drivers/android/binder_genl.c > new file mode 100644 > index 000000000000..29d79bdd9649 > --- /dev/null > +++ b/drivers/android/binder_genl.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_genl.yaml */ > +/* YNL-GEN kernel source */ > + > +#include <net/netlink.h> > +#include <net/genetlink.h> > + > +#include "binder_genl.h" > + > +#include <uapi/linux/android/binder_genl.h> > + > +/* BINDER_GENL_CMD_SET - do */ > +static const struct nla_policy binder_genl_set_nl_policy[BINDER_GENL_A_CMD_FLAGS + 1] = { > + [BINDER_GENL_A_CMD_CONTEXT] = { .type = NLA_NUL_STRING, }, > + [BINDER_GENL_A_CMD_PID] = { .type = NLA_U32, }, > + [BINDER_GENL_A_CMD_FLAGS] = NLA_POLICY_MASK(NLA_U32, 0xf), > +}; > + > +/* Ops table for binder_genl */ > +static const struct genl_split_ops binder_genl_nl_ops[] = { > + { > + .cmd = BINDER_GENL_CMD_SET, > + .doit = binder_genl_nl_set_doit, > + .policy = binder_genl_set_nl_policy, > + .maxattr = BINDER_GENL_A_CMD_FLAGS, > + .flags = GENL_CMD_CAP_DO, > + }, > +}; > + > +struct genl_family binder_genl_nl_family __ro_after_init = { > + .name = BINDER_GENL_FAMILY_NAME, > + .version = BINDER_GENL_FAMILY_VERSION, > + .netnsok = true, > + .parallel_ops = true, > + .module = THIS_MODULE, > + .split_ops = binder_genl_nl_ops, > + .n_split_ops = ARRAY_SIZE(binder_genl_nl_ops), > +}; > diff --git a/drivers/android/binder_genl.h b/drivers/android/binder_genl.h > new file mode 100644 > index 000000000000..9d68c155b7c4 > --- /dev/null > +++ b/drivers/android/binder_genl.h > @@ -0,0 +1,18 @@ > +/* 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_genl.yaml */ > +/* YNL-GEN kernel header */ > + > +#ifndef _LINUX_BINDER_GENL_GEN_H > +#define _LINUX_BINDER_GENL_GEN_H > + > +#include <net/netlink.h> > +#include <net/genetlink.h> > + > +#include <uapi/linux/android/binder_genl.h> > + > +int binder_genl_nl_set_doit(struct sk_buff *skb, struct genl_info *info); > + > +extern struct genl_family binder_genl_nl_family; > + > +#endif /* _LINUX_BINDER_GENL_GEN_H */ > diff --git a/drivers/android/binder_internal.h b/drivers/android/binder_internal.h > index f8d6be682f23..cc745367a519 100644 > --- a/drivers/android/binder_internal.h > +++ b/drivers/android/binder_internal.h > @@ -12,21 +12,35 @@ > #include <linux/stddef.h> > #include <linux/types.h> > #include <linux/uidgid.h> > +#include <net/genetlink.h> > #include <uapi/linux/android/binderfs.h> > #include "binder_alloc.h" > #include "dbitmap.h" > > +/** > + * struct binder_context - information about a binder domain > + * @binder_context_mgr_node: the context manager > + * @context_mgr_node_lock: the lock protecting the above context manager node > + * @binder_context_mgr_uid: the uid of the above context manager > + * @name: the name of the binder device > + * @report_portid: the netlink socket to receive binder reports > + * @report_flags: the categories of binder transactions that would > + * be reported (see enum binder_report_flag). > + * @report_seq: the seq number of the generic netlink report > + */ > struct binder_context { > struct binder_node *binder_context_mgr_node; > struct mutex context_mgr_node_lock; > kuid_t binder_context_mgr_uid; > const char *name; > + u32 report_portid; > + u32 report_flags; > + atomic_t report_seq; > }; > > /** > * 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 > * @miscdev: information about a binder character device node > * @context: binder context information > * @binderfs_inode: This is the inode of the root dentry of the super block > @@ -415,6 +429,8 @@ struct binder_ref { > * @binderfs_entry: process-specific binderfs log file > * @oneway_spam_detection_enabled: process enabled oneway spam detection > * or not > + * @report_flags: the categories of binder transactions that would > + * be reported (see enum binder_genl_flag). > * > * Bookkeeping structure for binder processes > */ > @@ -453,6 +469,7 @@ struct binder_proc { > spinlock_t outer_lock; > struct dentry *binderfs_entry; > bool oneway_spam_detection_enabled; > + u32 report_flags; > }; > > /** > @@ -582,4 +599,10 @@ struct binder_object { > }; > }; > > +/** > + * Add a binder device to binder_devices > + * @device: the new binder device to add to the global list > + */ > +void binder_add_device(struct binder_device *device); > + > #endif /* _LINUX_BINDER_INTERNAL_H */ > diff --git a/drivers/android/binder_trace.h b/drivers/android/binder_trace.h > index fe38c6fc65d0..551b6c9d9f73 100644 > --- a/drivers/android/binder_trace.h > +++ b/drivers/android/binder_trace.h > @@ -423,6 +423,41 @@ TRACE_EVENT(binder_return, > "unknown") > ); > > +TRACE_EVENT(binder_send_report, > + TP_PROTO(const char *name, u32 err, u32 pid, u32 tid, u32 to_pid, > + u32 to_tid, u32 reply, struct binder_transaction_data *tr), > + TP_ARGS(name, err, pid, tid, to_pid, to_tid, reply, tr), > + TP_STRUCT__entry( > + __field(const char *, name) > + __field(u32, err) > + __field(u32, pid) > + __field(u32, tid) > + __field(u32, to_pid) > + __field(u32, to_tid) > + __field(u32, reply) > + __field(u32, flags) > + __field(u32, code) > + __field(binder_size_t, data_size) > + ), > + TP_fast_assign( > + __entry->name = name; > + __entry->err = err; > + __entry->pid = pid; > + __entry->tid = tid; > + __entry->to_pid = to_pid; > + __entry->to_tid = to_tid; > + __entry->reply = reply; > + __entry->flags = tr->flags; > + __entry->code = tr->code; > + __entry->data_size = tr->data_size; > + ), > + TP_printk("%s: %d %d:%d -> %d:%d %s flags=0x08%x code=%d %llu", > + __entry->name, __entry->err, __entry->pid, __entry->tid, > + __entry->to_pid, __entry->to_tid, > + __entry->reply ? "reply" : "", > + __entry->flags, __entry->code, __entry->data_size) > +); > + > #endif /* _BINDER_TRACE_H */ > > #undef TRACE_INCLUDE_PATH > diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c > index ad1fa7abc323..bc6bae76ccaf 100644 > --- a/drivers/android/binderfs.c > +++ b/drivers/android/binderfs.c > @@ -207,6 +207,8 @@ static int binderfs_binder_device_create(struct inode *ref_inode, > fsnotify_create(root->d_inode, dentry); > inode_unlock(d_inode(root)); > > + binder_add_device(device); > + > return 0; > > err: > diff --git a/include/uapi/linux/android/binder_genl.h b/include/uapi/linux/android/binder_genl.h > new file mode 100644 > index 000000000000..f39b5c81e477 > --- /dev/null > +++ b/include/uapi/linux/android/binder_genl.h > @@ -0,0 +1,55 @@ > +/* 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_genl.yaml */ > +/* YNL-GEN uapi header */ > + > +#ifndef _UAPI_LINUX_ANDROID_BINDER_GENL_H > +#define _UAPI_LINUX_ANDROID_BINDER_GENL_H > + > +#define BINDER_GENL_FAMILY_NAME "binder_genl" > +#define BINDER_GENL_FAMILY_VERSION 1 > + > +/* > + * Define what kind of binder transactions should be reported. > + */ > +enum binder_genl_flag { > + BINDER_GENL_FLAG_FAILED = 1, > + BINDER_GENL_FLAG_DELAYED = 2, > + BINDER_GENL_FLAG_SPAM = 4, > + BINDER_GENL_FLAG_OVERRIDE = 8, > +}; > + > +enum { > + BINDER_GENL_A_CMD_CONTEXT = 1, > + BINDER_GENL_A_CMD_PID, > + BINDER_GENL_A_CMD_FLAGS, > + > + __BINDER_GENL_A_CMD_MAX, > + BINDER_GENL_A_CMD_MAX = (__BINDER_GENL_A_CMD_MAX - 1) > +}; > + > +enum { > + BINDER_GENL_A_REPORT_CONTEXT = 1, > + BINDER_GENL_A_REPORT_ERR, > + BINDER_GENL_A_REPORT_FROM_PID, > + BINDER_GENL_A_REPORT_FROM_TID, > + BINDER_GENL_A_REPORT_TO_PID, > + BINDER_GENL_A_REPORT_TO_TID, > + BINDER_GENL_A_REPORT_REPLY, > + BINDER_GENL_A_REPORT_FLAGS, > + BINDER_GENL_A_REPORT_CODE, > + BINDER_GENL_A_REPORT_DATA_SIZE, > + > + __BINDER_GENL_A_REPORT_MAX, > + BINDER_GENL_A_REPORT_MAX = (__BINDER_GENL_A_REPORT_MAX - 1) > +}; > + > +enum { > + BINDER_GENL_CMD_SET = 1, > + BINDER_GENL_CMD_REPORT, > + > + __BINDER_GENL_CMD_MAX, > + BINDER_GENL_CMD_MAX = (__BINDER_GENL_CMD_MAX - 1) > +}; > + > +#endif /* _UAPI_LINUX_ANDROID_BINDER_GENL_H */ > -- > 2.47.0.338.g60cca15819-goog > 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? Regards, -- Carlos Llamas ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next v9 1/1] binder: report txn errors via generic netlink 2024-12-10 0:44 ` Carlos Llamas @ 2024-12-10 1:26 ` Li Li 2024-12-10 3:16 ` Carlos Llamas 2024-12-10 3:31 ` Carlos Llamas 0 siblings, 2 replies; 8+ messages in thread From: Li Li @ 2024-12-10 1:26 UTC (permalink / raw) To: Carlos Llamas Cc: dualli, corbet, davem, edumazet, kuba, pabeni, donald.hunter, gregkh, arve, tkjos, maco, joel, brauner, surenb, arnd, masahiroy, bagasdotme, horms, linux-kernel, linux-doc, netdev, hridya, smoreland, kernel-team On Mon, Dec 9, 2024 at 4:44 PM Carlos Llamas <cmllamas@google.com> wrote: > > On Mon, Dec 09, 2024 at 11:22:47AM -0800, Li Li wrote: > > From: Li Li <dualli@google.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@google.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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next v9 1/1] binder: report txn errors via generic netlink 2024-12-10 1:26 ` Li Li @ 2024-12-10 3:16 ` Carlos Llamas 2024-12-10 3:31 ` Carlos Llamas 1 sibling, 0 replies; 8+ messages in thread From: Carlos Llamas @ 2024-12-10 3:16 UTC (permalink / raw) To: Li Li Cc: dualli, corbet, davem, edumazet, kuba, pabeni, donald.hunter, gregkh, arve, tkjos, maco, joel, brauner, surenb, arnd, masahiroy, bagasdotme, horms, linux-kernel, linux-doc, netdev, hridya, smoreland, kernel-team On Mon, Dec 09, 2024 at 05:26:14PM -0800, Li Li wrote: > On Mon, Dec 9, 2024 at 4:44 PM Carlos Llamas <cmllamas@google.com> wrote: > > > > On Mon, Dec 09, 2024 at 11:22:47AM -0800, Li Li wrote: > > > From: Li Li <dualli@google.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 I am familiar with the "genl" e.g. as in genlmsghdr. However, it wasn't immediately straight-forward to me the connection as it would have been with "netlink". I don't know what the convention is for this type of generic netlink interfaces. Perhaps "genl" is more appropriate, I just know that using "netlink" would have been more straight-forward (at least to me). Maybe most others that are new to this binder interface will go through the same "discovery" process I went through. > > > > > > > Signed-off-by: Li Li <dualli@google.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. Short is fine. However, what is "SET"? In the future we might add more commands to this interface and I can see this name being a problem at that point. > > > > +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. I'm absolutely confused as to what is "delayed" then? Isn't it async && frozen? A spam transaction can be if the caller has >=25% of the target's buffer capacity or over 50 transactions, regardless of whether the target is frozen or not. It seems to me these are two independent scenarios and a transaction can be either one or both. > > > > > > +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? I mean that without the flags check binder_genl_set_report() is small enough and the only caller is this. So you could just delete that function and write the code here in binder_genl_nl_set_doit(). > > > > +/** > > > + * 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. Cool, and this wouldn't be retried after it has failed right? ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next v9 1/1] binder: report txn errors via generic netlink 2024-12-10 1:26 ` Li Li 2024-12-10 3:16 ` Carlos Llamas @ 2024-12-10 3:31 ` Carlos Llamas 1 sibling, 0 replies; 8+ messages in thread From: Carlos Llamas @ 2024-12-10 3:31 UTC (permalink / raw) To: Li Li Cc: dualli, corbet, davem, edumazet, kuba, pabeni, donald.hunter, gregkh, arve, tkjos, maco, joel, brauner, surenb, arnd, masahiroy, bagasdotme, horms, linux-kernel, linux-doc, netdev, hridya, smoreland, kernel-team On Mon, Dec 09, 2024 at 05:26:14PM -0800, Li Li wrote: > On Mon, Dec 9, 2024 at 4:44 PM Carlos Llamas <cmllamas@google.com> wrote: > > > + > > > +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(). Ok, sorry this broke my brain trying to make sense of the arguments to send(). You imply that the nlmsghdr, genlmsghdr and so on need to be populated accordingly with these arguments. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next v9 1/1] binder: report txn errors via generic netlink 2024-12-09 19:22 ` [PATCH net-next v9 1/1] " Li Li 2024-12-10 0:44 ` Carlos Llamas @ 2024-12-10 11:35 ` kernel test robot 1 sibling, 0 replies; 8+ messages in thread From: kernel test robot @ 2024-12-10 11:35 UTC (permalink / raw) To: Li Li, dualli, corbet, davem, edumazet, kuba, pabeni, donald.hunter, gregkh, arve, tkjos, maco, joel, brauner, cmllamas, surenb, arnd, masahiroy, bagasdotme, horms, linux-kernel, linux-doc, netdev, hridya, smoreland Cc: oe-kbuild-all, kernel-team Hi Li, kernel test robot noticed the following build warnings: [auto build test WARNING on 6145fefc1e42c1895c0c1c2c8593de2c085d8c56] url: https://github.com/intel-lab-lkp/linux/commits/Li-Li/binder-report-txn-errors-via-generic-netlink/20241210-032559 base: 6145fefc1e42c1895c0c1c2c8593de2c085d8c56 patch link: https://lore.kernel.org/r/20241209192247.3371436-2-dualli%40chromium.org patch subject: [PATCH net-next v9 1/1] binder: report txn errors via generic netlink config: s390-randconfig-002-20241210 (https://download.01.org/0day-ci/archive/20241210/202412101942.kbghVu3V-lkp@intel.com/config) compiler: s390-linux-gcc (GCC) 14.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241210/202412101942.kbghVu3V-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202412101942.kbghVu3V-lkp@intel.com/ All warnings (new ones prefixed by >>): drivers/android/binder.c:1983: warning: Function parameter or struct member 'file' not described in 'binder_task_work_cb' drivers/android/binder.c:1983: warning: Excess struct member 'fd' description in 'binder_task_work_cb' drivers/android/binder.c:2434: warning: Function parameter or struct member 'offset' not described in 'binder_ptr_fixup' drivers/android/binder.c:2434: warning: Function parameter or struct member 'skip_size' not described in 'binder_ptr_fixup' drivers/android/binder.c:2434: warning: Function parameter or struct member 'fixup_data' not described in 'binder_ptr_fixup' drivers/android/binder.c:2434: warning: Function parameter or struct member 'node' not described in 'binder_ptr_fixup' drivers/android/binder.c:2454: warning: Function parameter or struct member 'offset' not described in 'binder_sg_copy' drivers/android/binder.c:2454: warning: Function parameter or struct member 'sender_uaddr' not described in 'binder_sg_copy' drivers/android/binder.c:2454: warning: Function parameter or struct member 'length' not described in 'binder_sg_copy' drivers/android/binder.c:2454: warning: Function parameter or struct member 'node' not described in 'binder_sg_copy' drivers/android/binder.c:4181: warning: Function parameter or struct member 'thread' not described in 'binder_free_buf' >> drivers/android/binder.c:7185: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst * Add a binder device to binder_devices vim +7185 drivers/android/binder.c 7183 7184 /** > 7185 * Add a binder device to binder_devices 7186 * @device: the new binder device to add to the global list 7187 * 7188 * Not reentrant as the list is not protected by any locks 7189 */ 7190 void binder_add_device(struct binder_device *device) 7191 { 7192 hlist_add_head(&device->hlist, &binder_devices); 7193 } 7194 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next v9 1/1] binder: report txn errors via generic netlink
@ 2024-12-10 16:50 kernel test robot
0 siblings, 0 replies; 8+ messages in thread
From: kernel test robot @ 2024-12-10 16:50 UTC (permalink / raw)
To: oe-kbuild; +Cc: lkp, Julia Lawall
BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <20241209192247.3371436-2-dualli@chromium.org>
References: <20241209192247.3371436-2-dualli@chromium.org>
TO: Li Li <dualli@chromium.org>
TO: dualli@google.com
TO: corbet@lwn.net
TO: davem@davemloft.net
TO: edumazet@google.com
TO: kuba@kernel.org
TO: pabeni@redhat.com
TO: donald.hunter@gmail.com
TO: gregkh@linuxfoundation.org
TO: arve@android.com
TO: tkjos@android.com
TO: maco@android.com
TO: joel@joelfernandes.org
TO: brauner@kernel.org
TO: cmllamas@google.com
TO: surenb@google.com
TO: arnd@arndb.de
TO: masahiroy@kernel.org
TO: bagasdotme@gmail.com
TO: horms@kernel.org
TO: linux-kernel@vger.kernel.org
TO: linux-doc@vger.kernel.org
TO: netdev@vger.kernel.org
TO: hridya@google.com
TO: smoreland@google.com
CC: kernel-team@android.com
Hi Li,
kernel test robot noticed the following build warnings:
[auto build test WARNING on 6145fefc1e42c1895c0c1c2c8593de2c085d8c56]
url: https://github.com/intel-lab-lkp/linux/commits/Li-Li/binder-report-txn-errors-via-generic-netlink/20241210-032559
base: 6145fefc1e42c1895c0c1c2c8593de2c085d8c56
patch link: https://lore.kernel.org/r/20241209192247.3371436-2-dualli%40chromium.org
patch subject: [PATCH net-next v9 1/1] binder: report txn errors via generic netlink
:::::: branch date: 21 hours ago
:::::: commit date: 21 hours ago
config: x86_64-randconfig-102-20241210 (https://download.01.org/0day-ci/archive/20241211/202412110006.vSu6pw9e-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Julia Lawall <julia.lawall@inria.fr>
| Closes: https://lore.kernel.org/r/202412110006.vSu6pw9e-lkp@intel.com/
cocci warnings: (new ones prefixed by >>)
>> drivers/android/binder.c:6535:31-57: WARNING avoid newline at end of message in NL_SET_ERR_MSG
vim +6535 drivers/android/binder.c
355b0502f6efea0 drivers/staging/android/binder.c Greg Kroah-Hartman 2011-11-30 6509
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6510 /**
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6511 * binder_genl_nl_set_doit() - .doit handler for BINDER_GENL_CMD_SET
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6512 * @skb: the metadata struct passed from netlink driver
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6513 * @info: the generic netlink struct passed from netlink driver
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6514 *
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6515 * Implements the .doit function to process binder genl commands.
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6516 */
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6517 int binder_genl_nl_set_doit(struct sk_buff *skb, struct genl_info *info)
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6518 {
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6519 int portid;
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6520 u32 pid;
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6521 u32 flags;
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6522 void *hdr;
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6523 struct binder_device *device;
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6524 struct binder_context *context = NULL;
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6525
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6526 hlist_for_each_entry(device, &binder_devices, hlist) {
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6527 if (!nla_strcmp(info->attrs[BINDER_GENL_A_CMD_CONTEXT],
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6528 device->context.name)) {
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6529 context = &device->context;
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6530 break;
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6531 }
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6532 }
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6533
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6534 if (!context) {
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 @6535 NL_SET_ERR_MSG(info->extack, "Unknown binder context\n");
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6536 return -EINVAL;
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6537 }
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6538
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6539 portid = nlmsg_hdr(skb)->nlmsg_pid;
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6540 pid = nla_get_u32(info->attrs[BINDER_GENL_A_CMD_PID]);
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6541 flags = nla_get_u32(info->attrs[BINDER_GENL_A_CMD_FLAGS]);
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6542
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6543 if (context->report_portid && context->report_portid != portid) {
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6544 NL_SET_ERR_MSG_FMT(info->extack,
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6545 "No permission to set flags from %d\n",
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6546 portid);
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6547 return -EPERM;
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6548 }
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6549
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6550 if (binder_genl_set_report(context, pid, flags) < 0) {
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6551 pr_err("Failed to set report flags %u for %u\n", flags, pid);
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6552 return -EINVAL;
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6553 }
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6554
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6555 skb = genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_KERNEL);
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6556 if (!skb) {
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6557 pr_err("Failed to alloc binder genl reply message\n");
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6558 return -ENOMEM;
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6559 }
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6560
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6561 hdr = genlmsg_iput(skb, info);
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6562 if (!hdr)
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6563 goto free_skb;
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6564
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6565 if (nla_put_string(skb, BINDER_GENL_A_CMD_CONTEXT, context->name) ||
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6566 nla_put_u32(skb, BINDER_GENL_A_CMD_PID, pid) ||
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6567 nla_put_u32(skb, BINDER_GENL_A_CMD_FLAGS, flags))
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6568 goto cancel_skb;
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6569
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6570 genlmsg_end(skb, hdr);
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6571
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6572 if (genlmsg_reply(skb, info)) {
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6573 pr_err("Failed to send binder genl reply message\n");
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6574 return -EFAULT;
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6575 }
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6576
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6577 if (!context->report_portid)
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6578 context->report_portid = portid;
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6579
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6580 return 0;
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6581
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6582 cancel_skb:
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6583 pr_err("Failed to add reply attributes to binder genl message\n");
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6584 genlmsg_cancel(skb, hdr);
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6585 free_skb:
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6586 pr_err("Free binder genl reply message on error\n");
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6587 nlmsg_free(skb);
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6588 return -EMSGSIZE;
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6589 }
86bd3e8e38707ae drivers/android/binder.c Li Li 2024-12-09 6590
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 8+ messages in threadend of thread, other threads:[~2024-12-10 16:51 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-12-09 19:22 [PATCH net-next v9 0/1] binder: report txn errors via generic netlink Li Li 2024-12-09 19:22 ` [PATCH net-next v9 1/1] " Li Li 2024-12-10 0:44 ` Carlos Llamas 2024-12-10 1:26 ` Li Li 2024-12-10 3:16 ` Carlos Llamas 2024-12-10 3:31 ` Carlos Llamas 2024-12-10 11:35 ` kernel test robot -- strict thread matches above, loose matches on Subject: below -- 2024-12-10 16:50 kernel test robot
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.