linux-kernel - Re: [patch 32/78] fix bad macro param in timer.c

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>] [day] [month] [year] [list]

Date:	Sun, 30 Jul 2006 19:31:15 -0400
From:	Steven Rostedt <rostedt@...dmis.org>
To:	akpm@...l.org
Cc:	torvalds@...l.org, oleg@...sign.ru,
	LKML <linux-kernel@...r.kernel.org>, Ingo Molnar <mingo@...e.hu>,
	Thomas Gleixner <tglx@...utronix.de>,
	Russell King <rmk@....linux.org.uk>
Subject: Re: [patch 32/78] fix bad macro param in timer.c

On Sun, 2006-07-30 at 03:03 -0700, akpm@...l.org wrote:
> From: Steven Rostedt <rostedt@...dmis.org>
> 
> We have
> 
> #define INDEX(N) (base->timer_jiffies >> (TVR_BITS + N * TVN_BITS)) & TVN_MASK
> 
> and it's used via
> 
> 	list = varray[i + 1]->vec + (INDEX(i + 1));
> 
> So, due to underparenthesisation, this INDEX(i+1) is now a ...  (TVR_BITS + i
> + 1 * TVN_BITS)) ...
> 
> So this bugfix changes behaviour.  Why did it work before?

Shear luck!

Well the outside most parenthesis was OK to be missing because the use
of those where done like this:

	list = base->tv2.vec + (INDEX(0));

Where the user of the INDEX macro put the parenthesis at location of use
and not in the macro (where it belonged).

The broken code was here:

	for (i = 0; i < 4; i++) {
		j = INDEX(i);
		do {
			if (list_empty(varray[i]->vec + j)) {
				j = (j + 1) & TVN_MASK;
				continue;
			}
			list_for_each_entry(nte, varray[i]->vec + j, entry)
				if (time_before(nte->expires, expires))
					expires = nte->expires;
			if (j < (INDEX(i)) && i < 3)
				list = varray[i + 1]->vec + (INDEX(i + 1));
			goto found;
		} while (j != (INDEX(i)));
	}
found:
	if (list) {
		/*
		 * The search wrapped. We need to look at the next list
		 * from next tv element that would cascade into tv element
		 * where we found the timer element.
		 */
		list_for_each_entry(nte, list, entry) {
			if (time_before(nte->expires, expires))
				expires = nte->expires;
		}
	}

Where the INDEX(i+1) was used.  So why did it work?

  Simple answer: It didn't

If i was anything but 0, it was broken.  But this was only used by s390
and arm.  Since it was for the next interrupt, could that next interrupt
be a problem (going into the second cascade)?  But it was probably
seldom wrong. That is, this would fail if the next interrupt was in the
second cascade, and was wrapped. Which may never of happened.  Also if
it did happen, it would have just missed the interrupt.

If an interrupt was missed, and no one was there to miss it, was it
really missed :-)

So, this is a bug fix, that changes behavior, but I believe that the
behavior that was changed, is for the better.

-- Steve

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