linux-kernel - Re: [PATCH v2 31/37] mac80211_hwsim: Replace hrtimer tasklet with softirq hrtimer

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]

Date:   Mon, 23 Oct 2017 12:42:44 +0200
From:   Johannes Berg <johannes@...solutions.net>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     Anna-Maria Gleixner <anna-maria@...utronix.de>,
        LKML <linux-kernel@...r.kernel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>, keescook@...omium.org,
        Christoph Hellwig <hch@....de>,
        John Stultz <john.stultz@...aro.org>,
        Kalle Valo <kvalo@...eaurora.org>,
        linux-wireless@...r.kernel.org
Subject: Re: [PATCH v2 31/37] mac80211_hwsim: Replace hrtimer tasklet with
 softirq hrtimer

> > I guess you mean you *can* build it? Surely you're introducing the new
> > HR timer modes in some patch that I didn't see? :-)
> 
> Sorry, we did not want to expose you to 30 patches fiddling with the core
> code. They are on LKML though.

Sure, no worries. I just didn't even realize I shouldn't be applying
the patch (myself) :-)

> > Right. Then again, why even pass it to init() and start()? Can you
> > start without going through start()?
> 
> There is a subtle magic with CLOCK_REALTIME timers.
> 
> CLOCK_REALTIME timers differentiate between ABS and REL modes. ABS timers
> are exposed to clock modifications (settimeofday() ...), REL timers are
> not. We solve that by associating them to different clock bases, which has
> to be done at init time, but the start function needs the REL/ABS
> information as well.
> 
> For CLOCK_MONOTONIC this is not really required, but the function is used
> for all clock bases, so we require the mode bits for all.

Hmm. Couldn't you just store that then from init to use in start?

If you don't store it, yet don't verify that you passed the same 
thing, do you at least check that it's compatible? Sounds like
something will totally go wrong if I pass CLOCK_REALTIME/ABS first and
then use REL for start, or vice versa?

Also, in the code I see only checking

if (mode != HRTIMER_MODE_ABS) {
	// change clock ID:
	// realtime -> monotonic
	// realtime_soft -> monotonic_soft
}

but you're passing HRTIMER_MODE_ABS_SOFT too, isn't that considered an
ABS mode?

But then, looking at the code again, I don't even see
HRTIMER_MODE_ABS_SOFT existing, it sounds like it should be

hrtimer_init(..., CLOCK_REALTIME_SOFT, HRTIMER_MODE_REL);

then?

At least then the code I mentioned above makes sense, but it still
feels pretty dangerous to not just store the mode and use it in start,
but to require passing it again. You even just introduced the same bug,
it just happened to not matter in this case since the clock isn't
realtime.

Or am I completely confused now?

johannes