[RFC] SPD basic actions per netdev

[RFC] SPD basic actions per netdev

[jamal]

This may be oversight in current implementation and possibly
nobody has needed it before - hence it is not functional.

I want to have a drop-all policy on a per-interface level
for incoming packets and add exceptions as i need them.
[using the flow table is cheap if you have xfrm built in].
i.e something along the lines of:

#eth0, wild-card drop all
ip xfrm policy add src 0.0.0.0/0 dst 0.0.0.0/0 dev eth0 \
       dir in ptype main action block priority $SOME-HIGH-value
#eth0, exception
ip xfrm policy add blah blah dev eth0 \
dir in ptype main action allow priority $SOME-small-value
#eth1, wild-card drop all
ip xfrm policy add src 0.0.0.0/0 dst 0.0.0.0/0 dev eth1 \
       dir in ptype main action block priority $SOME-HIGH-value
#eth1 exception ...

The problem is this works as long as i dont specify an interface.
i.e, this would work in the in-direction:

ip xfrm policy add src 0.0.0.0/0 dst 0.0.0.0/0 \
        dir in ptype main action block priority $SOME-HIGH-value

This would not work:
ip xfrm policy add src 0.0.0.0/0 dst 0.0.0.0/0 dev eth0 \
       dir in ptype main action block priority $SOME-HIGH-value


The checks in the selector matching is the culprit, example for v4:

__xfrm4_selector_match(struct xfrm_selector *sel, struct flowi *fl)
{
        return  .... &&
                .... &&
                (fl->oif == sel->ifindex || !sel->ifindex);
}

i.e in the second case i have a non-zero sel->ifindex but
a zero fl->oif; so it wont match.

