From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751827AbdBMEOy (ORCPT ); Sun, 12 Feb 2017 23:14:54 -0500 Received: from regular1.263xmail.com ([211.150.99.130]:52664 "EHLO regular1.263xmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750994AbdBMEOw (ORCPT ); Sun, 12 Feb 2017 23:14:52 -0500 X-263anti-spam: KSV:0;BIG:0; X-MAIL-GRAY: 0 X-MAIL-DELIVERY: 1 X-KSVirus-check: 0 X-ADDR-CHECKED4: 1 X-ABS-CHECKED: 1 X-SKE-CHECKED: 1 X-RL-SENDER: jeffy.chen@rock-chips.com X-FST-TO: briannorris@chromium.org X-SENDER-IP: 103.29.142.67 X-LOGIN-NAME: jeffy.chen@rock-chips.com X-UNIQUE-TAG: <3051f99543b0da7e1a66ed75b0804b0b> X-ATTACHMENT-NUM: 0 X-DNS-TYPE: 0 Message-ID: <58A132AD.9010808@rock-chips.com> Date: Mon, 13 Feb 2017 12:14:37 +0800 From: jeffy User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:19.0) Gecko/20130126 Thunderbird/19.0 MIME-Version: 1.0 To: Brian Norris CC: linux-bluetooth@vger.kernel.org, Douglas Anderson , Johan Hedberg , Peter Hurley , Johan Hedberg , netdev@vger.kernel.org, linux-kernel@vger.kernel.org, "David S. Miller" , Marcel Holtmann , Gustavo Padovan Subject: Re: [PATCH 2/3] Bluetooth: cmtp: fix possible might sleep error in cmtp_session References: <1485230871-22828-1-git-send-email-jeffy.chen@rock-chips.com> <1485230871-22828-2-git-send-email-jeffy.chen@rock-chips.com> <20170211014247.GC101282@google.com> In-Reply-To: <20170211014247.GC101282@google.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi brian, On 02/11/2017 09:43 AM, Brian Norris wrote: > Hi, > > On Tue, Jan 24, 2017 at 12:07:50PM +0800, Jeffy Chen wrote: >> It looks like cmtp_session has same pattern as the issue reported in >> old rfcomm: >> >> while (1) { >> set_current_state(TASK_INTERRUPTIBLE); >> if (condition) >> break; >> // may call might_sleep here >> schedule(); >> } >> __set_current_state(TASK_RUNNING); >> >> Which fixed at: >> dfb2fae Bluetooth: Fix nested sleeps >> >> So let's fix it at the same way, also follow the suggestion of: >> https://lwn.net/Articles/628628/ >> >> Signed-off-by: Jeffy Chen >> --- >> >> net/bluetooth/cmtp/core.c | 21 ++++++++++++++------- >> 1 file changed, 14 insertions(+), 7 deletions(-) >> >> diff --git a/net/bluetooth/cmtp/core.c b/net/bluetooth/cmtp/core.c >> index 9e59b66..6b03f2b 100644 >> --- a/net/bluetooth/cmtp/core.c >> +++ b/net/bluetooth/cmtp/core.c >> @@ -280,16 +280,16 @@ static int cmtp_session(void *arg) >> struct cmtp_session *session = arg; >> struct sock *sk = session->sock->sk; >> struct sk_buff *skb; >> - wait_queue_t wait; >> + DEFINE_WAIT_FUNC(wait, woken_wake_function); >> >> BT_DBG("session %p", session); >> >> set_user_nice(current, -15); >> >> - init_waitqueue_entry(&wait, current); >> add_wait_queue(sk_sleep(sk), &wait); >> while (1) { >> - set_current_state(TASK_INTERRUPTIBLE); >> + /* Ensure session->terminate is updated */ >> + smp_mb__before_atomic(); >> >> if (atomic_read(&session->terminate)) >> break; >> @@ -306,9 +306,8 @@ static int cmtp_session(void *arg) >> >> cmtp_process_transmit(session); >> >> - schedule(); >> + wait_woken(&wait, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT); >> } >> - __set_current_state(TASK_RUNNING); >> remove_wait_queue(sk_sleep(sk), &wait); >> >> down_write(&cmtp_session_sem); >> @@ -393,7 +392,11 @@ int cmtp_add_connection(struct cmtp_connadd_req *req, struct socket *sock) >> err = cmtp_attach_device(session); >> if (err < 0) { >> atomic_inc(&session->terminate); >> - wake_up_process(session->task); >> + >> + /* Ensure session->terminate is updated */ >> + smp_mb__after_atomic(); >> + > Same comment about the barrier. Done, there are barriers in wake functions indeed, thanx! > >> + wake_up_interruptible(sk_sleep(session->sock->sk)); >> up_write(&cmtp_session_sem); >> return err; >> } >> @@ -431,7 +434,11 @@ int cmtp_del_connection(struct cmtp_conndel_req *req) >> >> /* Stop session thread */ >> atomic_inc(&session->terminate); >> - wake_up_process(session->task); >> + >> + /* Ensure session->terminate is updated */ >> + smp_mb__after_atomic(); > And again. > > But otherwise I think this looks OK, again with the caveat that I don't > know Bluetooth/CMTP that well: > > Reviewed-by: Brian Norris > >> + >> + wake_up_interruptible(sk_sleep(session->sock->sk)); >> } else >> err = -ENOENT; >> >> -- >> 2.1.4 >> >> > >