| Message ID | 5a0e320544e253cc903cfd3292600b6bec044a5f.1286139129.git.arno@natisbad.org |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Mon, Oct 04, 2010 at 08:25:07AM +0200, Arnaud Ebalard wrote: > > At the moment, Linux XFRM stack includes the address when computing > the hash to perform state lookup by SPI. This patch changes XFRM > state hash computation to prevent destination address to be > used. This will later allow finding states for packets w/ mangled > destination addresses. I'm fine with doing this for inbound SAs. However, I can't see how we can do this for outbound SAs where the SPI is chosen by the remote end. Incidentally, it appears that our hash could do with some strengthening. Cheers,
Hi, Herbert Xu <herbert@gondor.apana.org.au> writes: > On Mon, Oct 04, 2010 at 08:25:07AM +0200, Arnaud Ebalard wrote: >> >> At the moment, Linux XFRM stack includes the address when computing >> the hash to perform state lookup by SPI. This patch changes XFRM >> state hash computation to prevent destination address to be >> used. This will later allow finding states for packets w/ mangled >> destination addresses. > > I'm fine with doing this for inbound SAs. However, I can't see > how we can do this for outbound SAs where the SPI is chosen by > the remote end. The change *does not* make the lookup in the hash table rely only on the spi, i.e. __xfrm_state_lookup() is still passed the address. It only removes the destination address from the computation of the hash. This allows passing NULL to __xfrm_state_lookup() specifically for input path and make the lookup only based on the SPI. The destination address check is done later (possibly after IRO remapping). Except if I really missed something, this has no impact on outbound SA (other hashtables are used in that case). > Incidentally, it appears that our hash could do with some > strengthening. After the change, xfrm_spi_hash() would contain: unsigned int h = (__force u32)spi ^ proto; return ((h ^ (h >> 10) ^ (h >> 20)) & hmask) which seems to spread the bits h correctly into hmask bits (I mean for the effort ;-) ). Are you thinking about something like changing the shifts based on the length of the mask? Cheers, a+ -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 04, 2010 at 10:51:38PM +0200, Arnaud Ebalard wrote: > > Herbert Xu <herbert@gondor.apana.org.au> writes: > > > I'm fine with doing this for inbound SAs. However, I can't see > > how we can do this for outbound SAs where the SPI is chosen by > > the remote end. > > The change *does not* make the lookup in the hash table rely only on the > spi, i.e. __xfrm_state_lookup() is still passed the address. It only > removes the destination address from the computation of the hash. This > allows passing NULL to __xfrm_state_lookup() specifically for input path > and make the lookup only based on the SPI. The destination address check > is done later (possibly after IRO remapping). > > Except if I really missed something, this has no impact on outbound SA > (other hashtables are used in that case). I'm thinking about the case where each remote end (or one remote end with many IP addresses) chooses to use a single SPI which then all gets hashed to the same bucket. Outbound SAs are hashed into the same SPI hash table as inbound SAs. > > Incidentally, it appears that our hash could do with some > > strengthening. > > After the change, xfrm_spi_hash() would contain: > > unsigned int h = (__force u32)spi ^ proto; > return ((h ^ (h >> 10) ^ (h >> 20)) & hmask) > > which seems to spread the bits h correctly into hmask bits (I mean for > the effort ;-) ). Are you thinking about something like changing the > shifts based on the length of the mask? What I meant here is that even without your change, it is relatively easy to cause many SAs to be hashed to the same bucket. Cheers,
On Tue, Oct 05, 2010 at 10:11:14AM +0800, Herbert Xu wrote: > > I'm thinking about the case where each remote end (or one remote > end with many IP addresses) chooses to use a single SPI which then > all gets hashed to the same bucket. > > Outbound SAs are hashed into the same SPI hash table as inbound SAs. Another solution would be to create a hash table for inbound SAs only. Unfortunately I don't think we have anything in our current user-interface to indicate whether an SA is inbound or outbound. So to do this you'll need to use a heuristic such as doing a lookup on the source/destination address at insertion time. Cheers,
Hi, Herbert Xu <herbert@gondor.apana.org.au> writes: > On Tue, Oct 05, 2010 at 10:11:14AM +0800, Herbert Xu wrote: >> >> I'm thinking about the case where each remote end (or one remote >> end with many IP addresses) chooses to use a single SPI which then >> all gets hashed to the same bucket. >> >> Outbound SAs are hashed into the same SPI hash table as inbound SAs. > > Another solution would be to create a hash table for inbound SAs > only. Unfortunately I don't think we have anything in our current > user-interface to indicate whether an SA is inbound or outbound. > > So to do this you'll need to use a heuristic such as doing a > lookup on the source/destination address at insertion time. I spent some time scratching my head trying to find a good solution. It would indeed be perfect to have a specific hash table for inbound SA. But as you point, this would only be via a heuristic at insertion time and there are various cases which would not work: a SA can be installed w/o any of the addresses being configured on the system. I think I will try the following alternative approach based on your comments and proposals: - drop my patch to change spi hash computation - handle destination address remapping during input upon failure of xfrm_state_lookup() - handle source address remapping as it is currently done in the patch, i.e. by comparing received one against x->props.saddr once the state found and do To support the destination address remapping, I will have to reverse the logic I currently have for destination remapping states, to allow the lookup to be done based on the on-wire address (CoA) instead of the address in the SA (HoA). If a remapping state is found for the on-wire address, then a new lookup is done using the associated HoA this time. I think it would make the feature easier less intrusive for the IPsec stack. Thanks for your support and patience, Herbert. a+ -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 07, 2010 at 10:13:04PM +0200, Arnaud Ebalard wrote: > > I think I will try the following alternative approach based on your > comments and proposals: > > - drop my patch to change spi hash computation > - handle destination address remapping during input upon failure of > xfrm_state_lookup() > - handle source address remapping as it is currently done in the patch, > i.e. by comparing received one against x->props.saddr once the > state found and do I'm fine with this from IPsec's point-of-view. On the other hand, if it is at all possible to setup these remapping entries in the SADB beforehand, that would definitely be my preferred solution. Cheers,
diff --git a/net/xfrm/xfrm_hash.h b/net/xfrm/xfrm_hash.h index 8e69533..19eeee7 100644 --- a/net/xfrm/xfrm_hash.h +++ b/net/xfrm/xfrm_hash.h @@ -4,16 +4,6 @@ #include <linux/xfrm.h> #include <linux/socket.h> -static inline unsigned int __xfrm4_addr_hash(xfrm_address_t *addr) -{ - return ntohl(addr->a4); -} - -static inline unsigned int __xfrm6_addr_hash(xfrm_address_t *addr) -{ - return ntohl(addr->a6[2] ^ addr->a6[3]); -} - static inline unsigned int __xfrm4_daddr_saddr_hash(xfrm_address_t *daddr, xfrm_address_t *saddr) { u32 sum = (__force u32)daddr->a4 + (__force u32)saddr->a4; @@ -60,18 +50,9 @@ static inline unsigned __xfrm_src_hash(xfrm_address_t *daddr, } static inline unsigned int -__xfrm_spi_hash(xfrm_address_t *daddr, __be32 spi, u8 proto, unsigned short family, - unsigned int hmask) +__xfrm_spi_hash(__be32 spi, u8 proto, unsigned int hmask) { unsigned int h = (__force u32)spi ^ proto; - switch (family) { - case AF_INET: - h ^= __xfrm4_addr_hash(daddr); - break; - case AF_INET6: - h ^= __xfrm6_addr_hash(daddr); - break; - } return (h ^ (h >> 10) ^ (h >> 20)) & hmask; } diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c index eb96ce5..b6a4d8d 100644 --- a/net/xfrm/xfrm_state.c +++ b/net/xfrm/xfrm_state.c @@ -30,7 +30,7 @@ /* Each xfrm_state may be linked to two tables: - 1. Hash table by (spi,daddr,ah/esp) to find SA by SPI. (input,ctl) + 1. Hash table by (spi,ah/esp) to find SA by SPI. (input,ctl) 2. Hash table by (daddr,family,reqid) to find what SAs exist for given destination/tunnel endpoint. (output) */ @@ -67,9 +67,9 @@ static inline unsigned int xfrm_src_hash(struct net *net, } static inline unsigned int -xfrm_spi_hash(struct net *net, xfrm_address_t *daddr, __be32 spi, u8 proto, unsigned short family) +xfrm_spi_hash(struct net *net, __be32 spi, u8 proto) { - return __xfrm_spi_hash(daddr, spi, proto, family, net->xfrm.state_hmask); + return __xfrm_spi_hash(spi, proto, net->xfrm.state_hmask); } static void xfrm_hash_transfer(struct hlist_head *list, @@ -95,9 +95,7 @@ static void xfrm_hash_transfer(struct hlist_head *list, hlist_add_head(&x->bysrc, nsrctable+h); if (x->id.spi) { - h = __xfrm_spi_hash(&x->id.daddr, x->id.spi, - x->id.proto, x->props.family, - nhashmask); + h = __xfrm_spi_hash(x->id.spi, x->id.proto, nhashmask); hlist_add_head(&x->byspi, nspitable+h); } } @@ -679,7 +677,7 @@ xfrm_init_tempstate(struct xfrm_state *x, struct flowi *fl, static struct xfrm_state *__xfrm_state_lookup(struct net *net, u32 mark, xfrm_address_t *daddr, __be32 spi, u8 proto, unsigned short family) { - unsigned int h = xfrm_spi_hash(net, daddr, spi, proto, family); + unsigned int h = xfrm_spi_hash(net, spi, proto); struct xfrm_state *x; struct hlist_node *entry; @@ -868,7 +866,7 @@ found: h = xfrm_src_hash(net, daddr, saddr, encap_family); hlist_add_head(&x->bysrc, net->xfrm.state_bysrc+h); if (x->id.spi) { - h = xfrm_spi_hash(net, &x->id.daddr, x->id.spi, x->id.proto, encap_family); + h = xfrm_spi_hash(net, x->id.spi, x->id.proto); hlist_add_head(&x->byspi, net->xfrm.state_byspi+h); } x->lft.hard_add_expires_seconds = net->xfrm.sysctl_acq_expires; @@ -942,9 +940,7 @@ static void __xfrm_state_insert(struct xfrm_state *x) hlist_add_head(&x->bysrc, net->xfrm.state_bysrc+h); if (x->id.spi) { - h = xfrm_spi_hash(net, &x->id.daddr, x->id.spi, x->id.proto, - x->props.family); - + h = xfrm_spi_hash(net, x->id.spi, x->id.proto); hlist_add_head(&x->byspi, net->xfrm.state_byspi+h); } @@ -1535,7 +1531,7 @@ int xfrm_alloc_spi(struct xfrm_state *x, u32 low, u32 high) } if (x->id.spi) { spin_lock_bh(&xfrm_state_lock); - h = xfrm_spi_hash(net, &x->id.daddr, x->id.spi, x->id.proto, x->props.family); + h = xfrm_spi_hash(net, x->id.spi, x->id.proto); hlist_add_head(&x->byspi, net->xfrm.state_byspi+h); spin_unlock_bh(&xfrm_state_lock);
In the new IPsec architecture [RFC4301], "for an SA used to carry unicast traffic, the Security Parameters Index (SPI) by itself suffices to specify an SA". Section 4.1 of [RFC4301] provides additional guidance on the topic. In the old IPsec architecture [RFC2401], a SA "is uniquely identified by a triple consisting of a Security Parameter Index (SPI), an IP Destination Address and a security protocol (AH or ESP) identifier". If an IPsec stack only supports the behavior mandated by the old IPsec architecture, SAD lookup on inbound packets require the use of both the SPI and the destination address of the SA. For inbound IPsec traffic, IRO remapping rules may exist on the MN to remap the destination address (CoA) into the HoA. In that case, by design, the address found in the destination address field of the packet (CoA) does not match the one in the SA (HoA). At the moment, Linux XFRM stack includes the address when computing the hash to perform state lookup by SPI. This patch changes XFRM state hash computation to prevent destination address to be used. This will later allow finding states for packets w/ mangled destination addresses. Signed-off-by: Arnaud Ebalard <arno@natisbad.org> --- net/xfrm/xfrm_hash.h | 21 +-------------------- net/xfrm/xfrm_state.c | 20 ++++++++------------ 2 files changed, 9 insertions(+), 32 deletions(-)