Herbert Xu wrote:
It's still a valid clean-up patch though.
Agreed. There is also a bug in my patch, tmpl->daddr can be 0 in which
case the daddr passed as an argument to xfrm_state_find() will be used.
My patch only checked tmpl->daddr, this patch fixes it. It also uses
afinfo->init_tempsel directly, but I didn't kill xfrm_init_tempsel() yet
because I need it for xfrm resolution.
There is another reason why it won't dead lock. We don't actually
ever hold the write lock on afinfo :) Is there any reason why we
dont't just use xfrm_state_afinfo_lock instead of afinfo->lock?
I don't think so. I also don't see a reason why the lock needs to be
held between xfrm_state_get_afinfo() and xfrm_state_put_afinfo(),
a reference count should be enough.
Regards
Patrick
# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
# 2005/04/03 18:41:22+02:00 kaber@xxxxxxxxxxxx
# [IPSEC]: Use correct daddr for duplicate state check
#
# Signed-off-by: Patrick McHardy <kaber@xxxxxxxxx>
#
# net/xfrm/xfrm_state.c
# 2005/04/03 18:41:14+02:00 kaber@xxxxxxxxxxxx +9 -9
# [IPSEC]: Use correct daddr for duplicate state check
#
# Signed-off-by: Patrick McHardy <kaber@xxxxxxxxx>
#
diff -Nru a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
--- a/net/xfrm/xfrm_state.c 2005-04-03 18:41:41 +02:00
+++ b/net/xfrm/xfrm_state.c 2005-04-03 18:41:41 +02:00
@@ -357,12 +357,6 @@
x = best;
if (!x && !error && !acquire_in_progress) {
- x0 = afinfo->state_lookup(&tmpl->id.daddr, tmpl->id.spi,
tmpl->id.proto);
- if (x0 != NULL) {
- xfrm_state_put(x0);
- error = -EEXIST;
- goto out;
- }
x = xfrm_state_alloc();
if (x == NULL) {
error = -ENOMEM;
@@ -370,9 +364,11 @@
}
/* Initialize temporary selector matching only
* to current session. */
- xfrm_init_tempsel(x, fl, tmpl, daddr, saddr, family);
+ afinfo->init_tempsel(x, fl, tmpl, daddr, saddr);
+
+ x0 = afinfo->state_lookup(&x->id.daddr, x->id.spi, x->id.proto);
- if (km_query(x, tmpl, pol) == 0) {
+ if (!x0 && km_query(x, tmpl, pol) == 0) {
x->km.state = XFRM_STATE_ACQ;
list_add_tail(&x->bydst, xfrm_state_bydst+h);
xfrm_state_hold(x);
@@ -386,10 +382,14 @@
x->timer.expires = jiffies + XFRM_ACQ_EXPIRES*HZ;
add_timer(&x->timer);
} else {
+ error = -ESRCH;
+ if (x0) {
+ xfrm_state_put(x0);
+ error = -EEXIST;
+ }
x->km.state = XFRM_STATE_DEAD;
xfrm_state_put(x);
x = NULL;
- error = -ESRCH;
}
}
out:
|