Subject: Re: [PATCH] tty: rpmsg: Fix race condition releasing tty port
To:     Jiri Slaby <jirislaby@kernel.org>,
        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>
 <8bbd1a77-5f88-bb97-db88-6842df2e3e3c@kernel.org>
From:   Arnaud POULIQUEN <arnaud.pouliquen@foss.st.com>
Message-ID: <2008dd33-5d23-961c-ca61-53ff3e34479a@foss.st.com>
Date:   Tue, 14 Dec 2021 16:38:06 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.14.0
MIME-Version: 1.0
In-Reply-To: <8bbd1a77-5f88-bb97-db88-6842df2e3e3c@kernel.org>
Content-Type: text/plain; charset="utf-8"
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hello Jiri,

On 12/14/21 7:43 AM, Jiri Slaby wrote:
> 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?

Please could you detail what seems rather wrong for you? Everything or do the
tty_port port struct independently of the rpmsg_tty_port structure?

Concerning the tty_port refcounting:
Yes it also an option that I have already tried without success, before
implementing this patch.
That said, as you pointed it out, I reimplemented it today in another way, and
this time it seems that it works without any runtime warning or error.
I need to perform more test to confirm, then I will propose a V2 based on
tty_port refcountingt and the .destruct tty_port_operations.

> 
>> - 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);

I would like to first fix the issue in rpmsg_tty.c in separate thread.
But yes this should not take me too much time to propose this helper next.

Thanks,
Arnaud

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