OpenVZ Forum


Home » Mailing lists » Devel » [PATCH] Fix inet_diag.ko register vs rcv race
[PATCH] Fix inet_diag.ko register vs rcv race [message #23822] Tue, 27 November 2007 13:09 Go to next message
Pavel Emelianov is currently offline  Pavel Emelianov
Messages: 1149
Registered: September 2006
Senior Member
The following race is possible when one cpu unregisters the handler
while other one is trying to receive a message and call this one:

CPU1:                                                 CPU2:
inet_diag_rcv()                                       inet_diag_unregister()
  mutex_lock(&inet_diag_mutex);
  netlink_rcv_skb(skb, &inet_diag_rcv_msg);
    if (inet_diag_table[nlh->nlmsg_type] == 
                               NULL) /* false handler is still registered */
    ...
    netlink_dump_start(idiagnl, skb, nlh,
                           inet_diag_dump, NULL);
           cb = kzalloc(sizeof(*cb), GFP_KERNEL);
                   /* sleep here freeing memory 
                    * or preempt
                    * or sleep later on nlk->cb_mutex
                    */
                                                         spin_lock(&inet_diag_register_lock);
                                                         inet_diag_table[type] = NULL;
    ...                                                  spin_unlock(&inet_diag_register_lock);
                                                         synchronize_rcu();
                                                         /* CPU1 is sleeping - RCU quiescent
                                                          * state is passed
                                                          */
                                                         return;
    /* inet_diag_dump is finally called: */
    inet_diag_dump()
      handler = inet_diag_table[cb->nlh->nlmsg_type];
      BUG_ON(handler == NULL); 
      /* OOPS! While we slept the unregister has set
       * handler to NULL :(
       */

Grep showed, that the register/unregister functions are called
from init/fini module callbacks for tcp_/dccp_diag, so it's OK
to use the inet_diag_mutex to synchronize manipulations with the
inet_diag_table and the access to it.

Signed-off-by: Pavel Emelyanov <xemul@openvz.org>

---
diff --git a/net/ipv4/inet_diag.c b/net/ipv4/inet_diag.c
index b017073..5fe32d5 100644
--- a/net/ipv4/inet_diag.c
+++ b/net/ipv4/inet_diag.c
@@ -853,8 +853,6 @@ static void inet_diag_rcv(struct sk_buff *skb)
 	mutex_unlock(&inet_diag_mutex);
 }
 
-static DEFINE_SPINLOCK(inet_diag_register_lock);
-
 int inet_diag_register(const struct inet_diag_handler *h)
 {
 	const __u16 type = h->idiag_type;
@@ -863,13 +861,13 @@ int inet_diag_register(const struct inet_diag_handler *h)
 	if (type >= INET_DIAG_GETSOCK_MAX)
 		goto out;
 
-	spin_lock(&inet_diag_register_lock);
+	mutex_lock(&inet_diag_mutex);
 	err = -EEXIST;
 	if (inet_diag_table[type] == NULL) {
 		inet_diag_table[type] = h;
 		err = 0;
 	}
-	spin_unlock(&inet_diag_register_lock);
+	mutex_unlock(&inet_diag_mutex);
 out:
 	return err;
 }
@@ -882,11 +880,9 @@ void inet_diag_unregister(const struct inet_diag_handler *h)
 	if (type >= INET_DIAG_GETSOCK_MAX)
 		return;
 
-	spin_lock(&inet_diag_register_lock);
+	mutex_lock(&inet_diag_mutex);
 	inet_diag_table[type] = NULL;
-	spin_unlock(&inet_diag_register_lock);
-
-	synchronize_rcu();
+	mutex_unlock(&inet_diag_mutex);
 }
 EXPORT_SYMBOL_GPL(inet_diag_unregister);
Re: [PATCH] Fix inet_diag.ko register vs rcv race [message #23968 is a reply to message #23822] Thu, 29 November 2007 12:37 Go to previous messageGo to next message
Herbert Xu is currently offline  Herbert Xu
Messages: 45
Registered: April 2007
Member
On Tue, Nov 27, 2007 at 04:09:43PM +0300, Pavel Emelyanov wrote:
> The following race is possible when one cpu unregisters the handler
> while other one is trying to receive a message and call this one:

Good catch! But I think we need a bit more to close this fully.

Dumps can resume asynchronously which means that they won't be
holding inet_diag_mutex.  We can fix that pretty easily by
giving that as our cb_mutex.

So could you add that to your patch and resubmit?

Arnaldo, synchronize_rcu() doesn't work on its own.  Whoever accesses
the object that it's supposed to protect has to use the correct RCU
primitives for this to work.

Synchronisation is like tango, it always takes two to make it work :)

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH] Fix inet_diag.ko register vs rcv race [message #23970 is a reply to message #23968] Thu, 29 November 2007 12:47 Go to previous message
Arnaldo Carvalho de M is currently offline  Arnaldo Carvalho de M
Messages: 27
Registered: October 2007
Junior Member
Em Thu, Nov 29, 2007 at 11:37:34PM +1100, Herbert Xu escreveu:
> On Tue, Nov 27, 2007 at 04:09:43PM +0300, Pavel Emelyanov wrote:
> > The following race is possible when one cpu unregisters the handler
> > while other one is trying to receive a message and call this one:
> 
> Good catch! But I think we need a bit more to close this fully.
> 
> Dumps can resume asynchronously which means that they won't be
> holding inet_diag_mutex.  We can fix that pretty easily by
> giving that as our cb_mutex.
> 
> So could you add that to your patch and resubmit?
> 
> Arnaldo, synchronize_rcu() doesn't work on its own.  Whoever accesses
> the object that it's supposed to protect has to use the correct RCU
> primitives for this to work.
> 
> Synchronisation is like tango, it always takes two to make it work :)

Agreed, I didn't checked that when refactoring inet_diag, leaving this
as it was before I put my hands on it :-)

- Arnaldo
Previous Topic: [PATCH][BRIDGE] Lost call to br_fdb_fini() in br_init() error path
Next Topic: [PATCH] Nicer WARN_ON in netstat_show
Goto Forum:
  


Current Time: Wed Feb 28 10:32:35 GMT 2024

Total time taken to generate the page: 0.02418 seconds