linux-kernel - Re: [patch v3 1/2] x86, ptrace: regset extensions to support xstate

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: <20100212034520.0FABFC821@magilla.sf.frob.com>
Date:	Thu, 11 Feb 2010 19:45:20 -0800 (PST)
From:	Roland McGrath <roland@...hat.com>
To:	Suresh Siddha <suresh.b.siddha@...el.com>
Cc:	Oleg Nesterov <oleg@...hat.com>, "H. Peter Anvin" <hpa@...or.com>,
	Ingo Molnar <mingo@...e.hu>,
	Thomas Gleixner <tglx@...utronix.de>,
	LKML <linux-kernel@...r.kernel.org>, hjl.tools@...il.com,
	peter.lachner@...el.com
Subject: Re: [patch v3 1/2] x86, ptrace: regset extensions to support xstate

> +	/*
> +	 * First copy the fxsave bytes 0..463.
> +	 */
> +	ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
> +				  &target->thread.xstate->xsave, 0,
> +				  offsetof(struct user_xstateregs,
> +					   i387.xstate_fx_sw));
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Copy the 48bytes defined by software.
> +	 */
> +	ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
> +				  xstate_fx_sw_bytes,
> +				  offsetof(struct user_xstateregs,
> +					   i387.xstate_fx_sw),
> +				  offsetof(struct user_xstateregs,
> +					   xsave_hdr));
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Copy the rest of xstate memory layout.
> +	 */
> +	ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
> +				  &target->thread.xstate->xsave.xsave_hdr,
> +				  offsetof(struct user_xstateregs,
> +					   xsave_hdr), -1);
> +	return ret;

I don't know why this didn't occur to me before.  Is there a reason not to
just copy xstate_fx_sw_bytes into the thread.xstate buffer?  You could just
do it right here, or in the places that really touch thread.xstate contents.

Then the trivial single user_regset_copyout() call would be all you need
here, and that seems simpler.  IMHO it's enough simpler that even if you
pay the 6-words memcpy every time here, it's worth it.

If you don't want to do that, that's fine too.  
The code looks correct as it is.

> --- tip.orig/arch/x86/include/asm/user.h
> +++ tip/arch/x86/include/asm/user.h

I don't have any special opinions about the placement, naming, etc. of
these asm/user.h bits (just that there be some symbolic form of the ABI
somewhere).  But perhaps folks want to think about it a bit more before we
bake it into the source API.

These definitions duplicate the asm/processor.h ones used inside the kernel.
IMHO these should be consolidated so asm/processor.h refers to the public types.

I'm not sure whether 'struct user_*' names make most sense for these or not.
They are processor-defined layouts.

> +struct user_ymmh_regs {
> +	/* 16 * 16 bytes for each YMMH-reg */
> +	__u32 ymmh_space[64];
> +};

This is the high half of %ymmN registers, where %xmmN is the low half.
Right?  Though of course documented in the chip manuals, it is rather
nonobvious that %ymmN are split this way in the storage, so I think it
merits a comment here saying so explicitly.

On these cosmetic issues I leave it up to you and the x86 maintainers.
I don't have strong opinions.  But I wanted to air what had occurred to me.

Modulo any cosmetic changes you want to make, this patch has my ACK
contingent on Oleg's ACK.

Thanks,
Roland
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/