netdev - Re: [PATCH] ircomm: release tty before sleeping potentially indefintely

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: <1362371063.3221.185.camel@thor.lan>
Date:	Sun, 03 Mar 2013 23:24:23 -0500
From:	Peter Hurley <peter@...leysoftware.com>
To:	David Miller <davem@...emloft.net>
Cc:	sasha.levin@...cle.com, samuel@...tiz.org,
	gregkh@...uxfoundation.org, jslaby@...e.cz, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ircomm: release tty before sleeping potentially
 indefintely

On Sun, 2013-03-03 at 21:36 -0500, David Miller wrote:
> From: Peter Hurley <peter@...leysoftware.com>
> Date: Sun, 03 Mar 2013 20:06:18 -0500
> 
> > But regardless, this function __cannot__ sleep holding the tty_lock().
> 
> So drop it across the schedule(), but recheck the termios after
> regrabbing it.

I'll have to do some research on that.

1) The code is using a deliberate snapshot.

	if (tty->termios.c_cflag & CLOCAL) {
		IRDA_DEBUG(1, "%s(), doing CLOCAL!\n", __func__ );
		do_clocal = 1;
	}

	.....

	while (1) {

		.........

		/*
		 * Check if link is ready now. Even if CLOCAL is
		 * specified, we cannot return before the IrCOMM link is
		 * ready
		 */
		if (!test_bit(ASYNCB_CLOSING, &port->flags) &&
		    (do_clocal || tty_port_carrier_raised(port)) &&
		    self->state == IRCOMM_TTY_READY)
		{
			break;
		}

2) The only reason this driver isn't using tty_port_block_til_ready() is
the lone state check:
		    self->state == IRCOMM_TTY_READY

I take it IRDA has some kind of virtual cabling protocol. But it's
unclear why this can't be implemented in the driver without duplicating
tty_port_block_til_ready(). For example, if the device can't do CLOCAL
open (meaning no underlying device attached prior to open) then why
specify that in the driver flags? Additionally, CLOCAL can be masked out
by the driver's set_termios() method. And then it could implement the
state check in its .carrier_raised() method.

3) The do_clocal snapshot is universally employed by every tty driver. I
don't mean that as some kind of lame excuse. But if this should change,
it should change across every tty driver with a really good reason.

4) Rechecking termios will change the way the user-space open() for this
device behaves. And I need to think more on how that might or might not
be a problem.

5) The code behavior pre-dates the 2005 check-in so I'll probably have
to do some code archaeology.

That's probably going to take some time.

In the meantime, while reviewing that code, I noticed there's a handful
of serious bugs in that one function that I'll send a patchset for.

Plus, someone could be back to me on if and why the driver needs to be
virtually cabled to open().

Regards,
Peter Hurley

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html