From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Google-Smtp-Source: ACJfBovflOBT0wb9pqEzwZouocbqUSEs2aUxPdMSQFxbFHXTPfUJrsIlpUcI8MyX1BCOWkh0N3RT ARC-Seal: i=1; a=rsa-sha256; t=1515137665; cv=none; d=google.com; s=arc-20160816; b=ujydiYdPYh06p/4DzutnTq6xLhaWqH8qA3RmuFvEgwIDyTU/dFKsgvKAUwStpaGuHw JOQOS4LXZh8Z9Nih0yPB4n+LbtINMI7WVyJDcMncQDkSmawaGDcqXGBCzX0ZWJJ5wFfF OE+EAwpIyaJz8Ia7d1Vq9AbKlzbPDfQpFQaBi3dLd6u228fmvU8K+sgqJXsTN+MCbp6S H46f6292a2p5v/mQySq9upB7WNyCHq7HwhhHlANrYIXH8rVf2D7xzRiLm+FH7v8rk0la D7wXKV4nSbTQIUst+GJ5G6X8kSrnBzCTPKkoAjQHBa9qQ7IP+2ND4bmKjeR9WCT18nKV bIWA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=content-language:in-reply-to:mime-version:user-agent:date :message-id:from:references:cc:to:subject:dmarc-filter :dkim-signature:dkim-signature:arc-authentication-results; bh=Ud6C1PeDgK/WRcfZIqga4YZuzOyxQE99DfOdDp9suFM=; b=aVmQ8nZp2OkJ9aOGGbo1N3BCD+jY3TjONz40QJ4imeTcZjAdBoJsqq8drL2ORyH/lH D11qQ4Dz/1JGvFWDqGyHxjRB2kXrEPl2624Zpve3+vzdyynMMVp6C6ZPBtJ9BxDoh3FQ dlppAeXp+iV2DFS8aNCETtERQ7ZFiNn5xRK0obR9npYm3mNKmeJAL3dgOpXe8VGHn3p+ cEamhpf05+Hby7ABkcB5vb2Zzwp9GUlnFnrd9DuWPOXgAx7p3l+1Tgy9pyQrFCRi9LFJ 0QeySAIIR3R8m2kowkSHAxNpN42i4s1+SEmmtTYDnOh7tq9cAKfH3Mthrxxj7TpCJOE1 rkNw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@codeaurora.org header.s=default header.b=ojcc0w6s; dkim=pass header.i=@codeaurora.org header.s=default header.b=NYiEJfAM; spf=pass (google.com: domain of gkohli@codeaurora.org designates 198.145.29.96 as permitted sender) smtp.mailfrom=gkohli@codeaurora.org Authentication-Results: mx.google.com; dkim=pass header.i=@codeaurora.org header.s=default header.b=ojcc0w6s; dkim=pass header.i=@codeaurora.org header.s=default header.b=NYiEJfAM; spf=pass (google.com: domain of gkohli@codeaurora.org designates 198.145.29.96 as permitted sender) smtp.mailfrom=gkohli@codeaurora.org DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 7AF44602B9 Authentication-Results: pdx-caf-mail.web.codeaurora.org; dmarc=none (p=none dis=none) header.from=codeaurora.org Authentication-Results: pdx-caf-mail.web.codeaurora.org; spf=none smtp.mailfrom=gkohli@codeaurora.org Subject: Re: [PATCH] tty: fix data race in n_tty_receive_buf_common To: Alan Cox Cc: jslaby@suse.com, gregkh@linuxfoundation.org, mikey@neuling.org, linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org References: <1514987332-14122-1-git-send-email-gkohli@codeaurora.org> <20180103193807.465e054e@alans-desktop> <0a456419-c836-08cf-070b-a254fb702b75@codeaurora.org> <20180104110920.169a1fe5@alans-desktop> <0dbd1f05-4c94-d1cc-3858-7bd4d38b9212@codeaurora.org> <20180104143716.5b09b1c7@alans-desktop> From: "Kohli, Gaurav" Message-ID: <93a7bd73-1123-90a7-b22d-02964ba29fb0@codeaurora.org> Date: Fri, 5 Jan 2018 13:04:19 +0530 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 MIME-Version: 1.0 In-Reply-To: <20180104143716.5b09b1c7@alans-desktop> Content-Type: multipart/alternative; boundary="------------F812ACCD698810D388F984E3" Content-Language: en-US X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: =?utf-8?q?1588579369033162085?= X-GMAIL-MSGID: =?utf-8?q?1588736992817246176?= X-Mailing-List: linux-kernel@vger.kernel.org List-ID: This is a multi-part message in MIME format. --------------F812ACCD698810D388F984E3 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit > >> 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. --------------F812ACCD698810D388F984E3 Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: 8bit 





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.

  


--------------F812ACCD698810D388F984E3--