Message ID | 20211214170646.25775-1-arnaud.pouliquen@foss.st.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | [v2] tty: rpmsg: Fix race condition releasing tty port | expand |
Hi, much better IMO. On 14. 12. 21, 18:06, 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: ... > diff --git a/drivers/tty/rpmsg_tty.c b/drivers/tty/rpmsg_tty.c > index dae2a4e44f38..69272ad92266 100644 > --- a/drivers/tty/rpmsg_tty.c > +++ b/drivers/tty/rpmsg_tty.c > @@ -53,9 +53,19 @@ static int rpmsg_tty_install(struct tty_driver *driver, struct tty_struct *tty) > > tty->driver_data = cport; > > + tty_port_get(&cport->port); Can't this fail? Like when racing with removal? > return tty_port_install(&cport->port, driver, tty); > } ... > static struct rpmsg_tty_port *rpmsg_tty_alloc_cport(void) > @@ -139,6 +156,8 @@ static struct rpmsg_tty_port *rpmsg_tty_alloc_cport(void) > > static void rpmsg_tty_release_cport(struct rpmsg_tty_port *cport) > { > + tty_port_destroy(&cport->port); > + You should not call tty_port_destroy when you use refcounting. The port is already destroyed when ->destruct() is called. (It has currently no bad effect calling it twice on a port though.) > @@ -146,7 +165,17 @@ static void rpmsg_tty_release_cport(struct rpmsg_tty_port *cport) > kfree(cport); > } > > -static const struct tty_port_operations rpmsg_tty_port_ops = { }; > +static void rpmsg_tty_destruct_port(struct tty_port *port) > +{ > + struct rpmsg_tty_port *cport = container_of(port, struct rpmsg_tty_port, port); > + > + rpmsg_tty_release_cport(cport); > +} > + > +static const struct tty_port_operations rpmsg_tty_port_ops = { > + .destruct = rpmsg_tty_destruct_port, > +}; > + > > static int rpmsg_tty_probe(struct rpmsg_device *rpdev) > { > @@ -179,7 +208,6 @@ static int rpmsg_tty_probe(struct rpmsg_device *rpdev) > return 0; > > err_destroy: > - tty_port_destroy(&cport->port); > rpmsg_tty_release_cport(cport); Couldn't you just put the port here? And inline rpmsg_tty_release_cport into the new rpmsg_tty_destruct_port? thanks,
On 12/15/21 7:49 AM, Jiri Slaby wrote: > Hi, > > much better IMO. > > On 14. 12. 21, 18:06, 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: > ... >> diff --git a/drivers/tty/rpmsg_tty.c b/drivers/tty/rpmsg_tty.c >> index dae2a4e44f38..69272ad92266 100644 >> --- a/drivers/tty/rpmsg_tty.c >> +++ b/drivers/tty/rpmsg_tty.c >> @@ -53,9 +53,19 @@ static int rpmsg_tty_install(struct tty_driver *driver, >> struct tty_struct *tty) >> tty->driver_data = cport; >> + tty_port_get(&cport->port); > > Can't this fail? Like when racing with removal? > >> return tty_port_install(&cport->port, driver, tty); >> } > ... >> static struct rpmsg_tty_port *rpmsg_tty_alloc_cport(void) >> @@ -139,6 +156,8 @@ static struct rpmsg_tty_port *rpmsg_tty_alloc_cport(void) >> static void rpmsg_tty_release_cport(struct rpmsg_tty_port *cport) >> { >> + tty_port_destroy(&cport->port); >> + > > You should not call tty_port_destroy when you use refcounting. The port is > already destroyed when ->destruct() is called. (It has currently no bad effect > calling it twice on a port though.) > >> @@ -146,7 +165,17 @@ static void rpmsg_tty_release_cport(struct rpmsg_tty_port >> *cport) >> kfree(cport); >> } >> -static const struct tty_port_operations rpmsg_tty_port_ops = { }; >> +static void rpmsg_tty_destruct_port(struct tty_port *port) >> +{ >> + struct rpmsg_tty_port *cport = container_of(port, struct rpmsg_tty_port, >> port); >> + >> + rpmsg_tty_release_cport(cport); >> +} >> + >> +static const struct tty_port_operations rpmsg_tty_port_ops = { >> + .destruct = rpmsg_tty_destruct_port, >> +}; >> + >> static int rpmsg_tty_probe(struct rpmsg_device *rpdev) >> { >> @@ -179,7 +208,6 @@ static int rpmsg_tty_probe(struct rpmsg_device *rpdev) >> return 0; >> err_destroy: >> - tty_port_destroy(&cport->port); >> rpmsg_tty_release_cport(cport); > > Couldn't you just put the port here? And inline rpmsg_tty_release_cport into the > new rpmsg_tty_destruct_port? > Thanks for all the insightful comments, V3 is coming. > thanks,
diff vs V1: - rework patch based on port refcounting. Applied and tested on fa55b7dcdc43 ("Linux 5.16-rc1", 2021-11-14) --- drivers/tty/rpmsg_tty.c | 43 +++++++++++++++++++++++++++++++++++------ 1 file changed, 37 insertions(+), 6 deletions(-) diff --git a/drivers/tty/rpmsg_tty.c b/drivers/tty/rpmsg_tty.c index dae2a4e44f38..69272ad92266 100644 --- a/drivers/tty/rpmsg_tty.c +++ b/drivers/tty/rpmsg_tty.c @@ -53,9 +53,19 @@ static int rpmsg_tty_install(struct tty_driver *driver, struct tty_struct *tty) tty->driver_data = cport; + tty_port_get(&cport->port); return tty_port_install(&cport->port, driver, tty); } +static void rpmsg_tty_cleanup(struct tty_struct *tty) +{ + struct tty_port *port = tty->port; + + WARN_ON(!port); + + tty_port_put(port); +} + static int rpmsg_tty_open(struct tty_struct *tty, struct file *filp) { return tty_port_open(tty->port, tty, filp); @@ -106,12 +116,19 @@ static unsigned int rpmsg_tty_write_room(struct tty_struct *tty) return size; } +static void rpmsg_tty_hangup(struct tty_struct *tty) +{ + tty_port_hangup(tty->port); +} + static const struct tty_operations rpmsg_tty_ops = { .install = rpmsg_tty_install, .open = rpmsg_tty_open, .close = rpmsg_tty_close, .write = rpmsg_tty_write, .write_room = rpmsg_tty_write_room, + .hangup = rpmsg_tty_hangup, + .cleanup = rpmsg_tty_cleanup, }; static struct rpmsg_tty_port *rpmsg_tty_alloc_cport(void) @@ -139,6 +156,8 @@ static struct rpmsg_tty_port *rpmsg_tty_alloc_cport(void) static void rpmsg_tty_release_cport(struct rpmsg_tty_port *cport) { + tty_port_destroy(&cport->port); + mutex_lock(&idr_lock); idr_remove(&tty_idr, cport->id); mutex_unlock(&idr_lock); @@ -146,7 +165,17 @@ static void rpmsg_tty_release_cport(struct rpmsg_tty_port *cport) kfree(cport); } -static const struct tty_port_operations rpmsg_tty_port_ops = { }; +static void rpmsg_tty_destruct_port(struct tty_port *port) +{ + struct rpmsg_tty_port *cport = container_of(port, struct rpmsg_tty_port, port); + + rpmsg_tty_release_cport(cport); +} + +static const struct tty_port_operations rpmsg_tty_port_ops = { + .destruct = rpmsg_tty_destruct_port, +}; + static int rpmsg_tty_probe(struct rpmsg_device *rpdev) { @@ -179,7 +208,6 @@ static int rpmsg_tty_probe(struct rpmsg_device *rpdev) return 0; err_destroy: - tty_port_destroy(&cport->port); rpmsg_tty_release_cport(cport); return ret; @@ -188,17 +216,20 @@ static int rpmsg_tty_probe(struct rpmsg_device *rpdev) static void rpmsg_tty_remove(struct rpmsg_device *rpdev) { struct rpmsg_tty_port *cport = dev_get_drvdata(&rpdev->dev); + struct tty_struct *tty; dev_dbg(&rpdev->dev, "Removing rpmsg tty device %d\n", cport->id); /* User hang up to release the tty */ - if (tty_port_initialized(&cport->port)) - tty_port_tty_hangup(&cport->port, false); + tty = tty_port_tty_get(&cport->port); + if (tty) { + tty_vhangup(tty); + tty_kref_put(tty); + } tty_unregister_device(rpmsg_tty_driver, cport->id); - tty_port_destroy(&cport->port); - rpmsg_tty_release_cport(cport); + tty_port_put(&cport->port); } static struct rpmsg_device_id rpmsg_driver_tty_id_table[] = {
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 : - manages the 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. - calls rpmsg_tty_release_cport function in the rpmsg_tty_destruct_port function instead of in the rpmsg_tty_remove function. - uses tty_vhangup and tty_port_hangup instead of tty_port_tty_hangup. Fixes: 7c0408d80579 ("tty: add rpmsg driver") Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com> ---