Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

xfrm refcount ips #1191

Closed
wants to merge 3 commits into from
Closed

xfrm refcount ips #1191

wants to merge 3 commits into from

Conversation

bradyallenjohnson
Copy link
Contributor

No description provided.

@bradyallenjohnson bradyallenjohnson force-pushed the topic/xfrm_refcount_ips branch 3 times, most recently from ea129f4 to 1a267ec Compare July 14, 2023 11:33
@antonyantony
Copy link
Collaborator

antonyantony commented Jul 14, 2023 via email

@bradyallenjohnson
Copy link
Contributor Author

bradyallenjohnson commented Jul 14, 2023 •
edited

@antonyantony Thanks for trying this, I havent gotten that far yet. I first tried with the simplest test basic-pluto-01 and get a failure: https://pastebin.com/6QjzjSaj I also get that failure on the git main branch, so Im not sure what is happening.

I believe the syntax should be interface-ip= without the left/right prefix. I'll try testing manually now too.

EDIT: The syntax is actually with the left/right prefix.

@bradyallenjohnson bradyallenjohnson force-pushed the topic/xfrm_refcount_ips branch 3 times, most recently from a240ba4 to 6c72022 Compare July 20, 2023 14:23
@bradyallenjohnson bradyallenjohnson marked this pull request as ready for review July 20, 2023 15:23
@bradyallenjohnson bradyallenjohnson force-pushed the topic/xfrm_refcount_ips branch 3 times, most recently from 251a51b to efa8299 Compare August 2, 2023 14:49
@cagney
Copy link
Collaborator

cagney commented Aug 2, 2023 

$ ./kvm failed
testing/pluto/ikev2-xfrmi-06 failed east:output-different road:iscntrl,output-different
testing/pluto/ikev2-xfrmi-08 failed west:LEAK,PRINTF_NULL,output-different
testing/pluto/ikev2-xfrmi-10 failed west:LEAK,PRINTF_NULL,output-different
testing/pluto/ikev1-xfrmi-05-remote-access-client failed east:output-different road:output-different

@cagney
Copy link
Collaborator

cagney commented Aug 2, 2023 

--- MASTER/testing/pluto/ikev2-xfrmi-06/road.console.txt
+++ OUTPUT/testing/pluto/ikev2-xfrmi-06/road.console.txt
@@ -22,6 +22,7 @@
 002 "road"[1] 192.1.2.23 #2: received INTERNAL_IP4_DNS 1.2.3.4
 002 "road"[1] 192.1.2.23 #2: received INTERNAL_IP4_DNS 8.8.8.8
 002 "road"[1] 192.1.2.23 #2: up-client output: updating resolvconf
+002 "road"[1] 192.1.2.23 #2: route-client output: PATH/libexec/ipsec/_updown.xfrm: doroute "ip -4 route replace 0.0.0.0/1   dev ipsec1 src 192.0.2.1 && 		ip -4 route replace 128.0.0.0/1   dev ipsec1 src 192.0.2.1" failed (Error: Invalid prefsrc address.)
 004 "road"[1] 192.1.2.23 #2: initiator established Child SA using #1; IPsec tunnel [192.0.2.1-192.0.2.1:0-65535 0] -> [0.0.0.0-255.255.255.255:0-65535 0] {ESP/ESN=>0xESPESP <0xESPESP xfrm=AES_GCM_16_256-NONE DPD=passive}
 road #
  ping -n -q -c 2 192.0.2.254
@@ -31,7 +32,7 @@
 rtt min/avg/max/mdev = 0.XXX/0.XXX/0.XXX/0.XXX ms
 road #
  ipsec whack --trafficstatus
-006 #2: "road"[1] 192.1.2.23, type=ESP, add_time=1234567890, inBytes=168, outBytes=168, maxBytes=2^63B, id='@east', lease=192.0.2.1/32
+006 #2: "road"[1] 192.1.2.23, type=ESP, add_time=1234567890, inBytes=0, outBytes=0, maxBytes=2^63B, id='@east', lease=192.0.2.1/32

