Message-ID: <8bbd1a77-5f88-bb97-db88-6842df2e3e3c@kernel.org>
Date:   Tue, 14 Dec 2021 07:43:10 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.4.0
Subject: Re: [PATCH] tty: rpmsg: Fix race condition releasing tty port
Content-Language: en-US
To:     Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc:     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
References: <20211213195346.12894-1-arnaud.pouliquen@foss.st.com>
From:   Jiri Slaby <jirislaby@kernel.org>
In-Reply-To: <20211213195346.12894-1-arnaud.pouliquen@foss.st.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi,

On 13. 12. 21, 20:53, Arnaud Pouliquen wrote:
> In current implementation 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 but also 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 (inspired from vcc.c):
> - moves the management of the port in the install and clean-up tty ops,
> - allocates the tty_port struct independently of the rpmsg_tty_port structure,

This looks rather wrong. Why not to use tty_port refcounting?

> - uses tty_vhangup and tty_port_hangup.

OK, but don't store a tty pointer as it looks racy. You should use 
tty_port_tty_get instead.

Hm, we look we need tty_port_tty_vhangup (aside from 
tty_port_tty_hangup). There are plenty of drivers doing:
     tty = tty_port_tty_get(port);
     if (tty) {
             tty_vhangup(port->tty);
             tty_kref_put(tty);


> Fixes: 7c0408d80579 ("tty: add rpmsg driver")
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>

thanks,
-- 
js
suse labs