linux-kernel - Re: [PATCH] Binder: Add timestamp and async from pid/tid to transaction record

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>] [day] [month] [year] [list]

Message-ID: <2023041359-marine-uninsured-bae9@gregkh>
Date:   Thu, 13 Apr 2023 14:17:43 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Chuang Zhang <zhangchuang3@...omi.corp-partner.google.com>
Cc:     arve@...roid.com, tkjos@...roid.com, maco@...roid.com,
        joel@...lfernandes.org, brauner@...nel.org, cmllamas@...gle.com,
        surenb@...gle.com, linux-kernel@...r.kernel.org,
        Chuang Zhang <zhangchuang3@...omi.com>
Subject: Re: [PATCH] Binder: Add timestamp and async from pid/tid to
 transaction record

On Thu, Apr 13, 2023 at 07:33:26PM +0800, Chuang Zhang wrote:
> On Thu, Apr 13, 2023 at 6:51 PM Greg KH <gregkh@...uxfoundation.org> wrote:
> > >       spin_lock(&t->lock);
> > >       to_proc = t->to_proc;
> > >       seq_printf(m,
> > > -                "%s %d: %pK from %d:%d to %d:%d code %x flags %x pri
> > %ld r%d",
> > > +                "%s %d: %pK from %d:%d to %d:%d code %x elapsed %lldms
> > flags %x pri %ld r%d",
> >
> > Why not add this at the end of the line instead of in the middle?  That
> > way any existing code that looks at this line isn't as broken as it
> > might be with your change :)
> >
> >> [chuang] Ok, so move it after t->need_reply or buffer->user_data?

After need_reply?

> > > --- a/drivers/android/binder_internal.h
> > > +++ b/drivers/android/binder_internal.h
> > > @@ -528,6 +528,9 @@ struct binder_transaction {
> > >       long    priority;
> > >       long    saved_priority;
> > >       kuid_t  sender_euid;
> > > +     int async_from_pid;
> > > +     int async_from_tid;
> >
> > As you are just using 0/1 for these variables, why not add them to the
> > bitfield location in this structure instead of wasting a whole int for
> > both of them?
> >
> >> [chuang] They are used to store the caller's process ID and thread ID,
> not just 1 or 0, so I understand that integers are still needed to save
> them. thanks!

Ah, I missed that these really were the pid itself.  Then an int will
not work at all, it has to be pid_t, right?

And how are pid namespaces being handled here?  I know it's just
debugging data but showing a pid from the wrong namespace probably would
not be a good idea as it might get confusing quickly.

thanks,

greg k-h