from the log:

| cmd(   0):2>&1 PLUTO_VERB='route-client' PLUTO_CONNECTION='road' PLUTO_CONNECTION_TYPE='tu:
| cmd(  80):nnel' PLUTO_VIRT_INTERFACE='ipsec1' PLUTO_INTERFACE='eth0' PLUTO_XFRMI_ROUTE='ye:
| cmd( 160):s' PLUTO_NEXT_HOP='192.1.3.254' PLUTO_ME='192.1.3.209' PLUTO_MY_ID='@road' PLUTO:
| cmd( 240):_CLIENT_FAMILY='ipv4' PLUTO_MY_CLIENT='192.0.2.1/32' PLUTO_MY_CLIENT_NET='192.0.:
| cmd( 320):2.1' PLUTO_MY_CLIENT_MASK='255.255.255.255' PLUTO_MY_PORT=0 PLUTO_MY_PROTOCOL=0 :
| cmd( 400):PLUTO_SA_REQID=16393 PLUTO_SA_TYPE='ESP' PLUTO_PEER='192.1.2.23' PLUTO_PEER_ID=':
| cmd( 480):@east' PLUTO_PEER_CLIENT='0.0.0.0/0' PLUTO_PEER_CLIENT_NET='0.0.0.0' PLUTO_PEER_:
| cmd( 560):CLIENT_MASK='0.0.0.0' PLUTO_PEER_PORT=0 PLUTO_PEER_PROTOCOL=0 PLUTO_PEER_CA='' P:
| cmd( 640):LUTO_STACK='xfrm' PLUTO_ADDTIME=1691002967 PLUTO_CONN_POLICY='IKEv2+PSK+ENCRYPT+:
| cmd( 720):TUNNEL+PFS+UP+IKEV2_ALLOW_NARROWING+IKE_FRAG_ALLOW+ESN_NO+ESN_YES' PLUTO_CONN_KI:
| cmd( 800):ND='CK_INSTANCE' PLUTO_CONN_ADDRFAMILY='ipv4' XAUTH_FAILED=0 PLUTO_MY_SOURCEIP=':
| cmd( 880):192.0.2.1' PLUTO_MOBIKE_EVENT='' PLUTO_IS_PEER_CISCO=0 PLUTO_PEER_DNS_INFO='1.2.:
| cmd( 960):3.4 8.8.8.8' PLUTO_PEER_DOMAIN_INFO='' PLUTO_PEER_BANNER='' PLUTO_CFG_SERVER=0 P:
| cmd(1040):LUTO_CFG_CLIENT=1 PLUTO_NM_CONFIGURED=0 PLUTO_INBYTES=0 PLUTO_OUTBYTES=0 PLUTO_X:
| cmd(1120):FRMI_FWMARK='1/0xffffffff' VTI_IFACE='' VTI_ROUTING='no' VTI_SHARED='no' SPI_IN=:
| cmd(1200):0x95a8264 SPI_OUT=0x89c40f8a ipsec _updown:
"road"[1] 192.1.2.23 #2: route-client output: /usr/local/libexec/ipsec/_updown.xfrm: doroute "ip -4 route replace 0.0.0.0/1   dev ipsec1 src 192.0.2.1 &&               ip -4 route replace 128.0.0.0/1   dev ipsec1 src 192.0.2.1" failed (Error: Invalid prefsrc address.)

@cagney
Copy link
Collaborator

cagney commented Aug 2, 2023 •
edited

The leak in ikev2-xfrmi-08 is

