Date: Wed, 3 Jan 2018 19:38:07 +0000
From: Alan Cox <gnomes@lxorguk.ukuu.org.uk>
To: Gaurav Kohli <gkohli@codeaurora.org>
Cc: jslaby@suse.com, gregkh@linuxfoundation.org, mikey@neuling.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] tty: fix data race in n_tty_receive_buf_common
Message-ID: <20180103193807.465e054e@alans-desktop>
In-Reply-To: <1514987332-14122-1-git-send-email-gkohli@codeaurora.org>
References: <1514987332-14122-1-git-send-email-gkohli@codeaurora.org>
Organization: Intel Corporation
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org

On Wed,  3 Jan 2018 19:18:52 +0530
Gaurav Kohli <gkohli@codeaurora.org> wrote:

> There can be a race, if receive_buf call comes before
> tty initialization completes in n_tty_open and tty->disc_data
> may be NULL.


This makes no sense. If the race exists then the check you do isn't good
enough because the ldsic dsta isn't valid even after the initial
assignment of tty->disc_data.

More to the point no ldisc receive method should ever be getting called
during the ldisc open.  Likewise we must avoid hitting the window of the
old one closing (potentialyl stale disc_data from the old ldisc)

Any change to the ldisc is supposed to occur under tty->ldisc_sem and the
code does an ldisc_ref before invoking the ldisc method.

The only cases I can see where we set an ldisc are

1. tty_set_ldisc

This correctly takes an ldisc_ref so cannot run in parallel with
tty_port_default_receive_buf.

2. tty_init_dev when we set up a new tty

At that point the tty is not supposed to be receiving data and sure
enough we don't call tty->ops->open until it has finished the ldisc set
up, nor do we tty_init_dev a port that is already running.

So given we don't activate the port until tty->ops->open() calls
tty_port_open calls the port activation routine I don't see a bug.

3. tty_release()

Here we take the locks in tty_ldisc_release so that is ok

4. hangup

Again we take the ldisc lock


So unless your driver is stuffing bytes into the tty either before it's
been told too or after it's been told to shut up I don't see a bug. And
if you driver is doing either of those then it's broken. Even then
port->tty ought to be NULL.

What does a full (all CPU) trace of the bug look like and what tty driver
are you using when you capture the trace ?

Alan