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

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

[Arnaud Pouliquen]

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.

Fixes: 7c0408d80579 ("tty: add rpmsg driver")
Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
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);
+
+	tty_port_put(port);
 }
 
 static int rpmsg_tty_open(struct tty_struct *tty, struct file *filp)
@@ -106,12 +117,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)
@@ -137,8 +155,10 @@ static struct rpmsg_tty_port *rpmsg_tty_alloc_cport(void)
 	return cport;
 }
 
-static void rpmsg_tty_release_cport(struct rpmsg_tty_port *cport)
+static void rpmsg_tty_destruct_port(struct tty_port *port)
 {
+	struct rpmsg_tty_port *cport = container_of(port, struct rpmsg_tty_port, port);
+
 	mutex_lock(&idr_lock);
 	idr_remove(&tty_idr, cport->id);
 	mutex_unlock(&idr_lock);
@@ -146,7 +166,10 @@ static void rpmsg_tty_release_cport(struct rpmsg_tty_port *cport)
 	kfree(cport);
 }
 
-static const struct tty_port_operations rpmsg_tty_port_ops = { };
+static const struct tty_port_operations rpmsg_tty_port_ops = {
+	.destruct = rpmsg_tty_destruct_port,
+};
+
 
 static int rpmsg_tty_probe(struct rpmsg_device *rpdev)
 {
@@ -166,7 +189,8 @@ static int rpmsg_tty_probe(struct rpmsg_device *rpdev)
 					   cport->id, dev);
 	if (IS_ERR(tty_dev)) {
 		ret = dev_err_probe(dev, PTR_ERR(tty_dev), "Failed to register tty port\n");
-		goto err_destroy;
+		tty_port_put(&cport->port);
+		return ret;
 	}
 
 	cport->rpdev = rpdev;
@@ -177,28 +201,25 @@ static int rpmsg_tty_probe(struct rpmsg_device *rpdev)
 		rpdev->src, rpdev->dst, cport->id);
 
 	return 0;
-
-err_destroy:
-	tty_port_destroy(&cport->port);
-	rpmsg_tty_release_cport(cport);
-
-	return ret;
 }
 
 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[] = {
-- 
2.17.1

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

[Greg Kroah-Hartman]

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@foss.st.com>
> ---
> 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

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

[Arnaud POULIQUEN]

Hello Greg,


On 12/21/21 9:18 AM, Greg Kroah-Hartman wrote:
> 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?

Thanks for pointing this!

My first answer was that this is part of the fix to make the hangup synchronous.
But making more tests I'm not able to reproduce the reproduce the race issue
using tty_port_tty_hangup.

I don't master enough the TTY framework to know if using tty_vhangup is safer...
The difference between tty_vhangup and tty_hangup seems only that __tty_hangup
is directly called in tty_vhangup while a work is created in tty_hangup.

But after that tty_kref_put calls queue_release_one_tty making the rest of the
release asynchronous. And this last part of the release is the cause of the race
condition i observed.

So i propose to just drop this part and keep the use of tty_port_tty_hangup.

The alternative is to add it in a separate patch as you propose. But from now I
have not more rational.

Any advice is welcome!

> 
>>
>> Fixes: 7c0408d80579 ("tty: add rpmsg driver")
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>> ---
>> 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?

Over-protection i will suppress it.

Thanks and Regards,
Arnaud

> 
> thanks,
> 
> greg k-h
>