netdev - Re: [PATCH 2/2] packet: convert socket list to RCU

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] [day] [month] [year] [list]

Message-ID: <1266592584.3136.41.camel@edumazet-laptop>
Date:	Fri, 19 Feb 2010 16:16:24 +0100
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Stephen Hemminger <shemminger@...tta.com>
Cc:	"David S. Miller" <davem@...emloft.net>,
	Li Zefan <lizf@...fujitsu.com>, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] packet: convert socket list to RCU

Le jeudi 18 février 2010 à 21:41 -0800, Stephen Hemminger a écrit :

> Convert AF_PACKET to use RCU, eliminating one more reader/writer lock.
> 
> I needed to create some minor additional socket list RCU infrastructure
> to make this work. Note: there is no need for a real sk_del_node_init_rcu(), 
> because sk_del_node_init is doing the equivalent thing to 
> hlst_del_init_rcu already; but added some comments to try and make that obvious.
> 
> Signed-off-by: Stephen Hemminger <shemminger@...tta.com>
> 

Stephen, I am a bit worried by the interaction between packet_release()
and packet_notifier()

With your version, packet_notifier() can run and let another cpu run
packet_release() un-contented. Both cpus could manipulate same po (and
particularly po->running)

Before your patch, the read_lock() done in packet_notifier() was
preventing packet_release() runnning at the same time.

Maybe packet_release() should lock po->bind_lock before manipulating
po->running, avoiding a refcount error.

Something like this preliminary patch :

[PATCH] packet: fix a race in packet_release

packet_release() has a potential race with packet_notifier(NETDEV_DOWN),
leading to a double __sock_put(). (dev_remove_pack() is safe)

Fix is to always use po->bind_lock before accessing po->running

Signed-off-by: Eric Dumazet <eric.dumazet@...il.com>
---
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 10f7295..b706031 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -1271,15 +1271,15 @@ static int packet_release(struct socket *sock)
 	 *	Unhook packet receive handler.
 	 */
 
+	spin_lock(&po->bind_lock);
 	if (po->running) {
-		/*
-		 *	Remove the protocol hook
-		 */
-		dev_remove_pack(&po->prot_hook);
+		__sock_put(sk);
 		po->running = 0;
 		po->num = 0;
-		__sock_put(sk);
-	}
+		spin_unlock(&po->bind_lock);
+		dev_remove_pack(&po->prot_hook);
+	} else
+		spin_unlock(&po->bind_lock);
 
 	packet_flush_mclist(sk);
 


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html