Re: Fwd: [PATCH v16 2/3] binder: report txn errors via generic netlink

[Carlos Llamas <cmllamas@google.com>]

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