[PATCH net-next] xfrm: Simplify SA looking up when using wildcard source address

[PATCH net-next] xfrm: Simplify SA looking up when using wildcard source address

[Fan Du]

I'm not quite sure I get this "wildcard source address" right,
IMHO if a host needs to protect every traffic for a given remote host,
then the source address is wildcard address, i.e. all ZEROs.
(Please correct me if I'm bloodly wrong。。。)

Here is the argument if above statement stands true:
__xfrm4/6_state_addr_check is a four steps check, all we need to do
is checking whether the destination address match. Passing saddr from
flow is worst option, as the checking needs to reach the fourth step.

So, simply this process by only checking destination address only when
using wildcard source address for looking up SAs.

Signed-off-by: Fan Du <fan.du@windriver.com>
---
 include/net/xfrm.h    |   31 +++++++++++++++++++++++++++++++
 net/xfrm/xfrm_state.c |    2 +-
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index e253bf0..fdb9343 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -1282,6 +1282,37 @@ xfrm_state_addr_check(const struct xfrm_state *x,
 }
 
 static __inline__ int
+__xfrm4_state_daddr_check(const struct xfrm_state *x,
+                                const xfrm_address_t *daddr)
+{
+        return ((daddr->a4 == x->id.daddr.a4) ? 1 : 0);
+}
+
+static __inline__ int
+__xfrm6_state_daddr_check(const struct xfrm_state *x,
+                         const xfrm_address_t *daddr)
+{
+        if (ipv6_addr_equal((struct in6_addr *)daddr, (struct in6_addr *)&x->id.daddr))
+                return 1;
+        else 
+                return 0;
+}
+
+static __inline__ int
+xfrm_state_daddr_check(const struct xfrm_state *x,
+                      const xfrm_address_t *daddr,
+                      unsigned short family)
+{
+        switch (family) {
+        case AF_INET:
+                return __xfrm4_state_daddr_check(x, daddr);
+        case AF_INET6:
+                return __xfrm6_state_daddr_check(x, daddr);
+        }    
+        return 0;
+}
+
+static __inline__ int
 xfrm_state_addr_flow_check(const struct xfrm_state *x, const struct flowi *fl,
 			   unsigned short family)
 {
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index e1373d5..87c99da 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -824,7 +824,7 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
 		    x->props.reqid == tmpl->reqid &&
 		    (mark & x->mark.m) == x->mark.v &&
 		    !(x->props.flags & XFRM_STATE_WILDRECV) &&
-		    xfrm_state_addr_check(x, daddr, saddr, encap_family) &&
+		    xfrm_state_daddr_check(x, daddr, encap_family) &&
 		    tmpl->mode == x->props.mode &&
 		    tmpl->id.proto == x->id.proto &&
 		    (tmpl->id.spi == x->id.spi || !tmpl->id.spi))
-- 
1.7.9.5

Re: [PATCH net-next] xfrm: Simplify SA looking up when using wildcard source address

[Sergei Shtylyov]

Hello.

On 09/23/2013 01:18 PM, Fan Du wrote:

> I'm not quite sure I get this "wildcard source address" right,
> IMHO if a host needs to protect every traffic for a given remote host,
> then the source address is wildcard address, i.e. all ZEROs.
> (Please correct me if I'm bloodly wrong。。。)

> Here is the argument if above statement stands true:
> __xfrm4/6_state_addr_check is a four steps check, all we need to do
> is checking whether the destination address match. Passing saddr from
> flow is worst option, as the checking needs to reach the fourth step.

> So, simply this process by only checking destination address only when
> using wildcard source address for looking up SAs.

> Signed-off-by: Fan Du <fan.du@windriver.com>
> ---
>   include/net/xfrm.h    |   31 +++++++++++++++++++++++++++++++
>   net/xfrm/xfrm_state.c |    2 +-
>   2 files changed, 32 insertions(+), 1 deletion(-)

> diff --git a/include/net/xfrm.h b/include/net/xfrm.h
> index e253bf0..fdb9343 100644
> --- a/include/net/xfrm.h
> +++ b/include/net/xfrm.h
> @@ -1282,6 +1282,37 @@ xfrm_state_addr_check(const struct xfrm_state *x,
>   }
>
>   static __inline__ int
> +__xfrm4_state_daddr_check(const struct xfrm_state *x,
> +                                const xfrm_address_t *daddr)
> +{
> +        return ((daddr->a4 == x->id.daddr.a4) ? 1 : 0);

    () not needed around the *return* expression, and ?: not needed too.

WBR, Sergei

Re: [PATCH net-next] xfrm: Simplify SA looking up when using wildcard source address

[Steffen Klassert]

On Mon, Sep 23, 2013 at 05:18:37PM +0800, Fan Du wrote:
> I'm not quite sure I get this "wildcard source address" right,
> IMHO if a host needs to protect every traffic for a given remote host,
> then the source address is wildcard address, i.e. all ZEROs.
> (Please correct me if I'm bloodly wrong。。。)

The above does not belong to a commit message, really.
If you are not sure and you want comments on your patch,
mark your patch as RFC. You should be sure that your patch
is correct when you submit, at least in the moment you
send it. I know that this can change a second after,
but in that moment you should be sure.

> 
> Here is the argument if above statement stands true:
> __xfrm4/6_state_addr_check is a four steps check, all we need to do
> is checking whether the destination address match. Passing saddr from
> flow is worst option, as the checking needs to reach the fourth step.
> 
> So, simply this process by only checking destination address only when
> using wildcard source address for looking up SAs.
> 
> Signed-off-by: Fan Du <fan.du@windriver.com>
> ---

If you have further comments on your patch that should not be
included in the commit message, you can add them here.

>  include/net/xfrm.h    |   31 +++++++++++++++++++++++++++++++
>  net/xfrm/xfrm_state.c |    2 +-
>  2 files changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/include/net/xfrm.h b/include/net/xfrm.h
> index e253bf0..fdb9343 100644
> --- a/include/net/xfrm.h
> +++ b/include/net/xfrm.h
> @@ -1282,6 +1282,37 @@ xfrm_state_addr_check(const struct xfrm_state *x,
>  }
>  
>  static __inline__ int
> +__xfrm4_state_daddr_check(const struct xfrm_state *x,
> +                                const xfrm_address_t *daddr)
> +{
> +        return ((daddr->a4 == x->id.daddr.a4) ? 1 : 0);
> +}
> +
> +static __inline__ int
> +__xfrm6_state_daddr_check(const struct xfrm_state *x,
> +                         const xfrm_address_t *daddr)
> +{
> +        if (ipv6_addr_equal((struct in6_addr *)daddr, (struct in6_addr *)&x->id.daddr))
> +                return 1;
> +        else 
> +                return 0;
> +}
> +
> +static __inline__ int
> +xfrm_state_daddr_check(const struct xfrm_state *x,
> +                      const xfrm_address_t *daddr,
> +                      unsigned short family)
> +{
> +        switch (family) {
> +        case AF_INET:
> +                return __xfrm4_state_daddr_check(x, daddr);
> +        case AF_INET6:
> +                return __xfrm6_state_daddr_check(x, daddr);
> +        }    
> +        return 0;
> +}

You used whitespaces where you should use tabs in the whole patch.
Please do the formating right to avoid cleanup patches.

Re: [PATCH net-next] xfrm: Simplify SA looking up when using wildcard source address

[Fan Du]

On 2013年09月24日 19:45, Steffen Klassert wrote:
> On Mon, Sep 23, 2013 at 05:18:37PM +0800, Fan Du wrote:
>> I'm not quite sure I get this "wildcard source address" right,
>> IMHO if a host needs to protect every traffic for a given remote host,
>> then the source address is wildcard address, i.e. all ZEROs.
>> (Please correct me if I'm bloodly wrong。。。)
>
> The above does not belong to a commit message, really.
> If you are not sure and you want comments on your patch,
> mark your patch as RFC. You should be sure that your patch
> is correct when you submit, at least in the moment you
> send it. I know that this can change a second after,
> but in that moment you should be sure.

One day without embarrassment is not my day :)
Have sent v2, please kindly review.

Thanks

>>
>> Here is the argument if above statement stands true:
>> __xfrm4/6_state_addr_check is a four steps check, all we need to do
>> is checking whether the destination address match. Passing saddr from
>> flow is worst option, as the checking needs to reach the fourth step.
>>
>> So, simply this process by only checking destination address only when
>> using wildcard source address for looking up SAs.
>>
>> Signed-off-by: Fan Du<fan.du@windriver.com>
>> ---
>
> If you have further comments on your patch that should not be
> included in the commit message, you can add them here.
>
>>   include/net/xfrm.h    |   31 +++++++++++++++++++++++++++++++
>>   net/xfrm/xfrm_state.c |    2 +-
>>   2 files changed, 32 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/net/xfrm.h b/include/net/xfrm.h
>> index e253bf0..fdb9343 100644
>> --- a/include/net/xfrm.h
>> +++ b/include/net/xfrm.h
>> @@ -1282,6 +1282,37 @@ xfrm_state_addr_check(const struct xfrm_state *x,
>>   }
>>
>>   static __inline__ int
>> +__xfrm4_state_daddr_check(const struct xfrm_state *x,
>> +                                const xfrm_address_t *daddr)
>> +{
>> +        return ((daddr->a4 == x->id.daddr.a4) ? 1 : 0);
>> +}
>> +
>> +static __inline__ int
>> +__xfrm6_state_daddr_check(const struct xfrm_state *x,
>> +                         const xfrm_address_t *daddr)
>> +{
>> +        if (ipv6_addr_equal((struct in6_addr *)daddr, (struct in6_addr *)&x->id.daddr))
>> +                return 1;
>> +        else
>> +                return 0;
>> +}
>> +
>> +static __inline__ int
>> +xfrm_state_daddr_check(const struct xfrm_state *x,
>> +                      const xfrm_address_t *daddr,
>> +                      unsigned short family)
>> +{
>> +        switch (family) {
>> +        case AF_INET:
>> +                return __xfrm4_state_daddr_check(x, daddr);
>> +        case AF_INET6:
>> +                return __xfrm6_state_daddr_check(x, daddr);
>> +        }
>> +        return 0;
>> +}
>
> You used whitespaces where you should use tabs in the whole patch.
> Please do the formating right to avoid cleanup patches.
>
> --
> 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
>

-- 
浮沉随浪只记今朝笑

--fan