Re: Q: tty_open() && hangup
From: Oleg Nesterov
Date: Sat Apr 16 2016 - 12:29:19 EST
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.