linux-kernel - Re: Q: tty_open() && hangup

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

Date:	Sat, 16 Apr 2016 17:27:32 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	Peter Hurley <peter@...leysoftware.com>
Cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Jiri Slaby <jslaby@...e.com>, Milos Vyletel <milos@...hat.com>,
	Stanislav Kozina <skozina@...hat.com>,
	linux-kernel@...r.kernel.org
Subject: Re: Q: tty_open() && hangup

Hi Peter,

On 04/15, Peter Hurley wrote:
> >
> > If we just want to add a preemption point then perhaps we should use
> > cond_resched/might_resched ?
>
> Yeah, it's just a preemption point that pre-dates cond_resched.

OK, so perhaps s/schedule/might_reched/ makes sense to make it more
clear.
> > So we can only get here if tty->ops->open returns -ERESTARTSYS without
> > signal_pending(). This doesn't look good. But OK, lets forget this.
>
> tty_port_block_til_ready() returns -ERESTARTSYS if !ASYNC_HUP_NOTIFY and
> tty was hungup or the h/w was reset

Yes, yes, I saw this code, and I am not saying this is buggy.

I meant this looks confusing. At least to me, until I grepped for ERESTARTSYS
I thought that these code paths do something non-trivial with signal handling.

IMHO, ERESTARTSYS should only be used if signal_pending() is true and we are
going to return this error code to user-space. But tty_port_block_til_ready()
returns ERESTARTSYS if !ASYNC_HUP_NOTIFY _or_ signal_pending(), that is why
tty_open() has to check signal_pending() too.

I think this deserves a cleanup, but this is minor and of course subjective,
please forget.

> >   2148                  /*
> >   2149                   * Need to reset f_op in case a hangup happened.
> >   2150                   */
> >   2151                  if (tty_hung_up_p(filp))
> >   2152                          filp->f_op = &tty_fops;
> >
> > And this is called after tty_add_file() which makes this filp visible to
> > __tty_hangup(), and without tty_lock().
>
> tty_release() has removed the filp from the files list already (with the
> tty lock held),

Heh. Can't understand how did I miss that ;)

Thanks Peter.

Oleg.