Re: [PATCH] tty: fix data race in n_tty_receive_buf_common

["Kohli, Gaurav" <gkohli@codeaurora.org>]

[-- Attachment #1: Type: text/plain, Size: 3291 bytes --]



>

>> Can you make that code available otherwise it's impossible to see what
>> the problem might be.

https://source.codeaurora.org/quic/la/kernel/msm-4.9/tree/drivers/tty/serial?h=msm-4.9 


As discussed , there not seems a problem as we are getting print request.
>>>     
>>> Ok no what I need to see is a trace of what each CPU is doing at the
>>> point you detect the problem. That way we can see what the path that
>>> races is.
>> Below is stack trace running by init in our case on one core
>> -006|n_tty_open(
>>       |    tty = 0xFFFFFFFF477AC880 -> (
>>       |      disc_data = 0xFFFFFF80197AD000,
>>
>>       |      port = 0xFFFFFFFFEDE40000))
>>       |  ldata = 0xFFFFFF80197AD000
>>
>>       |  trace_printk_fmt = 0xFFFFFF9F275125F8
>> -007|tty_ldisc_open.isra.3(
>>       |    tty = 0xFFFFFFFF477AC880)
>> -008|tty_ldisc_setup(
>>
>> -009|tty_init_dev(
>>       |    driver = 0xFFFFFFFFEDE2A480,
>>       |    idx = 0)
>>
>> -010|tty_open_by_driver(inline)
>> -010|tty_open(
> So core 1 is opening the tty from user space and that's a normal looking
> trace for an open of a port that was closed
>
>> Core 2:
>> -000|n_tty_receive_buf_common(
>>       |    tty = 0xFFFFFFFF477AC880,
>>
>>       |  ?)
>>       |  ldata_=_0x0
>>       |  __func__ = (110, 95, 116, 116, 121, 95, 114, 101, 99, 101, 105,
>> 118, 101, 95, 98, 117, 102, 95, 99, 111, 109, 109, 111, 110, 0)
>>       |  __u = (__val = 7079195495121566464, __c = (0))
>>       |  c = 127
>>       |  ldata = 0xFFFFFFFFF40DF97C
>>
>>       |  c = 0
>>       |  ldata = 0xFFFFFF9F26F46000
>>
>> -001|n_tty_receive_buf2(
>>       |    tty = 0xFFFFFFFF477AC880,
>>
>> -002|tty_ldisc_receive_buf(inline)
>> -002|receive_buf(inline)
>> -002|flush_to_ldisc(
> This is probably the important bit. As you say we are doing a flush to
> ldisc for a port even though it is not open.
>
> That's starting to make more sense. Becausee your driver is the console
> tty_port_shutdown doesn't stop everything (so console printk still
> works), and that means you can receive data and we have a window on
> reopening a tty that is only in use as a console where port->tty is valid
> but ldisc is not.
>
> I wonder what Jiri thinks but my first thougt is that tty_init_dev in
> fact needs to do
>
> 	tty_ldisc_lock(tty, 5 * HZ);
> 	tty_ldisc_setup(tty);
> 	tty_ldisc_unlock(tty)
>
> with the relevant error handling so that the flush_to_ldisc waits and
> either hits 'no ldisc' or 'ldisc valid'
>

But in above lock there is a chance, when flush_to_ldisc will occur 
first and acquired a lock in
tty_ldisc_ref

So this may fail, I am not much sure here, Please correct me if here i 
am missing something.

tty_ldisc_lock(tty, 5 * HZ);
	tty_ldisc_setup(tty);
	tty_ldisc_unlock(tty)


So can not we simply return from flush_to_ldisc ,when we know disc_data 
is not valid like
we are doing for tty and ldisc already?

if (tty->disc_data == NULL) {
                 tty_ldisc_deref(disc);
                 return;
         }

Regards
Gaurav


-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.


[-- Attachment #2: Type: text/html, Size: 5972 bytes --]