linux-kernel - Re: [PATCH 18/41] regset: new method and helpers for it

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20200629203028.GB2786714@ZenIV.linux.org.uk>
Date:   Mon, 29 Jun 2020 21:30:28 +0100
From:   Al Viro <viro@...iv.linux.org.uk>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     linux-arch <linux-arch@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        David Miller <davem@...emloft.net>,
        Tony Luck <tony.luck@...el.com>, Will Deacon <will@...nel.org>
Subject: Re: [PATCH 18/41] regset: new method and helpers for it

On Mon, Jun 29, 2020 at 12:23:34PM -0700, Linus Torvalds wrote:
> On Mon, Jun 29, 2020 at 11:28 AM Al Viro <viro@...iv.linux.org.uk> wrote:
> >
> > ->get2() takes task+regset+buffer, returns the amount of free space
> > left in the buffer on success and -E... on error.
> 
> Can we please give it a better name than "get2"?
> 
> That's not a great name to begin with, and it's a completely
> nonsensical name by the end of this series when you've removed the
> original "get" function.
> 
> So either:
> 
>  (a) add one final patch to rename "get2" all back to "get" after you
> got rid of the old "get"

Bad idea, IMO...

>  (b) or just call it something better to begin with. Maybe just
> "get_regset" instead?

<nod>

Frankly, other field names there are also nasty - 'set' is not
fun to grep for, but there are 'n' and 'size' as well.  There's
also 'bias'  and 'align' (both completely unused)...

> I'd prefer (b) just because I think it will be a lot clearer if we
> ever end up having old patches forward-ported (or, more likely,
> newpatches back-ported), so having a "get" function that changed
> semantics but got back the old name sounds bad to me.
> 
> Other than that, I can't really argue with the numbers:
> 
>  41 files changed, 1368 insertions(+), 2347 deletions(-)
> 
> looks good to me, and the code seems more understandable.

FWIW, there's also ->set() side of that thing.  Tons of boilerplate
in those; I hadn't seriously looked into that part, but I suspect
that "do copyin in the caller, pass the kernel buffer to the
method, always start at offset 0" would also trim a lot of crap.
I'd rather leave that one for later - this series had been painful
enough.

Other things in the same area: conversion of fdpic coredumps to
regset (fairly easy, I've a patch series doing that in the local
tree), conversion of the few remaining non-regset architectures
to regset-based coredump (kill a large chunk of rotting code in
fs/binfmt_elf.c, along with quite a few pieces of old per-arch
coredump helpers), unification of compat ELF (done locally,
mipsn32/mipso32 gone, all compat done via compat_binfmt_elf.c,
x32 horrors sanitized - IMO
#define PRSTATUS_SIZE \
       (test_thread_flag(TIF_X32) \
               ? sizeof(struct compat_elf_prstatus) \
               : sizeof(struct i386_elf_prstatus))
#define SET_PR_FPVALID(S) \
       (*(test_thread_flag(TIF_X32) \
               ? &(S)->pr_fpvalid      \
               : &((struct i386_elf_prstatus *)(S))->pr_fpvalid) = 1)
is much better than
#define PRSTATUS_SIZE(S, R) (R != sizeof(S.pr_reg) ? 144 : 296)
#define SET_PR_FPVALID(S, V, R) \
  do { *(int *) (((void *) &((S)->pr_reg)) + R) = (V); } \
  while (0)
) and a bunch of assorted cleanups that got trimmed from the
regset series...

I'll probably post fdpic and compat series tonight and get
back to uaccess and VFS stuff.