lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Mon, 13 Feb 2017 12:14:37 +0800 From: jeffy <jeffy.chen@...k-chips.com> To: Brian Norris <briannorris@...omium.org> CC: linux-bluetooth@...r.kernel.org, Douglas Anderson <dianders@...omium.org>, Johan Hedberg <johan.hedberg@...el.com>, Peter Hurley <peter@...leysoftware.com>, Johan Hedberg <johan.hedberg@...il.com>, netdev@...r.kernel.org, linux-kernel@...r.kernel.org, "David S. Miller" <davem@...emloft.net>, Marcel Holtmann <marcel@...tmann.org>, Gustavo Padovan <gustavo@...ovan.org> Subject: Re: [PATCH 2/3] Bluetooth: cmtp: fix possible might sleep error in cmtp_session 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 <jeffy.chen@...k-chips.com> >> --- >> >> 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 <briannorris@...omium.org> > >> + >> + wake_up_interruptible(sk_sleep(session->sock->sk)); >> } else >> err = -ENOENT; >> >> -- >> 2.1.4 >> >> > >
Powered by blists - more mailing lists