* [PATCH v16 0/3] binder: report txn errors via generic netlink
@ 2025-03-03 20:02 Li Li
2025-03-03 20:02 ` [PATCH v16 1/3] lsm, selinux: Add setup_report permission to binder Li Li
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Li Li @ 2025-03-03 20:02 UTC (permalink / raw)
To: dualli, corbet, davem, edumazet, kuba, pabeni, donald.hunter,
gregkh, arve, tkjos, maco, joel, brauner, cmllamas, surenb,
omosnace, shuah, arnd, masahiroy, bagasdotme, horms, tweek,
linux-kernel, linux-doc, netdev, selinux, hridya
Cc: smoreland, ynaffit, 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.
https://lore.kernel.org/all/20241209192247.3371436-1-dualli@chromium.org/
The 10th version renames binder genl to binder netlink, improves the
readability of the kernel doc and uses more descriptive variable names.
The function binder_add_device() is moved out to a new commit per request.
It also fixes a warning about newline used in NL_SET_ERR_MSG.
Thanks Carlos for his valuable suggestions!
https://lore.kernel.org/all/20241212224114.888373-1-dualli@chromium.org/
The 11th version simplifies the yaml filename to avoid redundant words in
variable names. This also makes binder netlink yaml more aligned with
other existing netlink specs. Another trivial change is to use reverse
xmas tree for function variables.
https://lore.kernel.org/all/20241218203740.4081865-1-dualli@chromium.org/
The 12th version makes Documentation/admin-guide/binder_netlink.rst aligned
with the binder netlink yaml change introduced in the 11th revision. It
doesn't change any source code.
https://lore.kernel.org/all/20241218212935.4162907-1-dualli@chromium.org/
The 13th version removes the unnecessary dependency to binder file ops.
Now the netlink configuration is reset using sock_priv_destroy. It also
requires CAP_NET_ADMIN to send commands to the driver. One of the
patches ("binderfs: add new binder devices to binder_devices") has been
merged to linux-next. To avoid conflict, switch to linux-next master
branch and remove the merged one. Adding sock_priv into netlink spec
results in CFI failure, which is fixed by the new trampoline patches.
https://lore.kernel.org/all/20250115102950.563615-1-dualli@chromium.org/
The 14th version fix the code style issue by wrapping the sock priv
in a separate struct, as suggested by Jakub. The other 2 patches are
no longer included in this patchset as the equvilent fix has already
been merged to upstream linux master branch, as well as net & net-next.
This version has already been rebased to TOT of linux-next.
https://lore.kernel.org/all/20250118080939.2835687-1-dualli@chromium.org/
The 15th version switches from unicast to multicast per feedback and
feature requriements from binder users. With this change, multiple user
space processes can listen to the binder reports from the kernel driver
at the same time. To receive the multicast messages, those user space
processs should query the mcast group id and join the mcast group. In
the previous unicast solution, a portid is saved in the kernel driver to
prevent unauthorized process to send commands to the kernel driver. In
this multicast solution, this is replaced by a new "setup_report"
permission in the "binder" class. Meanwhile, the sock_priv_destroy
callback and CAP_NET_ADMIN restriction are no longer required in favor
of the multicast solution and the new "setup_report" permission.
https://lore.kernel.org/all/20250226192047.734627-1-dualli@chromium.org/
The 16th version fixes the build error when CONFIG_SECURITY is disabled.
There's no other changes. Thank you, kernel test robot <lkp@intel.com>!
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
v10: improve the readability of kernel doc and variable names
v11: rename binder_netlinnk.yaml to binder.yaml
use reverse xmas tree for function variables
v12: make kernel doc aligned with source code
v13: use sock_priv_destroy to cleanup netlink
require CAP_NET_ADMIN to send netlink commands
add trampolines in ynl-gen to fix CFI failure
v14: wrap the sock priv in a separate struct
v15: switch from unicast to multicast netlink message
add a "setup_report" permission in the "binder" class
add generic_netlink to binder_features
v16: fix build error with CONFIG_SECURITY disabled
Li Li (2):
binder: report txn errors via generic netlink
binder: generic netlink binder_features flag
Thiébaud Weksteen (1):
lsm, selinux: Add setup_report permission to binder
Documentation/admin-guide/binder_netlink.rst | 108 +++++++++
Documentation/admin-guide/index.rst | 1 +
Documentation/netlink/specs/binder.yaml | 116 +++++++++
drivers/android/Kconfig | 1 +
drivers/android/Makefile | 2 +-
drivers/android/binder.c | 229 +++++++++++++++++-
drivers/android/binder_internal.h | 16 ++
drivers/android/binder_netlink.c | 46 ++++
drivers/android/binder_netlink.h | 23 ++
drivers/android/binder_trace.h | 35 +++
drivers/android/binderfs.c | 8 +
include/linux/lsm_hook_defs.h | 1 +
include/linux/security.h | 6 +
include/uapi/linux/android/binder_netlink.h | 57 +++++
security/security.c | 13 +
security/selinux/hooks.c | 7 +
security/selinux/include/classmap.h | 3 +-
.../filesystems/binderfs/binderfs_test.c | 1 +
18 files changed, 668 insertions(+), 5 deletions(-)
create mode 100644 Documentation/admin-guide/binder_netlink.rst
create mode 100644 Documentation/netlink/specs/binder.yaml
create mode 100644 drivers/android/binder_netlink.c
create mode 100644 drivers/android/binder_netlink.h
create mode 100644 include/uapi/linux/android/binder_netlink.h
base-commit: cd3215bbcb9d4321def93fea6cfad4d5b42b9d1d
--
2.48.1.711.g2feabab25a-goog
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v16 1/3] lsm, selinux: Add setup_report permission to binder
2025-03-03 20:02 [PATCH v16 0/3] binder: report txn errors via generic netlink Li Li
@ 2025-03-03 20:02 ` Li Li
2025-03-07 21:47 ` Paul Moore
2025-03-03 20:02 ` [PATCH v16 2/3] binder: report txn errors via generic netlink Li Li
2025-03-03 20:02 ` [PATCH v16 3/3] binder: generic netlink binder_features flag Li Li
2 siblings, 1 reply; 11+ messages in thread
From: Li Li @ 2025-03-03 20:02 UTC (permalink / raw)
To: dualli, corbet, davem, edumazet, kuba, pabeni, donald.hunter,
gregkh, arve, tkjos, maco, joel, brauner, cmllamas, surenb,
omosnace, shuah, arnd, masahiroy, bagasdotme, horms, tweek,
linux-kernel, linux-doc, netdev, selinux, hridya
Cc: smoreland, ynaffit, kernel-team
From: Thiébaud Weksteen <tweek@google.com>
Introduce a new permission "setup_report" to the "binder" class.
This persmission controls the ability to set up the binder generic
netlink driver to report certain binder transactions.
Signed-off-by: Thiébaud Weksteen <tweek@google.com>
Signed-off-by: Li Li <dualli@google.com>
---
include/linux/lsm_hook_defs.h | 1 +
include/linux/security.h | 6 ++++++
security/security.c | 13 +++++++++++++
security/selinux/hooks.c | 7 +++++++
security/selinux/include/classmap.h | 3 ++-
5 files changed, 29 insertions(+), 1 deletion(-)
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 2bf909fa3394..395528de689f 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -33,6 +33,7 @@ LSM_HOOK(int, 0, binder_transfer_binder, const struct cred *from,
const struct cred *to)
LSM_HOOK(int, 0, binder_transfer_file, const struct cred *from,
const struct cred *to, const struct file *file)
+LSM_HOOK(int, 0, binder_setup_report, const struct cred *to)
LSM_HOOK(int, 0, ptrace_access_check, struct task_struct *child,
unsigned int mode)
LSM_HOOK(int, 0, ptrace_traceme, struct task_struct *parent)
diff --git a/include/linux/security.h b/include/linux/security.h
index 1545d515a66b..b3c01254023e 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -338,6 +338,7 @@ int security_binder_transfer_binder(const struct cred *from,
const struct cred *to);
int security_binder_transfer_file(const struct cred *from,
const struct cred *to, const struct file *file);
+int security_binder_setup_report(const struct cred *to);
int security_ptrace_access_check(struct task_struct *child, unsigned int mode);
int security_ptrace_traceme(struct task_struct *parent);
int security_capget(const struct task_struct *target,
@@ -657,6 +658,11 @@ static inline int security_binder_transfer_file(const struct cred *from,
return 0;
}
+static inline int security_binder_setup_report(const struct cred *to)
+{
+ return 0;
+}
+
static inline int security_ptrace_access_check(struct task_struct *child,
unsigned int mode)
{
diff --git a/security/security.c b/security/security.c
index 8aa839232c73..382e3bbab215 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1043,6 +1043,19 @@ int security_binder_transfer_file(const struct cred *from,
return call_int_hook(binder_transfer_file, from, to, file);
}
+/**
+ * security_binder_setup_report() - Check if process allowed to set up binder reports.
+ * @to: receiving process
+ *
+ * Check whether @to is allowed to set up binder reports.
+ *
+ * Return: Returns 0 if permission is granted.
+ */
+int security_binder_setup_report(const struct cred *to)
+{
+ return call_int_hook(binder_setup_report, to);
+}
+
/**
* security_ptrace_access_check() - Check if tracing is allowed
* @child: target process
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 0d958f38ff9f..2fafa8feafdf 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2092,6 +2092,12 @@ static int selinux_binder_transfer_file(const struct cred *from,
&ad);
}
+static int selinux_binder_setup_report(const struct cred *to)
+{
+ return avc_has_perm(current_sid(), cred_sid(to), SECCLASS_BINDER,
+ BINDER__SETUP_REPORT, NULL);
+}
+
static int selinux_ptrace_access_check(struct task_struct *child,
unsigned int mode)
{
@@ -7217,6 +7223,7 @@ static struct security_hook_list selinux_hooks[] __ro_after_init = {
LSM_HOOK_INIT(binder_transaction, selinux_binder_transaction),
LSM_HOOK_INIT(binder_transfer_binder, selinux_binder_transfer_binder),
LSM_HOOK_INIT(binder_transfer_file, selinux_binder_transfer_file),
+ LSM_HOOK_INIT(binder_setup_report, selinux_binder_setup_report),
LSM_HOOK_INIT(ptrace_access_check, selinux_ptrace_access_check),
LSM_HOOK_INIT(ptrace_traceme, selinux_ptrace_traceme),
diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
index 04a9b480885e..156741f1ca3f 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -135,7 +135,8 @@ const struct security_class_mapping secclass_map[] = {
{ "kernel_service", { "use_as_override", "create_files_as", NULL } },
{ "tun_socket", { COMMON_SOCK_PERMS, "attach_queue", NULL } },
{ "binder",
- { "impersonate", "call", "set_context_mgr", "transfer", NULL } },
+ { "impersonate", "call", "set_context_mgr", "transfer",
+ "setup_report", NULL } },
{ "cap_userns", { COMMON_CAP_PERMS, NULL } },
{ "cap2_userns", { COMMON_CAP2_PERMS, NULL } },
{ "sctp_socket",
--
2.48.1.711.g2feabab25a-goog
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v16 2/3] binder: report txn errors via generic netlink
2025-03-03 20:02 [PATCH v16 0/3] binder: report txn errors via generic netlink Li Li
2025-03-03 20:02 ` [PATCH v16 1/3] lsm, selinux: Add setup_report permission to binder Li Li
@ 2025-03-03 20:02 ` Li Li
2025-03-11 4:12 ` Carlos Llamas
2025-03-03 20:02 ` [PATCH v16 3/3] binder: generic netlink binder_features flag Li Li
2 siblings, 1 reply; 11+ messages in thread
From: Li Li @ 2025-03-03 20:02 UTC (permalink / raw)
To: dualli, corbet, davem, edumazet, kuba, pabeni, donald.hunter,
gregkh, arve, tkjos, maco, joel, brauner, cmllamas, surenb,
omosnace, shuah, arnd, masahiroy, bagasdotme, horms, tweek,
linux-kernel, linux-doc, netdev, selinux, hridya
Cc: smoreland, ynaffit, kernel-team
From: Li Li <dualli@google.com>
Introduce generic netlink messages into the binder driver so that the
Linux/Android system administration processes 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 binder netlink sources and headers are automatically generated from
the corresponding binder netlink YAML spec. Don't modify them directly.
Signed-off-by: Li Li <dualli@google.com>
---
Documentation/admin-guide/binder_netlink.rst | 108 +++++++++
Documentation/admin-guide/index.rst | 1 +
Documentation/netlink/specs/binder.yaml | 116 ++++++++++
drivers/android/Kconfig | 1 +
drivers/android/Makefile | 2 +-
drivers/android/binder.c | 229 ++++++++++++++++++-
drivers/android/binder_internal.h | 16 ++
drivers/android/binder_netlink.c | 46 ++++
drivers/android/binder_netlink.h | 23 ++
drivers/android/binder_trace.h | 35 +++
include/uapi/linux/android/binder_netlink.h | 57 +++++
11 files changed, 630 insertions(+), 4 deletions(-)
create mode 100644 Documentation/admin-guide/binder_netlink.rst
create mode 100644 Documentation/netlink/specs/binder.yaml
create mode 100644 drivers/android/binder_netlink.c
create mode 100644 drivers/android/binder_netlink.h
create mode 100644 include/uapi/linux/android/binder_netlink.h
diff --git a/Documentation/admin-guide/binder_netlink.rst b/Documentation/admin-guide/binder_netlink.rst
new file mode 100644
index 000000000000..83f54f0d8c45
--- /dev/null
+++ b/Documentation/admin-guide/binder_netlink.rst
@@ -0,0 +1,108 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+==============================================================
+Generic Netlink for the Android Binder Driver (Binder Netlink)
+==============================================================
+
+The Generic Netlink subsystem in the Linux kernel provides a generic way for
+the Linux kernel to communicate with the user space applications via binder
+driver. It is used to report binder transaction errors and warnings to user
+space administration process. The driver allows multiple binder devices and
+their corresponding binder contexts. Each context and their processes can be
+configured independently to report their own transactions.
+
+Basically, the user space code uses the BINDER_CMD_REPORT_SETUP command to
+request what kind of binder transactions should be reported by the driver.
+The driver then echoes the attributes in a reply message to acknowledge the
+request. The BINDER_CMD_REPORT_SETUP command also registers the current
+user space process to receive the reports. This BINDER_CMD_REPORT_SETUP
+command is protected by SELinux, to prevent unauthorized user apps from
+triggering unexpected reports.
+
+Currently the driver reports these binder transaction errors and warnings.
+1. "FAILED" transactions that fail to reach the target process;
+2. "ASYNC_FROZEN" transactions that are delayed due to the target process
+being frozen by cgroup freezer; or
+3. "SPAM" transactions that are considered spamming according to existing
+logic in binder_alloc.c.
+
+When the specified binder transactions happen, the driver uses the
+BINDER_CMD_REPORT command to send a generic netlink message to the
+registered process, containing the payload defined in binder.yaml.
+
+More details about the flags, attributes and operations can be found at the
+the doc sections in Documentations/netlink/specs/binder.yaml and the
+kernel-doc comments of the new source code in binder.{h|c}.
+
+Using Binder Netlink
+--------------------
+
+The Binder Netlink 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.
+
+Usage example (user space pseudo code):
+
+::
+ /*
+ * send() below is overloaded to pack netlink commands and attributes
+ * to nlattr/genlmsghdr/nlmsghdr and then send to the netlink socket.
+ *
+ * recv() below is overloaded to receive the raw netlink message from
+ * the netlink socket, parse nlmsghdr/genlmsghdr to find the netlink
+ * command and then return the nlattr payload.
+ */
+
+ // 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 netlink
+ send(fd, CTRL_CMD_GETFAMILY, CTRL_ATTR_FAMILY_NAME,
+ BINDER_FAMILY_NAME);
+ void *data = recv(CTRL_CMD_NEWFAMILY);
+ if (!has_nla_type(data, CTRL_ATTR_FAMILY_ID)) {
+ // Binder Netlink isn't available on this version of Linux kernel
+ return;
+ }
+ __u16 id = nla(data)[CTRL_ATTR_FAMILY_ID];
+ __u32 grp = nla(data)[CTRL_ATTR_MCAST_GROUPS][CTRL_ATTR_MCAST_GRP_ID];
+
+ // join the mcast group to listen to the binder netlink messages
+ setsockopt(fd, SOL_NETLINK, NETLINK_ADD_MEMBERSHIP, &grp, sizeof(grp));
+
+ // enable per-context binder report
+ send(fd, id, BINDER_CMD_REPORT_SETUP, "binder", 0,
+ BINDER_FLAG_FAILED | BINDER_FLAG_DELAYED);
+
+ // confirm the per-context configuration
+ data = recv(fd, BINDER_CMD_REPLY);
+ char *context = nla(data)[BINDER_A_CMD_CONTEXT];
+ __u32 pid = nla(data)[BINDER_A_CMD_PID];
+ __u32 flags = nla(data)[BINDER_A_CMD_FLAGS];
+
+ // set optional per-process report, overriding the per-context one
+ send(fd, id, BINDER_CMD_REPORT_SETUP, "binder", getpid(),
+ BINDER_FLAG_SPAM | BINDER_REPORT_OVERRIDE);
+
+ // confirm the optional per-process configuration
+ data = recv(fd, BINDER_CMD_REPLY);
+ context = nla(data)[BINDER_A_CMD_CONTEXT];
+ pid = nla(data)[BINDER_A_CMD_PID];
+ flags = nla(data)[BINDER_A_CMD_FLAGS];
+
+ // wait and read all binder reports
+ while (running) {
+ data = recv(fd, BINDER_CMD_REPORT);
+ auto *attr = nla(data)[BINDER_A_REPORT_XXX];
+
+ // process binder report
+ do_something(*attr);
+ }
+
+ // clean up
+ send(fd, id, BINDER_CMD_REPORT_SETUP, 0, 0);
+ send(fd, id, BINDER_CMD_REPORT_SETUP, getpid(), 0);
+ close(fd);
diff --git a/Documentation/admin-guide/index.rst b/Documentation/admin-guide/index.rst
index c8af32a8f800..655f39ddf1cf 100644
--- a/Documentation/admin-guide/index.rst
+++ b/Documentation/admin-guide/index.rst
@@ -120,6 +120,7 @@ Block-layer and filesystem administration
:maxdepth: 1
bcache
+ binder_netlink
binderfs
blockdev/index
cifs/index
diff --git a/Documentation/netlink/specs/binder.yaml b/Documentation/netlink/specs/binder.yaml
new file mode 100644
index 000000000000..e184645f6c41
--- /dev/null
+++ b/Documentation/netlink/specs/binder.yaml
@@ -0,0 +1,116 @@
+# SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause)
+
+name: binder
+protocol: genetlink
+uapi-header: linux/android/binder_netlink.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, async-frozen, spam, override ]
+
+attribute-sets:
+ -
+ name: cmd
+ doc: The supported attributes of "report-setup" command.
+ attributes:
+ -
+ name: context
+ type: string
+ doc: The binder context to enable binder netlink report.
+ -
+ name: pid
+ type: u32
+ doc: The binder proc to enable binder netlink 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 netlink 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: report-setup
+ 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
+
+kernel-family:
+ headers: [ "binder_internal.h" ]
+
+mcast-groups:
+ list:
+ -
+ name: report
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..b8874dba884e 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_netlink.o
obj-$(CONFIG_ANDROID_BINDER_IPC_SELFTEST) += binder_alloc_selftest.o
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 76052006bd87..262c120a175a 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -72,6 +72,7 @@
#include <linux/cacheflush.h>
+#include "binder_netlink.h"
#include "binder_internal.h"
#include "binder_trace.h"
@@ -2990,6 +2991,110 @@ static void binder_set_txn_from_error(struct binder_transaction *t, int id,
binder_thread_dec_tmpref(from);
}
+/**
+ * binder_find_proc() - find the binder_proc by pid
+ * @pid: the target process
+ *
+ * Returns the struct binder_proc if the pid is found, or NULL otherwise.
+ */
+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)
+ break;
+ }
+ mutex_unlock(&binder_procs_lock);
+
+ return proc;
+}
+
+/**
+ * binder_netlink_enabled() - check if binder netlink reports are enabled
+ * @proc: the binder_proc to check
+ * @mask: the categories of binder netlink reports
+ *
+ * Returns true if certain binder netlink reports are enabled for this binder
+ * proc (when per-process overriding takes effect) or context.
+ */
+static bool binder_netlink_enabled(struct binder_proc *proc, u32 mask)
+{
+ struct binder_context *context = proc->context;
+
+ if (!genl_has_listeners(&binder_nl_family, &init_net, BINDER_NLGRP_REPORT))
+ return false;
+
+ if (proc->report_flags & BINDER_FLAG_OVERRIDE)
+ return (proc->report_flags & mask) != 0;
+ else
+ return (context->report_flags & mask) != 0;
+}
+
+/**
+ * binder_netlink_report() - report one binder netlink event
+ * @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 binder netlink message and send it.
+ */
+static void binder_netlink_report(struct binder_context *context, u32 err,
+ u32 pid, u32 tid, u32 to_pid, u32 to_tid,
+ u32 reply,
+ struct binder_transaction_data *tr)
+{
+ struct sk_buff *skb;
+ void *hdr;
+ int ret;
+
+ trace_binder_netlink_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 netlink message\n");
+ return;
+ }
+
+ hdr = genlmsg_put(skb, 0, atomic_inc_return(&context->report_seq),
+ &binder_nl_family, 0, BINDER_CMD_REPORT);
+ if (!hdr)
+ goto free_skb;
+
+ if (nla_put_string(skb, BINDER_A_REPORT_CONTEXT, context->name) ||
+ nla_put_u32(skb, BINDER_A_REPORT_ERR, err) ||
+ nla_put_u32(skb, BINDER_A_REPORT_FROM_PID, pid) ||
+ nla_put_u32(skb, BINDER_A_REPORT_FROM_TID, tid) ||
+ nla_put_u32(skb, BINDER_A_REPORT_TO_PID, to_pid) ||
+ nla_put_u32(skb, BINDER_A_REPORT_TO_TID, to_tid) ||
+ nla_put_u32(skb, BINDER_A_REPORT_REPLY, reply) ||
+ nla_put_u32(skb, BINDER_A_REPORT_FLAGS, tr->flags) ||
+ nla_put_u32(skb, BINDER_A_REPORT_CODE, tr->code) ||
+ nla_put_u32(skb, BINDER_A_REPORT_DATA_SIZE, tr->data_size))
+ goto cancel_skb;
+
+ genlmsg_end(skb, hdr);
+
+ ret = genlmsg_multicast(&binder_nl_family, skb, 0, BINDER_NLGRP_REPORT, GFP_KERNEL);
+ if (ret < 0)
+ pr_err("Failed to send binder netlink message: %d\n", ret);
+ return;
+
+cancel_skb:
+ pr_err("Failed to add attributes to binder netlink message\n");
+ genlmsg_cancel(skb, hdr);
+free_skb:
+ pr_err("Free binder netlink 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,
@@ -3683,10 +3788,17 @@ 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_netlink_enabled(proc, BINDER_FLAG_SPAM))
+ binder_netlink_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) {
@@ -3736,8 +3848,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_netlink_enabled(proc, BINDER_FLAG_ASYNC_FROZEN))
+ binder_netlink_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)
@@ -3799,6 +3918,13 @@ static void binder_transaction(struct binder_proc *proc,
binder_dec_node_tmpref(target_node);
}
+ if (binder_netlink_enabled(proc, BINDER_FLAG_FAILED))
+ binder_netlink_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, code %u size %lld-%lld line %d\n",
proc->pid, thread->pid, reply ? "reply" :
@@ -6334,6 +6460,98 @@ binder_defer_work(struct binder_proc *proc, enum binder_deferred_state defer)
mutex_unlock(&binder_deferred_lock);
}
+/**
+ * binder_nl_report_setup_doit() - netlink .doit handler
+ * @skb: the metadata struct passed from netlink driver
+ * @info: the generic netlink struct passed from netlink driver
+ *
+ * Implements the .doit function to process binder netlink commands.
+ */
+int binder_nl_report_setup_doit(struct sk_buff *skb, struct genl_info *info)
+{
+ struct binder_context *context = NULL;
+ struct binder_device *device;
+ struct binder_proc *proc;
+ u32 flags, pid;
+ void *hdr;
+ int ret;
+
+ ret = security_binder_setup_report(current_cred());
+ if (ret < 0) {
+ NL_SET_ERR_MSG(info->extack, "Permission denied");
+ return ret;
+ }
+
+ hlist_for_each_entry(device, &binder_devices, hlist) {
+ if (!nla_strcmp(info->attrs[BINDER_A_CMD_CONTEXT],
+ device->context.name)) {
+ context = &device->context;
+ break;
+ }
+ }
+
+ if (!context) {
+ NL_SET_ERR_MSG(info->extack, "Unknown binder context");
+ return -EINVAL;
+ }
+
+ pid = nla_get_u32(info->attrs[BINDER_A_CMD_PID]);
+ flags = nla_get_u32(info->attrs[BINDER_A_CMD_FLAGS]);
+
+ 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) {
+ NL_SET_ERR_MSG_FMT(info->extack,
+ "Invalid binder report pid %u",
+ pid);
+ ret = -EINVAL;
+ goto err_exit;
+ }
+
+ proc->report_flags = flags;
+ }
+
+ skb = genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_KERNEL);
+ if (!skb) {
+ pr_err("Failed to alloc binder netlink reply message\n");
+ ret = -ENOMEM;
+ goto err_exit;
+ }
+
+ hdr = genlmsg_iput(skb, info);
+ if (!hdr)
+ goto free_skb;
+
+ if (nla_put_string(skb, BINDER_A_CMD_CONTEXT, context->name) ||
+ nla_put_u32(skb, BINDER_A_CMD_PID, pid) ||
+ nla_put_u32(skb, BINDER_A_CMD_FLAGS, flags))
+ goto cancel_skb;
+
+ genlmsg_end(skb, hdr);
+
+ if (genlmsg_reply(skb, info)) {
+ pr_err("Failed to send binder netlink reply message\n");
+ ret = -EFAULT;
+ goto err_exit;
+ }
+
+ return 0;
+
+cancel_skb:
+ pr_err("Failed to add reply attributes to binder netlink message\n");
+ genlmsg_cancel(skb, hdr);
+free_skb:
+ pr_err("Free binder netlink reply message on error\n");
+ nlmsg_free(skb);
+ ret = -EMSGSIZE;
+err_exit:
+ return ret;
+}
+
static void print_binder_transaction_ilocked(struct seq_file *m,
struct binder_proc *proc,
const char *prefix,
@@ -7013,6 +7231,11 @@ static int __init binder_init(void)
if (ret)
goto err_init_binder_device_failed;
+ ret = genl_register_family(&binder_nl_family);
+ if (ret) {
+ pr_err("Failed to register binder netlink family\n");
+ goto err_init_binder_device_failed;
+ }
return ret;
err_init_binder_device_failed:
diff --git a/drivers/android/binder_internal.h b/drivers/android/binder_internal.h
index 6a66c9769c6c..8ddd1ddea8e0 100644
--- a/drivers/android/binder_internal.h
+++ b/drivers/android/binder_internal.h
@@ -11,15 +11,28 @@
#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_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_flags;
+ atomic_t report_seq;
};
/**
@@ -413,6 +426,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
*/
@@ -451,6 +466,7 @@ struct binder_proc {
spinlock_t outer_lock;
struct dentry *binderfs_entry;
bool oneway_spam_detection_enabled;
+ u32 report_flags;
};
/**
diff --git a/drivers/android/binder_netlink.c b/drivers/android/binder_netlink.c
new file mode 100644
index 000000000000..9dc1820951c3
--- /dev/null
+++ b/drivers/android/binder_netlink.c
@@ -0,0 +1,46 @@
+// 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.yaml */
+/* YNL-GEN kernel source */
+
+#include <net/netlink.h>
+#include <net/genetlink.h>
+
+#include "binder_netlink.h"
+
+#include <uapi/linux/android/binder_netlink.h>
+#include <binder_internal.h>
+
+/* BINDER_CMD_REPORT_SETUP - do */
+static const struct nla_policy binder_report_setup_nl_policy[BINDER_A_CMD_FLAGS + 1] = {
+ [BINDER_A_CMD_CONTEXT] = { .type = NLA_NUL_STRING, },
+ [BINDER_A_CMD_PID] = { .type = NLA_U32, },
+ [BINDER_A_CMD_FLAGS] = NLA_POLICY_MASK(NLA_U32, 0xf),
+};
+
+/* Ops table for binder */
+static const struct genl_split_ops binder_nl_ops[] = {
+ {
+ .cmd = BINDER_CMD_REPORT_SETUP,
+ .doit = binder_nl_report_setup_doit,
+ .policy = binder_report_setup_nl_policy,
+ .maxattr = BINDER_A_CMD_FLAGS,
+ .flags = GENL_CMD_CAP_DO,
+ },
+};
+
+static const struct genl_multicast_group binder_nl_mcgrps[] = {
+ [BINDER_NLGRP_REPORT] = { "report", },
+};
+
+struct genl_family binder_nl_family __ro_after_init = {
+ .name = BINDER_FAMILY_NAME,
+ .version = BINDER_FAMILY_VERSION,
+ .netnsok = true,
+ .parallel_ops = true,
+ .module = THIS_MODULE,
+ .split_ops = binder_nl_ops,
+ .n_split_ops = ARRAY_SIZE(binder_nl_ops),
+ .mcgrps = binder_nl_mcgrps,
+ .n_mcgrps = ARRAY_SIZE(binder_nl_mcgrps),
+};
diff --git a/drivers/android/binder_netlink.h b/drivers/android/binder_netlink.h
new file mode 100644
index 000000000000..0ef8c91ab319
--- /dev/null
+++ b/drivers/android/binder_netlink.h
@@ -0,0 +1,23 @@
+/* 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.yaml */
+/* YNL-GEN kernel header */
+
+#ifndef _LINUX_BINDER_GEN_H
+#define _LINUX_BINDER_GEN_H
+
+#include <net/netlink.h>
+#include <net/genetlink.h>
+
+#include <uapi/linux/android/binder_netlink.h>
+#include <binder_internal.h>
+
+int binder_nl_report_setup_doit(struct sk_buff *skb, struct genl_info *info);
+
+enum {
+ BINDER_NLGRP_REPORT,
+};
+
+extern struct genl_family binder_nl_family;
+
+#endif /* _LINUX_BINDER_GEN_H */
diff --git a/drivers/android/binder_trace.h b/drivers/android/binder_trace.h
index 16de1b9e72f7..6fb454bcd857 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_netlink_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/include/uapi/linux/android/binder_netlink.h b/include/uapi/linux/android/binder_netlink.h
new file mode 100644
index 000000000000..fa926ed19507
--- /dev/null
+++ b/include/uapi/linux/android/binder_netlink.h
@@ -0,0 +1,57 @@
+/* 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.yaml */
+/* YNL-GEN uapi header */
+
+#ifndef _UAPI_LINUX_ANDROID_BINDER_NETLINK_H
+#define _UAPI_LINUX_ANDROID_BINDER_NETLINK_H
+
+#define BINDER_FAMILY_NAME "binder"
+#define BINDER_FAMILY_VERSION 1
+
+/*
+ * Define what kind of binder transactions should be reported.
+ */
+enum binder_flag {
+ BINDER_FLAG_FAILED = 1,
+ BINDER_FLAG_ASYNC_FROZEN = 2,
+ BINDER_FLAG_SPAM = 4,
+ BINDER_FLAG_OVERRIDE = 8,
+};
+
+enum {
+ BINDER_A_CMD_CONTEXT = 1,
+ BINDER_A_CMD_PID,
+ BINDER_A_CMD_FLAGS,
+
+ __BINDER_A_CMD_MAX,
+ BINDER_A_CMD_MAX = (__BINDER_A_CMD_MAX - 1)
+};
+
+enum {
+ BINDER_A_REPORT_CONTEXT = 1,
+ BINDER_A_REPORT_ERR,
+ BINDER_A_REPORT_FROM_PID,
+ BINDER_A_REPORT_FROM_TID,
+ BINDER_A_REPORT_TO_PID,
+ BINDER_A_REPORT_TO_TID,
+ BINDER_A_REPORT_REPLY,
+ BINDER_A_REPORT_FLAGS,
+ BINDER_A_REPORT_CODE,
+ BINDER_A_REPORT_DATA_SIZE,
+
+ __BINDER_A_REPORT_MAX,
+ BINDER_A_REPORT_MAX = (__BINDER_A_REPORT_MAX - 1)
+};
+
+enum {
+ BINDER_CMD_REPORT_SETUP = 1,
+ BINDER_CMD_REPORT,
+
+ __BINDER_CMD_MAX,
+ BINDER_CMD_MAX = (__BINDER_CMD_MAX - 1)
+};
+
+#define BINDER_MCGRP_REPORT "report"
+
+#endif /* _UAPI_LINUX_ANDROID_BINDER_NETLINK_H */
--
2.48.1.711.g2feabab25a-goog
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v16 3/3] binder: generic netlink binder_features flag
2025-03-03 20:02 [PATCH v16 0/3] binder: report txn errors via generic netlink Li Li
2025-03-03 20:02 ` [PATCH v16 1/3] lsm, selinux: Add setup_report permission to binder Li Li
2025-03-03 20:02 ` [PATCH v16 2/3] binder: report txn errors via generic netlink Li Li
@ 2025-03-03 20:02 ` Li Li
2025-03-11 3:37 ` Carlos Llamas
2 siblings, 1 reply; 11+ messages in thread
From: Li Li @ 2025-03-03 20:02 UTC (permalink / raw)
To: dualli, corbet, davem, edumazet, kuba, pabeni, donald.hunter,
gregkh, arve, tkjos, maco, joel, brauner, cmllamas, surenb,
omosnace, shuah, arnd, masahiroy, bagasdotme, horms, tweek,
linux-kernel, linux-doc, netdev, selinux, hridya
Cc: smoreland, ynaffit, kernel-team
From: Li Li <dualli@google.com>
Add a flag to binder_features to indicate that the generic netlink
feature is available.
Signed-off-by: Li Li <dualli@google.com>
---
drivers/android/binderfs.c | 8 ++++++++
.../selftests/filesystems/binderfs/binderfs_test.c | 1 +
2 files changed, 9 insertions(+)
diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index 94c6446604fc..b3d21ccd81f2 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -59,6 +59,7 @@ struct binder_features {
bool oneway_spam_detection;
bool extended_error;
bool freeze_notification;
+ bool generic_netlink;
};
static const struct constant_table binderfs_param_stats[] = {
@@ -76,6 +77,7 @@ static struct binder_features binder_features = {
.oneway_spam_detection = true,
.extended_error = true,
.freeze_notification = true,
+ .generic_netlink = true,
};
static inline struct binderfs_info *BINDERFS_SB(const struct super_block *sb)
@@ -619,6 +621,12 @@ static int init_binder_features(struct super_block *sb)
if (IS_ERR(dentry))
return PTR_ERR(dentry);
+ dentry = binderfs_create_file(dir, "generic_netlink",
+ &binder_features_fops,
+ &binder_features.generic_netlink);
+ if (IS_ERR(dentry))
+ return PTR_ERR(dentry);
+
return 0;
}
diff --git a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
index 81db85a5cc16..96e64ebf910f 100644
--- a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
+++ b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
@@ -65,6 +65,7 @@ static int __do_binderfs_test(struct __test_metadata *_metadata)
"oneway_spam_detection",
"extended_error",
"freeze_notification",
+ "generic_netlink",
};
change_mountns(_metadata);
--
2.48.1.711.g2feabab25a-goog
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v16 1/3] lsm, selinux: Add setup_report permission to binder
2025-03-03 20:02 ` [PATCH v16 1/3] lsm, selinux: Add setup_report permission to binder Li Li
@ 2025-03-07 21:47 ` Paul Moore
2025-03-11 16:37 ` Li Li
0 siblings, 1 reply; 11+ messages in thread
From: Paul Moore @ 2025-03-07 21:47 UTC (permalink / raw)
To: Li Li
Cc: dualli, corbet, davem, edumazet, kuba, pabeni, donald.hunter,
gregkh, arve, tkjos, maco, joel, brauner, cmllamas, surenb,
omosnace, shuah, arnd, masahiroy, bagasdotme, horms, tweek,
linux-kernel, linux-doc, netdev, selinux, hridya, smoreland,
ynaffit, kernel-team
On Mon, Mar 3, 2025 at 3:02 PM Li Li <dualli@chromium.org> wrote:
>
> From: Thiébaud Weksteen <tweek@google.com>
>
> Introduce a new permission "setup_report" to the "binder" class.
> This persmission controls the ability to set up the binder generic
> netlink driver to report certain binder transactions.
>
> Signed-off-by: Thiébaud Weksteen <tweek@google.com>
> Signed-off-by: Li Li <dualli@google.com>
> ---
> include/linux/lsm_hook_defs.h | 1 +
> include/linux/security.h | 6 ++++++
> security/security.c | 13 +++++++++++++
> security/selinux/hooks.c | 7 +++++++
> security/selinux/include/classmap.h | 3 ++-
> 5 files changed, 29 insertions(+), 1 deletion(-)
...
> diff --git a/security/security.c b/security/security.c
> index 8aa839232c73..382e3bbab215 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1043,6 +1043,19 @@ int security_binder_transfer_file(const struct cred *from,
> return call_int_hook(binder_transfer_file, from, to, file);
> }
>
> +/**
> + * security_binder_setup_report() - Check if process allowed to set up binder reports.
Please keep the line length in the LSM and SELinux code to 80
characters or less.
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 0d958f38ff9f..2fafa8feafdf 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2092,6 +2092,12 @@ static int selinux_binder_transfer_file(const struct cred *from,
> &ad);
> }
>
> +static int selinux_binder_setup_report(const struct cred *to)
> +{
> + return avc_has_perm(current_sid(), cred_sid(to), SECCLASS_BINDER,
> + BINDER__SETUP_REPORT, NULL);
> +}
There should also be an associated patch{set} against the
selinux-testsuite to add tests for the binder/setup_report permission
introduced here. My apologies if you've already posted one, but I'm
looking now and I don't see anything either on the lists or on GH.
* https://github.com/SELinuxProject/selinux-testsuite
--
paul-moore.com
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v16 3/3] binder: generic netlink binder_features flag
2025-03-03 20:02 ` [PATCH v16 3/3] binder: generic netlink binder_features flag Li Li
@ 2025-03-11 3:37 ` Carlos Llamas
0 siblings, 0 replies; 11+ messages in thread
From: Carlos Llamas @ 2025-03-11 3:37 UTC (permalink / raw)
To: Li Li
Cc: dualli, corbet, davem, edumazet, kuba, pabeni, donald.hunter,
gregkh, arve, tkjos, maco, joel, brauner, surenb, omosnace, shuah,
arnd, masahiroy, bagasdotme, horms, tweek, linux-kernel,
linux-doc, netdev, selinux, hridya, smoreland, ynaffit,
kernel-team
On Mon, Mar 03, 2025 at 12:02:12PM -0800, Li Li wrote:
> From: Li Li <dualli@google.com>
>
> Add a flag to binder_features to indicate that the generic netlink
> feature is available.
>
> Signed-off-by: Li Li <dualli@google.com>
> ---
The implementation looks correct. However, the feature you are adding is
not "generic_netlink", maybe "transaction_report" or something similar?
Future features might use this same interface so lets avoid confusion.
Thanks,
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v16 2/3] binder: report txn errors via generic netlink
2025-03-03 20:02 ` [PATCH v16 2/3] binder: report txn errors via generic netlink Li Li
@ 2025-03-11 4:12 ` Carlos Llamas
[not found] ` <CANBPYPhR-C3VTv=ZHc1LJ0c7OG8-K2iGS62vXHmg9gcX0y89Cw@mail.gmail.com>
0 siblings, 1 reply; 11+ messages in thread
From: Carlos Llamas @ 2025-03-11 4:12 UTC (permalink / raw)
To: Li Li
Cc: dualli, corbet, davem, edumazet, kuba, pabeni, donald.hunter,
gregkh, arve, tkjos, maco, joel, brauner, surenb, omosnace, shuah,
arnd, masahiroy, bagasdotme, horms, tweek, linux-kernel,
linux-doc, netdev, selinux, hridya, smoreland, ynaffit,
kernel-team
On Mon, Mar 03, 2025 at 12:02:11PM -0800, Li Li wrote:
> From: Li Li <dualli@google.com>
>
> Introduce generic netlink messages into the binder driver so that the
> Linux/Android system administration processes 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 binder netlink sources and headers are automatically generated from
> the corresponding binder netlink YAML spec. Don't modify them directly.
>
> Signed-off-by: Li Li <dualli@google.com>
> ---
> Documentation/admin-guide/binder_netlink.rst | 108 +++++++++
> Documentation/admin-guide/index.rst | 1 +
> Documentation/netlink/specs/binder.yaml | 116 ++++++++++
> drivers/android/Kconfig | 1 +
> drivers/android/Makefile | 2 +-
> drivers/android/binder.c | 229 ++++++++++++++++++-
> drivers/android/binder_internal.h | 16 ++
> drivers/android/binder_netlink.c | 46 ++++
> drivers/android/binder_netlink.h | 23 ++
> drivers/android/binder_trace.h | 35 +++
> include/uapi/linux/android/binder_netlink.h | 57 +++++
> 11 files changed, 630 insertions(+), 4 deletions(-)
> create mode 100644 Documentation/admin-guide/binder_netlink.rst
> create mode 100644 Documentation/netlink/specs/binder.yaml
> create mode 100644 drivers/android/binder_netlink.c
> create mode 100644 drivers/android/binder_netlink.h
> create mode 100644 include/uapi/linux/android/binder_netlink.h
>
> diff --git a/Documentation/admin-guide/binder_netlink.rst b/Documentation/admin-guide/binder_netlink.rst
> new file mode 100644
> index 000000000000..83f54f0d8c45
> --- /dev/null
> +++ b/Documentation/admin-guide/binder_netlink.rst
> @@ -0,0 +1,108 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +==============================================================
> +Generic Netlink for the Android Binder Driver (Binder Netlink)
> +==============================================================
> +
> +The Generic Netlink subsystem in the Linux kernel provides a generic way for
> +the Linux kernel to communicate with the user space applications via binder
> +driver. It is used to report binder transaction errors and warnings to user
> +space administration process. The driver allows multiple binder devices and
> +their corresponding binder contexts. Each context and their processes can be
> +configured independently to report their own transactions.
> +
> +Basically, the user space code uses the BINDER_CMD_REPORT_SETUP command to
> +request what kind of binder transactions should be reported by the driver.
> +The driver then echoes the attributes in a reply message to acknowledge the
> +request. The BINDER_CMD_REPORT_SETUP command also registers the current
> +user space process to receive the reports. This BINDER_CMD_REPORT_SETUP
> +command is protected by SELinux, to prevent unauthorized user apps from
> +triggering unexpected reports.
> +
> +Currently the driver reports these binder transaction errors and warnings.
> +1. "FAILED" transactions that fail to reach the target process;
> +2. "ASYNC_FROZEN" transactions that are delayed due to the target process
> +being frozen by cgroup freezer; or
> +3. "SPAM" transactions that are considered spamming according to existing
> +logic in binder_alloc.c.
> +
> +When the specified binder transactions happen, the driver uses the
> +BINDER_CMD_REPORT command to send a generic netlink message to the
> +registered process, containing the payload defined in binder.yaml.
> +
> +More details about the flags, attributes and operations can be found at the
> +the doc sections in Documentations/netlink/specs/binder.yaml and the
> +kernel-doc comments of the new source code in binder.{h|c}.
> +
> +Using Binder Netlink
> +--------------------
> +
> +The Binder Netlink 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.
> +
> +Usage example (user space pseudo code):
> +
> +::
> + /*
> + * send() below is overloaded to pack netlink commands and attributes
> + * to nlattr/genlmsghdr/nlmsghdr and then send to the netlink socket.
> + *
> + * recv() below is overloaded to receive the raw netlink message from
> + * the netlink socket, parse nlmsghdr/genlmsghdr to find the netlink
> + * command and then return the nlattr payload.
> + */
> +
> + // 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 netlink
> + send(fd, CTRL_CMD_GETFAMILY, CTRL_ATTR_FAMILY_NAME,
> + BINDER_FAMILY_NAME);
> + void *data = recv(CTRL_CMD_NEWFAMILY);
> + if (!has_nla_type(data, CTRL_ATTR_FAMILY_ID)) {
> + // Binder Netlink isn't available on this version of Linux kernel
> + return;
> + }
> + __u16 id = nla(data)[CTRL_ATTR_FAMILY_ID];
> + __u32 grp = nla(data)[CTRL_ATTR_MCAST_GROUPS][CTRL_ATTR_MCAST_GRP_ID];
> +
> + // join the mcast group to listen to the binder netlink messages
> + setsockopt(fd, SOL_NETLINK, NETLINK_ADD_MEMBERSHIP, &grp, sizeof(grp));
> +
> + // enable per-context binder report
> + send(fd, id, BINDER_CMD_REPORT_SETUP, "binder", 0,
> + BINDER_FLAG_FAILED | BINDER_FLAG_DELAYED);
> +
> + // confirm the per-context configuration
> + data = recv(fd, BINDER_CMD_REPLY);
> + char *context = nla(data)[BINDER_A_CMD_CONTEXT];
> + __u32 pid = nla(data)[BINDER_A_CMD_PID];
> + __u32 flags = nla(data)[BINDER_A_CMD_FLAGS];
> +
> + // set optional per-process report, overriding the per-context one
> + send(fd, id, BINDER_CMD_REPORT_SETUP, "binder", getpid(),
> + BINDER_FLAG_SPAM | BINDER_REPORT_OVERRIDE);
> +
> + // confirm the optional per-process configuration
> + data = recv(fd, BINDER_CMD_REPLY);
> + context = nla(data)[BINDER_A_CMD_CONTEXT];
> + pid = nla(data)[BINDER_A_CMD_PID];
> + flags = nla(data)[BINDER_A_CMD_FLAGS];
> +
> + // wait and read all binder reports
> + while (running) {
> + data = recv(fd, BINDER_CMD_REPORT);
> + auto *attr = nla(data)[BINDER_A_REPORT_XXX];
> +
> + // process binder report
> + do_something(*attr);
> + }
> +
> + // clean up
> + send(fd, id, BINDER_CMD_REPORT_SETUP, 0, 0);
> + send(fd, id, BINDER_CMD_REPORT_SETUP, getpid(), 0);
> + close(fd);
> diff --git a/Documentation/admin-guide/index.rst b/Documentation/admin-guide/index.rst
> index c8af32a8f800..655f39ddf1cf 100644
> --- a/Documentation/admin-guide/index.rst
> +++ b/Documentation/admin-guide/index.rst
> @@ -120,6 +120,7 @@ Block-layer and filesystem administration
> :maxdepth: 1
>
> bcache
> + binder_netlink
> binderfs
> blockdev/index
> cifs/index
> diff --git a/Documentation/netlink/specs/binder.yaml b/Documentation/netlink/specs/binder.yaml
> new file mode 100644
> index 000000000000..e184645f6c41
> --- /dev/null
> +++ b/Documentation/netlink/specs/binder.yaml
> @@ -0,0 +1,116 @@
> +# SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause)
> +
> +name: binder
> +protocol: genetlink
> +uapi-header: linux/android/binder_netlink.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, async-frozen, spam, override ]
> +
> +attribute-sets:
> + -
> + name: cmd
> + doc: The supported attributes of "report-setup" command.
> + attributes:
> + -
> + name: context
> + type: string
> + doc: The binder context to enable binder netlink report.
> + -
> + name: pid
> + type: u32
> + doc: The binder proc to enable binder netlink 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 netlink 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: report-setup
> + 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
> +
> +kernel-family:
> + headers: [ "binder_internal.h" ]
> +
> +mcast-groups:
> + list:
> + -
> + name: report
> 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..b8874dba884e 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_netlink.o
> obj-$(CONFIG_ANDROID_BINDER_IPC_SELFTEST) += binder_alloc_selftest.o
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 76052006bd87..262c120a175a 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -72,6 +72,7 @@
>
> #include <linux/cacheflush.h>
>
> +#include "binder_netlink.h"
> #include "binder_internal.h"
> #include "binder_trace.h"
>
> @@ -2990,6 +2991,110 @@ static void binder_set_txn_from_error(struct binder_transaction *t, int id,
> binder_thread_dec_tmpref(from);
> }
>
> +/**
> + * binder_find_proc() - find the binder_proc by pid
> + * @pid: the target process
> + *
> + * Returns the struct binder_proc if the pid is found, or NULL otherwise.
> + */
> +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)
> + break;
Wait... can't there be multiple binder_proc instances matching the same
pid? I know that binder_proc is a bit of a misnomer but what should you
do in such case? Shouldn't you set the flags in _all_ matching pids?
Furthermore, there could be a single task talking on multiple contexts,
so you could be returning the 'proc' that doesn't match the context that
you are looking for right?
> + }
> + mutex_unlock(&binder_procs_lock);
> +
> + return proc;
> +}
> +
> +/**
> + * binder_netlink_enabled() - check if binder netlink reports are enabled
> + * @proc: the binder_proc to check
> + * @mask: the categories of binder netlink reports
> + *
> + * Returns true if certain binder netlink reports are enabled for this binder
> + * proc (when per-process overriding takes effect) or context.
> + */
> +static bool binder_netlink_enabled(struct binder_proc *proc, u32 mask)
> +{
> + struct binder_context *context = proc->context;
> +
> + if (!genl_has_listeners(&binder_nl_family, &init_net, BINDER_NLGRP_REPORT))
> + return false;
> +
> + if (proc->report_flags & BINDER_FLAG_OVERRIDE)
> + return (proc->report_flags & mask) != 0;
> + else
> + return (context->report_flags & mask) != 0;
> +}
> +
> +/**
> + * binder_netlink_report() - report one binder netlink event
> + * @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 binder netlink message and send it.
> + */
> +static void binder_netlink_report(struct binder_context *context, u32 err,
> + u32 pid, u32 tid, u32 to_pid, u32 to_tid,
Instead of all these parameters, is there a way to pass the transaction
itself? Isn't this info already populated there? I think it even holds
the info you are looking for from the 'binder_transaction_data' below.
> + u32 reply,
> + struct binder_transaction_data *tr)
> +{
> + struct sk_buff *skb;
> + void *hdr;
> + int ret;
> +
> + trace_binder_netlink_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 netlink message\n");
> + return;
> + }
> +
> + hdr = genlmsg_put(skb, 0, atomic_inc_return(&context->report_seq),
> + &binder_nl_family, 0, BINDER_CMD_REPORT);
> + if (!hdr)
> + goto free_skb;
> +
> + if (nla_put_string(skb, BINDER_A_REPORT_CONTEXT, context->name) ||
> + nla_put_u32(skb, BINDER_A_REPORT_ERR, err) ||
> + nla_put_u32(skb, BINDER_A_REPORT_FROM_PID, pid) ||
> + nla_put_u32(skb, BINDER_A_REPORT_FROM_TID, tid) ||
> + nla_put_u32(skb, BINDER_A_REPORT_TO_PID, to_pid) ||
> + nla_put_u32(skb, BINDER_A_REPORT_TO_TID, to_tid) ||
> + nla_put_u32(skb, BINDER_A_REPORT_REPLY, reply) ||
> + nla_put_u32(skb, BINDER_A_REPORT_FLAGS, tr->flags) ||
> + nla_put_u32(skb, BINDER_A_REPORT_CODE, tr->code) ||
> + nla_put_u32(skb, BINDER_A_REPORT_DATA_SIZE, tr->data_size))
> + goto cancel_skb;
> +
> + genlmsg_end(skb, hdr);
> +
> + ret = genlmsg_multicast(&binder_nl_family, skb, 0, BINDER_NLGRP_REPORT, GFP_KERNEL);
Thanks for switching to multicast. On this topic, we can only have a
single global configuration at a time correct? e.g. context vs per-proc.
So all listeners would ahve to work with the same setup?
> + if (ret < 0)
> + pr_err("Failed to send binder netlink message: %d\n", ret);
nit: can you please add an emtpy new line before the return?
> + return;
> +
> +cancel_skb:
> + pr_err("Failed to add attributes to binder netlink message\n");
> + genlmsg_cancel(skb, hdr);
> +free_skb:
> + pr_err("Free binder netlink 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,
> @@ -3683,10 +3788,17 @@ 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_netlink_enabled(proc, BINDER_FLAG_SPAM))
> + binder_netlink_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) {
> @@ -3736,8 +3848,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_netlink_enabled(proc, BINDER_FLAG_ASYNC_FROZEN))
> + binder_netlink_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)
> @@ -3799,6 +3918,13 @@ static void binder_transaction(struct binder_proc *proc,
> binder_dec_node_tmpref(target_node);
> }
>
> + if (binder_netlink_enabled(proc, BINDER_FLAG_FAILED))
> + binder_netlink_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, code %u size %lld-%lld line %d\n",
> proc->pid, thread->pid, reply ? "reply" :
> @@ -6334,6 +6460,98 @@ binder_defer_work(struct binder_proc *proc, enum binder_deferred_state defer)
> mutex_unlock(&binder_deferred_lock);
> }
>
> +/**
> + * binder_nl_report_setup_doit() - netlink .doit handler
> + * @skb: the metadata struct passed from netlink driver
> + * @info: the generic netlink struct passed from netlink driver
> + *
> + * Implements the .doit function to process binder netlink commands.
> + */
> +int binder_nl_report_setup_doit(struct sk_buff *skb, struct genl_info *info)
> +{
> + struct binder_context *context = NULL;
> + struct binder_device *device;
> + struct binder_proc *proc;
> + u32 flags, pid;
> + void *hdr;
> + int ret;
> +
> + ret = security_binder_setup_report(current_cred());
> + if (ret < 0) {
> + NL_SET_ERR_MSG(info->extack, "Permission denied");
> + return ret;
> + }
> +
> + hlist_for_each_entry(device, &binder_devices, hlist) {
> + if (!nla_strcmp(info->attrs[BINDER_A_CMD_CONTEXT],
> + device->context.name)) {
> + context = &device->context;
> + break;
> + }
> + }
> +
> + if (!context) {
> + NL_SET_ERR_MSG(info->extack, "Unknown binder context");
> + return -EINVAL;
> + }
> +
> + pid = nla_get_u32(info->attrs[BINDER_A_CMD_PID]);
> + flags = nla_get_u32(info->attrs[BINDER_A_CMD_FLAGS]);
> +
> + 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) {
> + NL_SET_ERR_MSG_FMT(info->extack,
> + "Invalid binder report pid %u",
> + pid);
> + ret = -EINVAL;
> + goto err_exit;
> + }
> +
> + proc->report_flags = flags;
> + }
> +
> + skb = genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_KERNEL);
> + if (!skb) {
> + pr_err("Failed to alloc binder netlink reply message\n");
> + ret = -ENOMEM;
> + goto err_exit;
> + }
> +
> + hdr = genlmsg_iput(skb, info);
> + if (!hdr)
> + goto free_skb;
> +
> + if (nla_put_string(skb, BINDER_A_CMD_CONTEXT, context->name) ||
> + nla_put_u32(skb, BINDER_A_CMD_PID, pid) ||
> + nla_put_u32(skb, BINDER_A_CMD_FLAGS, flags))
> + goto cancel_skb;
> +
> + genlmsg_end(skb, hdr);
> +
> + if (genlmsg_reply(skb, info)) {
> + pr_err("Failed to send binder netlink reply message\n");
> + ret = -EFAULT;
> + goto err_exit;
> + }
> +
> + return 0;
> +
> +cancel_skb:
> + pr_err("Failed to add reply attributes to binder netlink message\n");
> + genlmsg_cancel(skb, hdr);
> +free_skb:
> + pr_err("Free binder netlink reply message on error\n");
> + nlmsg_free(skb);
> + ret = -EMSGSIZE;
> +err_exit:
> + return ret;
> +}
> +
> static void print_binder_transaction_ilocked(struct seq_file *m,
> struct binder_proc *proc,
> const char *prefix,
> @@ -7013,6 +7231,11 @@ static int __init binder_init(void)
> if (ret)
> goto err_init_binder_device_failed;
>
> + ret = genl_register_family(&binder_nl_family);
> + if (ret) {
You don't undo init_binderfs() here? If that seems hard, then you can
move up the genl registration instead, leaving init_binderfs() last.
However, you would need to then undo the genl_register_family() call I
suppose.
> + pr_err("Failed to register binder netlink family\n");
> + goto err_init_binder_device_failed;
> + }
> return ret;
>
> err_init_binder_device_failed:
> diff --git a/drivers/android/binder_internal.h b/drivers/android/binder_internal.h
> index 6a66c9769c6c..8ddd1ddea8e0 100644
> --- a/drivers/android/binder_internal.h
> +++ b/drivers/android/binder_internal.h
> @@ -11,15 +11,28 @@
> #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_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_flags;
> + atomic_t report_seq;
> };
>
> /**
> @@ -413,6 +426,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
> */
> @@ -451,6 +466,7 @@ struct binder_proc {
> spinlock_t outer_lock;
> struct dentry *binderfs_entry;
> bool oneway_spam_detection_enabled;
> + u32 report_flags;
> };
>
> /**
> diff --git a/drivers/android/binder_netlink.c b/drivers/android/binder_netlink.c
> new file mode 100644
> index 000000000000..9dc1820951c3
> --- /dev/null
> +++ b/drivers/android/binder_netlink.c
> @@ -0,0 +1,46 @@
> +// 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.yaml */
> +/* YNL-GEN kernel source */
> +
> +#include <net/netlink.h>
> +#include <net/genetlink.h>
> +
> +#include "binder_netlink.h"
> +
> +#include <uapi/linux/android/binder_netlink.h>
> +#include <binder_internal.h>
> +
> +/* BINDER_CMD_REPORT_SETUP - do */
> +static const struct nla_policy binder_report_setup_nl_policy[BINDER_A_CMD_FLAGS + 1] = {
> + [BINDER_A_CMD_CONTEXT] = { .type = NLA_NUL_STRING, },
> + [BINDER_A_CMD_PID] = { .type = NLA_U32, },
> + [BINDER_A_CMD_FLAGS] = NLA_POLICY_MASK(NLA_U32, 0xf),
> +};
> +
> +/* Ops table for binder */
> +static const struct genl_split_ops binder_nl_ops[] = {
> + {
> + .cmd = BINDER_CMD_REPORT_SETUP,
> + .doit = binder_nl_report_setup_doit,
> + .policy = binder_report_setup_nl_policy,
> + .maxattr = BINDER_A_CMD_FLAGS,
> + .flags = GENL_CMD_CAP_DO,
> + },
> +};
> +
> +static const struct genl_multicast_group binder_nl_mcgrps[] = {
> + [BINDER_NLGRP_REPORT] = { "report", },
> +};
> +
> +struct genl_family binder_nl_family __ro_after_init = {
> + .name = BINDER_FAMILY_NAME,
> + .version = BINDER_FAMILY_VERSION,
> + .netnsok = true,
> + .parallel_ops = true,
> + .module = THIS_MODULE,
> + .split_ops = binder_nl_ops,
> + .n_split_ops = ARRAY_SIZE(binder_nl_ops),
> + .mcgrps = binder_nl_mcgrps,
> + .n_mcgrps = ARRAY_SIZE(binder_nl_mcgrps),
> +};
> diff --git a/drivers/android/binder_netlink.h b/drivers/android/binder_netlink.h
> new file mode 100644
> index 000000000000..0ef8c91ab319
> --- /dev/null
> +++ b/drivers/android/binder_netlink.h
> @@ -0,0 +1,23 @@
> +/* 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.yaml */
> +/* YNL-GEN kernel header */
> +
> +#ifndef _LINUX_BINDER_GEN_H
> +#define _LINUX_BINDER_GEN_H
> +
> +#include <net/netlink.h>
> +#include <net/genetlink.h>
> +
> +#include <uapi/linux/android/binder_netlink.h>
> +#include <binder_internal.h>
> +
> +int binder_nl_report_setup_doit(struct sk_buff *skb, struct genl_info *info);
> +
> +enum {
> + BINDER_NLGRP_REPORT,
> +};
> +
> +extern struct genl_family binder_nl_family;
> +
> +#endif /* _LINUX_BINDER_GEN_H */
> diff --git a/drivers/android/binder_trace.h b/drivers/android/binder_trace.h
> index 16de1b9e72f7..6fb454bcd857 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_netlink_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),
Similarly here I think you could get away with passing 'struct
binder_transaction' instead of all the individual fields.
> + 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/include/uapi/linux/android/binder_netlink.h b/include/uapi/linux/android/binder_netlink.h
> new file mode 100644
> index 000000000000..fa926ed19507
> --- /dev/null
> +++ b/include/uapi/linux/android/binder_netlink.h
> @@ -0,0 +1,57 @@
> +/* 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.yaml */
> +/* YNL-GEN uapi header */
> +
> +#ifndef _UAPI_LINUX_ANDROID_BINDER_NETLINK_H
> +#define _UAPI_LINUX_ANDROID_BINDER_NETLINK_H
> +
> +#define BINDER_FAMILY_NAME "binder"
> +#define BINDER_FAMILY_VERSION 1
> +
> +/*
> + * Define what kind of binder transactions should be reported.
> + */
> +enum binder_flag {
> + BINDER_FLAG_FAILED = 1,
> + BINDER_FLAG_ASYNC_FROZEN = 2,
> + BINDER_FLAG_SPAM = 4,
> + BINDER_FLAG_OVERRIDE = 8,
> +};
> +
> +enum {
> + BINDER_A_CMD_CONTEXT = 1,
> + BINDER_A_CMD_PID,
> + BINDER_A_CMD_FLAGS,
> +
> + __BINDER_A_CMD_MAX,
> + BINDER_A_CMD_MAX = (__BINDER_A_CMD_MAX - 1)
> +};
> +
> +enum {
> + BINDER_A_REPORT_CONTEXT = 1,
> + BINDER_A_REPORT_ERR,
> + BINDER_A_REPORT_FROM_PID,
> + BINDER_A_REPORT_FROM_TID,
> + BINDER_A_REPORT_TO_PID,
> + BINDER_A_REPORT_TO_TID,
> + BINDER_A_REPORT_REPLY,
> + BINDER_A_REPORT_FLAGS,
> + BINDER_A_REPORT_CODE,
> + BINDER_A_REPORT_DATA_SIZE,
> +
> + __BINDER_A_REPORT_MAX,
> + BINDER_A_REPORT_MAX = (__BINDER_A_REPORT_MAX - 1)
> +};
> +
> +enum {
> + BINDER_CMD_REPORT_SETUP = 1,
> + BINDER_CMD_REPORT,
> +
> + __BINDER_CMD_MAX,
> + BINDER_CMD_MAX = (__BINDER_CMD_MAX - 1)
> +};
> +
> +#define BINDER_MCGRP_REPORT "report"
> +
> +#endif /* _UAPI_LINUX_ANDROID_BINDER_NETLINK_H */
> --
> 2.48.1.711.g2feabab25a-goog
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v16 1/3] lsm, selinux: Add setup_report permission to binder
2025-03-07 21:47 ` Paul Moore
@ 2025-03-11 16:37 ` Li Li
0 siblings, 0 replies; 11+ messages in thread
From: Li Li @ 2025-03-11 16:37 UTC (permalink / raw)
To: Paul Moore
Cc: dualli, corbet, davem, edumazet, kuba, pabeni, donald.hunter,
gregkh, arve, tkjos, maco, joel, brauner, cmllamas, surenb,
omosnace, shuah, arnd, masahiroy, bagasdotme, horms, tweek,
linux-kernel, linux-doc, netdev, selinux, hridya, smoreland,
ynaffit, kernel-team
On Fri, Mar 7, 2025 at 1:47 PM Paul Moore <paul@paul-moore.com> wrote:
>
> On Mon, Mar 3, 2025 at 3:02 PM Li Li <dualli@chromium.org> wrote:
> >
> > From: Thiébaud Weksteen <tweek@google.com>
> >
> > Introduce a new permission "setup_report" to the "binder" class.
> > This persmission controls the ability to set up the binder generic
> > netlink driver to report certain binder transactions.
> >
> > Signed-off-by: Thiébaud Weksteen <tweek@google.com>
> > Signed-off-by: Li Li <dualli@google.com>
> > ---
> > include/linux/lsm_hook_defs.h | 1 +
> > include/linux/security.h | 6 ++++++
> > security/security.c | 13 +++++++++++++
> > security/selinux/hooks.c | 7 +++++++
> > security/selinux/include/classmap.h | 3 ++-
> > 5 files changed, 29 insertions(+), 1 deletion(-)
>
> ...
>
> > diff --git a/security/security.c b/security/security.c
> > index 8aa839232c73..382e3bbab215 100644
> > --- a/security/security.c
> > +++ b/security/security.c
> > @@ -1043,6 +1043,19 @@ int security_binder_transfer_file(const struct cred *from,
> > return call_int_hook(binder_transfer_file, from, to, file);
> > }
> >
> > +/**
> > + * security_binder_setup_report() - Check if process allowed to set up binder reports.
>
> Please keep the line length in the LSM and SELinux code to 80
> characters or less.
>
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 0d958f38ff9f..2fafa8feafdf 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -2092,6 +2092,12 @@ static int selinux_binder_transfer_file(const struct cred *from,
> > &ad);
> > }
> >
> > +static int selinux_binder_setup_report(const struct cred *to)
> > +{
> > + return avc_has_perm(current_sid(), cred_sid(to), SECCLASS_BINDER,
> > + BINDER__SETUP_REPORT, NULL);
> > +}
>
> There should also be an associated patch{set} against the
> selinux-testsuite to add tests for the binder/setup_report permission
> introduced here. My apologies if you've already posted one, but I'm
> looking now and I don't see anything either on the lists or on GH.
>
> * https://github.com/SELinuxProject/selinux-testsuite
>
> --
> paul-moore.com
Thank you very much! I'll add such a test, along with other binder
fixes mentioned by Carlos.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Fwd: [PATCH v16 2/3] binder: report txn errors via generic netlink
[not found] ` <CANBPYPhR-C3VTv=ZHc1LJ0c7OG8-K2iGS62vXHmg9gcX0y89Cw@mail.gmail.com>
@ 2025-03-12 18:49 ` Li Li
2025-03-12 20:14 ` Carlos Llamas
0 siblings, 1 reply; 11+ messages in thread
From: Li Li @ 2025-03-12 18:49 UTC (permalink / raw)
To: Carlos Llamas
Cc: Cc:, corbet, davem, edumazet, Jakub Kicinski, pabeni,
donald.hunter, Greg KH, Arve Hjønnevåg, tkjos, maco,
Joel Fernandes (Google), brauner, Suren Baghdasaryan, omosnace,
shuah, arnd, masahiroy, Bagas Sanjaya, Simon Horman, tweek, LKML,
linux-doc, netdev, selinux, Hridya Valsaraju, smoreland, ynaffit,
Android Kernel Team
Sorry for resending this email. My email client was wrongly set to HTML mode.
On Mon, Mar 10, 2025 at 9:13 PM Carlos Llamas <cmllamas@google.com> wrote:
>
> On Mon, Mar 03, 2025 at 12:02:11PM -0800, Li Li wrote:
> > From: Li Li <dualli@google.com>
> >
> > +/**
> > + * binder_find_proc() - find the binder_proc by pid
> > + * @pid: the target process
> > + *
> > + * Returns the struct binder_proc if the pid is found, or NULL otherwise.
> > + */
> > +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)
> > + break;
>
> Wait... can't there be multiple binder_proc instances matching the same
> pid? I know that binder_proc is a bit of a misnomer but what should you
> do in such case? Shouldn't you set the flags in _all_ matching pids?
>
> Furthermore, there could be a single task talking on multiple contexts,
> so you could be returning the 'proc' that doesn't match the context that
> you are looking for right?
>
You're right. I should update this logic to search the process within a
certain binder_context only.
> > + }
> > + mutex_unlock(&binder_procs_lock);
> > +
> > + return proc;
> > +}
> > +
> > +/**
> > + * binder_netlink_enabled() - check if binder netlink reports are enabled
> > + * @proc: the binder_proc to check
> > + * @mask: the categories of binder netlink reports
> > + *
> > + * Returns true if certain binder netlink reports are enabled for this binder
> > + * proc (when per-process overriding takes effect) or context.
> > + */
> > +static bool binder_netlink_enabled(struct binder_proc *proc, u32 mask)
> > +{
> > + struct binder_context *context = proc->context;
> > +
> > + if (!genl_has_listeners(&binder_nl_family, &init_net, BINDER_NLGRP_REPORT))
> > + return false;
> > +
> > + if (proc->report_flags & BINDER_FLAG_OVERRIDE)
> > + return (proc->report_flags & mask) != 0;
> > + else
> > + return (context->report_flags & mask) != 0;
> > +}
> > +
> > +/**
> > + * binder_netlink_report() - report one binder netlink event
> > + * @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 binder netlink message and send it.
> > + */
> > +static void binder_netlink_report(struct binder_context *context, u32 err,
> > + u32 pid, u32 tid, u32 to_pid, u32 to_tid,
>
> Instead of all these parameters, is there a way to pass the transaction
> itself? Isn't this info already populated there? I think it even holds
> the info you are looking for from the 'binder_transaction_data' below.
>
The binder_transaction_data doesn't include all of pid, tid, to_pid and to_tid.
> > + u32 reply,
> > + struct binder_transaction_data *tr)
> > +{
> > + struct sk_buff *skb;
> > + void *hdr;
> > + int ret;
> > +
> > + trace_binder_netlink_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 netlink message\n");
> > + return;
> > + }
> > +
> > + hdr = genlmsg_put(skb, 0, atomic_inc_return(&context->report_seq),
> > + &binder_nl_family, 0, BINDER_CMD_REPORT);
> > + if (!hdr)
> > + goto free_skb;
> > +
> > + if (nla_put_string(skb, BINDER_A_REPORT_CONTEXT, context->name) ||
> > + nla_put_u32(skb, BINDER_A_REPORT_ERR, err) ||
> > + nla_put_u32(skb, BINDER_A_REPORT_FROM_PID, pid) ||
> > + nla_put_u32(skb, BINDER_A_REPORT_FROM_TID, tid) ||
> > + nla_put_u32(skb, BINDER_A_REPORT_TO_PID, to_pid) ||
> > + nla_put_u32(skb, BINDER_A_REPORT_TO_TID, to_tid) ||
> > + nla_put_u32(skb, BINDER_A_REPORT_REPLY, reply) ||
> > + nla_put_u32(skb, BINDER_A_REPORT_FLAGS, tr->flags) ||
> > + nla_put_u32(skb, BINDER_A_REPORT_CODE, tr->code) ||
> > + nla_put_u32(skb, BINDER_A_REPORT_DATA_SIZE, tr->data_size))
> > + goto cancel_skb;
> > +
> > + genlmsg_end(skb, hdr);
> > +
> > + ret = genlmsg_multicast(&binder_nl_family, skb, 0, BINDER_NLGRP_REPORT, GFP_KERNEL);
>
> Thanks for switching to multicast. On this topic, we can only have a
> single global configuration at a time correct? e.g. context vs per-proc.
> So all listeners would ahve to work with the same setup?
>
We only have a single global configuration, which can include both
context and proc setup.
Yes, all listeners work with the same setup as we have only one
multicast group defined.
The user space code can demux it by checking the context field of the
netlink messages.
> > + if (ret < 0)
> > + pr_err("Failed to send binder netlink message: %d\n", ret);
>
> nit: can you please add an emtpy new line before the return?
>
Sure.
> > @@ -7013,6 +7231,11 @@ static int __init binder_init(void)
> > if (ret)
> > goto err_init_binder_device_failed;
> >
> > + ret = genl_register_family(&binder_nl_family);
> > + if (ret) {
>
> You don't undo init_binderfs() here? If that seems hard, then you can
> move up the genl registration instead, leaving init_binderfs() last.
> However, you would need to then undo the genl_register_family() call I
> suppose.
>
The current logic allows binder driver to continue working even if
genl_register_family
fails. But your suggestion makes sense. I'll move it up and undo it if
anything fails.
> > +TRACE_EVENT(binder_netlink_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),
>
> Similarly here I think you could get away with passing 'struct
> binder_transaction' instead of all the individual fields.
>
Same as above, the pid/tid fields are not in the struct
binder_transaction (or redacted for oneway txns).
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Fwd: [PATCH v16 2/3] binder: report txn errors via generic netlink
2025-03-12 18:49 ` Fwd: " Li Li
@ 2025-03-12 20:14 ` Carlos Llamas
2025-03-12 20:56 ` Li Li
0 siblings, 1 reply; 11+ messages in thread
From: Carlos Llamas @ 2025-03-12 20:14 UTC (permalink / raw)
To: Li Li
Cc: Cc:, corbet, davem, edumazet, Jakub Kicinski, pabeni,
donald.hunter, Greg KH, Arve Hjønnevåg, tkjos, maco,
Joel Fernandes (Google), brauner, Suren Baghdasaryan, omosnace,
shuah, arnd, masahiroy, Bagas Sanjaya, Simon Horman, tweek, LKML,
linux-doc, netdev, selinux, Hridya Valsaraju, smoreland, ynaffit,
Android Kernel Team
On Wed, Mar 12, 2025 at 11:49:02AM -0700, Li Li wrote:
> > > + mutex_lock(&binder_procs_lock);
> > > + hlist_for_each_entry(proc, &binder_procs, proc_node) {
> > > + if (proc->pid == pid)
> > > + break;
> >
> > Wait... can't there be multiple binder_proc instances matching the same
> > pid? I know that binder_proc is a bit of a misnomer but what should you
> > do in such case? Shouldn't you set the flags in _all_ matching pids?
> >
> > Furthermore, there could be a single task talking on multiple contexts,
> > so you could be returning the 'proc' that doesn't match the context that
> > you are looking for right?
> >
>
> You're right. I should update this logic to search the process within a
> certain binder_context only.
Also, note the comment about multiple 'struct binder_proc' matching the
same desired pid.
> > > +static void binder_netlink_report(struct binder_context *context, u32 err,
> > > + u32 pid, u32 tid, u32 to_pid, u32 to_tid,
> >
> > Instead of all these parameters, is there a way to pass the transaction
> > itself? Isn't this info already populated there? I think it even holds
> > the info you are looking for from the 'binder_transaction_data' below.
> >
>
> The binder_transaction_data doesn't include all of pid, tid, to_pid and to_tid.
I'm not referring to binder_transaction_data, I mean 'struct
binder_transaction'. I _think_ this should have all you need?
> > > + ret = genlmsg_multicast(&binder_nl_family, skb, 0, BINDER_NLGRP_REPORT, GFP_KERNEL);
> >
> > Thanks for switching to multicast. On this topic, we can only have a
> > single global configuration at a time correct? e.g. context vs per-proc.
> > So all listeners would ahve to work with the same setup?
> >
>
> We only have a single global configuration, which can include both
> context and proc setup.
> Yes, all listeners work with the same setup as we have only one
> multicast group defined.
> The user space code can demux it by checking the context field of the
> netlink messages.
Ack. I understand the demux solution. I was wondering if we'll need to
OR the different configurations (per-proc and flags) from each listener
in that case.
> > > +TRACE_EVENT(binder_netlink_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),
> >
> > Similarly here I think you could get away with passing 'struct
> > binder_transaction' instead of all the individual fields.
> >
>
> Same as above, the pid/tid fields are not in the struct
> binder_transaction (or redacted for oneway txns).
There is something off here. You have t->from_{pid|tid} and also
t->to_{proc|thead} that you can use. Isn't this what you are looking
for?
--
Carlos Llamas
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Fwd: [PATCH v16 2/3] binder: report txn errors via generic netlink
2025-03-12 20:14 ` Carlos Llamas
@ 2025-03-12 20:56 ` Li Li
0 siblings, 0 replies; 11+ messages in thread
From: Li Li @ 2025-03-12 20:56 UTC (permalink / raw)
To: Carlos Llamas
Cc: Cc:, corbet, davem, edumazet, Jakub Kicinski, pabeni,
donald.hunter, Greg KH, Arve Hjønnevåg, tkjos, maco,
Joel Fernandes (Google), brauner, Suren Baghdasaryan, omosnace,
shuah, arnd, masahiroy, Bagas Sanjaya, Simon Horman, tweek, LKML,
linux-doc, netdev, selinux, Hridya Valsaraju, smoreland, ynaffit,
Android Kernel Team
On Wed, Mar 12, 2025 at 1:14 PM Carlos Llamas <cmllamas@google.com> wrote:
>
> On Wed, Mar 12, 2025 at 11:49:02AM -0700, Li Li wrote:
> > > > + mutex_lock(&binder_procs_lock);
> > > > + hlist_for_each_entry(proc, &binder_procs, proc_node) {
> > > > + if (proc->pid == pid)
> > > > + break;
> > >
> > > Wait... can't there be multiple binder_proc instances matching the same
> > > pid? I know that binder_proc is a bit of a misnomer but what should you
> > > do in such case? Shouldn't you set the flags in _all_ matching pids?
> > >
> > > Furthermore, there could be a single task talking on multiple contexts,
> > > so you could be returning the 'proc' that doesn't match the context that
> > > you are looking for right?
> > >
> >
> > You're right. I should update this logic to search the process within a
> > certain binder_context only.
>
> Also, note the comment about multiple 'struct binder_proc' matching the
> same desired pid.
>
Yes, multiple matching can be found when the context is not specified.
I'll take care of that as well.
> > > > +static void binder_netlink_report(struct binder_context *context, u32 err,
> > > > + u32 pid, u32 tid, u32 to_pid, u32 to_tid,
> > >
> > > Instead of all these parameters, is there a way to pass the transaction
> > > itself? Isn't this info already populated there? I think it even holds
> > > the info you are looking for from the 'binder_transaction_data' below.
> > >
> >
> > The binder_transaction_data doesn't include all of pid, tid, to_pid and to_tid.
>
> I'm not referring to binder_transaction_data, I mean 'struct
> binder_transaction'. I _think_ this should have all you need?
>
Ah, yes, let me take a closer look and optimize this. Thanks!
> > > > + ret = genlmsg_multicast(&binder_nl_family, skb, 0, BINDER_NLGRP_REPORT, GFP_KERNEL);
> > >
> > > Thanks for switching to multicast. On this topic, we can only have a
> > > single global configuration at a time correct? e.g. context vs per-proc.
> > > So all listeners would ahve to work with the same setup?
> > >
> >
> > We only have a single global configuration, which can include both
> > context and proc setup.
> > Yes, all listeners work with the same setup as we have only one
> > multicast group defined.
> > The user space code can demux it by checking the context field of the
> > netlink messages.
>
> Ack. I understand the demux solution. I was wondering if we'll need to
> OR the different configurations (per-proc and flags) from each listener
> in that case.
>
They are already OR'ed from all listeners.
> > > > +TRACE_EVENT(binder_netlink_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),
> > >
> > > Similarly here I think you could get away with passing 'struct
> > > binder_transaction' instead of all the individual fields.
> > >
> >
> > Same as above, the pid/tid fields are not in the struct
> > binder_transaction (or redacted for oneway txns).
>
> There is something off here. You have t->from_{pid|tid} and also
> t->to_{proc|thead} that you can use. Isn't this what you are looking
> for?
>
> --
> Carlos Llamas
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2025-03-12 20:56 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-03-03 20:02 [PATCH v16 0/3] binder: report txn errors via generic netlink Li Li
2025-03-03 20:02 ` [PATCH v16 1/3] lsm, selinux: Add setup_report permission to binder Li Li
2025-03-07 21:47 ` Paul Moore
2025-03-11 16:37 ` Li Li
2025-03-03 20:02 ` [PATCH v16 2/3] binder: report txn errors via generic netlink Li Li
2025-03-11 4:12 ` Carlos Llamas
[not found] ` <CANBPYPhR-C3VTv=ZHc1LJ0c7OG8-K2iGS62vXHmg9gcX0y89Cw@mail.gmail.com>
2025-03-12 18:49 ` Fwd: " Li Li
2025-03-12 20:14 ` Carlos Llamas
2025-03-12 20:56 ` Li Li
2025-03-03 20:02 ` [PATCH v16 3/3] binder: generic netlink binder_features flag Li Li
2025-03-11 3:37 ` Carlos Llamas
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).