One approach to fix this is to pass the direction then i can do
in the function call, then i can do something along the lines of
matching if:
(fl_dir == FLOW_DIR_IN && (fl->iif == sel->ifindex || !sel->ifindex) ||
(fl->oif == sel->ifindex || !sel->ifindex);

Is there any reason the selector matching only assumes fl->oif?
Are there any unforeseen issues/breakages if i added a check for the
above.

cheers,
jamal

Re: [RFC] SPD basic actions per netdev

[jamal]

[-- Attachment #1: Type: text/plain, Size: 2130 bytes --]


And here's something i just tested on net-next that
fixes this for me.

cheers,
jamal

On Wed, 2010-03-31 at 12:38 -0400, jamal wrote:
> This may be oversight in current implementation and possibly
> nobody has needed it before - hence it is not functional.
> 
> I want to have a drop-all policy on a per-interface level
> for incoming packets and add exceptions as i need them.
> [using the flow table is cheap if you have xfrm built in].
> i.e something along the lines of:
> 
> #eth0, wild-card drop all
> ip xfrm policy add src 0.0.0.0/0 dst 0.0.0.0/0 dev eth0 \
>        dir in ptype main action block priority $SOME-HIGH-value
> #eth0, exception
> ip xfrm policy add blah blah dev eth0 \
> dir in ptype main action allow priority $SOME-small-value
> #eth1, wild-card drop all
> ip xfrm policy add src 0.0.0.0/0 dst 0.0.0.0/0 dev eth1 \
>        dir in ptype main action block priority $SOME-HIGH-value
> #eth1 exception ...
> 
> The problem is this works as long as i dont specify an interface.
> i.e, this would work in the in-direction:
> 
> ip xfrm policy add src 0.0.0.0/0 dst 0.0.0.0/0 \
>         dir in ptype main action block priority $SOME-HIGH-value
> 
> This would not work:
> ip xfrm policy add src 0.0.0.0/0 dst 0.0.0.0/0 dev eth0 \
>        dir in ptype main action block priority $SOME-HIGH-value
> 
> 
> The checks in the selector matching is the culprit, example for v4:
> 
> __xfrm4_selector_match(struct xfrm_selector *sel, struct flowi *fl)
> {
>         return  .... &&
>                 .... &&
>                 (fl->oif == sel->ifindex || !sel->ifindex);
> }
> 
> i.e in the second case i have a non-zero sel->ifindex but
> a zero fl->oif; so it wont match.
> 
> One approach to fix this is to pass the direction then i can do
> in the function call, then i can do something along the lines of
> matching if:
> (fl_dir == FLOW_DIR_IN && (fl->iif == sel->ifindex || !sel->ifindex) ||
> (fl->oif == sel->ifindex || !sel->ifindex);
> 
> Is there any reason the selector matching only assumes fl->oif?
> Are there any unforeseen issues/breakages if i added a check for the
> above.
> 
> cheers,
> jamal

[-- Attachment #2: spd-per-intf --]
[-- Type: text/x-patch, Size: 6044 bytes --]

diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index d74e080..ce18464 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -838,7 +838,7 @@ __be16 xfrm_flowi_dport(struct flowi *fl)
 }
 
 extern int xfrm_selector_match(struct xfrm_selector *sel, struct flowi *fl,
-			       unsigned short family);
+			       unsigned short family, int use_if);
 
 #ifdef CONFIG_SECURITY_NETWORK_XFRM
 /*	If neither has a context --> match
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 843e066..e7e230b 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -55,35 +55,37 @@ static struct xfrm_policy *__xfrm_policy_unlink(struct xfrm_policy *pol,
 						int dir);
 
 static inline int
-__xfrm4_selector_match(struct xfrm_selector *sel, struct flowi *fl)
+__xfrm4_selector_match(struct xfrm_selector *sel, struct flowi *fl, int uif)
 {
+	int use_if = uif ? uif : fl->oif;
 	return  addr_match(&fl->fl4_dst, &sel->daddr, sel->prefixlen_d) &&
 		addr_match(&fl->fl4_src, &sel->saddr, sel->prefixlen_s) &&
 		!((xfrm_flowi_dport(fl) ^ sel->dport) & sel->dport_mask) &&
 		!((xfrm_flowi_sport(fl) ^ sel->sport) & sel->sport_mask) &&
 		(fl->proto == sel->proto || !sel->proto) &&
-		(fl->oif == sel->ifindex || !sel->ifindex);
+		(use_if == sel->ifindex || !sel->ifindex);
 }
 
 static inline int
-__xfrm6_selector_match(struct xfrm_selector *sel, struct flowi *fl)
+__xfrm6_selector_match(struct xfrm_selector *sel, struct flowi *fl, int uif)
 {
+	int use_if = uif ? uif : fl->oif;
 	return  addr_match(&fl->fl6_dst, &sel->daddr, sel->prefixlen_d) &&
 		addr_match(&fl->fl6_src, &sel->saddr, sel->prefixlen_s) &&
 		!((xfrm_flowi_dport(fl) ^ sel->dport) & sel->dport_mask) &&
 		!((xfrm_flowi_sport(fl) ^ sel->sport) & sel->sport_mask) &&
 		(fl->proto == sel->proto || !sel->proto) &&
-		(fl->oif == sel->ifindex || !sel->ifindex);
+		(use_if == sel->ifindex || !sel->ifindex);
 }
 
 int xfrm_selector_match(struct xfrm_selector *sel, struct flowi *fl,
-		    unsigned short family)
+		    unsigned short family, int ifindex)
 {
 	switch (family) {
 	case AF_INET:
-		return __xfrm4_selector_match(sel, fl);
+		return __xfrm4_selector_match(sel, fl, ifindex);
 	case AF_INET6:
-		return __xfrm6_selector_match(sel, fl);
+		return __xfrm6_selector_match(sel, fl, ifindex);
 	}
 	return 0;
 }
@@ -917,14 +919,17 @@ static int xfrm_policy_match(struct xfrm_policy *pol, struct flowi *fl,
 			     u8 type, u16 family, int dir)
 {
 	struct xfrm_selector *sel = &pol->selector;
-	int match, ret = -ESRCH;
+	int use_if = fl->oif, match, ret = -ESRCH;
 
 	if (pol->family != family ||
 	    (fl->mark & pol->mark.m) != pol->mark.v ||
 	    pol->type != type)
 		return ret;
 
-	match = xfrm_selector_match(sel, fl, family);
+	if (dir == FLOW_DIR_IN)
+		use_if = fl->iif;
+
+	match = xfrm_selector_match(sel, fl, family, use_if);
 	if (match)
 		ret = security_xfrm_policy_lookup(pol->security, fl->secid,
 						  dir);
@@ -1041,7 +1046,7 @@ static struct xfrm_policy *xfrm_sk_policy_lookup(struct sock *sk, int dir, struc
 	read_lock_bh(&xfrm_policy_lock);
 	if ((pol = sk->sk_policy[dir]) != NULL) {
 		int match = xfrm_selector_match(&pol->selector, fl,
-						sk->sk_family);
+						sk->sk_family, 0);
 		int err = 0;
 
 		if (match) {
@@ -1918,6 +1923,7 @@ int __xfrm_policy_check(struct sock *sk, int dir, struct sk_buff *skb,
 	struct flowi fl;
 	u8 fl_dir;
 	int xerr_idx = -1;
+	int use_if = 0;
 
 	reverse = dir & ~XFRM_POLICY_MASK;
 	dir &= XFRM_POLICY_MASK;
@@ -1928,6 +1934,9 @@ int __xfrm_policy_check(struct sock *sk, int dir, struct sk_buff *skb,
 		return 0;
 	}
 
+	if (fl_dir ==  FLOW_DIR_IN)
+		use_if = fl.iif = skb->skb_iif;
+
 	nf_nat_decode_session(skb, &fl, family);
 
 	/* First, check used SA against their selectors. */
@@ -1936,7 +1945,7 @@ int __xfrm_policy_check(struct sock *sk, int dir, struct sk_buff *skb,
 
 		for (i=skb->sp->len-1; i>=0; i--) {
 			struct xfrm_state *x = skb->sp->xvec[i];
-			if (!xfrm_selector_match(&x->sel, &fl, family)) {
+			if (!xfrm_selector_match(&x->sel, &fl, family, use_if)) {
 				XFRM_INC_STATS(net, LINUX_MIB_XFRMINSTATEMISMATCH);
 				return 0;
 			}
@@ -1956,6 +1965,7 @@ int __xfrm_policy_check(struct sock *sk, int dir, struct sk_buff *skb,
 		pol = flow_cache_lookup(net, &fl, family, fl_dir,
 					xfrm_policy_lookup);
 
+
 	if (IS_ERR(pol)) {
 		XFRM_INC_STATS(net, LINUX_MIB_XFRMINPOLERROR);
 		return 0;
@@ -2243,7 +2253,7 @@ int xfrm_bundle_ok(struct xfrm_policy *pol, struct xfrm_dst *first,
 		if (first->origin && !flow_cache_uli_match(first->origin, fl))
 			return 0;
 		if (first->partner &&
-		    !xfrm_selector_match(first->partner, fl, family))
+		    !xfrm_selector_match(first->partner, fl, family, 0))
 			return 0;
 	}
 #endif
@@ -2253,7 +2263,7 @@ int xfrm_bundle_ok(struct xfrm_policy *pol, struct xfrm_dst *first,
 	do {
 		struct xfrm_dst *xdst = (struct xfrm_dst *)dst;
 
-		if (fl && !xfrm_selector_match(&dst->xfrm->sel, fl, family))
+		if (fl && !xfrm_selector_match(&dst->xfrm->sel, fl, family, 0))
 			return 0;
 		if (fl && pol &&
 		    !security_xfrm_state_pol_flow_match(dst->xfrm, pol, fl))
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 17d5b96..f47c90b 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -756,7 +756,7 @@ static void xfrm_state_look_at(struct xfrm_policy *pol, struct xfrm_state *x,
 	 */
 	if (x->km.state == XFRM_STATE_VALID) {
 		if ((x->sel.family &&
-		     !xfrm_selector_match(&x->sel, fl, x->sel.family)) ||
+		     !xfrm_selector_match(&x->sel, fl, x->sel.family, 0)) ||
 		    !security_xfrm_state_pol_flow_match(x, pol, fl))
 			return;
 
@@ -769,7 +769,7 @@ static void xfrm_state_look_at(struct xfrm_policy *pol, struct xfrm_state *x,
 		*acq_in_progress = 1;
 	} else if (x->km.state == XFRM_STATE_ERROR ||
 		   x->km.state == XFRM_STATE_EXPIRED) {
-		if (xfrm_selector_match(&x->sel, fl, x->sel.family) &&
+		if (xfrm_selector_match(&x->sel, fl, x->sel.family, 0) &&
 		    security_xfrm_state_pol_flow_match(x, pol, fl))
 			*error = -ESRCH;
 	}

Re: [RFC] SPD basic actions per netdev

[Herbert Xu]

On Wed, Mar 31, 2010 at 12:37:58PM -0400, jamal wrote:
> 
> This may be oversight in current implementation and possibly
> nobody has needed it before - hence it is not functional.
> 
> I want to have a drop-all policy on a per-interface level
> for incoming packets and add exceptions as i need them.
> [using the flow table is cheap if you have xfrm built in].
> i.e something along the lines of:
> 
> #eth0, wild-card drop all
> ip xfrm policy add src 0.0.0.0/0 dst 0.0.0.0/0 dev eth0 \
>        dir in ptype main action block priority $SOME-HIGH-value
> #eth0, exception
> ip xfrm policy add blah blah dev eth0 \
> dir in ptype main action allow priority $SOME-small-value
> #eth1, wild-card drop all
> ip xfrm policy add src 0.0.0.0/0 dst 0.0.0.0/0 dev eth1 \
>        dir in ptype main action block priority $SOME-HIGH-value
> #eth1 exception ...
> 
> The problem is this works as long as i dont specify an interface.
> i.e, this would work in the in-direction:
> 
> ip xfrm policy add src 0.0.0.0/0 dst 0.0.0.0/0 \
>         dir in ptype main action block priority $SOME-HIGH-value
> 
> This would not work:
> ip xfrm policy add src 0.0.0.0/0 dst 0.0.0.0/0 dev eth0 \
>        dir in ptype main action block priority $SOME-HIGH-value
> 
> 
> The checks in the selector matching is the culprit, example for v4:
> 
> __xfrm4_selector_match(struct xfrm_selector *sel, struct flowi *fl)
> {
>         return  .... &&
>                 .... &&
>                 (fl->oif == sel->ifindex || !sel->ifindex);
> }
> 
> i.e in the second case i have a non-zero sel->ifindex but
> a zero fl->oif; so it wont match.
> 
> One approach to fix this is to pass the direction then i can do
> in the function call, then i can do something along the lines of
> matching if:
> (fl_dir == FLOW_DIR_IN && (fl->iif == sel->ifindex || !sel->ifindex) ||
> (fl->oif == sel->ifindex || !sel->ifindex);
> 
> Is there any reason the selector matching only assumes fl->oif?
> Are there any unforeseen issues/breakages if i added a check for the
> above.

If we're going to change this then we should just add a second
interface field to the selector, rather than trying to overload
the existing one.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [RFC] SPD basic actions per netdev

[jamal]

On Thu, 2010-04-01 at 08:33 +0800, Herbert Xu wrote:

> If we're going to change this then we should just add a second
> interface field to the selector, rather than trying to overload
> the existing one.

Do you mean to have a selector->iif/oif? Sure that makes sense - but is
a much larger surgery and user space will need to be taught.

Did you look at the patch i sent? i tried to retain current behavior
except for the input check path. output path was working in classifying
with specific netdevs. 

cheers,
jamal

Re: [RFC] SPD basic actions per netdev

[Herbert Xu]

On Wed, Mar 31, 2010 at 10:35:23PM -0400, jamal wrote:
> On Thu, 2010-04-01 at 08:33 +0800, Herbert Xu wrote:
> 
> > If we're going to change this then we should just add a second
> > interface field to the selector, rather than trying to overload
> > the existing one.
> 
> Do you mean to have a selector->iif/oif? Sure that makes sense - but is
> a much larger surgery and user space will need to be taught.
> 
> Did you look at the patch i sent? i tried to retain current behavior
> except for the input check path. output path was working in classifying
> with specific netdevs. 

OK, I guess the chances of an existing app breaking is slim.

BTW, you should treat FLOW_DIR_FWD as FLOW_DIR_IN.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [RFC] SPD basic actions per netdev

[Timo Teräs]

Herbert Xu wrote:
> On Wed, Mar 31, 2010 at 10:35:23PM -0400, jamal wrote:
>> On Thu, 2010-04-01 at 08:33 +0800, Herbert Xu wrote:
>>
>>> If we're going to change this then we should just add a second
>>> interface field to the selector, rather than trying to overload
>>> the existing one.
>> Do you mean to have a selector->iif/oif? Sure that makes sense - but is
>> a much larger surgery and user space will need to be taught.
>>
>> Did you look at the patch i sent? i tried to retain current behavior
>> except for the input check path. output path was working in classifying
>> with specific netdevs. 
> 
> OK, I guess the chances of an existing app breaking is slim.
> 
> BTW, you should treat FLOW_DIR_FWD as FLOW_DIR_IN.

I think we need iif and oif. The separation is clear in in/fwd policies,
as each is matched properly. But 'out' policies are used for both:
locally generated, and forwarded traffic.

Basically it goes like:
 in - for policy_check for traffic that is received locally
 fwd - for policy_check for traffic that is forwarded
 out - all (local and forwarded) traffic that goes out of box

IMHO, it's slightly confusing that in/fwd is split, but out is not.
But that's the way it works. If you now override the how interface
is checked for 'out' policy, it'll break current behaviour.

- Timo

Re: [RFC] SPD basic actions per netdev

[Herbert Xu]

On Thu, Apr 01, 2010 at 07:52:34AM +0300, Timo Teräs wrote:
>
> IMHO, it's slightly confusing that in/fwd is split, but out is not.
> But that's the way it works. If you now override the how interface
> is checked for 'out' policy, it'll break current behaviour.

Unless I've misunderstood what his patch is trying to do, it would
seem that out policies would be completely unchanged.

Forward policies are not used on output.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [RFC] SPD basic actions per netdev

[Timo Teräs]

Herbert Xu wrote:
> On Thu, Apr 01, 2010 at 07:52:34AM +0300, Timo Teräs wrote:
>> IMHO, it's slightly confusing that in/fwd is split, but out is not.
>> But that's the way it works. If you now override the how interface
>> is checked for 'out' policy, it'll break current behaviour.
> 
> Unless I've misunderstood what his patch is trying to do, it would
> seem that out policies would be completely unchanged.
> 
> Forward policies are not used on output.

Oh, that right.

But my statement still holds. If iif/oif is swapped, it's changing
current semantics and can end up breaking setups. Both are still
valid for 'in' and 'fwd' policies too, right? What if I'm using
'in' policy to make sure that all stuff arriving via 'eth0' is
encrypted, but 'eth1' is trusted and does not need xfrm. This
would break.

I do like the idea very much. In fact I remember asking this exact
feature long time ago. But I think it should be done by explicitly
allowing user to specify both iif and oif; even if it's more
intrusive.

Re: [RFC] SPD basic actions per netdev

[Herbert Xu]

On Thu, Apr 01, 2010 at 09:20:40AM +0300, Timo Teräs wrote:
>
> But my statement still holds. If iif/oif is swapped, it's changing
> current semantics and can end up breaking setups. Both are still
> valid for 'in' and 'fwd' policies too, right? What if I'm using
> 'in' policy to make sure that all stuff arriving via 'eth0' is
> encrypted, but 'eth1' is trusted and does not need xfrm. This
> would break.

The thing is if you're currently specifying an ifindex in the
selector for inbound/forward, it probably just won't work as
it'll be matched against oif which is meaningless on inbound
and forward.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [RFC] SPD basic actions per netdev

[Timo Teräs]

Herbert Xu wrote:
> On Thu, Apr 01, 2010 at 09:20:40AM +0300, Timo Teräs wrote:
>> But my statement still holds. If iif/oif is swapped, it's changing
>> current semantics and can end up breaking setups. Both are still
>> valid for 'in' and 'fwd' policies too, right? What if I'm using
>> 'in' policy to make sure that all stuff arriving via 'eth0' is
>> encrypted, but 'eth1' is trusted and does not need xfrm. This
>> would break.
> 
> The thing is if you're currently specifying an ifindex in the
> selector for inbound/forward, it probably just won't work as
> it'll be matched against oif which is meaningless on inbound
> and forward.

On inbound it's always loopback interface. Does the same hold
true on forward? I was under the impression that it would
reflect the actual destination interface.

Re: [RFC] SPD basic actions per netdev

[Herbert Xu]

On Thu, Apr 01, 2010 at 09:32:06AM +0300, Timo Teräs wrote:
>
> On inbound it's always loopback interface. Does the same hold
> true on forward? I was under the impression that it would
> reflect the actual destination interface.

That's a good point.  I suppose there might just be some crazy
people out there using it this way.

So Jamal, I think this is a good reason why we do need a new
field instead of just overloading the current one.  Otherwise
inbound selectors and forward selectors will have different
semantics which is needlessly confusing.

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [RFC] SPD basic actions per netdev

[jamal]

[-- Attachment #1: Type: text/plain, Size: 1858 bytes --]

On Thu, 2010-04-01 at 14:39 +0800, Herbert Xu wrote:
> On Thu, Apr 01, 2010 at 09:32:06AM +0300, Timo Teräs wrote:
> >
> > On inbound it's always loopback interface. Does the same hold
> > true on forward? I was under the impression that it would
> > reflect the actual destination interface.
> 
> That's a good point.  I suppose there might just be some crazy
> people out there using it this way.
> 
> So Jamal, I think this is a good reason why we do need a new
> field instead of just overloading the current one.  Otherwise
> inbound selectors and forward selectors will have different
> semantics which is needlessly confusing.


So I followed the discussion up to about this point then confusion sets
in for me - in particular about loopback being used for policy_check()
which you guys seem to agree on.
Nod on:  IN+FWD should be treated the same way. Locally generated/OUT
works and I dont muck with that.
The current code is sufficiently clean such that all i need is to worry
about is __xfrm_policy_check() (which is invoked only for IN and FWD).
And thats the only thing i touch - the rest "works as it did before".

[Note: the flow struct used in __xfrm_policy_check() is local to it, so
my touching it affects only the scope of validation of IN/FWD. I dont
see loopback being used for policy check.
Note2: In the FWD policy check, the output dev hasnt been decided
yet at that point. So it sounds fair to define "dev blah" in FWD
direction to mean incoming device (as it is for IN/local destined).]

Q: So if all i want to achieve for now is to make sure that i can
specify a "dev blah" in the forward or in direction and have it work to
identify the incoming device, wouldnt this patch suffice?

I am attaching this patch with a fix to check for FWD as well if you
have a chance i would appreciate if you re-look at it again.

cheers,
jamal


[-- Attachment #2: spd-fw-in --]
[-- Type: text/x-patch, Size: 5827 bytes --]

diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index d74e080..ce18464 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -838,7 +838,7 @@ __be16 xfrm_flowi_dport(struct flowi *fl)
 }
 
 extern int xfrm_selector_match(struct xfrm_selector *sel, struct flowi *fl,
-			       unsigned short family);
+			       unsigned short family, int use_if);
 
 #ifdef CONFIG_SECURITY_NETWORK_XFRM
 /*	If neither has a context --> match
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 843e066..cad67b3 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -55,35 +55,37 @@ static struct xfrm_policy *__xfrm_policy_unlink(struct xfrm_policy *pol,
 						int dir);
 
 static inline int
-__xfrm4_selector_match(struct xfrm_selector *sel, struct flowi *fl)
+__xfrm4_selector_match(struct xfrm_selector *sel, struct flowi *fl, int uif)
 {
+	int use_if = uif ? uif : fl->oif;
 	return  addr_match(&fl->fl4_dst, &sel->daddr, sel->prefixlen_d) &&
 		addr_match(&fl->fl4_src, &sel->saddr, sel->prefixlen_s) &&
 		!((xfrm_flowi_dport(fl) ^ sel->dport) & sel->dport_mask) &&
 		!((xfrm_flowi_sport(fl) ^ sel->sport) & sel->sport_mask) &&
 		(fl->proto == sel->proto || !sel->proto) &&
-		(fl->oif == sel->ifindex || !sel->ifindex);
+		(use_if == sel->ifindex || !sel->ifindex);
 }
 
 static inline int
-__xfrm6_selector_match(struct xfrm_selector *sel, struct flowi *fl)
+__xfrm6_selector_match(struct xfrm_selector *sel, struct flowi *fl, int uif)
 {
+	int use_if = uif ? uif : fl->oif;
 	return  addr_match(&fl->fl6_dst, &sel->daddr, sel->prefixlen_d) &&
 		addr_match(&fl->fl6_src, &sel->saddr, sel->prefixlen_s) &&
 		!((xfrm_flowi_dport(fl) ^ sel->dport) & sel->dport_mask) &&
 		!((xfrm_flowi_sport(fl) ^ sel->sport) & sel->sport_mask) &&
 		(fl->proto == sel->proto || !sel->proto) &&
-		(fl->oif == sel->ifindex || !sel->ifindex);
+		(use_if == sel->ifindex || !sel->ifindex);
 }
 
 int xfrm_selector_match(struct xfrm_selector *sel, struct flowi *fl,
-		    unsigned short family)
+		    unsigned short family, int ifindex)
 {
 	switch (family) {
 	case AF_INET:
-		return __xfrm4_selector_match(sel, fl);
+		return __xfrm4_selector_match(sel, fl, ifindex);
 	case AF_INET6:
-		return __xfrm6_selector_match(sel, fl);
+		return __xfrm6_selector_match(sel, fl, ifindex);
 	}
 	return 0;
 }
@@ -917,14 +919,17 @@ static int xfrm_policy_match(struct xfrm_policy *pol, struct flowi *fl,
 			     u8 type, u16 family, int dir)
 {
 	struct xfrm_selector *sel = &pol->selector;
-	int match, ret = -ESRCH;
+	int use_if = 0, match, ret = -ESRCH;
 
 	if (pol->family != family ||
 	    (fl->mark & pol->mark.m) != pol->mark.v ||
 	    pol->type != type)
 		return ret;
 
-	match = xfrm_selector_match(sel, fl, family);
+	if (dir == FLOW_DIR_IN || dir == FLOW_DIR_FWD)
+		use_if = fl->iif;
+
+	match = xfrm_selector_match(sel, fl, family, use_if);
 	if (match)
 		ret = security_xfrm_policy_lookup(pol->security, fl->secid,
 						  dir);
@@ -1041,7 +1046,7 @@ static struct xfrm_policy *xfrm_sk_policy_lookup(struct sock *sk, int dir, struc
 	read_lock_bh(&xfrm_policy_lock);
 	if ((pol = sk->sk_policy[dir]) != NULL) {
 		int match = xfrm_selector_match(&pol->selector, fl,
-						sk->sk_family);
+						sk->sk_family, 0);
 		int err = 0;
 
 		if (match) {
@@ -1918,6 +1923,7 @@ int __xfrm_policy_check(struct sock *sk, int dir, struct sk_buff *skb,
 	struct flowi fl;
 	u8 fl_dir;
 	int xerr_idx = -1;
+	int use_if = 0;
 
 	reverse = dir & ~XFRM_POLICY_MASK;
 	dir &= XFRM_POLICY_MASK;
@@ -1928,6 +1934,9 @@ int __xfrm_policy_check(struct sock *sk, int dir, struct sk_buff *skb,
 		return 0;
 	}
 
+	if (fl_dir ==  FLOW_DIR_IN || fl_dir ==  FLOW_DIR_FWD)
+		fl.iif = use_if = skb->skb_iif;
+
 	nf_nat_decode_session(skb, &fl, family);
 
 	/* First, check used SA against their selectors. */
@@ -1936,7 +1945,7 @@ int __xfrm_policy_check(struct sock *sk, int dir, struct sk_buff *skb,
 
 		for (i=skb->sp->len-1; i>=0; i--) {
 			struct xfrm_state *x = skb->sp->xvec[i];
-			if (!xfrm_selector_match(&x->sel, &fl, family)) {
+			if (!xfrm_selector_match(&x->sel, &fl, family, use_if)) {
 				XFRM_INC_STATS(net, LINUX_MIB_XFRMINSTATEMISMATCH);
 				return 0;
 			}
@@ -2243,7 +2252,7 @@ int xfrm_bundle_ok(struct xfrm_policy *pol, struct xfrm_dst *first,
 		if (first->origin && !flow_cache_uli_match(first->origin, fl))
 			return 0;
 		if (first->partner &&
-		    !xfrm_selector_match(first->partner, fl, family))
+		    !xfrm_selector_match(first->partner, fl, family, 0))
 			return 0;
 	}
 #endif
@@ -2253,7 +2262,7 @@ int xfrm_bundle_ok(struct xfrm_policy *pol, struct xfrm_dst *first,
 	do {
 		struct xfrm_dst *xdst = (struct xfrm_dst *)dst;
 
-		if (fl && !xfrm_selector_match(&dst->xfrm->sel, fl, family))
+		if (fl && !xfrm_selector_match(&dst->xfrm->sel, fl, family, 0))
 			return 0;
 		if (fl && pol &&
 		    !security_xfrm_state_pol_flow_match(dst->xfrm, pol, fl))
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 17d5b96..f47c90b 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -756,7 +756,7 @@ static void xfrm_state_look_at(struct xfrm_policy *pol, struct xfrm_state *x,
 	 */
 	if (x->km.state == XFRM_STATE_VALID) {
 		if ((x->sel.family &&
-		     !xfrm_selector_match(&x->sel, fl, x->sel.family)) ||
+		     !xfrm_selector_match(&x->sel, fl, x->sel.family, 0)) ||
 		    !security_xfrm_state_pol_flow_match(x, pol, fl))
 			return;
 
@@ -769,7 +769,7 @@ static void xfrm_state_look_at(struct xfrm_policy *pol, struct xfrm_state *x,
 		*acq_in_progress = 1;
 	} else if (x->km.state == XFRM_STATE_ERROR ||
 		   x->km.state == XFRM_STATE_EXPIRED) {
-		if (xfrm_selector_match(&x->sel, fl, x->sel.family) &&
+		if (xfrm_selector_match(&x->sel, fl, x->sel.family, 0) &&
 		    security_xfrm_state_pol_flow_match(x, pol, fl))
 			*error = -ESRCH;
 	}

Re: [RFC] SPD basic actions per netdev

[Timo Teräs]

jamal wrote:
> Q: So if all i want to achieve for now is to make sure that i can
> specify a "dev blah" in the forward or in direction and have it work to
> identify the incoming device, wouldnt this patch suffice?
> 
> I am attaching this patch with a fix to check for FWD as well if you
> have a chance i would appreciate if you re-look at it again.

The thing is that currently FWD 'dev blah' matches the interface
to which the packet is being forwarded to. Someone might be using
this feature already.

Your patch changes semantics on how FWD policies are matched.

Re: [RFC] SPD basic actions per netdev

[jamal]

On Thu, 2010-04-01 at 14:47 +0300, Timo Teräs wrote:

> 
> The thing is that currently FWD 'dev blah' matches the interface
> to which the packet is being forwarded to. Someone might be using
> this feature already.

So this is the part i am missing i think. If i look at:

int ip_forward(struct sk_buff *skb)
{
.....
        if (!xfrm4_policy_check(NULL, XFRM_POLICY_FWD, skb))
                goto drop;
....
........later forwarding happens here ...
        if (!xfrm4_route_forward(skb))
                goto drop;
...
}

On entry we have a legit skb->skb_iif.
The validity check is before forwarding decision (where the interface
the packet is being forwarded to is recognized).

> Your patch changes semantics on how FWD policies are matched.

I agree if what you say earlier is true.

cheers,
jamal

Re: [RFC] SPD basic actions per netdev

[Timo Teräs]

jamal wrote:
> On Thu, 2010-04-01 at 14:47 +0300, Timo Teräs wrote:
> 
>> The thing is that currently FWD 'dev blah' matches the interface
>> to which the packet is being forwarded to. Someone might be using
>> this feature already.
> 
> So this is the part i am missing i think. If i look at:
> 
> int ip_forward(struct sk_buff *skb)
> {
> .....
>         if (!xfrm4_policy_check(NULL, XFRM_POLICY_FWD, skb))
>                 goto drop;
> ....
> ........later forwarding happens here ...
>         if (!xfrm4_route_forward(skb))
>                 goto drop;
> ...
> }
> 
> On entry we have a legit skb->skb_iif.
> The validity check is before forwarding decision (where the interface
> the packet is being forwarded to is recognized).

On entry to ip_forward the routing decision has already been made.
Both oif and iif are valid on entry.

Currently policy_check() uses oif for SPD matching.

Do note that xfrm4_route_forward() is a no-op if there's no matching
policy. It has nothing to do with routing decision, it's purpose
is to wrap the dst_entry with xfrm_dst if the flow matches a valid
SPD.

>> Your patch changes semantics on how FWD policies are matched.
> 
> I agree if what you say earlier is true.

Re: [RFC] SPD basic actions per netdev

[jamal]

On Thu, 2010-04-01 at 15:10 +0300, Timo Teräs wrote:

> On entry to ip_forward the routing decision has already been made.
> Both oif and iif are valid on entry.

ah, ok - yes;->

> Currently policy_check() uses oif for SPD matching.

indeed it does.

So i can see the dilemma with fwd path. It would be nice
to be able to classify on both iif and oif.

So that leaves only IN direction. If i only worried about that
and use skb->skb_iif then at least i wont be breaking the semantics
for FWD/OUT (i.e the patch without check for FWD). 

That would make semantics for selector ifindex as follows:
table    current    patch
----------------------------
OUT       fl->oif   fl->oif
FWD       fl->oif   fl->oif
IN        N/A       skb->skb_iif

By "N/A" it means really you cant set it. If you do it doesnt work.

cheers,
jamal