linux-kernel - Re: [PATCH v23 10/13] x86, vdso: Add 32 bit VDSO time support for 64 bit kernel

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: <CALCETrU33Yi85yNxx=JqE_X13xOsh__8SOnGdJyDBG1KKnXAhA@mail.gmail.com>
Date:	Thu, 27 Mar 2014 14:12:55 -0700
From:	Andy Lutomirski <luto@...capital.net>
To:	John Stultz <john.stultz@...aro.org>
Cc:	Stefani Seibold <stefani@...bold.net>,
	Greg KH <gregkh@...uxfoundation.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	X86 ML <x86@...nel.org>, Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>, Andi Kleen <ak@...ux.intel.com>,
	Andrea Arcangeli <aarcange@...hat.com>,
	Pavel Emelyanov <xemul@...allels.com>,
	Cyrill Gorcunov <gorcunov@...nvz.org>,
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
	Martin Runge <Martin.Runge@...de-schwarz.com>,
	Andreas Brief <Andreas.Brief@...de-schwarz.com>
Subject: Re: [PATCH v23 10/13] x86, vdso: Add 32 bit VDSO time support for 64
 bit kernel

On Thu, Mar 27, 2014 at 1:44 PM, John Stultz <john.stultz@...aro.org> wrote:
> On 03/17/2014 03:22 PM, Stefani Seibold wrote:
>> This patch add the VDSO time support for the IA32 Emulation Layer.
>>
>> Due the nature of the kernel headers and the LP64 compiler where the
>> size of a long and a pointer differs against a 32 bit compiler, there
>> is some type hacking necessary for optimal performance.
>>
>> The vsyscall_gtod_data struture must be a rearranged to serve 32- and
>> 64-bit code access at the same time:
>>
>> - The seqcount_t was replaced by an unsigned, this makes the
>>   vsyscall_gtod_data intedepend of kernel configuration and internal functions.
>> - All kernel internal structures are replaced by fix size elements
>>   which works for 32- and 64-bit access
>> - The inner struct clock was removed to pack the whole struct.
>>
>> The "unsigned seq" would be handled by functions derivated from seqcount_t.
>
> Sorry for being a bit late on the review here. I've been focused on some
> other things and figured I'd leave things to the x86 maintainers, since
> it didn't touch any core timekeeping code. But Andy pinged me after
> LSF-MM and so I'm trying to take a closer look.
>
> I know this has gone through tons of iterations, and I really am glad
> for Stefani's persistence. Also I do realize its already in the tip
> tree, so my thoughts here are probably too late to be worth much.
>
>
>> Signed-off-by: Stefani Seibold <stefani@...bold.net>
>> ---
>>  arch/x86/include/asm/vgtod.h          | 71 +++++++++++++++++++++------
>>  arch/x86/include/asm/vvar.h           |  5 ++
>>  arch/x86/kernel/vsyscall_gtod.c       | 34 ++++++++-----
>>  arch/x86/vdso/vclock_gettime.c        | 91 +++++++++++++++++++----------------
>>  arch/x86/vdso/vdso32/vclock_gettime.c | 21 ++++++++
>>  5 files changed, 155 insertions(+), 67 deletions(-)
>
> My initial issue with this patch is it is doing quite a few things that
> could have been split up a bit. There's a number of separate cleanups:
> seqlock change, the read_hpet_counter() cleanup, the vsyscall_gtod_data
> structure rework, and finally the gtod_long_t and  VDSO32_64 changes.
>
> Its probably too late now, but in the future it might be good to try to
> split these sorts of things up in smaller chunks.
>
> Overall, I don't see any obvious correctness issues with the code, so it
> looks ok to me, but I really worry the amount of open-coded logic and
> the quirkiness of trying to make the vsyscall_gtod_data structure usable
> for both architectures at the same time really makes this code quite
> fragile. Looking over this there were lots of "oh, that's broken..
> wait.. no.. actually that will work" moments. So I'm worried about the
> maintenance burden in the future from it.
>
> Having separate structures that are filled by update_vsyscall might be
> nice to avoid needing the open-coded bits, but that would require
> basically making a copy of all the data in the structure each update, so
> that might not be worth the cost.

This may actually enable a future cleanup: currently, the x32 vdso is
built with gcc in 64-bit mode, and objcopy turns the result into a
32-bit file.  This structure munging, along with a cleanup I'm working
on, will eliminate the need to do that.


>
> So as long as the x86 folks are ok with being on the hook for it, I'll
> not get in the way (most other arches have quirky/ugly implementations
> for their vdso/vsyscall/fsyscall/etc, so why not join the club :).
>
> I did run this through my timekeeping test suite and on 64bit nothing
> broke w/ either 32 or 64bit binaries, but I also don't have the glibc
> changes to enable the 32bit vdso, so all that says is its not showing
> any regressions for my existing setup.

Thanks for looking!

--Andy
--
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/