[dpdk-dev] [PATCH v1] net/tap: fix eBPF file descriptors leakage

[dpdk-dev] [PATCH v1] net/tap: fix eBPF file descriptors leakage

[Ophir Munk]

When a user creates an RSS rule, the tap PMD dynamically allocates
a 'flow' data structure, and uploads BPF programs (represented by file
descriptors) to the kernel.
The kernel might reject the rule (due to filters overlap, for example)
in which case, flow memory should be freed and BPF file descriptors
should be closed.
In the corrupted code there were scenarios where BPF file descriptors
were not closed.
The fix is to add a new function - tap_flow_free(), which will make sure
to always close BPF file descriptors before freeing the flow allocated
memory.

Fixes: 036d721a8229 ("net/tap: implement RSS using eBPF")

Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
---
 drivers/net/tap/tap_flow.c | 84 +++++++++++++++++++++++++++++-----------------
 1 file changed, 53 insertions(+), 31 deletions(-)

diff --git a/drivers/net/tap/tap_flow.c b/drivers/net/tap/tap_flow.c
index 6aa53a7..5c87f8e 100644
--- a/drivers/net/tap/tap_flow.c
+++ b/drivers/net/tap/tap_flow.c
@@ -212,6 +212,9 @@ struct action_data {
 		const struct rte_flow_action actions[],
 		struct rte_flow_error *error);
 
