From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751628AbdBMENQ (ORCPT ); Sun, 12 Feb 2017 23:13:16 -0500 Received: from regular1.263xmail.com ([211.150.99.135]:52135 "EHLO regular1.263xmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750975AbdBMENM (ORCPT ); Sun, 12 Feb 2017 23:13:12 -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: X-ATTACHMENT-NUM: 0 X-DNS-TYPE: 0 Message-ID: <58A1324C.9070205@rock-chips.com> Date: Mon, 13 Feb 2017 12:13:00 +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 1/3] Bluetooth: bnep: fix possible might sleep error in bnep_session References: <1485230871-22828-1-git-send-email-jeffy.chen@rock-chips.com> <20170211014044.GB101282@google.com> In-Reply-To: <20170211014044.GB101282@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:40 AM, Brian Norris wrote: > Hi, > > On Tue, Jan 24, 2017 at 12:07:49PM +0800, Jeffy Chen wrote: >> It looks like bnep_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/bnep/core.c | 15 +++++++++------ >> 1 file changed, 9 insertions(+), 6 deletions(-) >> >> diff --git a/net/bluetooth/bnep/core.c b/net/bluetooth/bnep/core.c >> index fbf251f..da04d51 100644 >> --- a/net/bluetooth/bnep/core.c >> +++ b/net/bluetooth/bnep/core.c >> @@ -484,16 +484,16 @@ static int bnep_session(void *arg) >> struct net_device *dev = s->dev; >> struct sock *sk = s->sock->sk; >> struct sk_buff *skb; >> - wait_queue_t wait; >> + DEFINE_WAIT_FUNC(wait, woken_wake_function); >> >> BT_DBG(""); >> >> 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(&s->terminate)) >> break; >> @@ -515,9 +515,8 @@ static int bnep_session(void *arg) >> break; >> netif_wake_queue(dev); >> >> - schedule(); >> + wait_woken(&wait, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT); >> } >> - __set_current_state(TASK_RUNNING); >> remove_wait_queue(sk_sleep(sk), &wait); >> >> /* Cleanup session */ >> @@ -666,7 +665,11 @@ int bnep_del_connection(struct bnep_conndel_req *req) >> s = __bnep_get_session(req->dst); >> if (s) { >> atomic_inc(&s->terminate); >> - wake_up_process(s->task); >> + >> + /* Ensure session->terminate is updated */ >> + smp_mb__after_atomic(); >> + > __wake_up() suggests: > > * It may be assumed that this function implies a write memory barrier before > * changing the task state if and only if any tasks are woken up. > > so the above barrier is probably unnecessary. I'm not so sure about the > one before atomic_read(); seems fine. Got it, thanx! > > Other than that, I this looks ok: > > Reviewed-by: Brian Norris > > But I haven't been testing BNEP. > > Brian > >> + wake_up_interruptible(sk_sleep(s->sock->sk)); >> } else >> err = -ENOENT; >> >> -- >> 2.1.4 >> >> > >