Message-ID: <20200221033016.GV23230@ZenIV.linux.org.uk>
Date:   Fri, 21 Feb 2020 03:30:16 +0000
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>,
        Arnd Bergmann <arnd@...db.de>
Subject: Re: [RFC] regset ->get() API

On Thu, Feb 20, 2020 at 03:31:56PM -0800, Linus Torvalds wrote:
> On Thu, Feb 20, 2020 at 3:29 PM Al Viro <viro@...iv.linux.org.uk> wrote:
> >
> > We do know that caller does not want more than the value it has passed in
> > 'size' argument, though.
> 
> Ok, fair enough. That's probably a good way to handle the "allocate in
> the caller".
> 
> So then I have no issues with that approach.

Turns out that "nobody uses those for userland destinations after that" is not
quite right - we have this:
int copy_fpstate_to_sigframe(void __user *buf, void __user *buf_fx, int size)
{
        struct task_struct *tsk = current;
        int ia32_fxstate = (buf != buf_fx);
        int ret;

        ia32_fxstate &= (IS_ENABLED(CONFIG_X86_32) ||
                         IS_ENABLED(CONFIG_IA32_EMULATION));

        if (!access_ok(buf, size))
                return -EACCES;

        if (!static_cpu_has(X86_FEATURE_FPU))
                return fpregs_soft_get(current, NULL, 0,
                        sizeof(struct user_i387_ia32_struct), NULL,
                        (struct _fpstate_32 __user *) buf) ? -1 : 1;

... with fpregs_soft_get() behing the shared helper that does, in turn,
call user_regset_copyout().  OTOH, _that_ sure as hell is "fill local
variable, then copy_to_user()" case.

Sigh...  Wish we had a quick way to do something along the lines of
"find all callchains leading to <function> that would not come via
-><method>" - doing that manually stank to high heaven ;-/  And in
cases like that nothing along the lines of "simulate a build" is
practical - call chains are all over arch/*, and config-sensitive
as well (32bit vs. 64bit is only beginning of that fun).  Thankfully,
none of those involved token-pasting...

Anyway, one observation that came out of that is that we might
be better off with signature change done first; less boilerplate
that way, contrary to what I expected.

Alternatively, we could introduce a new method, with one-by-one
conversion to it.  Hmm...
	int (*get2)(struct task_struct *target,
		    const struct user_regset *regset,
		    struct membuf to);
returning -E... on error and amount left unfilled on success, perhaps?
That seems to generate decent code and is pretty easy on the instances,
especially if membuf_write() et.al. are made to return to->left...
Things like

static int evr_get(struct task_struct *target, const struct user_regset *regset,
                   struct membuf to)
{
        flush_spe_to_thread(target);

        membuf_write(&to, &target->thread.evr, sizeof(target->thread.evr));

        BUILD_BUG_ON(offsetof(struct thread_struct, acc) + sizeof(u64) !=
                     offsetof(struct thread_struct, spefscr));

        return membuf_write(&to, &target->thread.acc, sizeof(u64));
}

in place of current

static int evr_get(struct task_struct *target, const struct user_regset *regset,
                   unsigned int pos, unsigned int count,
                   void *kbuf, void __user *ubuf)
{
        int ret;

        flush_spe_to_thread(target);

        ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
                                  &target->thread.evr,
                                  0, sizeof(target->thread.evr));

        BUILD_BUG_ON(offsetof(struct thread_struct, acc) + sizeof(u64) !=
                     offsetof(struct thread_struct, spefscr));

        if (!ret)
                ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
                                          &target->thread.acc,
                                          sizeof(target->thread.evr), -1);

        return ret;
}

and

static int vr_get(struct task_struct *target, const struct user_regset *regset,
                  struct membuf to)
{
	union {
		elf_vrreg_t reg;
		u32 word;
	} vrsave;

        flush_altivec_to_thread(target);

        BUILD_BUG_ON(offsetof(struct thread_vr_state, vscr) !=
                     offsetof(struct thread_vr_state, vr[32]));

        membuf_write(&to, &target->thread.vr_state, 33 * sizeof(vector128));
	/*
	 * Copy out only the low-order word of vrsave.
	 */
	memset(&vrsave, 0, sizeof(vrsave));
	vrsave.word = target->thread.vrsave;

	return membuf_write(&to, &vrsave, sizeof(vrsave);
}

instead of

static int vr_get(struct task_struct *target, const struct user_regset *regset,
                  unsigned int pos, unsigned int count,
                  void *kbuf, void __user *ubuf)
{
        int ret;

        flush_altivec_to_thread(target);

        BUILD_BUG_ON(offsetof(struct thread_vr_state, vscr) !=
                     offsetof(struct thread_vr_state, vr[32]));

        ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
                                  &target->thread.vr_state, 0,
                                  33 * sizeof(vector128));
        if (!ret) {
                /*
                 * Copy out only the low-order word of vrsave.
                 */
                int start, end;
                union {
                        elf_vrreg_t reg;
                        u32 word;
                } vrsave;
                memset(&vrsave, 0, sizeof(vrsave));

                vrsave.word = target->thread.vrsave;

                start = 33 * sizeof(vector128);
                end = start + sizeof(vrsave);
                ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf, &vrsave,
                                          start, end);
        }

        return ret;
}

Powered by blists - more mailing lists