vpp-swan: fix memory leaks
This patch fix the memory leaks discovered in the current
implementation, inlcuding expired data, spd dump, and host names.
Type: fix
Signed-off-by: Gabriel Oginski <gabrielx.oginski@intel.com>
Change-Id: I3794f5db3c58d1e78df25f242c91e7a67363de53
diff --git a/extras/strongswan/vpp_sswan/kernel_vpp_ipsec.c b/extras/strongswan/vpp_sswan/kernel_vpp_ipsec.c
index a51edcb..652de65 100644
--- a/extras/strongswan/vpp_sswan/kernel_vpp_ipsec.c
+++ b/extras/strongswan/vpp_sswan/kernel_vpp_ipsec.c
@@ -62,6 +62,14 @@
#define PRIO_BASE 384
+/**
+ * Every 2 seconds, the thread responsible for collecting the available
+ * interfaces will be executed.
+ * Retrying 5 times every 1 second ensures that there is enough time to check
+ * if the interface will be available.
+ */
+#define N_RETRY_GET_IF 5
+
u32 natt_port;
/**
@@ -133,6 +141,11 @@
* route-based IPsec
*/
bool use_tunnel_mode_sa;
+
+ /**
+ * Connections to VPP Stats
+ */
+ stat_client_main_t *sm;
};
/**
@@ -143,6 +156,7 @@
/** VPP SA ID */
uint32_t sa_id;
uint32_t stat_index;
+ kernel_ipsec_sa_id_t *sa_id_p;
} sa_t;
/**
@@ -156,6 +170,8 @@
uint32_t sw_if_index;
/** Policy count for this SPD */
refcount_t policy_num;
+ /** Name of the interface the SPD is bound to */
+ char *if_name;
} spd_t;
/**
@@ -413,9 +429,9 @@
manage_route (private_kernel_vpp_ipsec_t *this, bool add,
traffic_selector_t *dst_ts, host_t *src, host_t *dst)
{
- host_t *dst_net, *gateway;
+ host_t *dst_net = NULL, *gateway = NULL;
uint8_t prefixlen;
- char *if_name;
+ char *if_name = NULL;
route_entry_t *route;
bool route_exist = FALSE;
@@ -433,11 +449,11 @@
{
if (src->is_anyaddr (src))
{
- return;
+ goto error;
}
if (!charon->kernel->get_interface (charon->kernel, src, &if_name))
{
- return;
+ goto error;
}
}
route_exist =
@@ -478,7 +494,7 @@
if (!route_exist)
{
DBG2 (DBG_KNL, "del route but it not exist");
- return;
+ goto error;
}
if (ref_put (&route->refs))
{
@@ -489,6 +505,14 @@
gateway, dst, if_name, 1);
}
}
+error:
+ if (gateway != NULL)
+ gateway->destroy (gateway);
+ if (dst_net != NULL)
+ dst_net->destroy (dst_net);
+ if (if_name != NULL)
+ free (if_name);
+ return;
}
/**
@@ -796,6 +820,12 @@
ntohl (rmp->retval));
goto error;
}
+ /* address "out" needs to be freed after vec->send */
+ if (out != NULL)
+ {
+ free (out);
+ out = NULL;
+ }
mp->entry.is_outbound = 1;
if (vac->send (vac, (char *) mp, sizeof (*mp), &out, &out_len))
{
@@ -809,6 +839,12 @@
ntohl (rmp->retval));
goto error;
}
+ /* address "out" needs to be freed after vec->send */
+ if (out != NULL)
+ {
+ free (out);
+ out = NULL;
+ }
mp->entry.is_outbound = 0;
mp->entry.protocol = IP_API_PROTO_AH;
if (vac->send (vac, (char *) mp, sizeof (*mp), &out, &out_len))
@@ -823,6 +859,12 @@
ntohl (rmp->retval));
goto error;
}
+ /* address "out" needs to be freed after vec->send */
+ if (out != NULL)
+ {
+ free (out);
+ out = NULL;
+ }
mp->entry.is_outbound = 1;
if (vac->send (vac, (char *) mp, sizeof (*mp), &out, &out_len))
{
@@ -886,6 +928,12 @@
ntohl (rmp->retval));
goto error;
}
+ /* address "out" needs to be freed after vec->send */
+ if (out != NULL)
+ {
+ free (out);
+ out = NULL;
+ }
mp->entry.is_outbound = 1;
if (vac->send (vac, (char *) mp, sizeof (*mp), &out, &out_len))
{
@@ -954,64 +1002,68 @@
kernel_ipsec_policy_id_t *id,
kernel_ipsec_manage_policy_t *data)
{
- spd_t *spd;
+ spd_t *spd = NULL;
char *out = NULL, *interface = NULL;
int out_len;
uint32_t sw_if_index, spd_id = ~0, sad_id = ~0;
status_t rv = FAILED;
uint32_t priority, auto_priority;
chunk_t src_from, src_to, dst_from, dst_to;
- host_t *src, *dst, *addr;
- vl_api_ipsec_spd_entry_add_del_t *mp;
- vl_api_ipsec_spd_entry_add_del_reply_t *rmp;
+ host_t *src = NULL, *dst = NULL, *addr = NULL;
+ vl_api_ipsec_spd_entry_add_del_t *mp = NULL;
+ vl_api_ipsec_spd_entry_add_del_reply_t *rmp = NULL;
bool n_spd = false;
+ vl_api_ipsec_spd_dump_t *mp_dump = NULL;
+ vl_api_ipsec_spd_details_t *rmp_dump = NULL, *tmp = NULL;
mp = vl_msg_api_alloc (sizeof (*mp));
memset (mp, 0, sizeof (*mp));
this->mutex->lock (this->mutex);
- if (!id->interface)
+ if (id->dir == POLICY_FWD)
{
- addr = id->dir == POLICY_IN ? data->dst : data->src;
- for (int i = 0; i < 5; i++)
- {
- if (!charon->kernel->get_interface (charon->kernel, addr,
- &interface))
- {
- DBG1 (DBG_KNL, "policy no interface %H", addr);
- interface = NULL;
- sleep (1);
- }
-
- if (interface)
- {
- DBG1 (DBG_KNL, "policy have interface %H", addr);
- break;
- }
- }
- if (!interface)
- goto error;
- id->interface = interface;
+ DBG1 (DBG_KNL, "policy FWD interface");
+ rv = SUCCESS;
+ goto error;
}
+ addr = id->dir == POLICY_IN ? data->dst : data->src;
+ for (int i = 0; i < N_RETRY_GET_IF; i++)
+ {
+ if (!charon->kernel->get_interface (charon->kernel, addr, &interface))
+ {
+ DBG1 (DBG_KNL, "policy no interface %H", addr);
+ free (interface);
+ interface = NULL;
+ sleep (1);
+ }
+
+ if (interface)
+ {
+ DBG1 (DBG_KNL, "policy have interface %H", addr);
+ break;
+ }
+ }
+ if (!interface)
+ goto error;
DBG2 (DBG_KNL, "manage policy [%s] interface [%s]", add ? "ADD" : "DEL",
- id->interface);
+ interface);
- spd = this->spds->get (this->spds, id->interface);
+ spd = this->spds->get (this->spds, interface);
if (!spd)
{
if (!add)
{
DBG1 (DBG_KNL, "SPD for %s not found, should not be deleted",
- id->interface);
+ interface);
goto error;
}
- sw_if_index = get_sw_if_index (id->interface);
+ sw_if_index = get_sw_if_index (interface);
DBG1 (DBG_KNL, "firstly created, spd for %s found sw_if_index is %d",
- id->interface, sw_if_index);
+ interface, sw_if_index);
if (sw_if_index == ~0)
{
- DBG1 (DBG_KNL, "sw_if_index for %s not found", id->interface);
+ DBG1 (DBG_KNL, "sw_if_index for %s not found", interface);
goto error;
}
spd_id = ref_get (&this->next_spd_id);
@@ -1026,9 +1078,9 @@
sw_if_index);
goto error;
}
- INIT (spd, .spd_id = spd_id, .sw_if_index = sw_if_index,
- .policy_num = 0, );
- this->spds->put (this->spds, id->interface, spd);
+ INIT (spd, .spd_id = spd_id, .sw_if_index = sw_if_index, .policy_num = 0,
+ .if_name = strdup (interface), );
+ this->spds->put (this->spds, spd->if_name, spd);
n_spd = true;
}
@@ -1145,9 +1197,6 @@
mp->entry.remote_port_stop = htons (id->dst_ts->get_to_port (id->dst_ts));
/* check if policy exists in SPD */
- vl_api_ipsec_spd_dump_t *mp_dump;
- vl_api_ipsec_spd_details_t *rmp_dump, *tmp;
-
mp_dump = vl_msg_api_alloc (sizeof (*mp_dump));
memset (mp_dump, 0, sizeof (*mp_dump));
@@ -1216,7 +1265,17 @@
interface_add_del_spd (FALSE, spd->spd_id, spd->sw_if_index);
manage_bypass (FALSE, spd->spd_id, sad_id);
spd_add_del (FALSE, spd->spd_id);
- this->spds->remove (this->spds, id->interface);
+ this->spds->remove (this->spds, interface);
+ if (spd->if_name)
+ {
+ free (spd->if_name);
+ spd->if_name = NULL;
+ }
+ if (spd)
+ {
+ free (spd);
+ spd = NULL;
+ }
}
}
@@ -1229,9 +1288,18 @@
}
rv = SUCCESS;
error:
- free (out);
- vl_msg_api_free (mp_dump);
- vl_msg_api_free (mp);
+ if (out != NULL)
+ free (out);
+ if (mp_dump != NULL)
+ vl_msg_api_free (mp_dump);
+ if (mp != NULL)
+ vl_msg_api_free (mp);
+ if (src != NULL)
+ src->destroy (src);
+ if (dst != NULL)
+ dst->destroy (dst);
+ if (interface != NULL)
+ free (interface);
this->mutex->unlock (this->mutex);
return rv;
}
@@ -1276,6 +1344,15 @@
} vpp_sa_expired_t;
/**
+ * Clean up expire data
+ */
+static void
+expire_data_destroy (vpp_sa_expired_t *data)
+{
+ free (data);
+}
+
+/**
* Callback for expiration events
*/
static job_requeue_t
@@ -1294,6 +1371,10 @@
FALSE);
}
+ if (id->src)
+ id->src->destroy (id->src);
+ if (id->dst)
+ id->dst->destroy (id->dst);
free (id);
this->mutex->unlock (this->mutex);
return JOB_REQUEUE_NONE;
@@ -1334,8 +1415,9 @@
timeout = lifetime->time.life;
}
- job = callback_job_create ((callback_job_cb_t) sa_expired, expired,
- (callback_job_cleanup_t) free, NULL);
+ job =
+ callback_job_create ((callback_job_cb_t) sa_expired, expired,
+ (callback_job_cleanup_t) expire_data_destroy, NULL);
lib->scheduler->schedule_job (lib->scheduler, (job_t *) job, timeout);
}
@@ -1554,7 +1636,8 @@
this->mutex->lock (this->mutex);
INIT (sa_id, .src = id->src->clone (id->src),
.dst = id->dst->clone (id->dst), .spi = id->spi, .proto = id->proto, );
- INIT (sa, .sa_id = sad_id, .stat_index = ntohl (rmp->stat_index), );
+ INIT (sa, .sa_id = sad_id, .stat_index = ntohl (rmp->stat_index),
+ .sa_id_p = sa_id, );
DBG4 (DBG_KNL, "put sa by its sa_id %x !!!!!!", sad_id);
this->sas->put (this->sas, sa_id, sa);
schedule_expiration (this, data, id);
@@ -1581,31 +1664,48 @@
{
status_t rv = FAILED;
sa_t *sa;
- u32 *dir;
+ u32 *dir = NULL;
int i, k;
- stat_segment_data_t *res;
+ stat_segment_data_t *res = NULL;
u8 **pattern = 0;
uint64_t res_bytes = 0;
uint64_t res_packets = 0;
this->mutex->lock (this->mutex);
sa = this->sas->get (this->sas, id);
- this->mutex->unlock (this->mutex);
if (!sa)
{
+ this->mutex->unlock (this->mutex);
DBG1 (DBG_KNL, "SA not found");
return NOT_FOUND;
}
- int rv_stat = stat_segment_connect ("/run/vpp/stats.sock");
- if (rv_stat != 0)
+ if (this->sm == NULL)
{
- DBG1 (DBG_KNL, "Not connecting with stats segmentation");
- return NOT_FOUND;
+ stat_client_main_t *sm = NULL;
+ sm = stat_client_get ();
+
+ if (!sm)
+ {
+ DBG1 (DBG_KNL, "Not connecting with stats segmentation");
+ this->mutex->unlock (this->mutex);
+ return NOT_FOUND;
+ }
+ this->sm = sm;
+ int rv_stat = stat_segment_connect_r ("/run/vpp/stats.sock", this->sm);
+ if (rv_stat != 0)
+ {
+ stat_client_free (this->sm);
+ this->sm = NULL;
+ DBG1 (DBG_KNL, "Not connecting with stats segmentation");
+ this->mutex->unlock (this->mutex);
+ return NOT_FOUND;
+ }
}
+
vec_add1 (pattern, (u8 *) "/net/ipsec/sa");
- dir = stat_segment_ls ((u8 **) pattern);
- res = stat_segment_dump (dir);
+ dir = stat_segment_ls_r ((u8 **) pattern, this->sm);
+ res = stat_segment_dump_r (dir, this->sm);
/* i-loop for each results find by pattern - here two:
* 1. /net/ipsec/sa
* 2. /net/ipsec/sa/lost
@@ -1644,11 +1744,10 @@
break;
}
}
- stat_segment_data_free (res);
- stat_segment_disconnect ();
vec_free (pattern);
vec_free (dir);
+ stat_segment_data_free (res);
if (bytes)
{
@@ -1663,6 +1762,7 @@
*time = 0;
}
+ this->mutex->unlock (this->mutex);
rv = SUCCESS;
return rv;
}
@@ -1672,8 +1772,8 @@
{
char *out = NULL;
int out_len;
- vl_api_ipsec_sad_entry_add_del_t *mp;
- vl_api_ipsec_sad_entry_add_del_reply_t *rmp;
+ vl_api_ipsec_sad_entry_add_del_t *mp = NULL;
+ vl_api_ipsec_sad_entry_add_del_reply_t *rmp = NULL;
status_t rv = FAILED;
sa_t *sa;
@@ -1705,11 +1805,20 @@
goto error;
}
- vl_msg_api_free (mp);
- this->sas->remove (this->sas, id);
+ void *temp = this->sas->remove (this->sas, id);
+ if (sa->sa_id_p)
+ {
+ if (sa->sa_id_p->src)
+ sa->sa_id_p->src->destroy (sa->sa_id_p->src);
+ if (sa->sa_id_p->dst)
+ sa->sa_id_p->dst->destroy (sa->sa_id_p->dst);
+ free (sa->sa_id_p);
+ }
+ free (sa);
rv = SUCCESS;
error:
free (out);
+ vl_msg_api_free (mp);
this->mutex->unlock (this->mutex);
return rv;
}
@@ -1719,12 +1828,14 @@
enumerator_t *enumerator;
int out_len;
char *out;
- vl_api_ipsec_sad_entry_add_del_t *mp;
+ vl_api_ipsec_sad_entry_add_del_t *mp = NULL;
+ vl_api_ipsec_sad_entry_add_del_reply_t *rmp = NULL;
sa_t *sa = NULL;
+ status_t rv = FAILED;
this->mutex->lock (this->mutex);
enumerator = this->sas->create_enumerator (this->sas);
- while (enumerator->enumerate (enumerator, sa, NULL))
+ while (enumerator->enumerate (enumerator, &sa))
{
mp = vl_msg_api_alloc (sizeof (*mp));
memset (mp, 0, sizeof (*mp));
@@ -1736,16 +1847,38 @@
if (vac->send (vac, (char *) mp, sizeof (*mp), &out, &out_len))
{
DBG1 (DBG_KNL, "flush_sas failed!!!!");
- return FALSE;
+ goto error;
+ }
+ rmp = (void *) out;
+ if (rmp->retval)
+ {
+ DBG1 (DBG_KNL, "flush_sas failed!!!! rv: %d", ntohl (rmp->retval));
+ goto error;
+ }
+ if (sa->sa_id_p)
+ {
+ if (sa->sa_id_p->src)
+ sa->sa_id_p->src->destroy (sa->sa_id_p->src);
+ if (sa->sa_id_p->dst)
+ sa->sa_id_p->dst->destroy (sa->sa_id_p->dst);
}
free (out);
vl_msg_api_free (mp);
this->sas->remove_at (this->sas, enumerator);
+ free (sa->sa_id_p);
+ free (sa);
}
+ rv = SUCCESS;
+error:
+ if (out != NULL)
+ free (out);
+ if (mp != NULL)
+ vl_msg_api_free (mp);
+
enumerator->destroy (enumerator);
this->mutex->unlock (this->mutex);
- return SUCCESS;
+ return rv;
}
METHOD (kernel_ipsec_t, add_policy, status_t, private_kernel_vpp_ipsec_t *this,
@@ -1792,6 +1925,12 @@
this->sas->destroy (this->sas);
this->spds->destroy (this->spds);
this->routes->destroy (this->routes);
+ if (this->sm)
+ {
+ stat_segment_disconnect_r (this->sm);
+ stat_client_free (this->sm);
+ this->sm = NULL;
+ }
free (this);
}
@@ -1833,6 +1972,7 @@
.use_tunnel_mode_sa = lib->settings->get_bool(lib->settings,
"%s.plugins.kernel-vpp.use_tunnel_mode_sa",
TRUE, lib->ns),
+ .sm = NULL,
);
if (!init_spi (this))
diff --git a/extras/strongswan/vpp_sswan/kernel_vpp_net.c b/extras/strongswan/vpp_sswan/kernel_vpp_net.c
index a29a7c6..1ed5843 100644
--- a/extras/strongswan/vpp_sswan/kernel_vpp_net.c
+++ b/extras/strongswan/vpp_sswan/kernel_vpp_net.c
@@ -545,6 +545,7 @@
vl_api_ip_address_details_t *rmp, *tmp;
linked_list_t *addrs;
host_t *host;
+ enumerator_t *enumerator;
mp = vl_msg_api_alloc (sizeof (*mp));
clib_memset (mp, 0, sizeof (*mp));
@@ -591,6 +592,13 @@
vl_msg_api_free (mp);
free (out);
+ /* clean-up */
+ enumerator = entry->addrs->create_enumerator (entry->addrs);
+ while (enumerator->enumerate (enumerator, &host))
+ {
+ host->destroy (host);
+ }
+ enumerator->destroy (enumerator);
entry->addrs->destroy (entry->addrs);
entry->addrs =
linked_list_create_from_enumerator (addrs->create_enumerator (addrs));