linux-kernel - Re: [PATCH] use a stable clock reference in vdso vgetns

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: <20101004161555.GG1230@mothafucka.localdomain>
Date:	Mon, 4 Oct 2010 13:15:56 -0300
From:	Glauber Costa <glommer@...hat.com>
To:	john stultz <johnstul@...ibm.com>
Cc:	linux-kernel@...r.kernel.org, avi@...hat.com,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...e.hu>, "H. Peter Anvin" <hpa@...or.com>,
	Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH] use a stable clock reference in vdso vgetns

On Fri, Oct 01, 2010 at 10:49:53AM -0700, john stultz wrote:
> On Fri, Oct 1, 2010 at 6:09 AM, Glauber Costa <glommer@...hat.com> wrote:
> > When using vdso services for clock_gettime, we test for the ability
> > of a fine-grained measurement through the existance of a vread() function.
> >
> > However, from the time we test it, to the time we use it, vread() reference
> > may not be valid anymore. It happens, for example, when we change the current
> > clocksource from one that provides vread (say tsc) to one that lacks it
> > (say acpi_pm), in the middle of clock_gettime routine.
> >
> > seqlock does not really protect us, since readers here won't stop the writers
> > to change references. The proposed solution is to grab a copy of the clock
> > structure early, and use it as a stable reference onwards.
> 
> Ah. Good find! The fix looks reasonable to me. However, its likely the
> similar code in arch/x86/kernel/vsyscall_64.c will need a similar fix.
> 
> Awhile back there was some motivation to merge the two vdso/vsyscall
> implementations to avoid the duplication, but my memory is failing on
> why that didn't happen. I feel like it had to do with complication
> with the way the two implementations are mapped out to userland. Even
> so, it seems a shared forced inline method would resolve the issue, so
> maybe it just fell off the todo list?
Ok, I just stated that vsyscall is safe because of update_vsyscall. This is
just not the case. Update vsyscall will update the timer structures but leave
us with the problem that it will do it regardless of any readers. What makes
vsyscall "safe", is this:

                vread = __vsyscall_gtod_data.clock.vread; 
                ^
                |--- saves reference to vread                    uses --.
                if (unlikely(!__vsyscall_gtod_data.sysctl_enabled || !vread)) {
                        gettimeofday(tv,NULL);
                        return;
                }

                now = vread(); <=== uses again

This was, actually, my first idea on how to solve the problem for vdso. If we are
always reading this inside the seqlock, it should be okay.

In the way vdso's clock_gettime is structured, however, it would involve passing
down the expected seq + vread pointer down the function call chain, or wrapping
all callees inside the lock. Honestly, I believe the solution I posted is cleaner
than both, even having a slight difference wrt vsyscall's today implementation.

What do you think ?



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