* Re: [REGRESSION,BISECTED] Panic on ifup [not found] <4C39D834.8080206@iki.fi> @ 2010-07-11 17:09 ` George Spelvin 2010-07-12 7:01 ` Timo Teräs 0 siblings, 1 reply; 6+ messages in thread From: George Spelvin @ 2010-07-11 17:09 UTC (permalink / raw) To: linux, timo.teras; +Cc: davem, netdev > On 07/11/2010 03:38 PM, George Spelvin wrote: >> No, although /etc/ipec-tools.conf runs setkey. As I said, >> I was mostly running in single-user mode; a ps axf >> output is appended. >> >> Ah! A discovery! There are rules for the gateway! >> >> add <my_ip> <gw_ip> esp 0x200 -E aes-cbc >> <key>; >> add <gw_ip> <my_ip> esp 0x300 -E aes-cbc >> <key>; >> spdadd <gw_ip> <my_ip> any -P in ipsec >> esp/transport//use; >> spdadd <my_ip> <gw_ip> any -P out ipsec >> esp/transport//use; > This means optional encryption. Probably something goes wrong > constructing the bundle which results in encryption not being applied. > And it would look like xfrm_resolve_and_create_bundle() does not take > this into account properly. I need to fix it with optional policies. > > In the mean while, could confirm if everything works if you change the > last line to: > esp/transport//require; Will do. This might lead to no traffic if there's something else broken, however it should not crash. This change is needed only for the 'out' policy, as the bundles are used only on xmit code paths. > yup, xfrm_resolve_and_create_bundle looks to be the culprit. I'll try to > figure out a patch for testing. > Ok, this is basically what setkey did for you. Looks like it was ran > twice and you are missing flush and spdflush from setkey, so you get > duplicates here. Otherwise it's ok. Um, I am *not* missing flush and spdflush. The entire file, with comments and blank lines stripped, and some details censored, is: #!/usr/sbin/setkey -f flush; spdflush; add $LOCALNET.1 $LOCALNET.62 esp 0x200 -E aes-cbc <key redacted>; add $LOCALNET.62 $LOCALNET.1 esp 0x300 -E aes-cbc <key redacted>; add $LOCALNET.3 $LOCALNET.62 esp 0x400 -E aes-cbc <key redacted>; add $LOCALNET.62 $LOCALNET.3 esp 0x500 -E aes-cbc <key redacted>; spdadd $LOCALNET.1 $LOCALNET.62 any -P in ipsec esp/transport//use; spdadd $LOCALNET.62 $LOCALNET.1 any -P out ipsec esp/transport//use; spdadd $LOCALNET.3 $LOCALNET.62 any -P in ipsec esp/transport//use; spdadd $LOCALNET.62 $LOCALNET.3 any -P out ipsec esp/transport//use; Anyway, thank you very much! ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [REGRESSION,BISECTED] Panic on ifup 2010-07-11 17:09 ` [REGRESSION,BISECTED] Panic on ifup George Spelvin @ 2010-07-12 7:01 ` Timo Teräs 2010-07-13 1:50 ` George Spelvin 2010-07-13 4:20 ` David Miller 0 siblings, 2 replies; 6+ messages in thread From: Timo Teräs @ 2010-07-12 7:01 UTC (permalink / raw) To: George Spelvin; +Cc: davem, netdev On 07/11/2010 08:09 PM, George Spelvin wrote: >> On 07/11/2010 03:38 PM, George Spelvin wrote: >>> No, although /etc/ipec-tools.conf runs setkey. As I said, >>> I was mostly running in single-user mode; a ps axf >>> output is appended. >>> >>> Ah! A discovery! There are rules for the gateway! >>> >>> add <my_ip> <gw_ip> esp 0x200 -E aes-cbc >>> <key>; >>> add <gw_ip> <my_ip> esp 0x300 -E aes-cbc >>> <key>; >>> spdadd <gw_ip> <my_ip> any -P in ipsec >>> esp/transport//use; >>> spdadd <my_ip> <gw_ip> any -P out ipsec >>> esp/transport//use; > >> This means optional encryption. Probably something goes wrong >> constructing the bundle which results in encryption not being applied. >> And it would look like xfrm_resolve_and_create_bundle() does not take >> this into account properly. I need to fix it with optional policies. >> >> In the mean while, could confirm if everything works if you change the >> last line to: >> esp/transport//require; > > Will do. > > This might lead to no traffic if there's something else broken, however > it should not crash. This change is needed only for the 'out' policy, as > the bundles are used only on xmit code paths. > >> yup, xfrm_resolve_and_create_bundle looks to be the culprit. I'll try to >> figure out a patch for testing. Can you try the patch attached to end? >> Ok, this is basically what setkey did for you. Looks like it was ran >> twice and you are missing flush and spdflush from setkey, so you get >> duplicates here. Otherwise it's ok. > > Um, I am *not* missing flush and spdflush. The entire file, with comments > and blank lines stripped, and some details censored, is: > > #!/usr/sbin/setkey -f > flush; > spdflush; > add $LOCALNET.1 $LOCALNET.62 esp 0x200 -E aes-cbc <key redacted>; > add $LOCALNET.62 $LOCALNET.1 esp 0x300 -E aes-cbc <key redacted>; > add $LOCALNET.3 $LOCALNET.62 esp 0x400 -E aes-cbc <key redacted>; > add $LOCALNET.62 $LOCALNET.3 esp 0x500 -E aes-cbc <key redacted>; > spdadd $LOCALNET.1 $LOCALNET.62 any -P in ipsec esp/transport//use; > spdadd $LOCALNET.62 $LOCALNET.1 any -P out ipsec esp/transport//use; > spdadd $LOCALNET.3 $LOCALNET.62 any -P in ipsec esp/transport//use; > spdadd $LOCALNET.62 $LOCALNET.3 any -P out ipsec esp/transport//use; Ah, ok. Did not bother to double check the related IPs as I thought it was full ipsec.conf. Everything is okay then. And here goes the patch (which I've only compile tested so far). diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c index af1c173..200f8d7 100644 --- a/net/xfrm/xfrm_policy.c +++ b/net/xfrm/xfrm_policy.c @@ -1598,7 +1598,8 @@ xfrm_resolve_and_create_bundle(struct xfrm_policy **pols, int num_pols, if (err != -EAGAIN) XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTPOLERROR); return ERR_PTR(err); - } + } else if (err == 0) + return NULL; dst = xfrm_bundle_create(pols[0], xfrm, err, fl, dst_orig); if (IS_ERR(dst)) { @@ -1678,6 +1679,13 @@ xfrm_bundle_lookup(struct net *net, struct flowi *fl, u16 family, u8 dir, goto make_dummy_bundle; dst_hold(&xdst->u.dst); return oldflo; + } else if (new_xdst == NULL) { + num_xfrms = 0; + if (oldflo == NULL) + goto make_dummy_bundle; + xdst->num_xfrms = 0; + dst_hold(&xdst->u.dst); + return oldflo; } /* Kill the previous bundle */ @@ -1760,6 +1768,10 @@ restart: xfrm_pols_put(pols, num_pols); err = PTR_ERR(xdst); goto dropdst; + } else if (xdst == NULL) { + num_xfrms = 0; + drop_pols = num_pols; + goto no_transform; } spin_lock_bh(&xfrm_policy_sk_bundle_lock); ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [REGRESSION,BISECTED] Panic on ifup 2010-07-12 7:01 ` Timo Teräs @ 2010-07-13 1:50 ` George Spelvin 2010-07-13 4:20 ` David Miller 1 sibling, 0 replies; 6+ messages in thread From: George Spelvin @ 2010-07-13 1:50 UTC (permalink / raw) To: timo.teras; +Cc: davem, linux, netdev > And here goes the patch (which I've only compile tested so far). That does indeed fix it! Also applies and works with -rc5. Please queue for -rc6. (Unless you want to tweak the patch a bit; I haven't done any sort of code review on it.) Tested-by: George Spelvin <linux@horizon.com> > diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c > index af1c173..200f8d7 100644 > --- a/net/xfrm/xfrm_policy.c > +++ b/net/xfrm/xfrm_policy.c > @@ -1598,7 +1598,8 @@ xfrm_resolve_and_create_bundle(struct xfrm_policy > **pols, int num_pols, > if (err != -EAGAIN) > XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTPOLERROR); > return ERR_PTR(err); > - } > + } else if (err == 0) > + return NULL; > > dst = xfrm_bundle_create(pols[0], xfrm, err, fl, dst_orig); > if (IS_ERR(dst)) { This could be simplified to (if you want; it's smaller but uglier) if (err <= 0) { if (err != 0 && err != -EAGAIN) XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTPOLERROR); return ERR_PTR(err); /* Correctly returns NULL if err == 0 */ } > @@ -1678,6 +1679,13 @@ xfrm_bundle_lookup(struct net *net, struct flowi > *fl, u16 family, u8 dir, > goto make_dummy_bundle; > dst_hold(&xdst->u.dst); > return oldflo; > + } else if (new_xdst == NULL) { > + num_xfrms = 0; > + if (oldflo == NULL) > + goto make_dummy_bundle; > + xdst->num_xfrms = 0; > + dst_hold(&xdst->u.dst); > + return oldflo; > } > > /* Kill the previous bundle */ This I'm having a hard time simplifying. It resembles the previous block, but not enough. > @@ -1760,6 +1768,10 @@ restart: > xfrm_pols_put(pols, num_pols); > err = PTR_ERR(xdst); > goto dropdst; > + } else if (xdst == NULL) { > + num_xfrms = 0; > + drop_pols = num_pols; > + goto no_transform; > } > > spin_lock_bh(&xfrm_policy_sk_bundle_lock); > I see two nearby tests for xdst == NULL ("To accelerate a bit..."); I take it they can't be combined? ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [REGRESSION,BISECTED] Panic on ifup 2010-07-12 7:01 ` Timo Teräs 2010-07-13 1:50 ` George Spelvin @ 2010-07-13 4:20 ` David Miller 2010-07-13 7:29 ` [PATCH] xfrm: do not assume that template resolving always returns xfrms Timo Teräs 1 sibling, 1 reply; 6+ messages in thread From: David Miller @ 2010-07-13 4:20 UTC (permalink / raw) To: timo.teras; +Cc: linux, netdev From: Timo Teräs <timo.teras@iki.fi> Date: Mon, 12 Jul 2010 10:01:23 +0300 > And here goes the patch (which I've only compile tested so far). Timo, since this tested positively for the reporter, please make a formal submission once you are happy with the patch. Thanks. ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] xfrm: do not assume that template resolving always returns xfrms 2010-07-13 4:20 ` David Miller @ 2010-07-13 7:29 ` Timo Teräs 2010-07-14 21:17 ` David Miller 0 siblings, 1 reply; 6+ messages in thread From: Timo Teräs @ 2010-07-13 7:29 UTC (permalink / raw) To: netdev, David Miller, linux; +Cc: Timo Teräs xfrm_resolve_and_create_bundle() assumed that, if policies indicated presence of xfrms, bundle template resolution would always return some xfrms. This is not true for 'use' level policies which can result in no xfrm's being applied if there is no suitable xfrm states. This fixes a crash by this incorrect assumption. Reported-by: George Spelvin <linux@horizon.com> Bisected-by: George Spelvin <linux@horizon.com> Tested-by: George Spelvin <linux@horizon.com> Signed-off-by: Timo Teräs <timo.teras@iki.fi> --- net/xfrm/xfrm_policy.c | 15 +++++++++++++-- 1 files changed, 13 insertions(+), 2 deletions(-) diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c index af1c173..a7ec5a8 100644 --- a/net/xfrm/xfrm_policy.c +++ b/net/xfrm/xfrm_policy.c @@ -1594,8 +1594,8 @@ xfrm_resolve_and_create_bundle(struct xfrm_policy **pols, int num_pols, /* Try to instantiate a bundle */ err = xfrm_tmpl_resolve(pols, num_pols, fl, xfrm, family); - if (err < 0) { - if (err != -EAGAIN) + if (err <= 0) { + if (err != 0 && err != -EAGAIN) XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTPOLERROR); return ERR_PTR(err); } @@ -1678,6 +1678,13 @@ xfrm_bundle_lookup(struct net *net, struct flowi *fl, u16 family, u8 dir, goto make_dummy_bundle; dst_hold(&xdst->u.dst); return oldflo; + } else if (new_xdst == NULL) { + num_xfrms = 0; + if (oldflo == NULL) + goto make_dummy_bundle; + xdst->num_xfrms = 0; + dst_hold(&xdst->u.dst); + return oldflo; } /* Kill the previous bundle */ @@ -1760,6 +1767,10 @@ restart: xfrm_pols_put(pols, num_pols); err = PTR_ERR(xdst); goto dropdst; + } else if (xdst == NULL) { + num_xfrms = 0; + drop_pols = num_pols; + goto no_transform; } spin_lock_bh(&xfrm_policy_sk_bundle_lock); -- 1.7.0.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] xfrm: do not assume that template resolving always returns xfrms 2010-07-13 7:29 ` [PATCH] xfrm: do not assume that template resolving always returns xfrms Timo Teräs @ 2010-07-14 21:17 ` David Miller 0 siblings, 0 replies; 6+ messages in thread From: David Miller @ 2010-07-14 21:17 UTC (permalink / raw) To: timo.teras; +Cc: netdev, linux From: Timo Teräs <timo.teras@iki.fi> Date: Tue, 13 Jul 2010 10:29:42 +0300 > xfrm_resolve_and_create_bundle() assumed that, if policies indicated > presence of xfrms, bundle template resolution would always return > some xfrms. This is not true for 'use' level policies which can > result in no xfrm's being applied if there is no suitable xfrm states. > This fixes a crash by this incorrect assumption. > > Reported-by: George Spelvin <linux@horizon.com> > Bisected-by: George Spelvin <linux@horizon.com> > Tested-by: George Spelvin <linux@horizon.com> > Signed-off-by: Timo Teräs <timo.teras@iki.fi> Applied, thanks Timo. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-07-14 21:16 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <4C39D834.8080206@iki.fi>
2010-07-11 17:09 ` [REGRESSION,BISECTED] Panic on ifup George Spelvin
2010-07-12 7:01 ` Timo Teräs
2010-07-13 1:50 ` George Spelvin
2010-07-13 4:20 ` David Miller
2010-07-13 7:29 ` [PATCH] xfrm: do not assume that template resolving always returns xfrms Timo Teräs
2010-07-14 21:17 ` David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).