Login | Register For Free | Help
Search for: (Advanced)

Mailing List Archive: Linux: Kernel

[PATCH 01/10] random: make 'add_interrupt_randomness()' do something sane

 

 

Linux kernel RSS feed   Index | Next | Previous | View Threaded


tytso at mit

Jul 5, 2012, 11:12 AM

Post #1 of 17 (153 views)
Permalink
[PATCH 01/10] random: make 'add_interrupt_randomness()' do something sane

We've been moving away from add_interrupt_randomness() for various
reasons: it's too expensive to do on every interrupt, and flooding the
CPU with interrupts could theoretically cause bogus floods of entropy
from a somewhat externally controllable source.

This solves both problems by limiting the actual randomness addition
to just once a second or after 128 interrupts, whicever comes first.
During that time, the interrupt cycle data is buffered up in a per-cpu
pool. Also, we make sure the the nonblocking pool used by urandom is
initialized before we start feeding the normal input pool. This
assures that /dev/urandom is returning unpredictable data as soon as
possible.

(Based on an original patch by Linus, but significantly modified by
tytso.)

Tested-by: Eric Wustrow <ewust [at] umich>
Reported-by: Eric Wustrow <ewust [at] umich>
Reported-by: Nadia Heninger <nadiah [at] cs>
Reported-by: Zakir Durumeric <zakir [at] umich>
Reported-by: J. Alex Halderman <jhalderm [at] umich>.
Signed-off-by: Linus Torvalds <torvalds [at] linux-foundation>
Signed-off-by: "Theodore Ts'o" <tytso [at] mit>
Cc: stable [at] kernel
---
drivers/char/random.c | 85 ++++++++++++++++++++++++++++++++++++++---------
drivers/mfd/ab3100-core.c | 2 --
kernel/irq/handle.c | 3 +-
3 files changed, 71 insertions(+), 19 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index 4ec04a7..53b3e85 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -133,13 +133,9 @@
* add_input_randomness() uses the input layer interrupt timing, as well as
* the event type information from the hardware.
*
- * add_interrupt_randomness() uses the inter-interrupt timing as random
- * inputs to the entropy pool. Note that not all interrupts are good
- * sources of randomness! For example, the timer interrupts is not a
- * good choice, because the periodicity of the interrupts is too
- * regular, and hence predictable to an attacker. Network Interface
- * Controller interrupts are a better measure, since the timing of the
- * NIC interrupts are more unpredictable.
+ * add_interrupt_randomness() uses the interrupt timing as random
+ * inputs to the entropy pool. Using the cycle counters and the irq source
+ * as inputs, it feeds the randomness roughly once a second.
*
* add_disk_randomness() uses what amounts to the seek time of block
* layer request events, on a per-disk_devt basis, as input to the
@@ -256,6 +252,8 @@
#include <asm/processor.h>
#include <asm/uaccess.h>
#include <asm/irq.h>
+#include <asm/irq_regs.h>
+#include <asm/ptrace.h>
#include <asm/io.h>

