[strongSwan-dev] [PATCH] Avoid duplicate IKE SA from concurrent SADB_ACQUIRE

[Jean-Francois HREN]

Hello,

I'm trying to avoid duplicate IKE SA creation when concurrent SADB_ACQUIRE are received from a FreeBSD kernel:

[...]
Nov 22 11:27:29 14[KNL] received an SADB_ACQUIRE
Nov 22 11:27:29 14[KNL] creating acquire job for policy 192.168.1.1/32 === 192.168.1.2/32 with reqid {2}
Nov 22 11:27:29 16[MGR] checkout IKE_SA by config
Nov 22 11:27:29 03[JOB] watcher got notification, rebuilding
Nov 22 11:27:29 03[JOB] watcher going to poll() 5 fds
Nov 22 11:27:29 03[JOB] watched FD 7 ready to read
Nov 22 11:27:29 14[KNL] received an SADB_ACQUIRE
Nov 22 11:27:29 03[JOB] watcher going to poll() 4 fds
Nov 22 11:27:29 16[MGR] created IKE_SA (unnamed)[1]
Nov 22 11:27:29 14[KNL] creating acquire job for policy 192.168.1.1/32 === 192.168.1.2/32 with reqid {1}
Nov 22 11:27:29 16[MGR] tracking created IKE_SA (unnamed)[1]
Nov 22 11:27:29 03[JOB] watcher got notification, rebuilding
Nov 22 11:27:29 06[MGR] checkout IKE_SA by config
[...]

The IKE SA created by calling checkout_by_config() is not tracked until its checkin later thus leading to the creation of another unamed IKE SA with the same peer and IKE configurations.
To circumvent this, I added the created IKE SA as an entry in the IKE SA manager like done (somehow) when calling checkout_by_message().
With this, the second SADB_ACQUIRE finds the entry :

[...]
Nov 22 11:27:29 16[MGR] checkin IKE_SA (ZDI1NjM0NWFkMDUzYWVmZDU3ZDg0OTcyZTZkOTM3ZTEA)(fb8d6ee724d5d9157518502da724119d)[1]
Nov 22 11:27:29 16[MGR] checkin of IKE_SA successful
Nov 22 11:27:29 06[MGR] found existing IKE_SA 1 with a '(ZDI1NjM0NWFkMDUzYWVmZDU3ZDg0OTcyZTZkOTM3ZTEA)(fb8d6ee724d5d9157518502da724119d)' config
Nov 22 11:27:29 06[IKE] <(ZDI1NjM0NWFkMDUzYWVmZDU3ZDg0OTcyZTZkOTM3ZTEA)(fb8d6ee724d5d9157518502da724119d)|1> queueing CHILD_CREATE task
Nov 22 11:27:29 06[IKE] <(ZDI1NjM0NWFkMDUzYWVmZDU3ZDg0OTcyZTZkOTM3ZTEA)(fb8d6ee724d5d9157518502da724119d)|1> delaying task initiation, IKE_SA_INIT exchange in progress
[...]

avoiding duplicate IKE SA.

Is this approach correct ?

Regards,
Jean-François

---
src/libcharon/sa/ike_sa_manager.c | 13 +++++++++++++
1 file changed, 13 insertions(+)

diff --git a/src/libcharon/sa/ike_sa_manager.c b/src/libcharon/sa/ike_sa_manager.c
index c50c70860..cb31e8ce1 100644
--- a/src/libcharon/sa/ike_sa_manager.c
+++ b/src/libcharon/sa/ike_sa_manager.c
@@ -1462,6 +1462,19 @@ METHOD(ike_sa_manager_t, checkout_by_config, ike_sa_t*,
if (!ike_sa)
{ /* no IKE_SA using such a config, hand out a new */
ike_sa = checkout_new(this, peer_cfg->get_ike_version(peer_cfg), TRUE);
+ if (ike_sa)
+ {
+ ike_sa_id_t *ike_sa_id = ike_sa->get_id(ike_sa);
+ entry = entry_create();
+ entry->ike_sa_id = ike_sa_id->clone(ike_sa_id);
+ entry->ike_sa = ike_sa;
+ entry->checked_out = thread_current();
+ segment = put_entry(this, entry);
+ unlock_single_segment(this, segment);
+
+ DBG2(DBG_MGR, "tracking created IKE_SA %s[%u]", ike_sa->get_name(ike_sa),
+ ike_sa->get_unique_id(ike_sa));
+ }
}
charon->bus->set_sa(charon->bus, ike_sa);

[Tobias Brunner]

Hi Jean-Francois,

Post by Jean-Francois HREN
Nov 22 11:27:29 14[KNL] received an SADB_ACQUIRE
Nov 22 11:27:29 14[KNL] creating acquire job for policy 192.168.1.1/32
=== 192.168.1.2/32 with reqid {2}
Nov 22 11:27:29 16[MGR] checkout IKE_SA by config
Nov 22 11:27:29 03[JOB] watcher got notification, rebuilding
Nov 22 11:27:29 03[JOB] watcher going to poll() 5 fds
Nov 22 11:27:29 03[JOB] watched FD 7 ready to read
Nov 22 11:27:29 14[KNL] received an SADB_ACQUIRE
Nov 22 11:27:29 03[JOB] watcher going to poll() 4 fds
Nov 22 11:27:29 16[MGR] created IKE_SA (unnamed)[1]
Nov 22 11:27:29 14[KNL] creating acquire job for policy 192.168.1.1/32
=== 192.168.1.2/32 with reqid {1}
Nov 22 11:27:29 16[MGR] tracking created IKE_SA (unnamed)[1]
Nov 22 11:27:29 03[JOB] watcher got notification, rebuilding
Nov 22 11:27:29 06[MGR] checkout IKE_SA by config

Why are there duplicate policies with different reqids? The acquire
tracking in the trap manager is done via reqid. It's strange that
that's even possible. strongSwan only assigns unique reqids to
different policies, and for overlapping policies only an acquire for the
narrower policy should be triggered by the kernel. So you might want to
investigate and fix that.

Regards,
Tobias