netdev - Re: [Patch net] inet: fix sleeping inside inet_wait_for

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 01 Nov 2016 18:54:57 -0700
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Cong Wang <xiyou.wangcong@...il.com>
Cc:     netdev@...r.kernel.org, Andrey Konovalov <andreyknvl@...gle.com>,
        Peter Zijlstra <peterz@...radead.org>
Subject: Re: [Patch net] inet: fix sleeping inside inet_wait_for_connect()

On Tue, 2016-11-01 at 16:04 -0700, Cong Wang wrote:
> Andrey reported this kernel warning:

> Unlike commit 26cabd31259ba43f68026ce3f62b78094124333f
> ("sched, net: Clean up sk_wait_event() vs. might_sleep()"), the
> sleeping function is called before schedule_timeout(), this is indeed
> a bug. Fix this by moving the wait logic to the new API, it is similar
> to commit ff960a731788a7408b6f66ec4fd772ff18833211
> ("netdev, sched/wait: Fix sleeping inside wait event").
> 
> Reported-by: Andrey Konovalov <andreyknvl@...gle.com>
> Cc: Andrey Konovalov <andreyknvl@...gle.com>
> Cc: Eric Dumazet <eric.dumazet@...il.com>
> Cc: Peter Zijlstra <peterz@...radead.org>
> Signed-off-by: Cong Wang <xiyou.wangcong@...il.com>
> ---


Excellent.

I guess we could also define sk_wait_event_woken()
and use it instead of sk_wait_event(), and also in
inet_wait_for_connect()

+#define sk_wait_event_woken(__sk, __timeo, __condition, __wait)                \
+       ({      int __rc;                                               \
+               release_sock(__sk);                                     \
+               __rc = __condition;                                     \
+               if (!__rc) {                                            \
+                       *(__timeo) = wait_woken(__wait, TASK_INTERRUPTIBLE, \
+                                               *(__timeo));            \
+               }                                                       \
+               lock_sock(__sk);                                        \
+               __rc = __condition;                                     \
+               __rc;                                                   \
+       })

sk_wait_data() would need :

@@ -2078,14 +2080,14 @@ void __sk_flush_backlog(struct sock *sk)
  */
 int sk_wait_data(struct sock *sk, long *timeo, const struct sk_buff *skb)
 {
+       DEFINE_WAIT_FUNC(wait, woken_wake_function);
        int rc;
-       DEFINE_WAIT(wait);
 
-       prepare_to_wait(sk_sleep(sk), &wait, TASK_INTERRUPTIBLE);
+       add_wait_queue(sk_sleep(sk), &wait);
        sk_set_bit(SOCKWQ_ASYNC_WAITDATA, sk);
-       rc = sk_wait_event(sk, timeo, skb_peek_tail(&sk->sk_receive_queue) != skb);
+       rc = sk_wait_event_woken(sk, timeo, skb_peek_tail(&sk->sk_receive_queue) != skb, &wait);
        sk_clear_bit(SOCKWQ_ASYNC_WAITDATA, sk);
-       finish_wait(sk_sleep(sk), &wait);
+       remove_wait_queue(sk_sleep(sk), &wait);
        return rc;
 }
 EXPORT_SYMBOL(sk_wait_data);