linux-kernel - 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z9HrEdbI5JYu0pwS@google.com>
Date: Wed, 12 Mar 2025 20:14:09 +0000
From: Carlos Llamas <cmllamas@...gle.com>
To: Li Li <dualli@...omium.org>
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 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