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@xxxxxxxxxx> wrote:
>
> On Mon, Mar 03, 2025 at 12:02:11PM -0800, Li Li wrote:
> > From: Li Li <dualli@xxxxxxxxxx>
> >
> > +/**
> > + * 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).
