| Message ID | 1347283338-4249-3-git-send-email-nicolas.dichtel@6wind.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On 09/10/2012 09:22 AM, Nicolas Dichtel wrote: > When a policy is inserted or deleted, all dst should be recalculated. > > Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> > --- > net/xfrm/xfrm_policy.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c > index 741a32a..67f456d 100644 > --- a/net/xfrm/xfrm_policy.c > +++ b/net/xfrm/xfrm_policy.c > @@ -602,6 +602,7 @@ int xfrm_policy_insert(int dir, struct xfrm_policy *policy, int excl) > xfrm_pol_hold(policy); > net->xfrm.policy_count[dir]++; > atomic_inc(&flow_cache_genid); > + rt_genid_bump(net); > if (delpol) > __xfrm_policy_unlink(delpol, dir); > policy->index = delpol ? delpol->index : xfrm_gen_index(net, dir); > What about security_load_policy() and security_set_bools(). They also bumps the flow_cache_genid by way of selinux_xfrm_notify_policyload(). -vlad -- 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
Le 10/09/2012 16:21, Vlad Yasevich a écrit : > On 09/10/2012 09:22 AM, Nicolas Dichtel wrote: >> When a policy is inserted or deleted, all dst should be recalculated. >> >> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> >> --- >> net/xfrm/xfrm_policy.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c >> index 741a32a..67f456d 100644 >> --- a/net/xfrm/xfrm_policy.c >> +++ b/net/xfrm/xfrm_policy.c >> @@ -602,6 +602,7 @@ int xfrm_policy_insert(int dir, struct xfrm_policy >> *policy, int excl) >> xfrm_pol_hold(policy); >> net->xfrm.policy_count[dir]++; >> atomic_inc(&flow_cache_genid); >> + rt_genid_bump(net); >> if (delpol) >> __xfrm_policy_unlink(delpol, dir); >> policy->index = delpol ? delpol->index : xfrm_gen_index(net, dir); >> > > What about security_load_policy() and security_set_bools(). They also bumps the > flow_cache_genid by way of selinux_xfrm_notify_policyload(). Right. I'm not familiar with this part, but it seems you're right, rt_genid should be bumped too. -- 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
The goal of these patches is to fix the following problem: a session is
established (TCP, SCTP) and after a new policy is inserted. The current
code does not recalculate the route, thus the traffic is not encrypted.
The patch propose to check flow_cache_genid value when checking a dst
entry, which is incremented each time a policy is inserted or deleted.
v2: use net->ipv4.rt_genid instead of flow_cache_genid (and thus save a test
in fast path). Also move it to net->rt_genid, to be able to use it for IPv6
too. Note that IPv6 will have one more test in fast path.
v3: remove unrelated "#ifdef CONFIG_XFRM" in IPv6 part
bump rt_genid in selinux code (same place than flow_cache_genid)
Patches are tested with TCP and SCTP, IPv4 and IPv6.
Comments are welcome.
Regards,
Nicolas
--
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
From: Nicolas Dichtel <nicolas.dichtel@6wind.com> Date: Tue, 11 Sep 2012 10:09:43 +0200 > The goal of these patches is to fix the following problem: a session is > established (TCP, SCTP) and after a new policy is inserted. The current > code does not recalculate the route, thus the traffic is not encrypted. > > The patch propose to check flow_cache_genid value when checking a dst > entry, which is incremented each time a policy is inserted or deleted. > > v2: use net->ipv4.rt_genid instead of flow_cache_genid (and thus save a test > in fast path). Also move it to net->rt_genid, to be able to use it for IPv6 > too. Note that IPv6 will have one more test in fast path. > > v3: remove unrelated "#ifdef CONFIG_XFRM" in IPv6 part > bump rt_genid in selinux code (same place than flow_cache_genid) > > Patches are tested with TCP and SCTP, IPv4 and IPv6. These patches don't apply cleanly at all. In the net/ipv4/route.c code we don't initialize the genid to zero, we stick a random value there. And we don't increment it by one on flushes, instead we increment it by a random amount. I wonder what tree these were even against, the differences were so great. -- 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 09/17/2012 12:49 PM, David Miller wrote: > From: Nicolas Dichtel <nicolas.dichtel@6wind.com> > Date: Tue, 11 Sep 2012 10:09:43 +0200 > >> The goal of these patches is to fix the following problem: a session is >> established (TCP, SCTP) and after a new policy is inserted. The current >> code does not recalculate the route, thus the traffic is not encrypted. >> >> The patch propose to check flow_cache_genid value when checking a dst >> entry, which is incremented each time a policy is inserted or deleted. >> >> v2: use net->ipv4.rt_genid instead of flow_cache_genid (and thus save a test >> in fast path). Also move it to net->rt_genid, to be able to use it for IPv6 >> too. Note that IPv6 will have one more test in fast path. >> >> v3: remove unrelated "#ifdef CONFIG_XFRM" in IPv6 part >> bump rt_genid in selinux code (same place than flow_cache_genid) >> >> Patches are tested with TCP and SCTP, IPv4 and IPv6. > > These patches don't apply cleanly at all. > > In the net/ipv4/route.c code we don't initialize the genid to zero, > we stick a random value there. > > And we don't increment it by one on flushes, instead we increment > it by a random amount. > > I wonder what tree these were even against, the differences were > so great. > I think he expected you to take Eric's patch that removed those pieces. -vlad -- 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
From: Vlad Yasevich <vyasevich@gmail.com> Date: Mon, 17 Sep 2012 14:14:35 -0400 > I think he expected you to take Eric's patch that removed those > pieces. Eric's patch was a cleanup, so it went into 'net-next'. Nicolas's patch is a bonafide bug fix so needs to be targetted at 'net' -- 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
Le 17/09/2012 20:25, David Miller a écrit : > From: Vlad Yasevich <vyasevich@gmail.com> > Date: Mon, 17 Sep 2012 14:14:35 -0400 > >> I think he expected you to take Eric's patch that removed those >> pieces. > > Eric's patch was a cleanup, so it went into 'net-next'. > > Nicolas's patch is a bonafide bug fix so needs to be > targetted at 'net' My fault, it was 'net-next'. I will rebase them against 'net'. Should I post the last one (4/4) separately, against net-next? Because without the '3/4', the cleanup is a bit larger and will cause a conflict during the merge. -- 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
From: Nicolas Dichtel <nicolas.dichtel@6wind.com> Date: Mon, 17 Sep 2012 21:52:20 +0200 > Le 17/09/2012 20:25, David Miller a écrit : >> From: Vlad Yasevich <vyasevich@gmail.com> >> Date: Mon, 17 Sep 2012 14:14:35 -0400 >> >>> I think he expected you to take Eric's patch that removed those >>> pieces. >> >> Eric's patch was a cleanup, so it went into 'net-next'. >> >> Nicolas's patch is a bonafide bug fix so needs to be >> targetted at 'net' > My fault, it was 'net-next'. I will rebase them against 'net'. > > Should I post the last one (4/4) separately, against net-next? Because > without the '3/4', the cleanup is a bit larger and will cause a > conflict during the merge. Actually Nicolas, hold off on this for a bit. I'm reconsidering putting Eric's patch into 'net' and that would allow me to use your v3 patches as-is. Thanks. -- 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
From: David Miller <davem@davemloft.net> Date: Mon, 17 Sep 2012 15:54:58 -0400 (EDT) > I'm reconsidering putting Eric's patch into 'net' and that would > allow me to use your v3 patches as-is. And that's what I've done just now, thanks. -- 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/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c index 741a32a..67f456d 100644 --- a/net/xfrm/xfrm_policy.c +++ b/net/xfrm/xfrm_policy.c @@ -602,6 +602,7 @@ int xfrm_policy_insert(int dir, struct xfrm_policy *policy, int excl) xfrm_pol_hold(policy); net->xfrm.policy_count[dir]++; atomic_inc(&flow_cache_genid); + rt_genid_bump(net); if (delpol) __xfrm_policy_unlink(delpol, dir); policy->index = delpol ? delpol->index : xfrm_gen_index(net, dir);
When a policy is inserted or deleted, all dst should be recalculated. Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> --- net/xfrm/xfrm_policy.c | 1 + 1 file changed, 1 insertion(+)