Re: [PATCH] tty: rpmsg: Fix race condition releasing tty port

[Arnaud POULIQUEN]

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@xxxxxxxxxxx>
> 
> thanks,