[dpdk-dev,v5,1/2] librte_ether: add protection against overwrite device data
Commit Message
Added protection against overwrite device data in array rte_eth_dev_data[]
for the next secondary applications. Secondary process appends in the
first free place rather than at the beginning. This behavior prevents
overwriting devices data of primary process by secondary process.
Signed-off-by: Marcin Kerlin <marcinx.kerlin@intel.com>
---
lib/librte_ether/rte_ethdev.c | 89 +++++++++++++++++++++++++++++++---
lib/librte_ether/rte_ethdev.h | 12 +++++
lib/librte_ether/rte_ether_version.map | 6 +++
3 files changed, 100 insertions(+), 7 deletions(-)
Comments
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Marcin Kerlin
> Sent: Friday, September 30, 2016 3:01 PM
> To: dev@dpdk.org
> Cc: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>;
> thomas.monjalon@6wind.com; Kerlin, MarcinX <marcinx.kerlin@intel.com>
> Subject: [dpdk-dev] [PATCH v5 1/2] librte_ether: add protection against
> overwrite device data
>
> Added protection against overwrite device data in array rte_eth_dev_data[] for
> the next secondary applications. Secondary process appends in the first free
> place rather than at the beginning. This behavior prevents overwriting devices
> data of primary process by secondary process.
>
> Signed-off-by: Marcin Kerlin <marcinx.kerlin@intel.com>
Acked-by: Reshma Pattan <reshma.pattan@intel.com>
Hi Marcin,
2016-09-30 16:00, Marcin Kerlin:
> Added protection against overwrite device data in array rte_eth_dev_data[]
> for the next secondary applications. Secondary process appends in the
> first free place rather than at the beginning. This behavior prevents
> overwriting devices data of primary process by secondary process.
I've just realized that you are trying to fix an useless code.
Why not just remove the array rte_eth_dev_data[] at first?
We already have the array rte_eth_devices[] and there is a pointer to
rte_eth_dev_data in rte_eth_dev.
Is it just a workaround to be able to lookup the rte_eth_dev_data memzone
in the secondary process?
So wouldn't it be saner to have rte_eth_devices[] in a memzone?
Sergio, as the multi-process maintainer, I guess you have an idea :)
Hi Thomas,
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Thursday, October 06, 2016 11:41 AM
> To: Kerlin, MarcinX <marcinx.kerlin@intel.com>
> Cc: dev@dpdk.org; De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>;
> Gonzalez Monroy, Sergio <sergio.gonzalez.monroy@intel.com>
> Subject: Re: [PATCH v5 1/2] librte_ether: add protection against overwrite
> device data
>
> Hi Marcin,
>
> 2016-09-30 16:00, Marcin Kerlin:
> > Added protection against overwrite device data in array
> > rte_eth_dev_data[] for the next secondary applications. Secondary
> > process appends in the first free place rather than at the beginning.
> > This behavior prevents overwriting devices data of primary process by
> secondary process.
>
> I've just realized that you are trying to fix an useless code.
> Why not just remove the array rte_eth_dev_data[] at first?
because pointer to rte_eth_dev_data in rte_eth_devices[] is
just to array rte_eth_dev_data[].
rte_ethdev.c:214
eth_dev->data = &rte_eth_dev_data[port_id];
> We already have the array rte_eth_devices[] and there is a pointer to
> rte_eth_dev_data in rte_eth_dev.
As you write above there is a pointer, but after run secondary testpmd this pointer
will indicate data which hold from now data for secondary testpmd.
1) run testpmd [primary]
rte_eth_devices[0].data.name = 3:0.1
2) run testpmd [secondary]
This testpmd overwrite first index in rte_eth_dev_data[]
3:0.1 -> eth_pcap0
3) back to testpmd [primary]
rte_eth_devices[0].data.name = eth_pcap0
from now in primary testpmd our device name is eth_pcap0 but should be 3:0.1
>
> Is it just a workaround to be able to lookup the rte_eth_dev_data memzone in
> the secondary process?
No it is not workaround, it is protection against overwrite device data.
I think that my cover letter good explain what is wrong. I did there
short debug log.
> So wouldn't it be saner to have rte_eth_devices[] in a memzone?
So you mean that move rte_eth_devices[] to memzone + remove rte_eth_dev_data[].
What will indicate pointer inside rte_eth_dev rte_eth_devices[]:
(struct rte_eth_dev_data *data; /**< Pointer to device data */)
If I did not understand you please clarify more.
Regards,
Marcin
>
> Sergio, as the multi-process maintainer, I guess you have an idea :)
2016-10-06 13:57, Kerlin, MarcinX:
> Hi Thomas,
>
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> >
> > Hi Marcin,
> >
> > 2016-09-30 16:00, Marcin Kerlin:
> > > Added protection against overwrite device data in array
> > > rte_eth_dev_data[] for the next secondary applications. Secondary
> > > process appends in the first free place rather than at the beginning.
> > > This behavior prevents overwriting devices data of primary process by
> > secondary process.
> >
> > I've just realized that you are trying to fix an useless code.
> > Why not just remove the array rte_eth_dev_data[] at first?
>
> because pointer to rte_eth_dev_data in rte_eth_devices[] is
> just to array rte_eth_dev_data[].
>
> rte_ethdev.c:214
> eth_dev->data = &rte_eth_dev_data[port_id];
This line indicates that the pointer data is to the struct rte_eth_dev_data
of the port_id, not the array of every devices.
> > We already have the array rte_eth_devices[] and there is a pointer to
> > rte_eth_dev_data in rte_eth_dev.
>
> As you write above there is a pointer, but after run secondary testpmd this pointer
> will indicate data which hold from now data for secondary testpmd.
[...]
I think I've understood the bug.
I'm just saying you are fixing a weird design (rte_eth_dev_data[]).
> > Is it just a workaround to be able to lookup the rte_eth_dev_data memzone in
> > the secondary process?
>
> No it is not workaround, it is protection against overwrite device data.
> I think that my cover letter good explain what is wrong. I did there
> short debug log.
I'm talking about the initial introduction of rte_eth_dev_data[]
which seems to be a workaround for multi-process without touching
rte_eth_devices[] allocated as a global variable (not a memzone).
> > So wouldn't it be saner to have rte_eth_devices[] in a memzone?
>
> So you mean that move rte_eth_devices[] to memzone + remove rte_eth_dev_data[].
Yes
> What will indicate pointer inside rte_eth_dev rte_eth_devices[]:
> (struct rte_eth_dev_data *data; /**< Pointer to device data */)
After thinking more about it, I realize that rte_eth_devices cannot be
in a shared memzone because of its local pointers.
Sorry for the noise, I'll reconsider your patch.
2016-09-30 16:00, Marcin Kerlin:
> Added protection against overwrite device data in array rte_eth_dev_data[]
> for the next secondary applications. Secondary process appends in the
> first free place rather than at the beginning. This behavior prevents
> overwriting devices data of primary process by secondary process.
It would be good to state what is a secondary process.
You are trying to extend its capabilities to be able to initialize devices.
So primary and secondary processes are almost equivalent?
What happens if we do not create any device in the primary?
Answer from code review: "Cannot allocate memzone for ethernet port data\n"
The secondary process is a hack to me.
But it is fine to have such hack for debug or monitoring purpose.
I would like to understand what are the other use cases?
By the way, the code managing the shared data of a device should
be at the EAL level in order to be used by other interfaces like crypto.
> @@ -631,6 +692,8 @@ int
> rte_eth_dev_detach(uint8_t port_id, char *name)
> {
> struct rte_pci_addr addr;
> + struct rte_eth_dev_data *eth_dev_data = NULL;
> + char device[RTE_ETH_NAME_MAX_LEN];
> int ret = -1;
>
> if (name == NULL) {
> @@ -642,6 +705,15 @@ rte_eth_dev_detach(uint8_t port_id, char *name)
> if (rte_eth_dev_is_detachable(port_id))
> goto err;
>
> + /* get device name by port id */
> + if (rte_eth_dev_get_name_by_port(port_id, device))
> + goto err;
> +
> + /* look for an entry in the shared device data */
> + eth_dev_data = rte_eth_dev_get_dev_data_by_name(device);
> + if (eth_dev_data == NULL)
> + goto err;
Why not getting eth_dev_data from rte_eth_devices[port_id].data ?
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> /**
> * @internal
> + * Release device data kept in shared memory for all processes.
> + *
> + * @param port_id
> + * The port identifier of the device to release device data.
> + * @return
> + * - 0 on success, negative on error
> + */
> +int rte_eth_dev_release_dev_data(uint8_t port_id);
Why this function? It is not used.
You already have done the job in the detach function.
Hi Thomas,
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Thursday, October 06, 2016 4:53 PM
> To: Kerlin, MarcinX <marcinx.kerlin@intel.com>
> Cc: dev@dpdk.org; De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>
> Subject: Re: [PATCH v5 1/2] librte_ether: add protection against overwrite
> device data
>
> 2016-09-30 16:00, Marcin Kerlin:
> > Added protection against overwrite device data in array
> > rte_eth_dev_data[] for the next secondary applications. Secondary
> > process appends in the first free place rather than at the beginning.
> > This behavior prevents overwriting devices data of primary process by
> secondary process.
>
> It would be good to state what is a secondary process.
> You are trying to extend its capabilities to be able to initialize devices.
> So primary and secondary processes are almost equivalent?
> What happens if we do not create any device in the primary?
> Answer from code review: "Cannot allocate memzone for ethernet port data\n"
>
> The secondary process is a hack to me.
> But it is fine to have such hack for debug or monitoring purpose.
> I would like to understand what are the other use cases?
It's true, it is fine for debug or monitoring but If DPDK allow run secondary app with
devices then it should be safe or completely not allowed.
This bug has been observed while running secondary testpmd with virtual devices.
I will adapt to the decision of maintainers regards to design of secondary process.
>
> By the way, the code managing the shared data of a device should be at the
> EAL level in order to be used by other interfaces like crypto.
>
> > @@ -631,6 +692,8 @@ int
> > rte_eth_dev_detach(uint8_t port_id, char *name) {
> > struct rte_pci_addr addr;
> > + struct rte_eth_dev_data *eth_dev_data = NULL;
> > + char device[RTE_ETH_NAME_MAX_LEN];
> > int ret = -1;
> >
> > if (name == NULL) {
> > @@ -642,6 +705,15 @@ rte_eth_dev_detach(uint8_t port_id, char *name)
> > if (rte_eth_dev_is_detachable(port_id))
> > goto err;
> >
> > + /* get device name by port id */
> > + if (rte_eth_dev_get_name_by_port(port_id, device))
> > + goto err;
> > +
> > + /* look for an entry in the shared device data */
> > + eth_dev_data = rte_eth_dev_get_dev_data_by_name(device);
> > + if (eth_dev_data == NULL)
> > + goto err;
>
> Why not getting eth_dev_data from rte_eth_devices[port_id].data ?
because rte_eth_devices[port_id].data for some drivers (mainly virtual devices)
is pointer to local eth_dev_data (e.g rte_eth_pcap.c:816 and also other drivers).
This causes that local eth_dev_data is clearing rather than shared in memzone.
Naming is unique so if device was added then there (shared memzone) has to be.
>
> > --- a/lib/librte_ether/rte_ethdev.h
> > +++ b/lib/librte_ether/rte_ethdev.h
> > /**
> > * @internal
> > + * Release device data kept in shared memory for all processes.
> > + *
> > + * @param port_id
> > + * The port identifier of the device to release device data.
> > + * @return
> > + * - 0 on success, negative on error
> > + */
> > +int rte_eth_dev_release_dev_data(uint8_t port_id);
>
> Why this function? It is not used.
> You already have done the job in the detach function.
This function is using in testpmd.c:1640, basic wrapper for clean up.
Detach function is working only for detachable devices, release_dev_data()
no matter just clean up shared array before next run secondary e.g testpmd.
Regards,
Marcin
2016-10-07 12:23, Kerlin, MarcinX:
> Hi Thomas,
>
> > -----Original Message-----
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > Sent: Thursday, October 06, 2016 4:53 PM
> > To: Kerlin, MarcinX <marcinx.kerlin@intel.com>
> > Cc: dev@dpdk.org; De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>
> > Subject: Re: [PATCH v5 1/2] librte_ether: add protection against overwrite
> > device data
> >
> > 2016-09-30 16:00, Marcin Kerlin:
> > > Added protection against overwrite device data in array
> > > rte_eth_dev_data[] for the next secondary applications. Secondary
> > > process appends in the first free place rather than at the beginning.
> > > This behavior prevents overwriting devices data of primary process by
> > secondary process.
> >
> > It would be good to state what is a secondary process.
> > You are trying to extend its capabilities to be able to initialize devices.
> > So primary and secondary processes are almost equivalent?
> > What happens if we do not create any device in the primary?
> > Answer from code review: "Cannot allocate memzone for ethernet port data\n"
> >
> > The secondary process is a hack to me.
> > But it is fine to have such hack for debug or monitoring purpose.
> > I would like to understand what are the other use cases?
>
> It's true, it is fine for debug or monitoring but If DPDK allow run secondary app with
> devices then it should be safe or completely not allowed.
>
> This bug has been observed while running secondary testpmd with virtual devices.
>
> I will adapt to the decision of maintainers regards to design of secondary process.
>
> >
> > By the way, the code managing the shared data of a device should be at the
> > EAL level in order to be used by other interfaces like crypto.
> >
> > > @@ -631,6 +692,8 @@ int
> > > rte_eth_dev_detach(uint8_t port_id, char *name) {
> > > struct rte_pci_addr addr;
> > > + struct rte_eth_dev_data *eth_dev_data = NULL;
> > > + char device[RTE_ETH_NAME_MAX_LEN];
> > > int ret = -1;
> > >
> > > if (name == NULL) {
> > > @@ -642,6 +705,15 @@ rte_eth_dev_detach(uint8_t port_id, char *name)
> > > if (rte_eth_dev_is_detachable(port_id))
> > > goto err;
> > >
> > > + /* get device name by port id */
> > > + if (rte_eth_dev_get_name_by_port(port_id, device))
> > > + goto err;
> > > +
> > > + /* look for an entry in the shared device data */
> > > + eth_dev_data = rte_eth_dev_get_dev_data_by_name(device);
> > > + if (eth_dev_data == NULL)
> > > + goto err;
> >
> > Why not getting eth_dev_data from rte_eth_devices[port_id].data ?
>
> because rte_eth_devices[port_id].data for some drivers (mainly virtual devices)
> is pointer to local eth_dev_data (e.g rte_eth_pcap.c:816 and also other drivers).
> This causes that local eth_dev_data is clearing rather than shared in memzone.
>
> Naming is unique so if device was added then there (shared memzone) has to be.
Not sure to understand. Isn't it a bug to have local eth_dev_data?
It means these devices are not shared with secondary process?
> > > --- a/lib/librte_ether/rte_ethdev.h
> > > +++ b/lib/librte_ether/rte_ethdev.h
> > > /**
> > > * @internal
> > > + * Release device data kept in shared memory for all processes.
> > > + *
> > > + * @param port_id
> > > + * The port identifier of the device to release device data.
> > > + * @return
> > > + * - 0 on success, negative on error
> > > + */
> > > +int rte_eth_dev_release_dev_data(uint8_t port_id);
> >
> > Why this function? It is not used.
> > You already have done the job in the detach function.
>
> This function is using in testpmd.c:1640, basic wrapper for clean up.
Please explain the need for cleaning on testpmd exit.
Is it cleaning every devices even those used by the primary process?
I feel it is very weak to not clearly define which process owns a device.
> Detach function is working only for detachable devices, release_dev_data()
> no matter just clean up shared array before next run secondary e.g testpmd.
Yes freeing device resources is for detachable devices.
@@ -176,6 +176,19 @@ rte_eth_dev_allocated(const char *name)
return NULL;
}
+static struct rte_eth_dev_data *
+rte_eth_dev_get_dev_data_by_name(const char *name)
+{
+ unsigned int i;
+
+ for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
+ if (strcmp(rte_eth_dev_data[i].name, name) == 0)
+ return &rte_eth_dev_data[i];
+ }
+
+ return NULL;
+}
+
static uint8_t
rte_eth_dev_find_free_port(void)
{
@@ -188,10 +201,43 @@ rte_eth_dev_find_free_port(void)
return RTE_MAX_ETHPORTS;
}
+static uint8_t
+rte_eth_dev_find_free_dev_data_id(void)
+{
+ unsigned int i;
+
+ for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
+ if (!strlen(rte_eth_dev_data[i].name))
+ return i;
+ }
+ return RTE_MAX_ETHPORTS;
+}
+
+int
+rte_eth_dev_release_dev_data(uint8_t port_id)
+{
+ char device[RTE_ETH_NAME_MAX_LEN];
+ struct rte_eth_dev_data *eth_dev_data = NULL;
+
+ /* get device name by port id */
+ if (rte_eth_dev_get_name_by_port(port_id, device))
+ return -EINVAL;
+
+ /* look for an entry in the device data */
+ eth_dev_data = rte_eth_dev_get_dev_data_by_name(device);
+ if (eth_dev_data == NULL)
+ return -EINVAL;
+
+ /* clear an entry in the device data */
+ memset(eth_dev_data, 0, sizeof(struct rte_eth_dev_data));
+
+ return 0;
+}
+
struct rte_eth_dev *
rte_eth_dev_allocate(const char *name, enum rte_eth_dev_type type)
{
- uint8_t port_id;
+ uint8_t port_id, dev_data_id;
struct rte_eth_dev *eth_dev;
port_id = rte_eth_dev_find_free_port();
@@ -203,17 +249,35 @@ rte_eth_dev_allocate(const char *name, enum rte_eth_dev_type type)
if (rte_eth_dev_data == NULL)
rte_eth_dev_data_alloc();
+ do {
+ dev_data_id = rte_eth_dev_find_free_dev_data_id();
+ } while (!rte_spinlock_trylock(&rte_eth_dev_data[dev_data_id].lock)
+ && dev_data_id < RTE_MAX_ETHPORTS);
+
+ if (dev_data_id == RTE_MAX_ETHPORTS) {
+ RTE_PMD_DEBUG_TRACE("Reached maximum number of Ethernet ports "
+ "by all the processes\n");
+ return NULL;
+ }
+
if (rte_eth_dev_allocated(name) != NULL) {
RTE_PMD_DEBUG_TRACE("Ethernet Device with name %s already allocated!\n",
name);
return NULL;
}
+ if (rte_eth_dev_get_dev_data_by_name(name) != NULL) {
+ RTE_PMD_DEBUG_TRACE("Ethernet Device with name %s already "
+ "allocated by another process!\n", name);
+ return NULL;
+ }
+
eth_dev = &rte_eth_devices[port_id];
- eth_dev->data = &rte_eth_dev_data[port_id];
+ eth_dev->data = &rte_eth_dev_data[dev_data_id];
snprintf(eth_dev->data->name, sizeof(eth_dev->data->name), "%s", name);
eth_dev->data->port_id = port_id;
eth_dev->attached = DEV_ATTACHED;
+ rte_spinlock_unlock(ð_dev->data->lock);
eth_dev->dev_type = type;
nb_ports++;
return eth_dev;
@@ -417,9 +481,7 @@ rte_eth_dev_get_name_by_port(uint8_t port_id, char *name)
return -EINVAL;
}
- /* shouldn't check 'rte_eth_devices[i].data',
- * because it might be overwritten by VDEV PMD */
- tmp = rte_eth_dev_data[port_id].name;
+ tmp = rte_eth_devices[port_id].data->name;
strcpy(name, tmp);
return 0;
}
@@ -439,8 +501,7 @@ rte_eth_dev_get_port_by_name(const char *name, uint8_t *port_id)
for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
if (!strncmp(name,
- rte_eth_dev_data[i].name, strlen(name))) {
-
+ rte_eth_devices[i].data->name, strlen(name))) {
*port_id = i;
return 0;
@@ -631,6 +692,8 @@ int
rte_eth_dev_detach(uint8_t port_id, char *name)
{
struct rte_pci_addr addr;
+ struct rte_eth_dev_data *eth_dev_data = NULL;
+ char device[RTE_ETH_NAME_MAX_LEN];
int ret = -1;
if (name == NULL) {
@@ -642,6 +705,15 @@ rte_eth_dev_detach(uint8_t port_id, char *name)
if (rte_eth_dev_is_detachable(port_id))
goto err;
+ /* get device name by port id */
+ if (rte_eth_dev_get_name_by_port(port_id, device))
+ goto err;
+
+ /* look for an entry in the shared device data */
+ eth_dev_data = rte_eth_dev_get_dev_data_by_name(device);
+ if (eth_dev_data == NULL)
+ goto err;
+
if (rte_eth_dev_get_device_type(port_id) == RTE_ETH_DEV_PCI) {
ret = rte_eth_dev_get_addr_by_port(port_id, &addr);
if (ret < 0)
@@ -661,6 +733,9 @@ rte_eth_dev_detach(uint8_t port_id, char *name)
goto err;
}
+ /* clear an entry in the shared device data */
+ memset(eth_dev_data, 0, sizeof(struct rte_eth_dev_data));
+
return 0;
err:
@@ -1708,6 +1708,7 @@ struct rte_eth_dev_data {
enum rte_kernel_driver kdrv; /**< Kernel driver passthrough */
int numa_node; /**< NUMA node connection */
const char *drv_name; /**< Driver name */
+ rte_spinlock_t lock; /** Lock on allocate eth device */
};
/** Device supports hotplug detach */
@@ -1752,6 +1753,17 @@ struct rte_eth_dev *rte_eth_dev_allocated(const char *name);
/**
* @internal
+ * Release device data kept in shared memory for all processes.
+ *
+ * @param port_id
+ * The port identifier of the device to release device data.
+ * @return
+ * - 0 on success, negative on error
+ */
+int rte_eth_dev_release_dev_data(uint8_t port_id);
+
+/**
+ * @internal
* Allocates a new ethdev slot for an ethernet device and returns the pointer
* to that slot for the driver to use.
*
@@ -139,3 +139,9 @@ DPDK_16.07 {
rte_eth_dev_get_port_by_name;
rte_eth_xstats_get_names;
} DPDK_16.04;
+
+DPDK_16.11 {
+ global:
+
+ rte_eth_dev_release_dev_data;
+} DPDK_16.07;