+:
+: checking core files
+:
+PASS: core files
+:
+: checking memory leaks
+:
+Aug  2 15:03:38.285202: leak detective found 2 leaks, total size 112
+FAIL: memory leaks
+Aug  2 15:03:36.510297: leak-detective enabled
+Aug  2 15:03:38.285079: leak: struct pluto_xfrmi_ipaddr, item size: 104
+Aug  2 15:03:38.285142: leak: parse_linkinfo_data, item size: 8
+Aug  2 15:03:38.285202: leak detective found 2 leaks, total size 112
+:
+: checking reference leaks
+:
+ERROR: : '0x7f7be18d7f98' has count 1
+1193: Aug  2 15:03:36.818600: | newref struct pluto_xfrmi_ipaddr@0x7f7be18d7f98(0->1) (create_xfrmi_ipaddr() +388 programs/pluto/kernel_xfrm_interface.c)
+FAIL: reference leaks

was generated by running ../../guestbin/post-mortem.sh

@letoams
Copy link
Member

letoams commented Aug 2, 2023 via email

@bradyallenjohnson
Copy link
Contributor Author

The leak in ikev2-xfrmi-08 is

+:
+: checking core files
+:
+PASS: core files
+:
+: checking memory leaks
+:
+Aug  2 15:03:38.285202: leak detective found 2 leaks, total size 112
+FAIL: memory leaks
+Aug  2 15:03:36.510297: leak-detective enabled
+Aug  2 15:03:38.285079: leak: struct pluto_xfrmi_ipaddr, item size: 104
+Aug  2 15:03:38.285142: leak: parse_linkinfo_data, item size: 8
+Aug  2 15:03:38.285202: leak detective found 2 leaks, total size 112
+:
+: checking reference leaks
+:
+ERROR: : '0x7f7be18d7f98' has count 1
+1193: Aug  2 15:03:36.818600: | newref struct pluto_xfrmi_ipaddr@0x7f7be18d7f98(0->1) (create_xfrmi_ipaddr() +388 programs/pluto/kernel_xfrm_interface.c)
+FAIL: reference leaks

was generated by running ../../guestbin/post-mortem.sh

I fixed this memory leak and the test failure with changes I just pushed.
The test failure was because my code wasnt correctly handling the road warrior configuration.
The memory leak was because the code was not deleting pluto_xfrmi_ipaddr that were created at init when there are already IPs on the interface that were not created by pluto.

@cagney
Copy link
Collaborator

cagney commented Aug 4, 2023

The memory leak was because the code was not deleting pluto_xfrmi_ipaddr that were created at init when there are already IPs on the interface that were not created by pluto.

that one is tricky; re-running tests

@cagney
Copy link
Collaborator

cagney commented Aug 5, 2023 

$ ./kvm failed
testing/pluto/ikev2-xfrmi-08 failed west:output-different
testing/pluto/ikev2-xfrmi-10 failed west:output-different

pfree() instead of delref()?

+:
+: checking memory leaks
+:
+PASS: memory leaks
+:
+: checking reference leaks
+:
+ERROR: : '0x7ff2e7a9df98' has count 1
+1193: Aug  5 07:57:33.796067: | newref struct pluto_xfrmi_ipaddr@0x7ff2e7a9df98(0->1) (create_xfrmi_ipaddr() +401 programs/pluto/kernel_xfrm_interface.c)
+FAIL: reference leaks

@bradyallenjohnson
Copy link
Contributor Author 

$ ./kvm failed
testing/pluto/ikev2-xfrmi-08 failed west:output-different
testing/pluto/ikev2-xfrmi-10 failed west:output-different

pfree() instead of delref()?

+:
+: checking memory leaks
+:
+PASS: memory leaks
+:
+: checking reference leaks
+:
+ERROR: : '0x7ff2e7a9df98' has count 1
+1193: Aug  5 07:57:33.796067: | newref struct pluto_xfrmi_ipaddr@0x7ff2e7a9df98(0->1) (create_xfrmi_ipaddr() +401 programs/pluto/kernel_xfrm_interface.c)
+FAIL: reference leaks

Thanks for pointing that out. Strange I didnt see that when I ran the tests yesterday.

I was indeed calling pfree() without first calling delref(). Fixed and pushed.

@cagney
Copy link
Collaborator

cagney commented Aug 5, 2023

Strange I didnt see that when I ran the tests yesterday.

when run in batch mode kvm performs additional checks

