Message ID | 1314806703-5275-1-git-send-email-david.ward@ll.mit.edu |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
From: Julian Anastasov <ja@ssi.bg> Date: Wed, 31 Aug 2011 23:51:32 +0300 (EEST) > Such change will cause problems for callers that > use flowi4 in stack. Examples: > > ip_route_me_harder > icmp_route_lookup Right. -- 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
Hello, On Wed, 31 Aug 2011, David Ward wrote: > The entire flowi struct needs to be initialized by afinfo->decode_session, > because flow_hash_code operates over the entire struct and may otherwise > return different hash values for what is intended to be the same key. Such change will cause problems for callers that use flowi4 in stack. Examples: ip_route_me_harder icmp_route_lookup Not sure if adding size as parameter to flow_hash_code is better approach. May be flow_cache_lookup needs to determine size from family that can be used for flow_hash_code, flow_key_compare and the memcpy(&fle->key, key, sizeof(*key)) after fle = kmem_cache_alloc(flow_cachep, GFP_ATOMIC). The question is how to get size by family. > Signed-off-by: David Ward <david.ward@ll.mit.edu> > --- > net/ipv4/xfrm4_policy.c | 2 +- > net/ipv6/xfrm6_policy.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/ipv4/xfrm4_policy.c b/net/ipv4/xfrm4_policy.c > index fc5368a..afce24d 100644 > --- a/net/ipv4/xfrm4_policy.c > +++ b/net/ipv4/xfrm4_policy.c > @@ -114,7 +114,7 @@ _decode_session4(struct sk_buff *skb, struct flowi *fl, int reverse) > u8 *xprth = skb_network_header(skb) + iph->ihl * 4; > struct flowi4 *fl4 = &fl->u.ip4; > > - memset(fl4, 0, sizeof(struct flowi4)); > + memset(fl, 0, sizeof(struct flowi)); > fl4->flowi4_mark = skb->mark; > > if (!ip_is_fragment(iph)) { > diff --git a/net/ipv6/xfrm6_policy.c b/net/ipv6/xfrm6_policy.c > index d879f7e..9088d38 100644 > --- a/net/ipv6/xfrm6_policy.c > +++ b/net/ipv6/xfrm6_policy.c > @@ -129,7 +129,7 @@ _decode_session6(struct sk_buff *skb, struct flowi *fl, int reverse) > const unsigned char *nh = skb_network_header(skb); > u8 nexthdr = nh[IP6CB(skb)->nhoff]; > > - memset(fl6, 0, sizeof(struct flowi6)); > + memset(fl, 0, sizeof(struct flowi)); > fl6->flowi6_mark = skb->mark; > > ipv6_addr_copy(&fl6->daddr, reverse ? &hdr->saddr : &hdr->daddr); > -- > 1.7.4.1 Regards -- Julian Anastasov <ja@ssi.bg> -- 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
Hi Julian, On 08/31/2011 04:51 PM, Julian Anastasov wrote: > On Wed, 31 Aug 2011, David Ward wrote >> The entire flowi struct needs to be initialized by afinfo->decode_session, >> because flow_hash_code operates over the entire struct and may otherwise >> return different hash values for what is intended to be the same key. > Such change will cause problems for callers that > use flowi4 in stack. Examples: > > ip_route_me_harder > icmp_route_lookup Thanks for pointing this out. > Not sure if adding size as parameter to flow_hash_code > is better approach. May be flow_cache_lookup needs to > determine size from family that can be used for flow_hash_code, > flow_key_compare and the memcpy(&fle->key, key, sizeof(*key)) > after fle = kmem_cache_alloc(flow_cachep, GFP_ATOMIC). Makes sense to me. However should we just replace flow_key_compare with memcmp then, since the assumptions about constant size and alignment will no longer apply? Or should there be a separate flow_key_compare function for each family, and have all of the flowi* structures become __attribute__((__aligned__(BITS_PER_LONG/8))) ? David > The question is how to get size by family. > >> Signed-off-by: David Ward<david.ward@ll.mit.edu> >> --- >> net/ipv4/xfrm4_policy.c | 2 +- >> net/ipv6/xfrm6_policy.c | 2 +- >> 2 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/net/ipv4/xfrm4_policy.c b/net/ipv4/xfrm4_policy.c >> index fc5368a..afce24d 100644 >> --- a/net/ipv4/xfrm4_policy.c >> +++ b/net/ipv4/xfrm4_policy.c >> @@ -114,7 +114,7 @@ _decode_session4(struct sk_buff *skb, struct flowi *fl, int reverse) >> u8 *xprth = skb_network_header(skb) + iph->ihl * 4; >> struct flowi4 *fl4 =&fl->u.ip4; >> >> - memset(fl4, 0, sizeof(struct flowi4)); >> + memset(fl, 0, sizeof(struct flowi)); >> fl4->flowi4_mark = skb->mark; >> >> if (!ip_is_fragment(iph)) { >> diff --git a/net/ipv6/xfrm6_policy.c b/net/ipv6/xfrm6_policy.c >> index d879f7e..9088d38 100644 >> --- a/net/ipv6/xfrm6_policy.c >> +++ b/net/ipv6/xfrm6_policy.c >> @@ -129,7 +129,7 @@ _decode_session6(struct sk_buff *skb, struct flowi *fl, int reverse) >> const unsigned char *nh = skb_network_header(skb); >> u8 nexthdr = nh[IP6CB(skb)->nhoff]; >> >> - memset(fl6, 0, sizeof(struct flowi6)); >> + memset(fl, 0, sizeof(struct flowi)); >> fl6->flowi6_mark = skb->mark; >> >> ipv6_addr_copy(&fl6->daddr, reverse ?&hdr->saddr :&hdr->daddr); >> -- >> 1.7.4.1
Hello, On Thu, 1 Sep 2011, Ward, David - 0663 - MITLL wrote: > > Not sure if adding size as parameter to flow_hash_code > > is better approach. May be flow_cache_lookup needs to > > determine size from family that can be used for flow_hash_code, > > flow_key_compare and the memcpy(&fle->key, key, sizeof(*key)) > > after fle = kmem_cache_alloc(flow_cachep, GFP_ATOMIC). > > Makes sense to me. However should we just replace flow_key_compare with > memcmp then, since the assumptions about constant size and alignment will no > longer apply? Or should there be a separate flow_key_compare function for > each family, and have all of the flowi* structures become > __attribute__((__aligned__(BITS_PER_LONG/8))) ? I don't know this code well but I guess memcmp is not preferred. IMHO, as the callers provide per-family structures and we do not want to change that, these structures must be aligned to long type as required by flow_compare_t and jhash2 (at least u32) usage. The second option is to use memcmp and jhash instead of jhash2 to avoid such alignment but I guess other developers will oppose it. More opinions are needed here. Regards -- Julian Anastasov <ja@ssi.bg> -- 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
These fixes to the flow cache are needed with the conversion to AF-specific
flowi structs. They are written so as to avoid introducing AF-specific code
into net/core/flow.c.
Note that __xfrm_policy_check (in net/xfrm/xfrm_policy.c) still allocates a
struct flowi on the stack and passes it to flow_cache_lookup. My understanding
is that since this is on the stack, this will not be aligned, and therefore it
will cause problems with flow_hash_code and flow_key_compare. Is that correct?
Signed-off-by: David Ward <david.ward@ll.mit.edu>
David Ward (2):
net: Align AF-specific flowi structs to long
net: Handle different key sizes between address families in flow
cache
include/net/flow.h | 25 ++++++++++++++++++++++---
net/core/flow.c | 28 ++++++++++++++++------------
2 files changed, 38 insertions(+), 15 deletions(-)
--
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
diff --git a/net/ipv4/xfrm4_policy.c b/net/ipv4/xfrm4_policy.c index fc5368a..afce24d 100644 --- a/net/ipv4/xfrm4_policy.c +++ b/net/ipv4/xfrm4_policy.c @@ -114,7 +114,7 @@ _decode_session4(struct sk_buff *skb, struct flowi *fl, int reverse) u8 *xprth = skb_network_header(skb) + iph->ihl * 4; struct flowi4 *fl4 = &fl->u.ip4; - memset(fl4, 0, sizeof(struct flowi4)); + memset(fl, 0, sizeof(struct flowi)); fl4->flowi4_mark = skb->mark; if (!ip_is_fragment(iph)) { diff --git a/net/ipv6/xfrm6_policy.c b/net/ipv6/xfrm6_policy.c index d879f7e..9088d38 100644 --- a/net/ipv6/xfrm6_policy.c +++ b/net/ipv6/xfrm6_policy.c @@ -129,7 +129,7 @@ _decode_session6(struct sk_buff *skb, struct flowi *fl, int reverse) const unsigned char *nh = skb_network_header(skb); u8 nexthdr = nh[IP6CB(skb)->nhoff]; - memset(fl6, 0, sizeof(struct flowi6)); + memset(fl, 0, sizeof(struct flowi)); fl6->flowi6_mark = skb->mark; ipv6_addr_copy(&fl6->daddr, reverse ? &hdr->saddr : &hdr->daddr);
The entire flowi struct needs to be initialized by afinfo->decode_session, because flow_hash_code operates over the entire struct and may otherwise return different hash values for what is intended to be the same key. Signed-off-by: David Ward <david.ward@ll.mit.edu> --- net/ipv4/xfrm4_policy.c | 2 +- net/ipv6/xfrm6_policy.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)