[dpdk-dev] [PATCH] Fix librte_pmd_pcap driver double stop error

[dpdk-dev] [PATCH] Fix librte_pmd_pcap driver double stop error

[Nicolás Pernas Maradei]

From: Nicolás Pernas Maradei <nico@emutex.com>

librte_pmd_pcap driver was opening the pcap/interfaces only at init time and
closing them only when the port was being stopped. This behaviour would cause
problems (leading to segfault) if the user closed the port 2 times. The first
time the pcap/interfaces would be normally closed but libpcap would throw an
error causing a segfault if the closed pcaps/interfaces were closed again.
This behaviour is solved by re-openning pcaps/interfaces when the port is
started (only if these weren't open already for example at init time).

Signed-off-by: Nicolás Pernas Maradei <nico@emutex.com>
---
 lib/librte_pmd_pcap/rte_eth_pcap.c | 254 +++++++++++++++++++++++++++++--------
 1 file changed, 202 insertions(+), 52 deletions(-)

diff --git a/lib/librte_pmd_pcap/rte_eth_pcap.c b/lib/librte_pmd_pcap/rte_eth_pcap.c
index eebe768..f4d501d 100644
--- a/lib/librte_pmd_pcap/rte_eth_pcap.c
+++ b/lib/librte_pmd_pcap/rte_eth_pcap.c
@@ -66,6 +66,8 @@ struct pcap_rx_queue {
 	struct rte_mempool *mb_pool;
 	volatile unsigned long rx_pkts;
 	volatile unsigned long err_pkts;
+	const char *name;
+	const char *type;
 };
 
 struct pcap_tx_queue {
@@ -73,17 +75,23 @@ struct pcap_tx_queue {
 	pcap_t *pcap;
 	volatile unsigned long tx_pkts;
 	volatile unsigned long err_pkts;
+	const char *name;
+	const char *type;
 };
 
 struct rx_pcaps {
 	unsigned num_of_rx;
 	pcap_t *pcaps[RTE_PMD_RING_MAX_RX_RINGS];
+	const char *names[RTE_PMD_RING_MAX_RX_RINGS];
+	const char *types[RTE_PMD_RING_MAX_RX_RINGS];
 };
 
 struct tx_pcaps {
 	unsigned num_of_tx;
 	pcap_dumper_t *dumpers[RTE_PMD_RING_MAX_TX_RINGS];
 	pcap_t *pcaps[RTE_PMD_RING_MAX_RX_RINGS];
+	const char *names[RTE_PMD_RING_MAX_RX_RINGS];
+	const char *types[RTE_PMD_RING_MAX_RX_RINGS];
 };
 
 struct pmd_internals {
@@ -105,6 +113,10 @@ const char *valid_arguments[] = {
 	NULL
 };
 
+static int open_single_tx_pcap(const char *pcap_filename, pcap_dumper_t **dumper);
+static int open_single_rx_pcap(const char *pcap_filename, pcap_t **pcap);
+static int open_single_iface(const char *iface, pcap_t **pcap);
+
 static struct ether_addr eth_addr = { .addr_bytes = { 0, 0, 0, 0x1, 0x2, 0x3 } };
 static const char *drivername = "Pcap PMD";
 static struct rte_eth_link pmd_link = {
@@ -253,6 +265,59 @@ eth_pcap_tx(void *queue,
 static int
 eth_dev_start(struct rte_eth_dev *dev)
 {
+	unsigned i;
+	struct pmd_internals *internals = dev->data->dev_private;
+	unsigned nb_rxq = internals->nb_rx_queues;
+	unsigned nb_txq = internals->nb_tx_queues;
+	struct pcap_tx_queue *tx;
+	struct pcap_rx_queue *rx;
+
+	/* Special iface case. Single pcap is open and shared between tx/rx. */
+	if (nb_rxq == nb_txq && nb_rxq == 1) {
+		tx = &internals->tx_queue[0];
+		rx = &internals->rx_queue[0];
+
+		if (!tx->pcap && strcmp(tx->type, ETH_PCAP_IFACE_ARG) == 0) {
+			if (open_single_iface(tx->name, &tx->pcap) < 0)
+				return -1;
+			rx->pcap = tx->pcap;
+			return 0;
+		}
+	}
+
+	/* If not open already, open tx pcaps/dumpers */
+	for (i = 0; i < internals->nb_tx_queues; i++) {
+		tx = &internals->tx_queue[i];
+
+		if (!tx->dumper && strcmp(tx->type, ETH_PCAP_TX_PCAP_ARG) == 0) {
+			if (open_single_tx_pcap(tx->name, &tx->dumper) < 0)
+				return -1;
+		}
+
+		else if (!tx->pcap && strcmp(tx->type, ETH_PCAP_TX_IFACE_ARG) == 0) {
+			if (open_single_iface(tx->name, &tx->pcap) < 0)
+				return -1;
+		}
+	}
+
+	/* If not open already, open rx pcaps */
+	for (i = 0; i < internals->nb_rx_queues; i++) {
+		rx = &internals->rx_queue[i];
+
+		if (rx->pcap != NULL)
+			continue;
+
+		if (strcmp(rx->type, ETH_PCAP_RX_PCAP_ARG) == 0) {
+			if (open_single_rx_pcap(rx->name, &rx->pcap) < 0)
+				return -1;
+		}
+
+		else if (strcmp(rx->type, ETH_PCAP_RX_IFACE_ARG) == 0) {
+			if (open_single_iface(rx->name, &rx->pcap) < 0)
+				return -1;
+		}
+	}
+
 	dev->data->dev_link.link_status = 1;
 	return 0;
 }
@@ -266,17 +331,45 @@ static void
 eth_dev_stop(struct rte_eth_dev *dev)
 {
 	unsigned i;
-	pcap_dumper_t *dumper;
-	pcap_t *pcap;
 	struct pmd_internals *internals = dev->data->dev_private;
+	struct pcap_tx_queue *tx;
+	struct pcap_rx_queue *rx;
+	unsigned nb_rxq = internals->nb_rx_queues;
+	unsigned nb_txq = internals->nb_tx_queues;
+
+	/* Special iface case. Single pcap is open and shared between tx/rx. */
+	if (internals->rx_queue->pcap == internals->tx_queue->pcap &&
+		nb_rxq == nb_txq && nb_rxq == 1) {
+
+		tx = &internals->tx_queue[0];
+		rx = &internals->rx_queue[0];
+		pcap_close(tx->pcap);
+		tx->pcap = NULL;
+		rx->pcap = NULL;
+		return;
+	}
 
 	for (i = 0; i < internals->nb_tx_queues; i++) {
-		dumper = internals->tx_queue[i].dumper;
-		if(dumper != NULL)
-			pcap_dump_close(dumper);
-		pcap = internals->tx_queue[i].pcap;
-		if(pcap != NULL)
-			pcap_close(pcap);
+		tx = &internals->tx_queue[i];
+
+		if (tx->dumper != NULL) {
+			pcap_dump_close(tx->dumper);
+			tx->dumper = NULL;
+		}
+
+		if (tx->pcap != NULL) {
+			pcap_close(tx->pcap);
+			tx->pcap = NULL;
+		}
+	}
+
+	for (i = 0; i < internals->nb_rx_queues; i++) {
+		rx = &internals->rx_queue[i];
+
+		if (rx->pcap != NULL) {
+			pcap_close(rx->pcap);
+			rx->pcap = NULL;
+		}
 	}
 
 	dev->data->dev_link.link_status = 0;
@@ -409,55 +502,79 @@ static struct eth_dev_ops ops = {
  * reference of it for use it later on.
  */
 static int
-open_rx_pcap(const char *key __rte_unused, const char *value, void *extra_args)
+open_rx_pcap(const char *key, const char *value, void *extra_args)
 {
 	unsigned i;
 	const char *pcap_filename = value;
 	struct rx_pcaps *pcaps = extra_args;
-	pcap_t *rx_pcap;
+	pcap_t *pcap = NULL;
 
 	for (i = 0; i < pcaps->num_of_rx; i++) {
-		if ((rx_pcap = pcap_open_offline(pcap_filename, errbuf)) == NULL) {
-			RTE_LOG(ERR, PMD, "Couldn't open %s: %s\n", pcap_filename, errbuf);
+		if (open_single_rx_pcap(pcap_filename, &pcap) < 0)
 			return -1;
-		}
-		pcaps->pcaps[i] = rx_pcap;
+
+		pcaps->pcaps[i] = pcap;
+		pcaps->names[i] = pcap_filename;
+		pcaps->types[i] = key;
 	}
 
 	return 0;
 }
 
+static int
+open_single_rx_pcap(const char *pcap_filename, pcap_t **pcap)
+{
+	if ((*pcap = pcap_open_offline(pcap_filename, errbuf)) == NULL) {
+		RTE_LOG(ERR, PMD, "Couldn't open %s: %s\n", pcap_filename, errbuf);
+		return -1;
+	}
+	return 0;
+}
+
 /*
  * Opens a pcap file for writing and stores a reference to it
  * for use it later on.
  */
 static int
-open_tx_pcap(const char *key __rte_unused, const char *value, void *extra_args)
+open_tx_pcap(const char *key, const char *value, void *extra_args)
 {
 	unsigned i;
 	const char *pcap_filename = value;
 	struct tx_pcaps *dumpers = extra_args;
-	pcap_t *tx_pcap;
 	pcap_dumper_t *dumper;
 
 	for (i = 0; i < dumpers->num_of_tx; i++) {
-		/*
-		 * We need to create a dummy empty pcap_t to use it
-		 * with pcap_dump_open(). We create big enough an Ethernet
-		 * pcap holder.
-		 */
-		if ((tx_pcap = pcap_open_dead(DLT_EN10MB, RTE_ETH_PCAP_SNAPSHOT_LEN))
-				== NULL) {
-			RTE_LOG(ERR, PMD, "Couldn't create dead pcap\n");
+		if (open_single_tx_pcap(pcap_filename, &dumper) < 0)
 			return -1;
-		}
 
-		/* The dumper is created using the previous pcap_t reference */
-		if ((dumper = pcap_dump_open(tx_pcap, pcap_filename)) == NULL) {
-			RTE_LOG(ERR, PMD, "Couldn't open %s for writing.\n", pcap_filename);
-			return -1;
-		}
 		dumpers->dumpers[i] = dumper;
+		dumpers->names[i] = pcap_filename;
+		dumpers->types[i] = key;
+	}
+
+	return 0;
+}
+
+static int
+open_single_tx_pcap(const char *pcap_filename, pcap_dumper_t **dumper)
+{
+	pcap_t *tx_pcap;
+	/*
+	 * We need to create a dummy empty pcap_t to use it
+	 * with pcap_dump_open(). We create big enough an Ethernet
+	 * pcap holder.
+	 */
+
+	if ((tx_pcap = pcap_open_dead(DLT_EN10MB, RTE_ETH_PCAP_SNAPSHOT_LEN))
+			== NULL) {
+		RTE_LOG(ERR, PMD, "Couldn't create dead pcap\n");
+		return -1;
+	}
+
+	/* The dumper is created using the previous pcap_t reference */
+	if ((*dumper = pcap_dump_open(tx_pcap, pcap_filename)) == NULL) {
+		RTE_LOG(ERR, PMD, "Couldn't open %s for writing.\n", pcap_filename);
+		return -1;
 	}
 
 	return 0;
@@ -482,13 +599,19 @@ open_iface_live(const char *iface, pcap_t **pcap) {
  * Opens an interface for reading and writing
  */
 static inline int
-open_rx_tx_iface(const char *key __rte_unused, const char *value, void *extra_args)
+open_rx_tx_iface(const char *key, const char *value, void *extra_args)
 {
 	const char *iface = value;
-	pcap_t **pcap = extra_args;
+	struct rx_pcaps *pcaps = extra_args;
+	pcap_t *pcap = NULL;
 
-	if(open_iface_live(iface, pcap) < 0)
+	if (open_single_iface(iface, &pcap) < 0)
 		return -1;
+
+	pcaps->pcaps[0] = pcap;
+	pcaps->names[0] = iface;
+	pcaps->types[0] = key;
+
 	return 0;
 }
 
@@ -496,7 +619,7 @@ open_rx_tx_iface(const char *key __rte_unused, const char *value, void *extra_ar
  * Opens a NIC for reading packets from it
  */
 static inline int
-open_rx_iface(const char *key __rte_unused, const char *value, void *extra_args)
+open_rx_iface(const char *key, const char *value, void *extra_args)
 {
 	unsigned i;
 	const char *iface = value;
@@ -504,9 +627,11 @@ open_rx_iface(const char *key __rte_unused, const char *value, void *extra_args)
 	pcap_t *pcap = NULL;
 
 	for (i = 0; i < pcaps->num_of_rx; i++) {
-		if(open_iface_live(iface, &pcap) < 0)
+		if (open_single_iface(iface, &pcap) < 0)
 			return -1;
 		pcaps->pcaps[i] = pcap;
+		pcaps->names[i] = iface;
+		pcaps->types[i] = key;
 	}
 
 	return 0;
@@ -515,8 +640,8 @@ open_rx_iface(const char *key __rte_unused, const char *value, void *extra_args)
 /*
  * Opens a NIC for writing packets to it
  */
-static inline int
-open_tx_iface(const char *key __rte_unused, const char *value, void *extra_args)
+static int
+open_tx_iface(const char *key, const char *value, void *extra_args)
 {
 	unsigned i;
 	const char *iface = value;
@@ -524,14 +649,26 @@ open_tx_iface(const char *key __rte_unused, const char *value, void *extra_args)
 	pcap_t *pcap;
 
 	for (i = 0; i < pcaps->num_of_tx; i++) {
-		if(open_iface_live(iface, &pcap) < 0)
+		if (open_single_iface(iface, &pcap) < 0)
 			return -1;
 		pcaps->pcaps[i] = pcap;
+		pcaps->names[i] = iface;
+		pcaps->types[i] = key;
 	}
 
 	return 0;
 }
 
+static int
+open_single_iface(const char *iface, pcap_t **pcap)
+{
+	if (open_iface_live(iface, pcap) < 0) {
+		RTE_LOG(ERR, PMD, "Couldn't open interface %s\n", iface);
+		return -1;
+	}
+
+	return 0;
+}
 
 static int
 rte_pmd_init_internals(const char *name, const unsigned nb_rx_queues,
@@ -617,9 +754,10 @@ rte_pmd_init_internals(const char *name, const unsigned nb_rx_queues,
 }
 
 static int
-rte_eth_from_pcaps_n_dumpers(const char *name, pcap_t * const rx_queues[],
+rte_eth_from_pcaps_n_dumpers(const char *name,
+		struct rx_pcaps *rx_queues,
 		const unsigned nb_rx_queues,
-		pcap_dumper_t * const tx_queues[],
+		struct tx_pcaps *tx_queues,
 		const unsigned nb_tx_queues,
 		const unsigned numa_node,
 		struct rte_kvargs *kvlist)
@@ -639,10 +777,14 @@ rte_eth_from_pcaps_n_dumpers(const char *name, pcap_t * const rx_queues[],
 		return -1;
 
 	for (i = 0; i < nb_rx_queues; i++) {
-		internals->rx_queue->pcap = rx_queues[i];
+		internals->rx_queue->pcap = rx_queues->pcaps[i];
+		internals->rx_queue->name = rx_queues->names[i];
+		internals->rx_queue->type = rx_queues->types[i];
 	}
 	for (i = 0; i < nb_tx_queues; i++) {
-		internals->tx_queue->dumper = tx_queues[i];
+		internals->tx_queue->dumper = tx_queues->dumpers[i];
+		internals->tx_queue->name = tx_queues->names[i];
+		internals->tx_queue->type = tx_queues->types[i];
 	}
 
 	eth_dev->rx_pkt_burst = eth_pcap_rx;
@@ -651,10 +793,12 @@ rte_eth_from_pcaps_n_dumpers(const char *name, pcap_t * const rx_queues[],
 	return 0;
 }
 
+	struct rx_pcaps pcaps;
 static int
-rte_eth_from_pcaps(const char *name, pcap_t * const rx_queues[],
+rte_eth_from_pcaps(const char *name,
+		struct rx_pcaps *rx_queues,
 		const unsigned nb_rx_queues,
-		pcap_t * const tx_queues[],
+		struct tx_pcaps *tx_queues,
 		const unsigned nb_tx_queues,
 		const unsigned numa_node,
 		struct rte_kvargs *kvlist)
@@ -674,10 +818,14 @@ rte_eth_from_pcaps(const char *name, pcap_t * const rx_queues[],
 		return -1;
 
 	for (i = 0; i < nb_rx_queues; i++) {
-		internals->rx_queue->pcap = rx_queues[i];
+		internals->rx_queue->pcap = rx_queues->pcaps[i];
+		internals->rx_queue->name = rx_queues->names[i];
+		internals->rx_queue->type = rx_queues->types[i];
 	}
 	for (i = 0; i < nb_tx_queues; i++) {
-		internals->tx_queue->pcap = tx_queues[i];
+		internals->tx_queue->pcap = tx_queues->pcaps[i];
+		internals->tx_queue->name = tx_queues->names[i];
+		internals->tx_queue->type = tx_queues->types[i];
 	}
 
 	eth_dev->rx_pkt_burst = eth_pcap_rx;
@@ -715,11 +863,13 @@ rte_pmd_pcap_devinit(const char *name, const char *params)
 	if (rte_kvargs_count(kvlist, ETH_PCAP_IFACE_ARG) == 1) {
 
 		ret = rte_kvargs_process(kvlist, ETH_PCAP_IFACE_ARG,
-				&open_rx_tx_iface, &pcaps.pcaps[0]);
+				&open_rx_tx_iface, &pcaps);
 		if (ret < 0)
 			return -1;
-
-		return rte_eth_from_pcaps(name, pcaps.pcaps, 1, pcaps.pcaps, 1,
+		dumpers.pcaps[0] = pcaps.pcaps[0];
+		dumpers.names[0] = pcaps.names[0];
+		dumpers.types[0] = pcaps.types[0];
+		return rte_eth_from_pcaps(name, &pcaps, 1, &dumpers, 1,
 				numa_node, kvlist);
 	}
 
@@ -760,10 +910,10 @@ rte_pmd_pcap_devinit(const char *name, const char *params)
 		return -1;
 
 	if (using_dumpers)
-		return rte_eth_from_pcaps_n_dumpers(name, pcaps.pcaps, pcaps.num_of_rx,
-				dumpers.dumpers, dumpers.num_of_tx, numa_node, kvlist);
+		return rte_eth_from_pcaps_n_dumpers(name, &pcaps, pcaps.num_of_rx,
+				&dumpers, dumpers.num_of_tx, numa_node, kvlist);
 
-	return rte_eth_from_pcaps(name, pcaps.pcaps, pcaps.num_of_rx, dumpers.pcaps,
+	return rte_eth_from_pcaps(name, &pcaps, pcaps.num_of_rx, &dumpers,
 			dumpers.num_of_tx, numa_node, kvlist);
 
 }
-- 
1.9.1

Re: [dpdk-dev] [PATCH] Fix librte_pmd_pcap driver double stop error

[Thomas Monjalon]

2014-09-10 17:17, Nicolás Pernas Maradei:
> From: Nicolás Pernas Maradei <nico@emutex.com>
> 
> librte_pmd_pcap driver was opening the pcap/interfaces only at init time and
> closing them only when the port was being stopped. This behaviour would cause
> problems (leading to segfault) if the user closed the port 2 times. The first
> time the pcap/interfaces would be normally closed but libpcap would throw an
> error causing a segfault if the closed pcaps/interfaces were closed again.
> This behaviour is solved by re-openning pcaps/interfaces when the port is
> started (only if these weren't open already for example at init time).
> 
> Signed-off-by: Nicolás Pernas Maradei <nico@emutex.com>
> ---
>  lib/librte_pmd_pcap/rte_eth_pcap.c | 254 +++++++++++++++++++++++++++++--------
>  1 file changed, 202 insertions(+), 52 deletions(-)

Someone to review this patch?

-- 
Thomas

Re: [dpdk-dev] [PATCH] Fix librte_pmd_pcap driver double stop error

[Neil Horman]

On Wed, Sep 10, 2014 at 05:17:05PM -0300, Nicolás Pernas Maradei wrote:
> From: Nicolás Pernas Maradei <nico@emutex.com>
> 
> librte_pmd_pcap driver was opening the pcap/interfaces only at init time and
> closing them only when the port was being stopped. This behaviour would cause
> problems (leading to segfault) if the user closed the port 2 times. The first
> time the pcap/interfaces would be normally closed but libpcap would throw an
> error causing a segfault if the closed pcaps/interfaces were closed again.
> This behaviour is solved by re-openning pcaps/interfaces when the port is
> started (only if these weren't open already for example at init time).
> 
> Signed-off-by: Nicolás Pernas Maradei <nico@emutex.com>

This patch assigns pointers to strings that are allocated in the devargs_list.
Given that there exists an api interface free_devargs_list(), I'm not sure that
whats being done here is consistently safe.  It seems like you should dup the
strings to make sure you always have the storage allocated, or find some other
method to store the needed information.

Neil

Re: [dpdk-dev] [PATCH] Fix librte_pmd_pcap driver double stop error

[Nicolás Pernas Maradei]

Hi,

You are correct, the parameters received in the driver are allocated in 
devargs_list (char *params variable). However, they already get strdup'd 
in rte_kvargs_parse(). This newly allocated string is part of kvlist and 
never freed up. The params variable is never used again so it can be 
freed by someone else using free_devargs_list(). I'd say it's safe 
enough to set up pointers in the way it's currently done.

Nico.

On 2014-09-29 15:24, Neil Horman wrote:
> On Wed, Sep 10, 2014 at 05:17:05PM -0300, Nicolás Pernas Maradei wrote:
>> From: Nicolás Pernas Maradei <nico@emutex.com>
>> 
>> librte_pmd_pcap driver was opening the pcap/interfaces only at init 
>> time and
>> closing them only when the port was being stopped. This behaviour 
>> would cause
>> problems (leading to segfault) if the user closed the port 2 times. 
>> The first
>> time the pcap/interfaces would be normally closed but libpcap would 
>> throw an
>> error causing a segfault if the closed pcaps/interfaces were closed 
>> again.
>> This behaviour is solved by re-openning pcaps/interfaces when the port 
>> is
>> started (only if these weren't open already for example at init time).
>> 
>> Signed-off-by: Nicolás Pernas Maradei <nico@emutex.com>
> 
> This patch assigns pointers to strings that are allocated in the 
> devargs_list.
> Given that there exists an api interface free_devargs_list(), I'm not 
> sure that
> whats being done here is consistently safe.  It seems like you should 
> dup the
> strings to make sure you always have the storage allocated, or find 
> some other
> method to store the needed information.
> 
> Neil

Re: [dpdk-dev] [PATCH] Fix librte_pmd_pcap driver double stop error

[Neil Horman]

On Sat, Oct 04, 2014 at 07:14:21PM +0100, Nicolás Pernas Maradei wrote:
> Hi,
> 
> You are correct, the parameters received in the driver are allocated in
> devargs_list (char *params variable). However, they already get strdup'd in
> rte_kvargs_parse(). This newly allocated string is part of kvlist and never
> freed up. The params variable is never used again so it can be freed by
> someone else using free_devargs_list(). I'd say it's safe enough to set up
> pointers in the way it's currently done.
> 
> Nico.
> 

ok, that seems reasonable
Acked-by: Neil Horman <nhorman@tuxdriver.com>

> On 2014-09-29 15:24, Neil Horman wrote:
> >On Wed, Sep 10, 2014 at 05:17:05PM -0300, Nicolás Pernas Maradei wrote:
> >>From: Nicolás Pernas Maradei <nico@emutex.com>
> >>
> >>librte_pmd_pcap driver was opening the pcap/interfaces only at init time
> >>and
> >>closing them only when the port was being stopped. This behaviour would
> >>cause
> >>problems (leading to segfault) if the user closed the port 2 times. The
> >>first
> >>time the pcap/interfaces would be normally closed but libpcap would
> >>throw an
> >>error causing a segfault if the closed pcaps/interfaces were closed
> >>again.
> >>This behaviour is solved by re-openning pcaps/interfaces when the port
> >>is
> >>started (only if these weren't open already for example at init time).
> >>
> >>Signed-off-by: Nicolás Pernas Maradei <nico@emutex.com>
> >
> >This patch assigns pointers to strings that are allocated in the
> >devargs_list.
> >Given that there exists an api interface free_devargs_list(), I'm not sure
> >that
> >whats being done here is consistently safe.  It seems like you should dup
> >the
> >strings to make sure you always have the storage allocated, or find some
> >other
> >method to store the needed information.
> >
> >Neil
>