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

[Li Li <dualli@chromium.org>]

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