Page MenuHomeFreeBSD

IPSEC SPD searching is slow perform it only once during forwarding
AbandonedPublic

Authored by eri on Jul 3 2015, 6:29 PM.

Details

Reviewers
ae
gnn
Summary

SPD lookup is done twice for every packet when IPSEC is active this impacts the forwarding performance.
Reduce the overhead by doing the search only once on the forwarding path.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

eri updated this revision to Diff 6675.Jul 3 2015, 6:29 PM
eri retitled this revision from to IPSEC SPD searching is slow perform it only once during forwarding.
eri updated this object.
eri edited the test plan for this revision. (Show Details)
eri added reviewers: ae, gnn.
eri set the repository for this revision to rS FreeBSD src repository.
eri added a project: network.
eri added a subscriber: network.
eri updated this revision to Diff 6676.Jul 3 2015, 6:30 PM

Increment can't forward counter only for forwarded packets.

eri updated this revision to Diff 6677.Jul 3 2015, 9:00 PM

Correct paramter on ip_ipsec to ipsec_in_reject

eri updated this revision to Diff 6682.EditedJul 4 2015, 8:14 AM
eri updated this object.

After more testing, with help of Olivier, keep the workaround of testing if any SP is present to not impact general forwarding in non-IPSEC usage case.

In D2986#58711, @eri wrote:

After more testing, with help of Olivier, keep the workaround of testing if any SP is present to not impact general forwarding in non-IPSEC usage case.

Here is the forwarding bench results (value in paquet-per-second) without any SP configured:

x forwarding-NOIPSEC-in-kernel (head r285000)
+ forwarding-IPSEC-in-kernel-but-not-enabled (head r285000)
* forwarding-D2986-IPSEC-in-kernel-but-not-enabled (head r285000 with D2986) 
+--------------------------------------------------------------------------+
|*                    * *  x*     +      +  xxx+ *+          +            x|
|                             |______________M_A________________|          |
|                                   |__________A_________|                 |
|       |_______________MA________________|                                |
+--------------------------------------------------------------------------+
    N           Min           Max        Median           Avg        Stddev
x   5       1180885       1310062       1231130       1236671     46402.755
+   5       1200343       1273193       1236747     1234650.6       27525.7
No difference proven at 95.0% confidence
*   5       1109849       1241128       1172876     1175047.6     46705.073
No difference proven at 95.0% confidence
eri updated this revision to Diff 6685.Jul 4 2015, 8:53 AM

Simplify logic in ip6_forward.c code by gathering the whole code under the same IPSEC ifdef.
This also allows next change on moving all these code to IPSEC specifc files and helps for the goal of having IPSEC as module.

ae edited edge metadata.Jul 4 2015, 5:56 PM

First time SPD is looked for INPUT policy and you have removed this functionality.

eri added a comment.Jul 4 2015, 6:12 PM

Andrey what is the use case for that?

eri updated this revision to Diff 6739.Jul 6 2015, 9:40 PM
eri edited edge metadata.

Another update by doing the search on the incoming policies only if there is one outgoing.

Normally the SPs are always in double and managed by a daemon!
The search is only for testing if this packet needs to be dropped or all IPsec processing has been done.

ae added a comment.Jul 7 2015, 9:44 AM

Regarding of the use case. I think such INBOUND policy can be used like a firewall rule. You can add a DISCARD policy for some traffic flows. We don't use them, but someone can use.

sys/netinet6/ip6_forward.c
155

In case when you have no OUTBOUND policies, but have some INBOUND DISCARD policy, SPD will not be applied and packet will not be rejected.

eri added a comment.Jul 7 2015, 10:22 AM

So you are saying someone will only create a IN SP for just discarding packets without any ipsec policy in place?
Sounds like a very weird scenario when the user can just load a module in previous versions of FreeBSD for firewalling!

The above patch supports still the inbound filtering/discarding but relies on the user to have some ipsec policy already in place to protect packets.
Also the same consistency/check is not maintained on applications using IPSEC activated/encapsulated sockets.

I also understand that this code path also has the purpose of trying to check if the full policy has been applied to the packet but it skips some locking and its not correct, just that the issue is not hit often since SPDs/ISR on IPSEC do not get modified very often as they are read-mostly properties.

ae added inline comments.Jul 7 2015, 10:22 AM
sys/netinet6/ip6_forward.c
295

This part of comment has become stale.

ae added a comment.Jul 7 2015, 10:30 AM

I just noticed, that your change breaks some of the functionality. I personally don't use it :)

ae added a comment.Jul 7 2015, 10:42 AM
In D2986#59234, @eri wrote:

I also understand that this code path also has the purpose of trying to check if the full policy has been applied to the packet but it skips some locking and its not correct, just that the issue is not hit often since SPDs/ISR on IPSEC do not get modified very often as they are read-mostly properties.

Each consumer of SPD entry holds reference to SPD, and there is no way to modify SPD entry. You only can unlink existing entry from the chain and add new entry. So, I don't see the problem here. The main problem is with locking of SAD and ISRs.

eri added a comment.EditedJul 7 2015, 10:53 AM
In D2986#59244, @ae wrote:
In D2986#59234, @eri wrote:

I also understand that this code path also has the purpose of trying to check if the full policy has been applied to the packet but it skips some locking and its not correct, just that the issue is not hit often since SPDs/ISR on IPSEC do not get modified very often as they are read-mostly properties.

Each consumer of SPD entry holds reference to SPD, and there is no way to modify SPD entry. You only can unlink existing entry from the chain and add new entry. So, I don't see the problem here. The main problem is with locking of SAD and ISRs.

Yes, that is correct that ISR and SAD are the problematic ones especially ISR needs more thought.

gnn edited edge metadata.Jul 27 2015, 2:56 PM

This looks stuck. Ermal, do you want to take another try on this one?

gnn edited edge metadata.Oct 28 2015, 6:11 PM
gnn added a subscriber: loos.
eri abandoned this revision.Feb 20 2017, 11:44 PM