linux-kernel - Re: [PATCH] TTY: Fix loss of echoed characters (2nd follow-on PATCH attached)

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, 09 Sep 2008 07:00:36 -0600
From:	Joe Peterson <joe@...rush.com>
To:	Andrew Morton <akpm@...ux-foundation.org>
CC:	alan@...rguk.ukuu.org.uk, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] TTY: Fix loss of echoed characters (2nd follow-on PATCH
 attached)

Andrew Morton wrote:
> On Mon, 08 Sep 2008 10:11:46 -0600
> Joe Peterson <joe@...rush.com> wrote:
> 
>> +	spin_lock_irqsave(&tty->echo_lock, flags);
>>  	lock_kernel();
> 
> Taking a spinlock outside lock_kernel() isn't good, and is quite unusual.
> - It might be ab/ba deadlockable (I didn't check) (I trust you always
>   test with lockdep enabled?)

Indeed - and, as Alan said, lock_kernel() can sleep (a nuance I had not
realized until looking more into the kernel locking mechanisms just
now).  Although I have seen no issues during testing (and I do have
lockdep in the kernel), you are 100% right.

I had wanted to keep from disturbing the locking situation in n_tty, but
maybe it is time to get rid of the BKL there.

My echo buffer patches actually isolate the tty column state stuff to
the output processing functions now anyway, so the BLK may not really be
necessary at this point.  This inspires me to look into this.

> swapping the above two lines would presumably be an easy fix, but one
> wonders whether we still need lock_kernel() in there once you've added
> this lock.

I don't think this is a good idea either, since I don't want to spinlock
during the output processing, which calls the driver output func.  I
think a mutex is more appropriate anyway (and there are some already
defined and in use for tty write locking, etc.) - let me know if you
think otherwise.  I will play around with this and re-post a patch for
review soon.

						-Thanks, Joe
--
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/