+static void
+tap_flow_free(struct rte_flow *flow);
+
 static int
 tap_flow_destroy(struct rte_eth_dev *dev,
 		 struct rte_flow *flow,
@@ -1311,6 +1314,36 @@ struct tap_flow_items {
 }
 
 /**
+ * Free the flow opened file descriptors and allocated memory
+ *
+ * @param[in] flow
+ *   Pointer to the flow to free
+ *
+ */
+static void
+tap_flow_free(struct rte_flow *flow)
+{
+	int i;
+
+	if (!flow)
+		return;
+
+	/* Close flow BPF file descriptors */
+	for (i = 0; i < SEC_MAX; i++)
+		if (flow->bpf_fd[i] != 0) {
+			close(flow->bpf_fd[i]);
+			flow->bpf_fd[i] = 0;
+		}
+
+	/* Release the map key for this RSS rule */
+	bpf_rss_key(KEY_CMD_RELEASE, &flow->key_idx);
+	flow->key_idx = 0;
+
+	/* Free flow allocated memory */
+	rte_free(flow);
+}
+
+/**
  * Create a flow.
  *
  * @see rte_flow_create()
@@ -1428,7 +1461,7 @@ struct tap_flow_items {
 	if (remote_flow)
 		rte_free(remote_flow);
 	if (flow)
-		rte_free(flow);
+		tap_flow_free(flow);
 	return NULL;
 }
 
@@ -1450,7 +1483,6 @@ struct tap_flow_items {
 		     struct rte_flow_error *error)
 {
 	struct rte_flow *remote_flow = flow->remote_flow;
-	int i;
 	int ret = 0;
 
 	LIST_REMOVE(flow, next);
@@ -1476,22 +1508,6 @@ struct tap_flow_items {
 			"couldn't receive kernel ack to our request");
 		goto end;
 	}
-	/* Close opened BPF file descriptors of this flow */
-	for (i = 0; i < SEC_MAX; i++)
-		if (flow->bpf_fd[i] != 0) {
-			close(flow->bpf_fd[i]);
-			flow->bpf_fd[i] = 0;
-		}
-
-	/* Release map key for this RSS rule */
-	ret = bpf_rss_key(KEY_CMD_RELEASE, &flow->key_idx);
-	if (ret < 0) {
-		rte_flow_error_set(
-			error, EINVAL, RTE_FLOW_ERROR_TYPE_HANDLE, NULL,
-			"Failed to release BPF RSS key");
-
-		goto end;
-	}
 
 	if (remote_flow) {
 		remote_flow->msg.nh.nlmsg_flags = NLM_F_REQUEST | NLM_F_ACK;
@@ -1520,7 +1536,7 @@ struct tap_flow_items {
 end:
 	if (remote_flow)
 		rte_free(remote_flow);
-	rte_free(flow);
+	tap_flow_free(flow);
 	return ret;
 }
 
@@ -1778,6 +1794,7 @@ int tap_flow_implicit_destroy(struct pmd_internals *pmd,
 }
 
 #define MAX_RSS_KEYS 256
+#define KEY_IDX_OFFSET (3 * MAX_RSS_KEYS)
 #define SEC_NAME_CLS_Q "cls_q"
 
 const char *sec_name[SEC_MAX] = {
@@ -1934,38 +1951,45 @@ static int rss_enable(struct pmd_internals *pmd,
 static int bpf_rss_key(enum bpf_rss_key_e cmd, __u32 *key_idx)
 {
 	__u32 i;
-	int err = -1;
+	int err = 0;
 	static __u32 num_used_keys;
 	static __u32 rss_keys[MAX_RSS_KEYS] = {KEY_STAT_UNSPEC};
 	static __u32 rss_keys_initialized;
 
 	switch (cmd) {
 	case KEY_CMD_GET:
-		if (!rss_keys_initialized)
+		if (!rss_keys_initialized) {
+			err = -1;
 			break;
+		}
 
-		if (num_used_keys == RTE_DIM(rss_keys))
+		if (num_used_keys == RTE_DIM(rss_keys)) {
+			err = -1;
 			break;
+		}
 
 		*key_idx = num_used_keys % RTE_DIM(rss_keys);
 		while (rss_keys[*key_idx] == KEY_STAT_USED)
 			*key_idx = (*key_idx + 1) % RTE_DIM(rss_keys);
 
 		rss_keys[*key_idx] = KEY_STAT_USED;
+		/* add offset to return a key out of range */
+		*key_idx += KEY_IDX_OFFSET;
 		num_used_keys++;
-		err = 0;
 	break;
 
 	case KEY_CMD_RELEASE:
-		if (!rss_keys_initialized) {
-			err = 0;
+		if (!rss_keys_initialized)
+			break;
+
+		/* subtract offest to restore real key index in range */
+		__u32 key = *key_idx - KEY_IDX_OFFSET;
+		if (key >= RTE_DIM(rss_keys))
 			break;
-		}
 
-		if (rss_keys[*key_idx] == KEY_STAT_USED) {
-			rss_keys[*key_idx] = KEY_STAT_AVAILABLE;
+		if (rss_keys[key] == KEY_STAT_USED) {
+			rss_keys[key] = KEY_STAT_AVAILABLE;
 			num_used_keys--;
-			err = 0;
 		}
 	break;
 
@@ -1975,7 +1999,6 @@ static int bpf_rss_key(enum bpf_rss_key_e cmd, __u32 *key_idx)
 
 		rss_keys_initialized = 1;
 		num_used_keys = 0;
-		err = 0;
 	break;
 
 	case KEY_CMD_DEINIT:
@@ -1984,7 +2007,6 @@ static int bpf_rss_key(enum bpf_rss_key_e cmd, __u32 *key_idx)
 
 		rss_keys_initialized = 0;
 		num_used_keys = 0;
-		err = 0;
 	break;
 
 	default:
-- 
1.8.3.1

[dpdk-dev] [PATCH v2 0/2] fix eBPF TAP RSS leakage and map key handling

[Ophir Munk]

v2:
fix eBPF RSS map key handling

v1:
fix eBPF file descriptors leakage

Ophir Munk (2):
  net/tap: fix eBPF file descriptors leakage
  net/tap: fix eBPF RSS map key handling

 drivers/net/tap/tap_flow.c | 104 +++++++++++++++++++++++++++++++--------------
 1 file changed, 73 insertions(+), 31 deletions(-)

-- 
2.7.4

[dpdk-dev] [PATCH v2 1/2] net/tap: fix eBPF file descriptors leakage

[Ophir Munk]

When a user creates an RSS rule, the tap PMD dynamically allocates
a 'flow' data structure, and uploads BPF programs (represented by file
descriptors) to the kernel.
The kernel might reject the rule (due to filters overlap, for example)
in which case, flow memory should be freed and BPF file descriptors
should be closed.
In the corrupted code there were scenarios where BPF file descriptors
were not closed.
The fix is to add a new function - tap_flow_free(), which will make sure
to always close BPF file descriptors before freeing the flow allocated
memory.

Fixes: 036d721a8229 ("net/tap: implement RSS using eBPF")

Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
---
 drivers/net/tap/tap_flow.c | 57 ++++++++++++++++++++++++++++++----------------
 1 file changed, 38 insertions(+), 19 deletions(-)

diff --git a/drivers/net/tap/tap_flow.c b/drivers/net/tap/tap_flow.c
index 6aa53a7..533879d 100644
--- a/drivers/net/tap/tap_flow.c
+++ b/drivers/net/tap/tap_flow.c
@@ -212,6 +212,10 @@ tap_flow_create(struct rte_eth_dev *dev,
 		const struct rte_flow_action actions[],
 		struct rte_flow_error *error);
 
+static void
+tap_flow_free(struct pmd_internals *pmd,
+	struct rte_flow *flow);
+
 static int
 tap_flow_destroy(struct rte_eth_dev *dev,
 		 struct rte_flow *flow,
@@ -1311,6 +1315,38 @@ tap_flow_set_handle(struct rte_flow *flow)
 }
 
 /**
+ * Free the flow opened file descriptors and allocated memory
+ *
+ * @param[in] flow
+ *   Pointer to the flow to free
+ *
+ */
+static void
+tap_flow_free(struct pmd_internals *pmd, struct rte_flow *flow)
+{
+	int i;
+
+	if (!flow)
+		return;
+
+	if (pmd->rss_enabled) {
+		/* Close flow BPF file descriptors */
+		for (i = 0; i < SEC_MAX; i++)
+			if (flow->bpf_fd[i] != 0) {
+				close(flow->bpf_fd[i]);
+				flow->bpf_fd[i] = 0;
+			}
+
+		/* Release the map key for this RSS rule */
+		bpf_rss_key(KEY_CMD_RELEASE, &flow->key_idx);
+		flow->key_idx = 0;
+	}
+
+	/* Free flow allocated memory */
+	rte_free(flow);
+}
+
+/**
  * Create a flow.
  *
  * @see rte_flow_create()
@@ -1428,7 +1464,7 @@ tap_flow_create(struct rte_eth_dev *dev,
 	if (remote_flow)
 		rte_free(remote_flow);
 	if (flow)
-		rte_free(flow);
+		tap_flow_free(pmd, flow);
 	return NULL;
 }
 
@@ -1450,7 +1486,6 @@ tap_flow_destroy_pmd(struct pmd_internals *pmd,
 		     struct rte_flow_error *error)
 {
 	struct rte_flow *remote_flow = flow->remote_flow;
-	int i;
 	int ret = 0;
 
 	LIST_REMOVE(flow, next);
@@ -1476,22 +1511,6 @@ tap_flow_destroy_pmd(struct pmd_internals *pmd,
 			"couldn't receive kernel ack to our request");
 		goto end;
 	}
-	/* Close opened BPF file descriptors of this flow */
-	for (i = 0; i < SEC_MAX; i++)
-		if (flow->bpf_fd[i] != 0) {
-			close(flow->bpf_fd[i]);
-			flow->bpf_fd[i] = 0;
-		}
-
-	/* Release map key for this RSS rule */
-	ret = bpf_rss_key(KEY_CMD_RELEASE, &flow->key_idx);
-	if (ret < 0) {
-		rte_flow_error_set(
-			error, EINVAL, RTE_FLOW_ERROR_TYPE_HANDLE, NULL,
-			"Failed to release BPF RSS key");
-
-		goto end;
-	}
 
 	if (remote_flow) {
 		remote_flow->msg.nh.nlmsg_flags = NLM_F_REQUEST | NLM_F_ACK;
@@ -1520,7 +1539,7 @@ tap_flow_destroy_pmd(struct pmd_internals *pmd,
 end:
 	if (remote_flow)
 		rte_free(remote_flow);
-	rte_free(flow);
+	tap_flow_free(pmd, flow);
 	return ret;
 }
 
-- 
2.7.4

[dpdk-dev] [PATCH v2 2/2] net/tap: fix eBPF RSS map key handling

[Ophir Munk]

This commit addresses a case of RSS and non RSS flows mixture.
In the corrupted code, if a non-RSS flow is destroyed, it has an eBPF map
index 0 (initialized upon flow creation). This might unintentionally remove
RSS entry 0 from eBPF map.
To fix this issue, add an offset to the real index during a KEY_CMD_GET
operation, and subtract this offset during a KEY_CMD_RELEASE operation, in
order to restore the real index.
Thus, if a non RSS flow is falsely trying to release map entry 0 - The
offset subtraction will calculate the real map index as an
out-of-range value, and the release operation will be silently ignored.

Fixes: 036d721a8229 ("net/tap: implement RSS using eBPF")

Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
---
 drivers/net/tap/tap_flow.c | 47 ++++++++++++++++++++++++++++++++++------------
 1 file changed, 35 insertions(+), 12 deletions(-)

diff --git a/drivers/net/tap/tap_flow.c b/drivers/net/tap/tap_flow.c
index 533879d..18cfb91 100644
--- a/drivers/net/tap/tap_flow.c
+++ b/drivers/net/tap/tap_flow.c
@@ -1797,6 +1797,7 @@ tap_flow_implicit_flush(struct pmd_internals *pmd, struct rte_flow_error *error)
 }
 
 #define MAX_RSS_KEYS 256
+#define KEY_IDX_OFFSET (3 * MAX_RSS_KEYS)
 #define SEC_NAME_CLS_Q "cls_q"
 
 const char *sec_name[SEC_MAX] = {
@@ -1953,38 +1954,62 @@ static int rss_enable(struct pmd_internals *pmd,
 static int bpf_rss_key(enum bpf_rss_key_e cmd, __u32 *key_idx)
 {
 	__u32 i;
-	int err = -1;
+	int err = 0;
 	static __u32 num_used_keys;
 	static __u32 rss_keys[MAX_RSS_KEYS] = {KEY_STAT_UNSPEC};
 	static __u32 rss_keys_initialized;
 
 	switch (cmd) {
 	case KEY_CMD_GET:
-		if (!rss_keys_initialized)
+		if (!rss_keys_initialized) {
+			err = -1;
 			break;
+		}
 
-		if (num_used_keys == RTE_DIM(rss_keys))
+		if (num_used_keys == RTE_DIM(rss_keys)) {
+			err = -1;
 			break;
+		}
 
 		*key_idx = num_used_keys % RTE_DIM(rss_keys);
 		while (rss_keys[*key_idx] == KEY_STAT_USED)
 			*key_idx = (*key_idx + 1) % RTE_DIM(rss_keys);
 
 		rss_keys[*key_idx] = KEY_STAT_USED;
+
+		/*
+		 * Add an offset to key_idx in order to handle a case of
+		 * RSS and non RSS flows mixture.
+		 * If a non RSS flow is destroyed it has an eBPF map
+		 * index 0 (initialized on flow creation) and might
+		 * unintentionally remove RSS entry 0 from eBPF map.
+		 * To avoid this issue, add an offset to the real index
+		 * during a KEY_CMD_GET operation and subtract this offset
+		 * during a KEY_CMD_RELEASE operation in order to restore
+		 * the real index.
+		 */
+		*key_idx += KEY_IDX_OFFSET;
 		num_used_keys++;
-		err = 0;
 	break;
 
 	case KEY_CMD_RELEASE:
-		if (!rss_keys_initialized) {
-			err = 0;
+		if (!rss_keys_initialized)
+			break;
+
+		/*
+		 * Subtract offest to restore real key index
+		 * If a non RSS flow is falsely trying to release map
+		 * entry 0 - the offset subtraction will calculate the real
+		 * map index as an out-of-range value and the release operation
+		 * will be silently ignored.
+		 */
+		__u32 key = *key_idx - KEY_IDX_OFFSET;
+		if (key >= RTE_DIM(rss_keys))
 			break;
-		}
 
-		if (rss_keys[*key_idx] == KEY_STAT_USED) {
-			rss_keys[*key_idx] = KEY_STAT_AVAILABLE;
+		if (rss_keys[key] == KEY_STAT_USED) {
+			rss_keys[key] = KEY_STAT_AVAILABLE;
 			num_used_keys--;
-			err = 0;
 		}
 	break;
 
@@ -1994,7 +2019,6 @@ static int bpf_rss_key(enum bpf_rss_key_e cmd, __u32 *key_idx)
 
 		rss_keys_initialized = 1;
 		num_used_keys = 0;
-		err = 0;
 	break;
 
 	case KEY_CMD_DEINIT:
@@ -2003,7 +2027,6 @@ static int bpf_rss_key(enum bpf_rss_key_e cmd, __u32 *key_idx)
 
 		rss_keys_initialized = 0;
 		num_used_keys = 0;
-		err = 0;
 	break;
 
 	default:
-- 
2.7.4

Re: [dpdk-dev] [PATCH v2 0/2] fix eBPF TAP RSS leakage and map key handling

[Pascal Mazon]

The patches look good to me.
Good catch.

Acked-by: Pascal Mazon <pascal.mazon@6wind.com> for the series.

On 30/01/2018 17:00, Ophir Munk wrote:
> v2:
> fix eBPF RSS map key handling
>
> v1:
> fix eBPF file descriptors leakage
>
> Ophir Munk (2):
>   net/tap: fix eBPF file descriptors leakage
>   net/tap: fix eBPF RSS map key handling
>
>  drivers/net/tap/tap_flow.c | 104 +++++++++++++++++++++++++++++++--------------
>  1 file changed, 73 insertions(+), 31 deletions(-)
>

Re: [dpdk-dev] [PATCH v2 0/2] fix eBPF TAP RSS leakage and map key handling

[Ferruh Yigit]

On 1/30/2018 4:10 PM, Pascal Mazon wrote:
> The patches look good to me.
> Good catch.
> 
> On 30/01/2018 17:00, Ophir Munk wrote:
>> v2:
>> fix eBPF RSS map key handling
>>
>> v1:
>> fix eBPF file descriptors leakage
>>
>> Ophir Munk (2):
>>   net/tap: fix eBPF file descriptors leakage
>>   net/tap: fix eBPF RSS map key handling

> Acked-by: Pascal Mazon <pascal.mazon@6wind.com>

Series applied to dpdk-next-net/master, thanks.