Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1578917 > unrolled thread
| Started by | Brian Norris <briannorris@chromium.org> |
|---|---|
| First post | 2017-02-11 02:50 +0100 |
| Last post | 2017-02-13 05:20 +0100 |
| Articles | 2 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH 1/3] Bluetooth: bnep: fix possible might sleep error in bnep_session Brian Norris <briannorris@chromium.org> - 2017-02-11 02:50 +0100
Re: [PATCH 1/3] Bluetooth: bnep: fix possible might sleep error in bnep_session jeffy <jeffy.chen@rock-chips.com> - 2017-02-13 05:20 +0100
| From | Brian Norris <briannorris@chromium.org> |
|---|---|
| Date | 2017-02-11 02:50 +0100 |
| Subject | Re: [PATCH 1/3] Bluetooth: bnep: fix possible might sleep error in bnep_session |
| Message-ID | <t9uZ3-2Mc-15@gated-at.bofh.it> |
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 <jeffy.chen@rock-chips.com>
> ---
>
> 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.
Other than that, I this looks ok:
Reviewed-by: Brian Norris <briannorris@chromium.or>
But I haven't been testing BNEP.
Brian
> + wake_up_interruptible(sk_sleep(s->sock->sk));
> } else
> err = -ENOENT;
>
> --
> 2.1.4
>
>
[toc] | [next] | [standalone]
| From | jeffy <jeffy.chen@rock-chips.com> |
|---|---|
| Date | 2017-02-13 05:20 +0100 |
| Message-ID | <taghj-6YW-13@gated-at.bofh.it> |
| In reply to | #1578917 |
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 <jeffy.chen@rock-chips.com>
>> ---
>>
>> 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 <briannorris@chromium.or>
>
> But I haven't been testing BNEP.
>
> Brian
>
>> + wake_up_interruptible(sk_sleep(s->sock->sk));
>> } else
>> err = -ENOENT;
>>
>> --
>> 2.1.4
>>
>>
>
>
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web