Date: Sat, 16 Apr 2016 17:27:32 +0200
From: Oleg Nesterov <oleg@redhat.com>
To: Peter Hurley <peter@hurleysoftware.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jiri Slaby <jslaby@suse.com>, Milos Vyletel <milos@redhat.com>,
        Stanislav Kozina <skozina@redhat.com>, linux-kernel@vger.kernel.org
Subject: Re: Q: tty_open() && hangup
Message-ID: <20160416152732.GA32130@redhat.com>
References: <20160415171950.GA17586@redhat.com> <57114102.3050507@hurleysoftware.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <57114102.3050507@hurleysoftware.com>
User-Agent: Mutt/1.5.18 (2008-05-17)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1691
Lines: 47

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.