On Wed, Dec 15, 2021 at 04:31:21PM +0100, Arnaud Pouliquen wrote:
> 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 while it is still 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 :
> - Introduce the .destruct port ops function to release the allocated
> rpmsg_tty_port structure.
> - Manages the tty port refcounting to trig the .destruct port ops,
> - Introduces the rpmsg_tty_cleanup function to ensure that the TTY is
> removed before decreasing the port refcount.
> - Uses tty_vhangup and tty_port_hangup instead of tty_port_tty_hangup.
Shouldn't this hangup change be a separate change?
>
> Fixes: 7c0408d80579 ("tty: add rpmsg driver")
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@xxxxxxxxxxx>
> ---
> delta vs V2: taking into account Jiri Slaby's comments:
> - Inline rpmsg_tty_release_cport in rpmsg_tty_destruct_port,
> - call tty_port_put in case of error in rpmsg_tty_probe,
> - use tty_port_get port return in rpmsg_tty_install to take into account
> NULL port return case.
>
> Applied and tested on fa55b7dcdc43 ("Linux 5.16-rc1", 2021-11-14)
> ---
> drivers/tty/rpmsg_tty.c | 49 +++++++++++++++++++++++++++++------------
> 1 file changed, 35 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/tty/rpmsg_tty.c b/drivers/tty/rpmsg_tty.c
> index dae2a4e44f38..cdc590c63f03 100644
> --- a/drivers/tty/rpmsg_tty.c
> +++ b/drivers/tty/rpmsg_tty.c
> @@ -50,10 +50,21 @@ static int rpmsg_tty_cb(struct rpmsg_device *rpdev, void *data, int len, void *p
> static int rpmsg_tty_install(struct tty_driver *driver, struct tty_struct *tty)
> {
> struct rpmsg_tty_port *cport = idr_find(&tty_idr, tty->index);
> + struct tty_port *port = tty->port;
>
> tty->driver_data = cport;
>
> - return tty_port_install(&cport->port, driver, tty);
> + port = tty_port_get(&cport->port);
> + return tty_port_install(port, driver, tty);
> +}
> +
> +static void rpmsg_tty_cleanup(struct tty_struct *tty)
> +{
> + struct tty_port *port = tty->port;
> +
> + WARN_ON(!port);
How can this ever trigger? Shouldn't you do something if it can?
thanks,
greg k-h
