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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CANBPYPi8sT0m4bj9JhVn38rrRBg7nV9kduc34QC_edwHS-wNZg@mail.gmail.com>
Date: Wed, 12 Mar 2025 13:56:32 -0700
From: Li Li <dualli@...omium.org>
To: Carlos Llamas <cmllamas@...gle.com>
Cc: "Cc:" <dualli@...gle.com>, corbet@....net, davem@...emloft.net, 
	edumazet@...gle.com, Jakub Kicinski <kuba@...nel.org>, pabeni@...hat.com, 
	donald.hunter@...il.com, Greg KH <gregkh@...uxfoundation.org>, 
	Arve Hjønnevåg <arve@...roid.com>, tkjos@...roid.com, 
	maco@...roid.com, "Joel Fernandes (Google)" <joel@...lfernandes.org>, brauner@...nel.org, 
	Suren Baghdasaryan <surenb@...gle.com>, omosnace@...hat.com, shuah@...nel.org, arnd@...db.de, 
	masahiroy@...nel.org, Bagas Sanjaya <bagasdotme@...il.com>, 
	Simon Horman <horms@...nel.org>, tweek@...gle.com, LKML <linux-kernel@...r.kernel.org>, 
	linux-doc@...r.kernel.org, netdev@...r.kernel.org, selinux@...r.kernel.org, 
	Hridya Valsaraju <hridya@...gle.com>, smoreland@...gle.com, ynaffit@...gle.com, 
	Android Kernel Team <kernel-team@...roid.com>
Subject: Re: Fwd: [PATCH v16 2/3] binder: report txn errors via generic netlink

On Wed, Mar 12, 2025 at 1:14 PM Carlos Llamas <cmllamas@...gle.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