diff mbox series

app/testpmd: fix use of indirect action after port close

Message ID 20220307164821.821406-1-dkozlyuk@nvidia.com (mailing list archive)
State Accepted, archived
Delegated to: Thomas Monjalon
Headers
Series app/testpmd: fix use of indirect action after port close |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/github-robot: build success github build: passed
ci/iol-mellanox-Performance success Performance Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/iol-aarch64-unit-testing success Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-abi-testing success Testing PASS

Commit Message

Dmitry Kozlyuk March 7, 2022, 4:48 p.m. UTC 
  When a port was closed, indirect actions could remain
with their handles no longer valid.
If a newly attached device was assigned the same ID as the closed port,
those indirect actions became accessible again.
Any attempt to use them resulted in an undefined behavior.
Automatically flush indirect actions when a port is closed.

Fixes: 4b61b8774be9 ("ethdev: introduce indirect flow action")
Cc: stable@dpdk.org

Signed-off-by: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
Acked-by: Matan Azrad <matan@nvidia.com>
---
 app/test-pmd/config.c  | 31 +++++++++++++++++++++++++++++++
 app/test-pmd/testpmd.c |  1 +
 app/test-pmd/testpmd.h |  1 +
 3 files changed, 33 insertions(+)

Comments

Singh, Aman Deep March 30, 2022, 2:23 p.m. UTC | #1 
Hi Dmitry,

On 3/7/2022 10:18 PM, Dmitry Kozlyuk wrote:
> When a port was closed, indirect actions could remain
> with their handles no longer valid.
> If a newly attached device was assigned the same ID as the closed port,
> those indirect actions became accessible again.
> Any attempt to use them resulted in an undefined behavior.
> Automatically flush indirect actions when a port is closed.
>
> Fixes: 4b61b8774be9 ("ethdev: introduce indirect flow action")
> Cc: stable@dpdk.org
>
> Signed-off-by: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
> Acked-by: Matan Azrad <matan@nvidia.com>
> ---
From: guides/prog_guide/rte_flow.rst
/"If ``RTE_ETH_DEV_CAPA_FLOW_SHARED_OBJECT_KEEP`` is advertised,//
//this means that the PMD can keep at least some indirect actions//
//across device stop and start.

