linux-kernel - Re: [RFC PATCH 27/33] Add the Xen virtual console driver.

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:	Tue, 18 Jul 2006 12:24:38 +0200
From:	Arjan van de Ven <arjan@...radead.org>
To:	Chris Wright <chrisw@...s-sol.org>
Cc:	linux-kernel@...r.kernel.org, virtualization@...ts.osdl.org,
	xen-devel@...ts.xensource.com,
	Jeremy Fitzhardinge <jeremy@...p.org>, Andi Kleen <ak@...e.de>,
	Andrew Morton <akpm@...l.org>,
	Rusty Russell <rusty@...tcorp.com.au>,
	Zachary Amsden <zach@...are.com>,
	Ian Pratt <ian.pratt@...source.com>,
	Christian Limpach <Christian.Limpach@...cam.ac.uk>
Subject: Re: [RFC PATCH 27/33] Add the Xen virtual console driver.

On Tue, 2006-07-18 at 00:00 -0700, Chris Wright wrote:
> +			} else if (sysrq_requested) {
> +				unsigned long sysrq_timeout =
> +					sysrq_requested + HZ*2;
> +				sysrq_requested = 0;
> +				if (time_before(jiffies, sysrq_timeout)) {
> +					spin_unlock_irqrestore(
> +						&xencons_lock, flags);
> +					handle_sysrq(
> +						buf[i], regs, xencons_tty);
> +					spin_lock_irqsave(
> +						&xencons_lock, flags);
> +					continue;
> +				}

Lindent can be harmful...

> +static void xencons_wait_until_sent(struct tty_struct *tty, int timeout)
> +{
> +	unsigned long orig_jiffies = jiffies;
> +
> +	if (TTY_INDEX(tty) != 0)
> +		return;
> +
> +	while (DRV(tty->driver)->chars_in_buffer(tty)) {
> +		set_current_state(TASK_INTERRUPTIBLE);
> +		schedule_timeout(1);
> +		if (signal_pending(current))
> +			break;
> +		if (timeout && time_after(jiffies, orig_jiffies + timeout))
> +			break;
> +	}
> +
> +	set_current_state(TASK_RUNNING);
> +}

hmm somehow I find this code scary; we had similar code recently
elsewhere where this turned out to be a real issue; you now sleep for
"1" time, so you sleep for a fixed time if you aren't getting wakeups,
but if you are getting wakeups your code is upside down, I would expect
it to look like

+	set_current_state(TASK_INTERRUPTIBLE);
+	while (DRV(tty->driver)->chars_in_buffer(tty))
+		schedule_timeout(1);
+		if (signal_pending(current))
+			break;
+		if (timeout && time_after(jiffies, orig_jiffies + timeout))
+			break;
+		set_current_state(TASK_INTERRUPTIBLE);
+	}

instead, so that you don't have the wakeup race..




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