Page MenuHomeFreeBSD

Optimize the case where we have IPSEC enabled but do not have security policies.
ClosedPublic

Authored by gnn on Oct 23 2015, 8:17 PM.
Tags
None
Referenced Files
Unknown Object (File)
Dec 20 2023, 12:08 AM
Unknown Object (File)
Nov 11 2023, 7:08 AM
Unknown Object (File)
Nov 11 2023, 4:43 AM
Unknown Object (File)
Jun 24 2023, 9:57 PM
Unknown Object (File)
May 5 2023, 9:46 AM
Unknown Object (File)
Feb 18 2023, 7:27 AM
Unknown Object (File)
Feb 16 2023, 4:16 AM
Unknown Object (File)
Jan 7 2023, 3:24 PM

Details

Summary

Turning on IPSEC used to introduce a slight amount of performance degradation (7%) for host host TCP connections over 10Gbps links, even when there were no secuirty policies in place. There is no change in performance on 1Gbps network links. Testing GENERIC vs. GENERIC-NOIPSEC vs. GENERIC with this change shows that the new code removes any overhead introduced by having IPSEC always in the kernel.

Test Plan

Tested using iperf-twohost-nooffload test in the netperf repo. NICs that have TSO/LRO are unaffected by the IPSEC code because they bypass much of the stack. The test turns off these features before executing.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 913
Build 913: arc lint + arc unit

Event Timeline

gnn retitled this revision from to Optimize the case where we have IPSEC enabled but do not have security policies..
gnn updated this object.
gnn edited the test plan for this revision. (Show Details)

My only comment is that if you have a simple enough test.. i.e. "are there any policies (regardless of direction) then I'd go for guarding the callers rather that returning immediately.. in other words don't even do the call.
amd64 may have very cheap function calls but they still cause register spills etc. some other processors may cost more..

general concept is good though.

ipsec_hdrsiz is calle d in 6 places (according to fxr) but some of those places offer a much larger saving than just the function call. Some largish #ifdef IPSEC clauses could be wrapped with a guarding 'if'..

ipsec46_in_reject in 2 places (in the same file), but they are already in ipsec (ipsec4_in_reject and ipsec6_in_reject).. guarding THEM saves TWO function calls (*) and you'd probbaly only have to guard he ones not in ipsec as I think THEY are called from both within and without ipsec. ((*)though it might get inlined)
ipsec_hdrsiz_tcp in 1 place and might not win much either way
ip_ipsec_output in 1 place, but skipping the call would also include skipping the switch statement used for the return value.

Only one of the checks is completely generic, most all of the rest have an idea of the direction of the packets.

The calls to ipsec_hdrsiz are already protected in the place you point out. See the tcp_subr.c change above.

I prefer to keep these changes as encapsulated as possible within IPSEC rather than unduly polluting the rest of the stack with them.

That being said, a later pass might address these issues.

This revision is now accepted and ready to land.Oct 27 2015, 12:48 AM

I know this has been committed but please check my remarks.

sys/netinet/ip_ipsec.c
165

Shouldn't this be direction OUT?

sys/netinet/ip_ipsec.c
165

Good catch! Fixing now.

I discovered that this change breaks 'net.inet.ipsec.def_policy' handling. If you set default_policy to "DISCARD", it will not work until you add some security policies to SPD. Don't know how it is useful or popular, but this probably should be mentioned somewhere.