netdev
[Top] [All Lists]

Re: Fwd: atomic_dec_and_test for child dst needed in dst_destroy?

To: "Christoph Lameter"@calsoftinc.com, netdev@xxxxxxxxxxx, davem@xxxxxxxxxxxxx
Subject: Re: Fwd: atomic_dec_and_test for child dst needed in dst_destroy?
From: pravin b shelar <pravins@xxxxxxxxxxxxxx>
Date: Wed, 06 Apr 2005 14:23:52 +0530
In-reply-to: <b82a8917050406002339f732ca@xxxxxxxxxxxxxx>
References: <Pine.LNX.4.58.0504051155260.13697@xxxxxxxxxxxxxxxxx> <E1DIvr8-0002yy-00@xxxxxxxxxxxxxxxxxxxxxxxx> <b82a8917050406002339f732ca@xxxxxxxxxxxxxx>
Sender: netdev-bounce@xxxxxxxxxxx
User-agent: Mozilla Thunderbird 1.0 (X11/20041206)

This is racy (albeit very unlikely) because dst may be freed by
dst_run_gc after the atomic_dec.

When we are not the real parent of the dst (e.g., when we're xfrm_dst
and the child is an rtentry), it may already be on the GC list.

In fact the current code is buggy to, we need to check dst->flags
before the dec as dst may no longer be valid afterwards.


This patch will remove the need for atomic_dec_and_test for this particular case.
Now we can break down the atomic_dec_and_test to atomic_dec & atomic_read.

Please comment.

Regards
Pravin.


Index: linux-2.6.11-dsta/net/core/dst.c
===================================================================
--- linux-2.6.11-dsta.orig/net/core/dst.c       2005-04-06 00:18:32.000000000 
-0700
+++ linux-2.6.11-dsta/net/core/dst.c    2005-04-06 01:31:10.000000000 -0700
@@ -198,17 +198,19 @@
 
        dst = child;
        if (dst) {
-               if (atomic_dec_and_test(&dst->__refcnt)) {
-                       /* We were real parent of this dst, so kill child. */
-                       if (dst->flags&DST_NOHASH)
-                               goto again;
-               } else {
-                       /* Child is still referenced, return it for freeing. */
-                       if (dst->flags&DST_NOHASH)
-                               return dst;
-                       /* Child is still in his hash table */
+               if (dst->flags&DST_NOHASH) {
+                       atomic_dec(&dst->__refcnt);
+                       if (atomic_read(&dst->__refcnt))
+                               /* Child is still referenced, return it for 
freeing. */
+                               return dst;     
+
+                       /*We were real parent of this dst, so kill child. */
+                       goto again;
                }
+               else /* Child is still in his hash table */
+                       atomic_dec(&dst->__refcnt);
        }
+
        return NULL;
 }
 
<Prev in Thread] Current Thread [Next in Thread>