Project

General

Profile

Feature #2677

pfkey interface: do not update SPs if not needed

Added by Emeric Poupon about 2 years ago. Updated about 2 years ago.

Status:
Closed
Priority:
Normal
Category:
kernel-interface
Target version:
Start date:
01.06.2018
Due date:
Estimated time:
Resolution:
Fixed

Description

Hello,

As discussed here (https://lists.strongswan.org/pipermail/dev/2018-April/001910.html), charon updates SPs even if there is no change.

Please see the attached patch proposal to not update the SP if it is the same.

Regards,

nospupdate.patch (703 Bytes) nospupdate.patch Emeric Poupon, 01.06.2018 09:15

Associated revisions

Revision 50c4c1bb (diff)
Added by Tobias Brunner about 2 years ago

kernel-pfkey: Avoid updating policies if nothing significant changed

The FreeBSD kernel doesn't update policies atomically, causing
unnecessary traffic loss during simple rekeyings.

Fixes #2677.

History

#1 Updated by Tobias Brunner about 2 years ago

  • Status changed from New to Feedback

The patch is not enough for upstream, but if it works for you, go ahead and use it.

#2 Updated by Emeric Poupon about 2 years ago

Tobias Brunner wrote:

The patch is not enough for upstream, but if it works for you, go ahead and use it.

Hello,

Do you plan to make a clean patch for this?
What would be needed to upstream it?

#3 Updated by Tobias Brunner about 2 years ago

Do you plan to make a clean patch for this?

I hadn't planned to do that, because I think it should rather be fixed in the kernel.

What would be needed to upstream it?

Compare more data that could potentially be different and is relevant for policy installation (e.g. priority, reqid, IPs). And accessing current_sa is unsafe this way (it's not initialized so you at least have to first check found to be reasonably sure that the enumerated list wasn't empty and a value was assigned).

I pushed something to that end to the 2677-pfkey-policy-update branch (only compile-tested on Linux).

#4 Updated by Tobias Brunner about 2 years ago

  • Category changed from libcharon to kernel-interface
  • Assignee set to Tobias Brunner

#5 Updated by Emeric Poupon about 2 years ago

Tobias Brunner wrote:

Do you plan to make a clean patch for this?

I hadn't planned to do that, because I think it should rather be fixed in the kernel.

What would be needed to upstream it?

Compare more data that could potentially be different and is relevant for policy installation (e.g. priority, reqid, IPs). And accessing current_sa is unsafe this way (it's not initialized so you at least have to first check found to be reasonably sure that the enumerated list wasn't empty and a value was assigned).

I pushed something to that end to the 2677-pfkey-policy-update branch (only compile-tested on Linux).

Thanks for you support!
Do you want us to test your branch so that you can merge it in the next release?

#6 Updated by Emeric Poupon about 2 years ago

Tobias Brunner wrote:

Do you plan to make a clean patch for this?

I hadn't planned to do that, because I think it should rather be fixed in the kernel.

Yes, I do agree with you about the initial problem (the race that must be fixed in the kernel)

Here the point is more to not bother the kernel to update something that does not need to be updated (when you renegotiate dozens of tunnels per second on very large configurations, there is enough pressure on the kernel and its locks)

#7 Updated by Tobias Brunner about 2 years ago

  • Target version set to 5.7.0

Do you plan to make a clean patch for this?

I hadn't planned to do that, because I think it should rather be fixed in the kernel.

Yes, I do agree with you about the initial problem (the race that must be fixed in the kernel)

It doesn't actually seem to be a race, the kernel simply removes the existing policy before creating and adding the new one (without holding any locks).

Here the point is more to not bother the kernel to update something that does not need to be updated (when you renegotiate dozens of tunnels per second on very large configurations, there is enough pressure on the kernel and its locks)

Yes, I agree.

Do you want us to test your branch so that you can merge it in the next release?

That would be great. Thinking about this over lunch, I noticed that adding policies is not actually that much of a problem. Because that results only in update == TRUE if the priority is higher, or there is a difference in the reqids, in which case it makes sense to update the policy anyway (if the priority is the same or lower, there will be no update - that was always the case to avoid unnecessary updates). However, when removing policies after a rekeying and a new state is moved to the front of the list, we should check whether an update is necessary (we can do the check in both cases e.g. if running on older kernels that don't yet have the priority field). I've updated the branch accordingly.

#8 Updated by Emeric Poupon about 2 years ago

Tobias Brunner wrote:

Do you plan to make a clean patch for this?

I hadn't planned to do that, because I think it should rather be fixed in the kernel.

Yes, I do agree with you about the initial problem (the race that must be fixed in the kernel)

It doesn't actually seem to be a race, the kernel simply removes the existing policy before creating and adding the new one (without holding any locks).

Here the point is more to not bother the kernel to update something that does not need to be updated (when you renegotiate dozens of tunnels per second on very large configurations, there is enough pressure on the kernel and its locks)

Yes, I agree.

Do you want us to test your branch so that you can merge it in the next release?

That would be great. Thinking about this over lunch, I noticed that adding policies is not actually that much of a problem. Because that results only in update == TRUE if the priority is higher, or there is a difference in the reqids, in which case it makes sense to update the policy anyway (if the priority is the same or lower, there will be no update - that was always the case to avoid unnecessary updates). However, when removing policies after a rekeying and a new state is moved to the front of the list, we should check whether an update is necessary (we can do the check in both cases e.g. if running on older kernels that don't yet have the priority field). I've updated the branch accordingly.

Hello,

I confirm the fix you provided is OK with our tests (no regression) and it does fix the issue.
Thanks again for your support.

Emeric

#9 Updated by Tobias Brunner about 2 years ago

  • Status changed from Feedback to Closed
  • Resolution set to Fixed

I confirm the fix you provided is OK with our tests (no regression) and it does fix the issue.

Great, thanks for testing. Pushed to master.

Also available in: Atom PDF