/*
@@ -421,7 +419,9 @@ struct entropy_store {
spinlock_t lock;
unsigned add_ptr;
int entropy_count;
+ int entropy_total;
int input_rotate;
+ unsigned int initialized:1;
__u8 last_data[EXTRACT_SIZE];
};

@@ -454,6 +454,10 @@ static struct entropy_store nonblocking_pool = {
.pool = nonblocking_pool_data
};

+static __u32 const twist_table[8] = {
+ 0x00000000, 0x3b6e20c8, 0x76dc4190, 0x4db26158,
+ 0xedb88320, 0xd6d6a3e8, 0x9b64c2b0, 0xa00ae278 };
+
/*
* This function adds bytes into the entropy "pool". It does not
* update the entropy estimate. The caller should call
@@ -467,9 +471,6 @@ static struct entropy_store nonblocking_pool = {
static void mix_pool_bytes_extract(struct entropy_store *r, const void *in,
int nbytes, __u8 out[64])
{
- static __u32 const twist_table[8] = {
- 0x00000000, 0x3b6e20c8, 0x76dc4190, 0x4db26158,
- 0xedb88320, 0xd6d6a3e8, 0x9b64c2b0, 0xa00ae278 };
unsigned long i, j, tap1, tap2, tap3, tap4, tap5;
int input_rotate;
int wordmask = r->poolinfo->poolwords - 1;
@@ -528,6 +529,35 @@ static void mix_pool_bytes(struct entropy_store *r, const void *in, int bytes)
mix_pool_bytes_extract(r, in, bytes, NULL);
}

+struct fast_pool {
+ __u32 pool[4];
+ unsigned long last;
+ unsigned short count;
+ unsigned short rotate;
+};
+
+/*
+ * This is a fast mixing routine used by the interrupt randomness
+ * collector. It's hardcoded for an 128 bit pool and assumes that any
+ * locks that might be needed are taken by the caller.
+ */
+static void fast_mix(struct fast_pool *f, const void *in, int nbytes)
+{
+ const char *bytes = in;
+ __u32 w;
+ unsigned i = f->count;
+ unsigned input_rotate = f->rotate;
+
+ while (nbytes--) {
+ w = rol32(*bytes++, input_rotate & 31) ^ f->pool[i & 3] ^
+ f->pool[(i + 1) & 3];
+ f->pool[i & 3] = (w >> 3) ^ twist_table[w & 7];
+ input_rotate += (i++ & 3) ? 7 : 14;
+ }
+ f->count = i;
+ f->rotate = input_rotate;
+}
+
/*
* Credit (or debit) the entropy store with n bits of entropy
*/
@@ -551,6 +581,12 @@ static void credit_entropy_bits(struct entropy_store *r, int nbits)
entropy_count = r->poolinfo->POOLBITS;
r->entropy_count = entropy_count;

+ if (!r->initialized && nbits > 0) {
+ r->entropy_total += nbits;
+ if (r->entropy_total > 128)
+ r->initialized = 1;
+ }
+
/* should we wake readers? */
if (r == &input_pool && entropy_count >= random_read_wakeup_thresh) {
wake_up_interruptible(&random_read_wait);
@@ -700,17 +736,35 @@ void add_input_randomness(unsigned int type, unsigned int code,
}
EXPORT_SYMBOL_GPL(add_input_randomness);

+static DEFINE_PER_CPU(struct fast_pool, irq_randomness);
+
void add_interrupt_randomness(int irq)
{
- struct timer_rand_state *state;
+ struct entropy_store *r;
+ struct fast_pool *fast_pool = &__get_cpu_var(irq_randomness);
+ struct pt_regs *regs = get_irq_regs();
+ unsigned long now = jiffies;
+ __u32 input[4];
+
+ input[0] = get_cycles() ^ jiffies;
+ input[1] = irq;
+ if (regs) {
+ __u64 ip = instruction_pointer(regs);
+ input[2] = ip;
+ input[3] = ip >> 32;
+ }

- state = get_timer_rand_state(irq);
+ fast_mix(fast_pool, input, sizeof(input));

- if (state == NULL)
+ if ((fast_pool->count & 255) &&
+ !time_after(now, fast_pool->last + HZ))
return;

- DEBUG_ENT("irq event %d\n", irq);
- add_timer_randomness(state, 0x100 + irq);
+ fast_pool->last = now;
+
+ r = nonblocking_pool.initialized ? &input_pool : &nonblocking_pool;
+ mix_pool_bytes(r, &fast_pool->pool, sizeof(fast_pool->pool));
+ credit_entropy_bits(r, 1);
}

#ifdef CONFIG_BLOCK
@@ -971,6 +1025,7 @@ static void init_std_data(struct entropy_store *r)

spin_lock_irqsave(&r->lock, flags);
r->entropy_count = 0;
+ r->entropy_total = 0;
spin_unlock_irqrestore(&r->lock, flags);

now = ktime_get_real();
diff --git a/drivers/mfd/ab3100-core.c b/drivers/mfd/ab3100-core.c
index 1efad20..9522d6b 100644
--- a/drivers/mfd/ab3100-core.c
+++ b/drivers/mfd/ab3100-core.c
@@ -409,8 +409,6 @@ static irqreturn_t ab3100_irq_handler(int irq, void *data)
u32 fatevent;
int err;

- add_interrupt_randomness(irq);
-
err = ab3100_get_register_page_interruptible(ab3100, AB3100_EVENTA1,
event_regs, 3);
if (err)
diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c
index bdb1803..8270db0 100644
--- a/kernel/irq/handle.c
+++ b/kernel/irq/handle.c
@@ -172,8 +172,7 @@ handle_irq_event_percpu(struct irq_desc *desc, struct irqaction *action)
action = action->next;
} while (action);

