Messages in this thread | ![/](/images/icornerl.gif) | | Date | Tue, 14 Dec 2021 07:43:10 +0100 | Subject | Re: [PATCH] tty: rpmsg: Fix race condition releasing tty port | From | 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@foss.st.com>
thanks, -- js suse labs
| ![\](/images/icornerr.gif) |