* [PATCH 1/2] dccp: Unlock sock before calling sk_free()
@ 2017-03-01 19:35 Arnaldo Carvalho de Melo
2017-03-01 19:35 ` [PATCH 2/2] net: Introduce sk_clone_lock() error path routine Arnaldo Carvalho de Melo
2017-03-02 21:59 ` [PATCH 1/2] dccp: Unlock sock before calling sk_free() David Miller
0 siblings, 2 replies; 5+ messages in thread
From: Arnaldo Carvalho de Melo @ 2017-03-01 19:35 UTC (permalink / raw)
To: David S . Miller
Cc: netdev, Arnaldo Carvalho de Melo, Cong Wang, Eric Dumazet,
Gerrit Renker, Thomas Gleixner
From: Arnaldo Carvalho de Melo <acme@redhat.com>
The code where sk_clone() came from created a new socket and locked it,
but then, on the error path didn't unlock it.
This problem stayed there for a long while, till b0691c8ee7c2 ("net:
Unlock sock before calling sk_free()") fixed it, but unfortunately the
callers of sk_clone() (now sk_clone_locked()) were not audited and the
one in dccp_create_openreq_child() remained.
Now in the age of the syskaller fuzzer, this was finally uncovered, as
reported by Dmitry:
---- 8< ----
I've got the following report while running syzkaller fuzzer on
86292b33d4b7 ("Merge branch 'akpm' (patches from Andrew)")
[ BUG: held lock freed! ]
4.10.0+ #234 Not tainted
-------------------------
syz-executor6/6898 is freeing memory
ffff88006286cac0-ffff88006286d3b7, with a lock still held there!
(slock-AF_INET6){+.-...}, at: [<ffffffff8362c2c9>] spin_lock
include/linux/spinlock.h:299 [inline]
(slock-AF_INET6){+.-...}, at: [<ffffffff8362c2c9>]
sk_clone_lock+0x3d9/0x12c0 net/core/sock.c:1504
5 locks held by syz-executor6/6898:
#0: (sk_lock-AF_INET6){+.+.+.}, at: [<ffffffff839a34b4>] lock_sock
include/net/sock.h:1460 [inline]
#0: (sk_lock-AF_INET6){+.+.+.}, at: [<ffffffff839a34b4>]
inet_stream_connect+0x44/0xa0 net/ipv4/af_inet.c:681
#1: (rcu_read_lock){......}, at: [<ffffffff83bc1c2a>]
inet6_csk_xmit+0x12a/0x5d0 net/ipv6/inet6_connection_sock.c:126
#2: (rcu_read_lock){......}, at: [<ffffffff8369b424>] __skb_unlink
include/linux/skbuff.h:1767 [inline]
#2: (rcu_read_lock){......}, at: [<ffffffff8369b424>] __skb_dequeue
include/linux/skbuff.h:1783 [inline]
#2: (rcu_read_lock){......}, at: [<ffffffff8369b424>]
process_backlog+0x264/0x730 net/core/dev.c:4835
#3: (rcu_read_lock){......}, at: [<ffffffff83aeb5c0>]
ip6_input_finish+0x0/0x1700 net/ipv6/ip6_input.c:59
#4: (slock-AF_INET6){+.-...}, at: [<ffffffff8362c2c9>] spin_lock
include/linux/spinlock.h:299 [inline]
#4: (slock-AF_INET6){+.-...}, at: [<ffffffff8362c2c9>]
sk_clone_lock+0x3d9/0x12c0 net/core/sock.c:1504
Fix it just like was done by b0691c8ee7c2 ("net: Unlock sock before calling
sk_free()").
Reported-by: Dmitry Vyukov <dvyukov@google.com>
Cc: Cong Wang <xiyou.wangcong@gmail.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Gerrit Renker <gerrit@erg.abdn.ac.uk>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/20170301153510.GE15145@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
net/dccp/minisocks.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/dccp/minisocks.c b/net/dccp/minisocks.c
index 53eddf99e4f6..d20d948a98ed 100644
--- a/net/dccp/minisocks.c
+++ b/net/dccp/minisocks.c
@@ -122,6 +122,7 @@ struct sock *dccp_create_openreq_child(const struct sock *sk,
/* It is still raw copy of parent, so invalidate
* destructor and make plain sk_free() */
newsk->sk_destruct = NULL;
+ bh_unlock_sock(newsk);
sk_free(newsk);
return NULL;
}
--
2.9.3
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/2] net: Introduce sk_clone_lock() error path routine
2017-03-01 19:35 [PATCH 1/2] dccp: Unlock sock before calling sk_free() Arnaldo Carvalho de Melo
@ 2017-03-01 19:35 ` Arnaldo Carvalho de Melo
2017-03-02 21:59 ` David Miller
2017-03-03 9:34 ` Sergei Shtylyov
2017-03-02 21:59 ` [PATCH 1/2] dccp: Unlock sock before calling sk_free() David Miller
1 sibling, 2 replies; 5+ messages in thread
From: Arnaldo Carvalho de Melo @ 2017-03-01 19:35 UTC (permalink / raw)
To: David S . Miller
Cc: netdev, Arnaldo Carvalho de Melo, Cong Wang, Dmitry Vyukov,
Eric Dumazet, Gerrit Renker, Thomas Gleixner
From: Arnaldo Carvalho de Melo <acme@redhat.com>
When handling problems in cloning a socket with the sk_clone_locked()
function we need to perform several steps that were open coded in it and
its callers, so introduce a routine to avoid this duplication:
sk_free_unlock_clone().
Cc: Cong Wang <xiyou.wangcong@gmail.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Gerrit Renker <gerrit@erg.abdn.ac.uk>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/n/net-ui6laqkotycunhtmqryl9bfx@git.kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
include/net/sock.h | 1 +
net/core/sock.c | 16 +++++++++++-----
net/dccp/minisocks.c | 6 +-----
3 files changed, 13 insertions(+), 10 deletions(-)
diff --git a/include/net/sock.h b/include/net/sock.h
index c4f5e6fca17c..93d1160bcd32 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1520,6 +1520,7 @@ struct sock *sk_alloc(struct net *net, int family, gfp_t priority,
void sk_free(struct sock *sk);
void sk_destruct(struct sock *sk);
struct sock *sk_clone_lock(const struct sock *sk, const gfp_t priority);
+void sk_free_unlock_clone(struct sock *sk);
struct sk_buff *sock_wmalloc(struct sock *sk, unsigned long size, int force,
gfp_t priority);
diff --git a/net/core/sock.c b/net/core/sock.c
index 4eca27dc5c94..a3d9bb20f65d 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -1540,11 +1540,7 @@ struct sock *sk_clone_lock(const struct sock *sk, const gfp_t priority)
is_charged = sk_filter_charge(newsk, filter);
if (unlikely(!is_charged || xfrm_sk_clone_policy(newsk, sk))) {
- /* It is still raw copy of parent, so invalidate
- * destructor and make plain sk_free() */
- newsk->sk_destruct = NULL;
- bh_unlock_sock(newsk);
- sk_free(newsk);
+ sk_free_unlock_clone(newsk);
newsk = NULL;
goto out;
}
@@ -1593,6 +1589,16 @@ struct sock *sk_clone_lock(const struct sock *sk, const gfp_t priority)
}
EXPORT_SYMBOL_GPL(sk_clone_lock);
+void sk_free_unlock_clone(struct sock *sk)
+{
+ /* It is still raw copy of parent, so invalidate
+ * destructor and make plain sk_free() */
+ sk->sk_destruct = NULL;
+ bh_unlock_sock(sk);
+ sk_free(sk);
+}
+EXPORT_SYMBOL_GPL(sk_free_unlock_clone);
+
void sk_setup_caps(struct sock *sk, struct dst_entry *dst)
{
u32 max_segs = 1;
diff --git a/net/dccp/minisocks.c b/net/dccp/minisocks.c
index d20d948a98ed..e267e6f4c9a5 100644
--- a/net/dccp/minisocks.c
+++ b/net/dccp/minisocks.c
@@ -119,11 +119,7 @@ struct sock *dccp_create_openreq_child(const struct sock *sk,
* Activate features: initialise CCIDs, sequence windows etc.
*/
if (dccp_feat_activate_values(newsk, &dreq->dreq_featneg)) {
- /* It is still raw copy of parent, so invalidate
- * destructor and make plain sk_free() */
- newsk->sk_destruct = NULL;
- bh_unlock_sock(newsk);
- sk_free(newsk);
+ sk_free_unlock_clone(newsk);
return NULL;
}
dccp_init_xmit_timers(newsk);
--
2.9.3
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] dccp: Unlock sock before calling sk_free()
2017-03-01 19:35 [PATCH 1/2] dccp: Unlock sock before calling sk_free() Arnaldo Carvalho de Melo
2017-03-01 19:35 ` [PATCH 2/2] net: Introduce sk_clone_lock() error path routine Arnaldo Carvalho de Melo
@ 2017-03-02 21:59 ` David Miller
1 sibling, 0 replies; 5+ messages in thread
From: David Miller @ 2017-03-02 21:59 UTC (permalink / raw)
To: acme; +Cc: netdev, acme, xiyou.wangcong, edumazet, gerrit, tglx
From: Arnaldo Carvalho de Melo <acme@kernel.org>
Date: Wed, 1 Mar 2017 16:35:07 -0300
> From: Arnaldo Carvalho de Melo <acme@redhat.com>
>
> The code where sk_clone() came from created a new socket and locked it,
> but then, on the error path didn't unlock it.
>
> This problem stayed there for a long while, till b0691c8ee7c2 ("net:
> Unlock sock before calling sk_free()") fixed it, but unfortunately the
> callers of sk_clone() (now sk_clone_locked()) were not audited and the
> one in dccp_create_openreq_child() remained.
>
> Now in the age of the syskaller fuzzer, this was finally uncovered, as
> reported by Dmitry:
...
> Fix it just like was done by b0691c8ee7c2 ("net: Unlock sock before calling
> sk_free()").
>
> Reported-by: Dmitry Vyukov <dvyukov@google.com>
> Cc: Cong Wang <xiyou.wangcong@gmail.com>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Gerrit Renker <gerrit@erg.abdn.ac.uk>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Link: http://lkml.kernel.org/r/20170301153510.GE15145@kernel.org
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Applied and queued up for -stable.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] net: Introduce sk_clone_lock() error path routine
2017-03-01 19:35 ` [PATCH 2/2] net: Introduce sk_clone_lock() error path routine Arnaldo Carvalho de Melo
@ 2017-03-02 21:59 ` David Miller
2017-03-03 9:34 ` Sergei Shtylyov
1 sibling, 0 replies; 5+ messages in thread
From: David Miller @ 2017-03-02 21:59 UTC (permalink / raw)
To: acme; +Cc: netdev, acme, xiyou.wangcong, dvyukov, edumazet, gerrit, tglx
From: Arnaldo Carvalho de Melo <acme@kernel.org>
Date: Wed, 1 Mar 2017 16:35:08 -0300
> From: Arnaldo Carvalho de Melo <acme@redhat.com>
>
> When handling problems in cloning a socket with the sk_clone_locked()
> function we need to perform several steps that were open coded in it and
> its callers, so introduce a routine to avoid this duplication:
> sk_free_unlock_clone().
>
> Cc: Cong Wang <xiyou.wangcong@gmail.com>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Gerrit Renker <gerrit@erg.abdn.ac.uk>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Link: http://lkml.kernel.org/n/net-ui6laqkotycunhtmqryl9bfx@git.kernel.org
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Applied.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] net: Introduce sk_clone_lock() error path routine
2017-03-01 19:35 ` [PATCH 2/2] net: Introduce sk_clone_lock() error path routine Arnaldo Carvalho de Melo
2017-03-02 21:59 ` David Miller
@ 2017-03-03 9:34 ` Sergei Shtylyov
1 sibling, 0 replies; 5+ messages in thread
From: Sergei Shtylyov @ 2017-03-03 9:34 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, David S . Miller
Cc: netdev, Arnaldo Carvalho de Melo, Cong Wang, Dmitry Vyukov,
Eric Dumazet, Gerrit Renker, Thomas Gleixner
Hello!
On 3/1/2017 10:35 PM, Arnaldo Carvalho de Melo wrote:
> From: Arnaldo Carvalho de Melo <acme@redhat.com>
>
> When handling problems in cloning a socket with the sk_clone_locked()
> function we need to perform several steps that were open coded in it and
> its callers, so introduce a routine to avoid this duplication:
> sk_free_unlock_clone().
>
> Cc: Cong Wang <xiyou.wangcong@gmail.com>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Gerrit Renker <gerrit@erg.abdn.ac.uk>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Link: http://lkml.kernel.org/n/net-ui6laqkotycunhtmqryl9bfx@git.kernel.org
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
[...]
> diff --git a/net/core/sock.c b/net/core/sock.c
> index 4eca27dc5c94..a3d9bb20f65d 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -1540,11 +1540,7 @@ struct sock *sk_clone_lock(const struct sock *sk, const gfp_t priority)
> is_charged = sk_filter_charge(newsk, filter);
>
> if (unlikely(!is_charged || xfrm_sk_clone_policy(newsk, sk))) {
> - /* It is still raw copy of parent, so invalidate
> - * destructor and make plain sk_free() */
> - newsk->sk_destruct = NULL;
> - bh_unlock_sock(newsk);
> - sk_free(newsk);
> + sk_free_unlock_clone(newsk);
> newsk = NULL;
> goto out;
> }
> @@ -1593,6 +1589,16 @@ struct sock *sk_clone_lock(const struct sock *sk, const gfp_t priority)
> }
> EXPORT_SYMBOL_GPL(sk_clone_lock);
>
> +void sk_free_unlock_clone(struct sock *sk)
> +{
> + /* It is still raw copy of parent, so invalidate
> + * destructor and make plain sk_free() */
Could fix the comment style to the preferred one, while at it:
/* bla
* bla
*/
> + sk->sk_destruct = NULL;
> + bh_unlock_sock(sk);
> + sk_free(sk);
> +}
> +EXPORT_SYMBOL_GPL(sk_free_unlock_clone);
> +
> void sk_setup_caps(struct sock *sk, struct dst_entry *dst)
> {
> u32 max_segs = 1;
[...]
MBR, Sergei
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-03-03 11:23 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-01 19:35 [PATCH 1/2] dccp: Unlock sock before calling sk_free() Arnaldo Carvalho de Melo
2017-03-01 19:35 ` [PATCH 2/2] net: Introduce sk_clone_lock() error path routine Arnaldo Carvalho de Melo
2017-03-02 21:59 ` David Miller
2017-03-03 9:34 ` Sergei Shtylyov
2017-03-02 21:59 ` [PATCH 1/2] dccp: Unlock sock before calling sk_free() David Miller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.