- if (random & IRQF_SAMPLE_RANDOM)
- add_interrupt_randomness(irq);
+ add_interrupt_randomness(irq);

if (!noirqdebug)
note_interrupt(irq, desc, retval);
--
1.7.11.1.108.gb129051

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


mpm at selenic

Jul 5, 2012, 11:47 AM

Post #2 of 17 (146 views)
Permalink
Re: [PATCH 01/10] random: make 'add_interrupt_randomness()' do something sane [In reply to]

On Thu, 2012-07-05 at 14:12 -0400, Theodore Ts'o wrote:
> We've been moving away from add_interrupt_randomness() for various
> reasons: it's too expensive to do on every interrupt, and flooding the
> CPU with interrupts could theoretically cause bogus floods of entropy
> from a somewhat externally controllable source.
>
> This solves both problems by limiting the actual randomness addition
> to just once a second or after 128 interrupts, whicever comes first.
> During that time, the interrupt cycle data is buffered up in a per-cpu
> pool. Also, we make sure the the nonblocking pool used by urandom is
> initialized before we start feeding the normal input pool. This
> assures that /dev/urandom is returning unpredictable data as soon as
> possible.
>
> (Based on an original patch by Linus, but significantly modified by
> tytso.)

This series generally looks good to me, thanks for working on this. Some
notes:

a) now that you have a lockless collection path, you can drop the
trickle logic that protects against thrashing the lock
b) you can drop the last of the IRQF_SAMPLE_RANDOM users (and the
corresponding deprecation notice)
c) this code looks like it might credit timer interrupts with entropy on
a system otherwise sitting in the idle loop:

> - if (state == NULL)
> + if ((fast_pool->count & 255) &&
> + !time_after(now, fast_pool->last + HZ))
> return;
>
> - DEBUG_ENT("irq event %d\n", irq);
> - add_timer_randomness(state, 0x100 + irq);
> + fast_pool->last = now;
> +
> + r = nonblocking_pool.initialized ? &input_pool : &nonblocking_pool;
> + mix_pool_bytes(r, &fast_pool->pool, sizeof(fast_pool->pool));
> + credit_entropy_bits(r, 1);

I think you should demand a minimum number of events > HZ to actually
credit any valid entropy.

But I also still think the whole entropy counting/blocking scheme ought
to be dropped and /dev/random should become identical to /dev/urandom.
This series gets very close to the point where that's feasible.

--
Mathematics is the supreme nostalgia of our time.


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


torvalds at linux-foundation

Jul 5, 2012, 11:52 AM

Post #3 of 17 (153 views)
Permalink
Re: [PATCH 01/10] random: make 'add_interrupt_randomness()' do something sane [In reply to]

On Thu, Jul 5, 2012 at 11:47 AM, Matt Mackall <mpm [at] selenic> wrote:
>
> I think you should demand a minimum number of events > HZ to actually
> credit any valid entropy.

There already is. It's 1.

If we don't get a single non-timer interrupt, this code will never be called.

And if we only get a single interrupt in over one HZ, then there very
arguably is one bit of entropy in 'jiffies' at the time of that
interrupt.

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


mpm at selenic

Jul 5, 2012, 2:39 PM

Post #4 of 17 (148 views)
Permalink
Re: [PATCH 01/10] random: make 'add_interrupt_randomness()' do something sane [In reply to]

On Thu, 2012-07-05 at 11:52 -0700, Linus Torvalds wrote:
> On Thu, Jul 5, 2012 at 11:47 AM, Matt Mackall <mpm [at] selenic> wrote:
> >
> > I think you should demand a minimum number of events > HZ to actually
> > credit any valid entropy.
>
> There already is. It's 1.

> If we don't get a single non-timer interrupt, this code will never be called.

