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

[Arnaud POULIQUEN <arnaud.pouliquen@foss.st.com>]

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
>