Date:   Tue, 21 Dec 2021 09:18:25 +0100
From:   Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To:     Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
Cc:     Jiri Slaby <jirislaby@kernel.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Mathieu Poirier <mathieu.poirier@linaro.org>,
        linux-remoteproc@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-stm32@st-md-mailman.stormreply.com
Subject: Re: [PATCH v3] tty: rpmsg: Fix race condition releasing tty port
Message-ID: <YcGN0fDn2hqAdrP9@kroah.com>
References: <20211215153121.30010-1-arnaud.pouliquen@foss.st.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211215153121.30010-1-arnaud.pouliquen@foss.st.com>
Precedence: bulk

On Wed, Dec 15, 2021 at 04:31:21PM +0100, Arnaud Pouliquen wrote:
> The tty_port struct is part of the rpmsg_tty_port structure.
> The issue is that the rpmsg_tty_port structure is freed on
> rpmsg_tty_remove while it is still referenced in the tty_struct.
> Its release is not predictable due to workqueues.
> 
> For instance following ftrace shows that rpmsg_tty_close is called after
> rpmsg_tty_release_cport:
> 
>      nr_test.sh-389     [000] .....   212.093752: rpmsg_tty_remove <-rpmsg_dev_
> remove
>              cat-1191    [001] .....   212.095697: tty_release <-__fput
>       nr_test.sh-389     [000] .....   212.099166: rpmsg_tty_release_cport <-rpm
> sg_tty_remove
>              cat-1191    [001] .....   212.115352: rpmsg_tty_close <-tty_release
>              cat-1191    [001] .....   212.115371: release_tty <-tty_release_str
> 
> As consequence, the port must be free only when user has released the TTY
> interface.
> 
> This path :
> - Introduce the .destruct port ops function to release the allocated
>   rpmsg_tty_port structure.
> - Manages the tty port refcounting to trig the .destruct port ops,
> - Introduces the rpmsg_tty_cleanup function to ensure that the TTY is
>   removed before decreasing the port refcount.
> - Uses tty_vhangup and tty_port_hangup instead of tty_port_tty_hangup.

Shouldn't this hangup change be a separate change?

> 
> Fixes: 7c0408d80579 ("tty: add rpmsg driver")
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> ---
> delta vs V2: taking into account Jiri Slaby's comments:
>  - Inline rpmsg_tty_release_cport in rpmsg_tty_destruct_port,
>  - call tty_port_put in case of error in rpmsg_tty_probe,
>  - use tty_port_get port return in rpmsg_tty_install to take into account
>    NULL port return case.
> 
> Applied and tested on fa55b7dcdc43 ("Linux 5.16-rc1", 2021-11-14)
> ---
>  drivers/tty/rpmsg_tty.c | 49 +++++++++++++++++++++++++++++------------
>  1 file changed, 35 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/tty/rpmsg_tty.c b/drivers/tty/rpmsg_tty.c
> index dae2a4e44f38..cdc590c63f03 100644
> --- a/drivers/tty/rpmsg_tty.c
> +++ b/drivers/tty/rpmsg_tty.c
> @@ -50,10 +50,21 @@ static int rpmsg_tty_cb(struct rpmsg_device *rpdev, void *data, int len, void *p
>  static int rpmsg_tty_install(struct tty_driver *driver, struct tty_struct *tty)
>  {
>  	struct rpmsg_tty_port *cport = idr_find(&tty_idr, tty->index);
> +	struct tty_port *port = tty->port;
>  
>  	tty->driver_data = cport;
>  
> -	return tty_port_install(&cport->port, driver, tty);
> +	port = tty_port_get(&cport->port);
> +	return tty_port_install(port, driver, tty);
> +}
> +
> +static void rpmsg_tty_cleanup(struct tty_struct *tty)
> +{
> +	struct tty_port *port = tty->port;
> +
> +	WARN_ON(!port);

How can this ever trigger?  Shouldn't you do something if it can?

thanks,

greg k-h