net: Check for fullsock in sock_i_uid()

PATCH In review Subash Abhinov Kasiviswanathan 3 participants 5 messages Nov 2, 2016 – Nov 3, 2016

Subsystems (3)

Networking Network drivers Sockets

lore

Share & tools

lore

Subash Abhinov Kasiviswanathan

author

2016-11-02 05:28 UTC #0 lore raw

PATCH 1/1 net: Check for fullsock in sock_i_uid()

sock_i_uid() acquires the sk_callback_lock which does not exist for sockets in TCP_NEW_SYN_RECV state. This results in errors showing up as spinlock bad magic.

Signed-off-by Subash Abhinov Kasiviswanathan <[email protected]>

Cc Eric Dumazet <[email protected]>

---
 net/core/sock.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

net/core/sock.c +4 -1

index c73e28f..af15ef0 100644
@@ -1727,7 +1727,10 @@ void sock_efree(struct sk_buff *skb)
 
 kuid_t sock_i_uid(struct sock *sk)
 {
-	kuid_t uid;
+	kuid_t uid = GLOBAL_ROOT_UID;
+
+	if (!sk_fullsock(sk))
+		return uid;
 
 	read_lock_bh(&sk->sk_callback_lock);
 	uid = sk->sk_socket ? SOCK_INODE(sk->sk_socket)->i_uid : GLOBAL_ROOT_UID;

Signature · 1 line

1.9.1

Eric Dumazet

2016-11-02 08:39 UTC #1 lore raw

Quoted 16 lines · Subash Abhinov Kasiviswanathan

sock_i_uid() acquires the sk_callback_lock which does not exist for sockets in TCP_NEW_SYN_RECV state. This results in errors showing up as spinlock bad magic.
Signed-off-by Subash Abhinov Kasiviswanathan <[email protected]>
Cc Eric Dumazet <[email protected]>
---
 net/core/sock.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
net/core/sock.c +0 -0
index c73e28f..af15ef0 100644
@@ -1727,7 +1727,10 @@ void sock_efree(struct sk_buff *skb)
 

net/core/sock.c void sock_efree(struct sk_buff *skb) · line 1736

Quoted 10 lines

net/core/sock.c +4 -1

 kuid_t sock_i_uid(struct sock *sk)
 {
-	kuid_t uid;
+	kuid_t uid = GLOBAL_ROOT_UID;
+
+	if (!sk_fullsock(sk))
+		return uid;
 
 	read_lock_bh(&sk->sk_callback_lock);
 	uid = sk->sk_socket ? SOCK_INODE(sk->sk_socket)->i_uid : GLOBAL_ROOT_UID;

This would be a bug in the caller.

Can you give us the complete stack trace leading to the problem you had ?

Thanks !

[email protected]

author

2016-11-02 17:05 UTC #2 lore raw

Quoted 6 lines

This would be a bug in the caller.
Can you give us the complete stack trace leading to the problem you had ?
Thanks !

Thanks Eric for the clarification. In that case, the bug is in the IDLETIMER target in Android kernel.
https://android.googlesource.com/kernel/common/+/android-4.4/net/netfilter/xt_IDLETIMER.c#356

Here is the call stack.

-003|rwlock_bug(?, ?)
-004|arch_read_lock(inline)
-004|do_raw_read_lock(lock = 0xFFFFFFC0354E79C8)
-005|raw_read_lock_bh(lock = 0xFFFFFFC0354E79C8)
-006|sock_i_uid(sk = 0xFFFFFFC0354E77B0)
-007|from_kuid_munged(inline)
-007|reset_timer(info = 0xFFFFFFC04D17D218, skb = 0xFFFFFFC018AB98C0)
-008|idletimer_tg_target(skb = 0xFFFFFFC018AB98C0, ?)
-009|ipt_do_table(skb = 0xFFFFFFC018AB98C0, state = 0xFFFFFFC0017E6F30, ?)
-010|iptable_mangle_hook(?, skb = 0xFFFFFFC018AB98C0, state = 0xFFFFFFC0017E6F30)
-011|nf_iterate(head = 0xFFFFFFC0019D55B8, skb = 0xFFFFFFC018AB98C0, state = 0xFFFFFFC0017E6F30, elemp =
-012|nf_hook_slow(skb = 0xFFFFFFC018AB98C0, state = 0xFFFFFFC0017E6F30)
-013|NF_HOOK_COND(inline)
-013|ip_output(net = 0xFFFFFFC0019D4B00, sk = 0xFFFFFFC0354E77B0, skb = 0xFFFFFFC018AB98C0)
-014|ip_local_out(net = 0xFFFFFFC0019D4B00, sk = 0xFFFFFFC0354E77B0, skb = 0xFFFFFFC018AB98C0)
-015|ip_build_and_send_pkt(skb = 0xFFFFFFC018AB98C0, sk = 0xFFFFFFC023F2E880, saddr = 1688053952, daddr =
-016|tcp_v4_send_synack(sk = 0xFFFFFFC023F2E880, ?, ?, req = 0xFFFFFFC0354E77B0, foc = 0xFFFFFFC0017E7110
-017|atomic_sub_return(inline)
-017|reqsk_put(inline)
-017|tcp_conn_request(?, af_ops = 0xFFFFFFC001080FC8, sk = 0xFFFFFFC023F2E880, ?)
-018|tcp_v4_conn_request(?, ?)
-019|tcp_rcv_state_process(sk = 0xFFFFFFC023F2E880, skb = 0xFFFFFFC018ABAD00)
-020|tcp_v4_do_rcv(sk = 0xFFFFFFC023F2E880, skb = 0xFFFFFFC018ABAD00)
-021|tcp_v4_rcv(skb = 0xFFFFFFC018ABAD00)
-022|ip_local_deliver_finish(net = 0xFFFFFFC0019D4B00, ?, skb = 0xFFFFFFC018ABAD00)
-023|NF_HOOK_THRESH(inline)
-023|NF_HOOK(inline)
-023|ip_local_deliver(skb = 0xFFFFFFC018ABAD00)
-024|ip_rcv_finish(net = 0xFFFFFFC0019D4B00, ?, skb = 0xFFFFFFC018ABAD00)
-025|NF_HOOK_THRESH(inline)
-025|NF_HOOK(inline)
-025|ip_rcv(skb = 0xFFFFFFC018ABAD00, dev = 0xFFFFFFC023474000, ?, ?)
-026|deliver_skb(inline)
-026|deliver_ptype_list_skb(inline)
-026|__netif_receive_skb_core(skb = 0x0A73, pfmemalloc = FALSE)
-027|__netif_receive_skb(skb = 0xFFFFFFC0BA455D40)
-028|netif_receive_skb_internal(skb = 0xFFFFFFC0BA455D40)
-029|netif_receive_skb(skb = 0xFFFFFFC0BA455D40)

Eric Dumazet

2016-11-02 17:18 UTC #3 lore raw

Quoted 5 lines · On Wed, 2016-11-02 at 11:05 -0600, [email protected] wrote:

Quoted 5 lines
This would be a bug in the caller.
Can you give us the complete stack trace leading to the problem you had ?

Quoted 7 lines

Quoted 1 line
Thanks !
Thanks Eric for the clarification. In that case, the bug is in the IDLETIMER target in Android kernel.
https://android.googlesource.com/kernel/common/+/android-4.4/net/netfilter/xt_IDLETIMER.c#356
Here is the call stack.

Sure, please fix Android, and not add ugly work around in linux kernel.

Lorenzo, have'nt you already fixed all these bugs ?

if (skb && skb->sk)

	timer->uid = from_kuid_munged(current_user_ns(),
				 sock_i_uid(skb_to_full_sk(skb)));

Thanks

Lorenzo Colitti

2016-11-03 17:34 UTC #4 lore raw

Quoted 1 line · Eric Dumazet

Lorenzo, have'nt you already fixed all these bugs ?

Not yet. There's still a fair bit of out-of-tree code left. Other than per-UID routing, xt_qtaguid is the big one, but there's also xt_quota2 and xt_idletimer. to fix.

Quoted 3 lines

if (skb && skb->sk)

        timer->uid = from_kuid_munged(current_user_ns(),
                                 sock_i_uid(skb_to_full_sk(skb)));

Thanks! I didn't know about skb_to_full_sk.