/Please check, if we are inline with the guidelines given in the section.
/
/
>   app/test-pmd/config.c  | 31 +++++++++++++++++++++++++++++++
>   app/test-pmd/testpmd.c |  1 +
>   app/test-pmd/testpmd.h |  1 +
>   3 files changed, 33 insertions(+)
>
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index cc8e7aa138..04baa319e0 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -1844,6 +1844,37 @@ port_action_handle_destroy(portid_t port_id,
>   	return ret;
>   }
>   
> +int
> +port_action_handle_flush(portid_t port_id)
> +{
> +	struct rte_port *port;
> +	struct port_indirect_action **tmp;
> +	int ret = 0;
> +
> +	if (port_id_is_invalid(port_id, ENABLED_WARN) ||
> +	    port_id == (portid_t)RTE_PORT_ALL)
> +		return -EINVAL;
> +	port = &ports[port_id];
> +	tmp = &port->actions_list;
> +	while (*tmp != NULL) {
> +		struct rte_flow_error error;
> +		struct port_indirect_action *pia = *tmp;
> +
> +		/* Poisoning to make sure PMDs update it in case of error. */
> +		memset(&error, 0x44, sizeof(error));
> +		if (pia->handle != NULL &&
> +		    rte_flow_action_handle_destroy
> +					(port_id, pia->handle, &error) != 0) {
> +			printf("Indirect action #%u not destroyed\n", pia->id);
> +			ret = port_flow_complain(&error);
> +			tmp = &pia->next;
> +		} else {
> +			*tmp = pia->next;
> +			free(pia);
> +		}
> +	}
> +	return ret;
> +}
>   
>   /** Get indirect action by port + id */
>   struct rte_flow_action_handle *
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index fe2ce19f99..f07f76f8f5 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -3139,6 +3139,7 @@ close_port(portid_t pid)
>   		if (is_proc_primary()) {
>   			port_flow_flush(pi);
>   			port_flex_item_flush(pi);
> +			port_action_handle_flush(pi);
>   			rte_eth_dev_close(pi);
>   		}
>   
> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
> index 31f766c965..e25663e5b3 100644
> --- a/app/test-pmd/testpmd.h
> +++ b/app/test-pmd/testpmd.h
> @@ -907,6 +907,7 @@ int port_action_handle_create(portid_t port_id, uint32_t id,
>   			      const struct rte_flow_action *action);
>   int port_action_handle_destroy(portid_t port_id,
>   			       uint32_t n, const uint32_t *action);
> +int port_action_handle_flush(portid_t port_id);
>   struct rte_flow_action_handle *port_action_handle_get_by_id(portid_t port_id,
>   							    uint32_t id);
>   int port_action_handle_update(portid_t port_id, uint32_t id,
Dmitry Kozlyuk March 30, 2022, 10:56 p.m. UTC | #2 
Hi Aman,

> From: Singh, Aman Deep <aman.deep.singh@intel.com>
> Sent: Wednesday, March 30, 2022 5:24 PM
> [...]
> On 3/7/2022 10:18 PM, Dmitry Kozlyuk wrote:
> > When a port was closed, indirect actions could remain
> > with their handles no longer valid.
> > If a newly attached device was assigned the same ID as the closed port,
> > those indirect actions became accessible again.
> > Any attempt to use them resulted in an undefined behavior.
> > Automatically flush indirect actions when a port is closed.
> >
> > Fixes: 4b61b8774be9 ("ethdev: introduce indirect flow action")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
> > Acked-by: Matan Azrad <matan@nvidia.com>
> > ---
> From: guides/prog_guide/rte_flow.rst
> /"If ``RTE_ETH_DEV_CAPA_FLOW_SHARED_OBJECT_KEEP`` is advertised,//
> //this means that the PMD can keep at least some indirect actions//
> //across device stop and start.
> 
> /Please check, if we are inline with the guidelines given in the section.

This patch is related to port closing, not port stopping.
Flow API resources are owned by the port,
so they cannot be valid when the port is closed and its ethdev removed.
TestPMD was keeping indirect action handles contrary to this.

As for RTE_ETH_DEV_CAPA_FLOW_SHARED_OBJECT_KEEP,
there is already "flow indirect_action destroy"
to erase indirect actions before stopping the port if this is desired.
I had another patch in mind to add "flow indirect_action flush"
for convenience, but it is independent of this fix.
Dmitry Kozlyuk April 10, 2022, 8:31 p.m. UTC | #3 
Hi Aman,

> From: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
> Sent: Thursday, March 31, 2022 1:57 AM
> [...]
> > From: Singh, Aman Deep <aman.deep.singh@intel.com>
> > Sent: Wednesday, March 30, 2022 5:24 PM
> > [...]
> > On 3/7/2022 10:18 PM, Dmitry Kozlyuk wrote:
> > > When a port was closed, indirect actions could remain
> > > with their handles no longer valid.
> > > If a newly attached device was assigned the same ID as the closed
> port,
> > > those indirect actions became accessible again.
> > > Any attempt to use them resulted in an undefined behavior.
> > > Automatically flush indirect actions when a port is closed.
> > >
> > > Fixes: 4b61b8774be9 ("ethdev: introduce indirect flow action")
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
> > > Acked-by: Matan Azrad <matan@nvidia.com>
> > > ---
> > From: guides/prog_guide/rte_flow.rst
> > /"If ``RTE_ETH_DEV_CAPA_FLOW_SHARED_OBJECT_KEEP`` is advertised,//
> > //this means that the PMD can keep at least some indirect actions//
> > //across device stop and start.
> >
> > /Please check, if we are inline with the guidelines given in the
> section.
> 
> This patch is related to port closing, not port stopping.
> Flow API resources are owned by the port,
> so they cannot be valid when the port is closed and its ethdev removed.
> TestPMD was keeping indirect action handles contrary to this.
> 
> As for RTE_ETH_DEV_CAPA_FLOW_SHARED_OBJECT_KEEP,
> there is already "flow indirect_action destroy"
> to erase indirect actions before stopping the port if this is desired.
> I had another patch in mind to add "flow indirect_action flush"
> for convenience, but it is independent of this fix.

Did my response clear up your concern?
In short: this patch is aligned with the defined flow resource behavior.
Singh, Aman Deep April 12, 2022, 5:41 a.m. UTC | #4 
On 4/11/2022 2:01 AM, Dmitry Kozlyuk wrote:
> Hi Aman,
>
>> From: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
>> Sent: Thursday, March 31, 2022 1:57 AM
>> [...]
>>> From: Singh, Aman Deep <aman.deep.singh@intel.com>
>>> Sent: Wednesday, March 30, 2022 5:24 PM
>>> [...]
>>> On 3/7/2022 10:18 PM, Dmitry Kozlyuk wrote:
>>>> When a port was closed, indirect actions could remain
>>>> with their handles no longer valid.
>>>> If a newly attached device was assigned the same ID as the closed
>> port,
>>>> those indirect actions became accessible again.
>>>> Any attempt to use them resulted in an undefined behavior.
>>>> Automatically flush indirect actions when a port is closed.
>>>>
>>>> Fixes: 4b61b8774be9 ("ethdev: introduce indirect flow action")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
>>>> Acked-by: Matan Azrad <matan@nvidia.com>
>>>> ---
>>> From: guides/prog_guide/rte_flow.rst
>>> /"If ``RTE_ETH_DEV_CAPA_FLOW_SHARED_OBJECT_KEEP`` is advertised,//
>>> //this means that the PMD can keep at least some indirect actions//
>>> //across device stop and start.
>>>
>>> /Please check, if we are inline with the guidelines given in the
>> section.
>>
>> This patch is related to port closing, not port stopping.
>> Flow API resources are owned by the port,
>> so they cannot be valid when the port is closed and its ethdev removed.
>> TestPMD was keeping indirect action handles contrary to this.
Agreed, at port close these should be flushed. LGTM

Acked-by: Aman Singh <aman.deep.singh@intel.com>
>>
>> As for RTE_ETH_DEV_CAPA_FLOW_SHARED_OBJECT_KEEP,
>> there is already "flow indirect_action destroy"
>> to erase indirect actions before stopping the port if this is desired.
>> I had another patch in mind to add "flow indirect_action flush"
>> for convenience, but it is independent of this fix.
> Did my response clear up your concern?
> In short: this patch is aligned with the defined flow resource behavior.
Thomas Monjalon May 25, 2022, 10:52 a.m. UTC | #5 
12/04/2022 07:41, Singh, Aman Deep:
> On 4/11/2022 2:01 AM, Dmitry Kozlyuk wrote:
> >> From: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
> >> Sent: Thursday, March 31, 2022 1:57 AM
> >> [...]
> >>> From: Singh, Aman Deep <aman.deep.singh@intel.com>
> >>> Sent: Wednesday, March 30, 2022 5:24 PM
> >>> [...]
> >>> On 3/7/2022 10:18 PM, Dmitry Kozlyuk wrote:
> >>>> When a port was closed, indirect actions could remain
> >>>> with their handles no longer valid.
> >>>> If a newly attached device was assigned the same ID as the closed
> >> port,
> >>>> those indirect actions became accessible again.
> >>>> Any attempt to use them resulted in an undefined behavior.
> >>>> Automatically flush indirect actions when a port is closed.
> >>>>
> >>>> Fixes: 4b61b8774be9 ("ethdev: introduce indirect flow action")
> >>>> Cc: stable@dpdk.org
> >>>>
> >>>> Signed-off-by: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
> >>>> Acked-by: Matan Azrad <matan@nvidia.com>
> >>>> ---
> >>> From: guides/prog_guide/rte_flow.rst
> >>> /"If ``RTE_ETH_DEV_CAPA_FLOW_SHARED_OBJECT_KEEP`` is advertised,//
> >>> //this means that the PMD can keep at least some indirect actions//
> >>> //across device stop and start.
> >>>
> >>> /Please check, if we are inline with the guidelines given in the
> >> section.
> >>
> >> This patch is related to port closing, not port stopping.
> >> Flow API resources are owned by the port,
> >> so they cannot be valid when the port is closed and its ethdev removed.
> >> TestPMD was keeping indirect action handles contrary to this.
> Agreed, at port close these should be flushed. LGTM
> 
> Acked-by: Aman Singh <aman.deep.singh@intel.com>

Applied in next-net, thanks.
diff mbox series

Patch

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index cc8e7aa138..04baa319e0 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -1844,6 +1844,37 @@  port_action_handle_destroy(portid_t port_id,
 	return ret;
 }
 
+int
+port_action_handle_flush(portid_t port_id)
+{
+	struct rte_port *port;
+	struct port_indirect_action **tmp;
+	int ret = 0;
+
+	if (port_id_is_invalid(port_id, ENABLED_WARN) ||
+	    port_id == (portid_t)RTE_PORT_ALL)
+		return -EINVAL;
+	port = &ports[port_id];
+	tmp = &port->actions_list;
+	while (*tmp != NULL) {
+		struct rte_flow_error error;
+		struct port_indirect_action *pia = *tmp;
+
+		/* Poisoning to make sure PMDs update it in case of error. */
+		memset(&error, 0x44, sizeof(error));
+		if (pia->handle != NULL &&
+		    rte_flow_action_handle_destroy
+					(port_id, pia->handle, &error) != 0) {
+			printf("Indirect action #%u not destroyed\n", pia->id);
+			ret = port_flow_complain(&error);
+			tmp = &pia->next;
+		} else {
+			*tmp = pia->next;
+			free(pia);
+		}
+	}
+	return ret;
+}
 
 /** Get indirect action by port + id */
 struct rte_flow_action_handle *
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index fe2ce19f99..f07f76f8f5 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -3139,6 +3139,7 @@  close_port(portid_t pid)
 		if (is_proc_primary()) {
 			port_flow_flush(pi);
 			port_flex_item_flush(pi);
+			port_action_handle_flush(pi);
 			rte_eth_dev_close(pi);
 		}
 
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 31f766c965..e25663e5b3 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -907,6 +907,7 @@  int port_action_handle_create(portid_t port_id, uint32_t id,
 			      const struct rte_flow_action *action);
 int port_action_handle_destroy(portid_t port_id,
 			       uint32_t n, const uint32_t *action);
+int port_action_handle_flush(portid_t port_id);
 struct rte_flow_action_handle *port_action_handle_get_by_id(portid_t port_id,
 							    uint32_t id);
 int port_action_handle_update(portid_t port_id, uint32_t id,