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

[Jiri Slaby]

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

thanks,
--
js
suse labs