Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 4 Sep 2020 15:04:55 +0200
From:   Artem Savkov <asavkov@redhat.com>
To:     Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Cc:     gregkh@linuxfoundation.org, jirislaby@kernel.org,
        threeearcat@gmail.com, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] pty: do tty_flip_buffer_push without port->lock in
 pty_write
Message-ID: <20200904130455.GA52548@shodan.usersys.redhat.com>
References: <20200901120157.3412245-1-asavkov@redhat.com>
 <20200904074333.GA410@jagdpanzerIV.localdomain>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20200904074333.GA410@jagdpanzerIV.localdomain>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hello Sergey,

On Fri, Sep 04, 2020 at 04:43:33PM +0900, Sergey Senozhatsky wrote:
> On (20/09/01 14:01), Artem Savkov wrote:
> [..]
> > It looks like the commit was aimed to protect tty_insert_flip_string and
> > there is no need for tty_flip_buffer_push to be under this lock.
> >
> [..]
> > @@ -120,10 +120,10 @@ static int pty_write(struct tty_struct *tty, const unsigned char *buf, int c)
> >  		spin_lock_irqsave(&to->port->lock, flags);
> >  		/* Stuff the data into the input queue of the other end */
> >  		c = tty_insert_flip_string(to->port, buf, c);
> > +		spin_unlock_irqrestore(&to->port->lock, flags);
> >  		/* And shovel */
> >  		if (c)
> >  			tty_flip_buffer_push(to->port);
> > -		spin_unlock_irqrestore(&to->port->lock, flags);
> 
> Performing unprotected
> 
> 	smp_store_release(&buf->tail->commit, buf->tail->used);
> 
> does not look safe to me.
> 
> 
> This path can be called concurrently - "pty_write vs console's IRQ handler
> (TX/RX)", for instance.
> 
> Doing this
> 
> 	queue_work(system_unbound_wq, &buf->work);
> 
> outside of port->lock scope also sounds like possible concurrent data
> modification.
> 
> I'm not sure I see how this patch is safe.

Yes, indeed I see how this might be unsafe, but this argument doesn't
hold well with console drivers other than 8250 - most of them seem to
call tty_flip_buffer_push() outside of port->lock, many even unlock and
then relock right around this call to avoid similar possible deadlocks.
Even 8250 itself used to do this "recently". After all potentially
corrupted console is better than a deadlock.

I know this is no excuse to add unsafe code but unfortunately I don't
see a better solution at the moment, although admittedly I am not very
familiar with tty code.

-- 
 Artem