xia/lpm: remove routing/forwarding deadlock #17
Conversation
|
Hi Cody, I found a rare bug that can occur at local_newroute() or main_newroute(). Consider the following execution:
|
cjdoucette
force-pushed
the
lpm-deadlock
branch
2 times, most recently
from
7046281 to
2f3ca73
Compare
|
Thanks, Michel. I just rebased so that the patch has |
Due to the way that locking was written into this toy LPM implementation,
the read-only code in lpm_deliver() obtains a *write* lock on the tree FIB.
The effect of this is the unnatural combination of first obtaining an RCU
read lock and then a FIB write lock in lpm_deliver(). This conflicts with
functions like local_newroute(), which first obtain a FIB write lock and
then wait for RCU readers to finish before flushing anchors. The problem
is that deadlock can occur with the following interleaving:
thread1 thread2
======= =======
local_newroute(): get write lock
lpm_deliver(): get RCU read lock
local_newroute(): wait for RCU readers
lpm_deliver(): wait for write lock
Notice that this deadlock cannot occur if lpm_deliver() gets the write
lock first.
To fix this, we can duplicate the FIB entry whose anchor needs to be
flushed, replace the old entry with the duplicate, and then release the
lock. This allows writers of the tree, including the code in lpm_deliver(),
to proceed. We then wait an RCU synchronization to flush the old entry's
anchor, since routing mechanism code (not involved with the LPM tree)
may still be reading that entry. Once all these readers are done, the old
entry can be reclaimed.
This removes the deadlock, but future iterations of the LPM principal
should use RCU instead of rwlocks to avoid this unnatural locking.
Since the underlying structure of a FIB entry is not known to the principal, it cannot copy a FIB entry. This patch adds a function to every FIB that makes a copy of a FIB entry that includes FIB-specific data. This patch also fixes a bug in the LPM principal by making use of the new copy function.
cjdoucette
force-pushed
the
lpm-deadlock
branch
from
2f3ca73 to
407b4ff
Compare
|
The pull request is ready for review again. I separated the other bug found into a separate patch, since it is a distinct change that affected multiple files. |
|
Patch xia/lpm: remove routing/forwarding deadlock is ready for merge. |
| * should reference FIB entries previously allocated by | ||
| * fxid_ppal_alloc(). | ||
| */ | ||
| void (*fxid_copy)(struct fib_xid *old_fxid, struct fib_xid *new_fxid); |
There was a problem hiding this comment.
Would you mind swapping the position of the parameters? The assigning order is followed by the C library, and it bothers to go against it. A few examples: memcpy(), memmove(), and strcpy().
| @@ -171,6 +171,12 @@ static void list_fxid_init(struct fib_xid *fxid, int table_id, int entry_type) | |||
| fxid->dead.xtbl = NULL; | |||
| } | |||
|
|
|||
| static void list_fxid_copy(struct fib_xid *old_fxid, struct fib_xid *new_fxid) | |||
| { | |||
| memcpy(new_fxid, old_fxid, | |||
There was a problem hiding this comment.
Although it's unlikely that new_fxid == old_fxid, it would be safer to replace memcpy() with memmove().
| @@ -105,6 +105,7 @@ static int newroute_flush_anchor_unlock(struct fib_xid_table *xtbl, | |||
| * node with the new one in the tree. | |||
| */ | |||
| xdst_init_anchor(&dup_llpm->anchor); | |||
| lpm_rt_iops->fxid_copy(&dup_llpm->common, pred_fxid); | |||
There was a problem hiding this comment.
If you've inverted the parameters of fxid_copy() above, you fixed a bug here.
| @@ -136,6 +137,7 @@ static int newroute_flush_anchor_unlock(struct fib_xid_table *xtbl, | |||
| } | |||
|
|
|||
| dup_mrd->gw = fxid_mrd(pred_fxid)->gw; | |||
| lpm_rt_iops->fxid_copy(&dup_mrd->common, pred_fxid); | |||
There was a problem hiding this comment.
If you've inverted the parameters of fxid_copy() above, you fixed another bug here.
| @@ -109,6 +109,12 @@ static void tree_fxid_init(struct fib_xid *fxid, int table_id, int entry_type) | |||
| fxid->dead.xtbl = NULL; | |||
| } | |||
|
|
|||
| static void tree_fxid_copy(struct fib_xid *old_fxid, struct fib_xid *new_fxid) | |||
| { | |||
| memcpy(new_fxid, old_fxid, | |||
There was a problem hiding this comment.
Although it's unlikely that new_fxid == old_fxid, it would be safer to replace memcpy() with memmove().
Due to the way that locking was written into this toy LPM implementation,
the read-only code in lpm_deliver() obtains a write lock on the tree FIB.
The effect of this is the unnatural combination of first obtaining an RCU
read lock and then a FIB write lock in lpm_deliver(). This conflicts with
functions like local_newroute(), which first obtain a FIB write lock and
then wait for RCU readers to finish before flushing anchors. The problem
is that deadlock can occur with the following interleaving:
Notice that this deadlock cannot occur if lpm_deliver() gets the write
lock first.
To fix this, we can duplicate the FIB entry whose anchor needs to be
flushed, replace the old entry with the duplicate, and then release the
lock. This allows writers of the tree, including the code in lpm_deliver(),
to proceed. We then wait an RCU synchronization to flush the old entry's
anchor, since routing mechanism code (not involved with the LPM tree)
may still be reading that entry. Once all these readers are done, the old
entry can be reclaimed.
This removes the deadlock, but future iterations of the LPM principal
should use RCU instead of rwlocks to avoid this unnatural locking.