the tests now pass; I'll do a full test run next; thanks

@cagney
Copy link
Collaborator

cagney commented Aug 6, 2023

clean result:

$ ./kvm failed
Summary:
  stats/bad/ignored/status!=good: 1
  stats/freebsd/ignored/status!=good: 3
  stats/netbsd/ignored/status!=good: 26
  stats/openbsd/ignored/status!=good: 3
  stats/skiptest/ignored/status!=good: 13
  stats/wip/ignored/status!=good: 147
  tests/bad/results/untested: 1
  tests/freebsd/results/untested: 3
  tests/good/results/passed: 924
  tests/netbsd/results/untested: 26
  tests/openbsd/results/untested: 3
  tests/skiptest/results/untested: 13
  tests/wip/results/untested: 147
  total: 1117
  total/ignored: 193
  total/passed: 924

(had to re-run some unreliable tests, but that is unrelated)

@cagney
Copy link
Collaborator

cagney commented Aug 6, 2023

For reference, I also ran the wip xfrmi tests using:

$ ./kvm check testing/pluto/*xfrmi*

this is how they compare:

$ ./testing/utils/kvmresults.py --test-status 'good|wip' --test-name xfrmi --print result,test-status,test-name .
passed good ikev2-xfrmi-01
passed good ikev2-xfrmi-02
passed wip ikev2-xfrmi-02-responder
passed good ikev2-xfrmi-03
passed good ikev2-xfrmi-04
failed wip ikev2-xfrmi-05-remote-access-client
passed good ikev2-xfrmi-06
passed good ikev2-xfrmi-07
passed good ikev2-xfrmi-08
passed good ikev2-xfrmi-09-systemd-networkd
passed good ikev2-xfrmi-10
passed good ikev2-xfrmi-11-default-manual-route
passed good ikev2-xfrmi-11-vti-default-route
passed good ikev2-xfrmi-11-default-route
passed good ikev2-xfrmi-12-two-default-routes
passed good ikev2-xfrmi-13-multiple-tunnels
unresolved wip ikev2-xfrmi-14-fwmark
passed good ikev1-xfrmi-01
failed wip ikev1-xfrmi-02
failed wip ikev1-xfrmi-02-tcpdump
failed wip ikev1-xfrmi-02-aggr
passed good ikev1-xfrmi-04
passed good ikev1-xfrmi-04-aggr
passed good ikev1-xfrmi-05-remote-access-client
unresolved wip ikev2-xfrmi-15-interface-ip
passed wip ikev2-xfrmi-16-rekey
passed good interop-ikev2-xfrmi-strongswan-01

@cagney cagney added this to the v5.0 milestone Aug 6, 2023
@cagney cagney self-requested a review August 9, 2023 11:48
cagney
cagney approved these changes Aug 9, 2023
View reviewed changes
Copy link
Collaborator

@cagney cagney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test results don't show any regressions.

@bradyallenjohnson
Add XFRM I/F IP mgmt (with ref-counting) to Pluto
76625e5 
- Part 3 of 3: Reference count IPs on XFRM interfaces in Pluto

Signed-off-by: Brady Johnson <bradyallenjohnson@gmail.com>
@bradyallenjohnson
Remove XFRM I/F IP management from updown script
6bfb05b 
- Part 2 of 3: Reference count IPs on XFRM interfaces in Pluto

Signed-off-by: Brady Johnson <bradyallenjohnson@gmail.com>
@bradyallenjohnson
Enable interface-ip configuration option
5259d13 
- Part 1 of 3: Reference count IPs on XFRM interfaces in Pluto

Signed-off-by: Brady Johnson <bradyallenjohnson@gmail.com>
@paulwouters
Copy link
Member

merged!

@bradyallenjohnson bradyallenjohnson deleted the topic/xfrm_refcount_ips branch November 28, 2023 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cagney cagney cagney approved these changes

@paulwouters paulwouters paulwouters approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v5.0rc1
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@bradyallenjohnson @antonyantony @cagney @letoams @paulwouters