From my read, this code path gets called on timer interrupts too. Thus
the number of events per HZ will be HZ at a minimum on systems that
haven't gone tickless. If such systems a) don't have a higher-res time
source and b) are halted or equivalent, such samples will be completely
deterministic. So the threshold for crediting entropy should be HZ + 1.

But perhaps I've missed something.

(As I expressed in my last message, this is strictly a correctness issue
and not a practical one. I've long held that the entropy counting model
is bogus and should be abandoned.)

--
Mathematics is the supreme nostalgia of our time.


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


torvalds at linux-foundation

Jul 5, 2012, 2:47 PM

Post #5 of 17 (152 views)
Permalink
Re: [PATCH 01/10] random: make 'add_interrupt_randomness()' do something sane [In reply to]

On Thu, Jul 5, 2012 at 2:39 PM, Matt Mackall <mpm [at] selenic> wrote:
>
> From my read, this code path gets called on timer interrupts too.

That's hopefully never true for any normal cases (timers are *very*
special - they tend to go through their own architecture-specific
stuff). On modern PC's, for example, the timers happen through the
local apic timers directly.

In fact, with SMP, it's a really bad idea to use a normal irq for
timer interrupts, since you really really want per-CPU-core timers.

But yes, we should probably make sure that *if* the architecture uses
regular interrupts for timers we don't count them. The problematic
embedded platforms are often pretty crap hardware: even when they are
SMP, they might use an external timer irq (and then we broadcast the
thing). I think we have the IRQF_TIMER flag for that, so we could add
a check for that.

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


tytso at mit

Jul 5, 2012, 3:00 PM

Post #6 of 17 (147 views)
Permalink
Re: [PATCH 01/10] random: make 'add_interrupt_randomness()' do something sane [In reply to]

On Thu, Jul 05, 2012 at 02:47:52PM -0700, Linus Torvalds wrote:
> On Thu, Jul 5, 2012 at 2:39 PM, Matt Mackall <mpm [at] selenic> wrote:
> >
> > From my read, this code path gets called on timer interrupts too.
>
> That's hopefully never true for any normal cases (timers are *very*
> special - they tend to go through their own architecture-specific
> stuff). On modern PC's, for example, the timers happen through the
> local apic timers directly.
>
> In fact, with SMP, it's a really bad idea to use a normal irq for
> timer interrupts, since you really really want per-CPU-core timers.
>
> But yes, we should probably make sure that *if* the architecture uses
> regular interrupts for timers we don't count them. The problematic
> embedded platforms are often pretty crap hardware: even when they are
> SMP, they might use an external timer irq (and then we broadcast the
> thing). I think we have the IRQF_TIMER flag for that, so we could add
> a check for that.

Like this?

- Ted

diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c
index 8270db0..0ce4863 100644
--- a/kernel/irq/handle.c
+++ b/kernel/irq/handle.c
@@ -133,7 +133,7 @@ irqreturn_t
handle_irq_event_percpu(struct irq_desc *desc, struct irqaction *action)
{
irqreturn_t retval = IRQ_NONE;
- unsigned int random = 0, irq = desc->irq_data.irq;
+ unsigned int random = 1, irq = desc->irq_data.irq;

do {
irqreturn_t res;
@@ -161,7 +161,8 @@ handle_irq_event_percpu(struct irq_desc *desc, struct irqaction *action)

/* Fall through to add to randomness */
case IRQ_HANDLED:
- random |= action->flags;
+ if (action->flags & __IRQF_TIMER)
+ random = 0;
break;

default:
@@ -172,7 +173,8 @@ handle_irq_event_percpu(struct irq_desc *desc, struct irqaction *action)
action = action->next;
} while (action);

- add_interrupt_randomness(irq);
+ if (random)
+ add_interrupt_randomness(irq);

if (!noirqdebug)
note_interrupt(irq, desc, retval);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


torvalds at linux-foundation

Jul 5, 2012, 3:21 PM

Post #7 of 17 (148 views)
Permalink
Re: [PATCH 01/10] random: make 'add_interrupt_randomness()' do something sane [In reply to]

On Thu, Jul 5, 2012 at 3:00 PM, Theodore Ts'o <tytso [at] mit> wrote:
>
> Like this?

Looks fine to me.

Although I think it might be better to stay closer to what we used to
do, and just 'or' in the action flags rather than make it some
conditional. And then at the end, do

if (!(flags & __IRQF_TIMER))
add_interrupt_randomness(irq)

instead on that or'ed flags value. Otherwise gcc will create silly
conditional moves (or worse still, conditional branches) just for that
"random" variable assignment.

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


mpm at selenic

Jul 5, 2012, 3:31 PM

Post #8 of 17 (148 views)
Permalink
Re: [PATCH 01/10] random: make 'add_interrupt_randomness()' do something sane [In reply to]

On Thu, 2012-07-05 at 15:21 -0700, Linus Torvalds wrote:
> On Thu, Jul 5, 2012 at 3:00 PM, Theodore Ts'o <tytso [at] mit> wrote:
> >
> > Like this?
>
> Looks fine to me.
>
> Although I think it might be better to stay closer to what we used to
> do, and just 'or' in the action flags rather than make it some
> conditional. And then at the end, do
>
> if (!(flags & __IRQF_TIMER))
> add_interrupt_randomness(irq)

On systems with a timer interrupt and a sched_clock() that's
asynchronous to it, this actually loses a great source of entropy for
headless systems. Also: extra branch in fast path.

It's better to mix and not credit than to not mix at all. Instead just
check the fast count against HZ before the credit.

--
Mathematics is the supreme nostalgia of our time.


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


torvalds at linux-foundation

Jul 5, 2012, 3:35 PM

Post #9 of 17 (149 views)
Permalink
Re: [PATCH 01/10] random: make 'add_interrupt_randomness()' do something sane [In reply to]

On Thu, Jul 5, 2012 at 3:31 PM, Matt Mackall <mpm [at] selenic> wrote:
>
> It's better to mix and not credit than to not mix at all. Instead just
> check the fast count against HZ before the credit.

That sounds like a good idea.

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


tytso at mit

Jul 5, 2012, 4:21 PM

Post #10 of 17 (142 views)
Permalink
Re: [PATCH 01/10] random: make 'add_interrupt_randomness()' do something sane [In reply to]

On Thu, Jul 05, 2012 at 05:31:22PM -0500, Matt Mackall wrote:
> On systems with a timer interrupt and a sched_clock() that's
> asynchronous to it, this actually loses a great source of entropy for
> headless systems. Also: extra branch in fast path.
>
> It's better to mix and not credit than to not mix at all. Instead just
> check the fast count against HZ before the credit.

I'm not sure what you mean by this?

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


torvalds at linux-foundation

Jul 5, 2012, 7:59 PM

Post #11 of 17 (135 views)
Permalink
Re: [PATCH 01/10] random: make 'add_interrupt_randomness()' do something sane [In reply to]

On Thu, Jul 5, 2012 at 4:21 PM, Theodore Ts'o <tytso [at] mit> wrote:
> On Thu, Jul 05, 2012 at 05:31:22PM -0500, Matt Mackall wrote:
>>
>> It's better to mix and not credit than to not mix at all. Instead just
>> check the fast count against HZ before the credit.
>
> I'm not sure what you mean by this?

So what I think Matt meant was to check number of timer interrupts
against the fast count.

IOW, always call "add_interrupt_randomness()", but then in that
function, when you determine the amount of entropy, check if there
were non-timer interrupts in the last HZ cycle. If there were purely
timer interrupts, you can still mix in the cycle information, but it's
likely to be fairly weak.

That said, if we do have a real TSC, I suspect there's tons of entropy
in it even for a pure timer interrupt, so it's more like "if the
*only* source of randomness we have is the timer interrupt and the
jiffies value, that doesn't really add any entropy at all".

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


tytso at mit

Jul 6, 2012, 6:01 AM

Post #12 of 17 (131 views)
Permalink
Re: [PATCH 01/10] random: make 'add_interrupt_randomness()' do something sane [In reply to]

On Thu, Jul 05, 2012 at 07:59:00PM -0700, Linus Torvalds wrote:
> On Thu, Jul 5, 2012 at 4:21 PM, Theodore Ts'o <tytso [at] mit> wrote:
> > On Thu, Jul 05, 2012 at 05:31:22PM -0500, Matt Mackall wrote:
> >>
> >> It's better to mix and not credit than to not mix at all. Instead just
> >> check the fast count against HZ before the credit.
> >
> > I'm not sure what you mean by this?
>
> So what I think Matt meant was to check number of timer interrupts
> against the fast count.
>
> IOW, always call "add_interrupt_randomness()", but then in that
> function, when you determine the amount of entropy, check if there
> were non-timer interrupts in the last HZ cycle. If there were purely
> timer interrupts, you can still mix in the cycle information, but it's
> likely to be fairly weak.

I'm going to have to plead ignorance to how the timer code works these
days. With the advent of clockevents and NOHZ, it's gotten a lot more
complicated. What in the world is "fast count"? I've grepped for it,
and I can't find it.

I can simply not credit entropy of the timer is on the irq, but I
think you and Matt were suggesting more subtle. I just have no idea
how to tell if there were non-timer interrupts during the last HZ
cycle. Can you give me a hint?

Thanks,

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


torvalds at linux-foundation

Jul 6, 2012, 9:24 AM

Post #13 of 17 (133 views)
Permalink
Re: [PATCH 01/10] random: make 'add_interrupt_randomness()' do something sane [In reply to]

On Fri, Jul 6, 2012 at 6:01 AM, Theodore Ts'o <tytso [at] mit> wrote:
> What in the world is "fast count"? I've grepped for it,
> and I can't find it.

It's your own fast-pool counter that Matt was talking about.

> I can simply not credit entropy of the timer is on the irq, but I
> think you and Matt were suggesting more subtle. I just have no idea
> how to tell if there were non-timer interrupts during the last HZ
> cycle. Can you give me a hint?

So instead of not calling the add_interrupt_randomness() at all if the
__IRQF_TIMER bit was set, just pass it in as an argument. That way you
can still use the cycle counter for mixing stuff, but the random.c
code could at least (perhaps in the future) decide that if all it has
seen is timer interrupts, and get_cycles() always returns zero (no
cycle counter), we won't count it as entropy.

At least with no-HZ there's still going to be *some* data there
because you won't get called for every tick, so things like random app
timers etc will affect even the timer interrupt distribution, but...

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


tytso at mit

Jul 6, 2012, 9:52 AM

Post #14 of 17 (132 views)
Permalink
Re: [PATCH 01/10] random: make 'add_interrupt_randomness()' do something sane [In reply to]

On Fri, Jul 06, 2012 at 09:24:00AM -0700, Linus Torvalds wrote:
> On Fri, Jul 6, 2012 at 6:01 AM, Theodore Ts'o <tytso [at] mit> wrote:
> > What in the world is "fast count"? I've grepped for it,
> > and I can't find it.
>
> It's your own fast-pool counter that Matt was talking about.

When he said "check it against HZ", it confused me, since there's no
way to compare it against HZ. But yes, I can certainly not give any
credit for entropy if __IRQF_TIMER is set, or keep track of whether
the previous interrupt had __IRQF_TIMER set in its descriptor. That's
simple enough.

I thought he was saying there was some way to distinguish between
interrupts triggered by the clock interrupt versus other devices on
the same irq channel --- and I couldn't figure out any to do that in
an architecture independent way.

- Ted

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


tytso at mit

Jul 6, 2012, 2:59 PM

Post #15 of 17 (132 views)
Permalink
Re: [PATCH 01/10] random: make 'add_interrupt_randomness()' do something sane [In reply to]

On Thu, Jul 05, 2012 at 03:01:12PM -0400, Eric Wustrow wrote:
> Will this do the long path in add_interrupt_randomness every 16 interrupts
> instead of 128?

Yes, but given that benchmarks didn't show any performance degradation
even under a worst case scenario (i.e., no interrupt mitigation, and a
crazy number of interrupts/second), I decided to leave things as they
are.

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


mpm at selenic

Jul 9, 2012, 12:15 PM

Post #16 of 17 (123 views)
Permalink
Re: [PATCH 01/10] random: make 'add_interrupt_randomness()' do something sane [In reply to]

On Fri, 2012-07-06 at 12:52 -0400, Theodore Ts'o wrote:
> On Fri, Jul 06, 2012 at 09:24:00AM -0700, Linus Torvalds wrote:
> > On Fri, Jul 6, 2012 at 6:01 AM, Theodore Ts'o <tytso [at] mit> wrote:
> > > What in the world is "fast count"? I've grepped for it,
> > > and I can't find it.
> >
> > It's your own fast-pool counter that Matt was talking about.
>
> When he said "check it against HZ", it confused me, since there's no
> way to compare it against HZ. But yes, I can certainly not give any
> credit for entropy if __IRQF_TIMER is set, or keep track of whether
> the previous interrupt had __IRQF_TIMER set in its descriptor. That's
> simple enough.
>
> I thought he was saying there was some way to distinguish between
> interrupts triggered by the clock interrupt versus other devices on
> the same irq channel --- and I couldn't figure out any to do that in
> an architecture independent way.

Sorry.. offline for the weekend.

Let me restate:

- on some architectures, we will call into the RNG on timer interrupts
- this is generally desirable, as most time sources are asynchronous to
sched_clock() and thus a source of entropy
- we also want to keep conditional checks like IRQF_TIMER off the fast
path
- but on systems where the timer interrupt is the primary time source,
we may get effectively no entropy when the system is quiescent
- so we should check the fast pool count against HZ before crediting
- but even then, we still should mix the fast pool

Something like:

add_some_randomness(...) /* always mix */
if (fast_pool->count > HZ) {
fast_pool->count = 0;
credit_entropy_pool(...); /* only credit when we've got > HZ events */
}

That should be safe on all systems.

--
Mathematics is the supreme nostalgia of our time.


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


tglx at linutronix

Jul 25, 2012, 11:43 AM

Post #17 of 17 (87 views)
Permalink
Re: [PATCH 01/10] random: make 'add_interrupt_randomness()' do something sane [In reply to]

On Fri, 6 Jul 2012, Linus Torvalds wrote:

> On Fri, Jul 6, 2012 at 6:01 AM, Theodore Ts'o <tytso [at] mit> wrote:
> > What in the world is "fast count"? I've grepped for it,
> > and I can't find it.
>
> It's your own fast-pool counter that Matt was talking about.
>
> > I can simply not credit entropy of the timer is on the irq, but I
> > think you and Matt were suggesting more subtle. I just have no idea
> > how to tell if there were non-timer interrupts during the last HZ
> > cycle. Can you give me a hint?
>
> So instead of not calling the add_interrupt_randomness() at all if the
> __IRQF_TIMER bit was set, just pass it in as an argument. That way you
> can still use the cycle counter for mixing stuff, but the random.c
> code could at least (perhaps in the future) decide that if all it has
> seen is timer interrupts, and get_cycles() always returns zero (no
> cycle counter), we won't count it as entropy.
>
> At least with no-HZ there's still going to be *some* data there
> because you won't get called for every tick, so things like random app
> timers etc will affect even the timer interrupt distribution, but...

There is only one constellation which will not be able to deliver
randomness via the timer interrupt and a "cycle counter":

Systems, which do not have an usable fine granular clocksource and
therefor are completely based on jiffies. Those systems have neither
high resolution timers nor NOHZ idle for obvious reasons.

Though almost all systems which we care about have a more or less
useful clocksource and register it proper with the core.

So we could do the following in the core code when a clocksource is
registered:

Aside of considering it to handle the timekeeping stuff, which has
different criteria, we could filter out the clocksource with the
highest frequency and provide a function which allows us to access it
for randomness and other purposes. IOW, a generic "get_cycles"
implementation.

Now, if the only available clocksource is jiffies, which is completely
useless, as it gets incremented once per timer interrupt (and there is
no distribution possible due to the lack of NOHZ support) then the
function returns 0 which allows the random code to discard it as a
entropy source.

We might consider to put a low frequency limit into that selection
process as well, but that needs some thought and experimentation.

Thanks,

tglx




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

Linux kernel RSS feed   Index | Next | Previous | View Threaded
 
 


Interested in having your list archived? Contact Gossamer Threads
 
  Web Applications & Managed Hosting Powered by Gossamer Threads Inc.