[dpdk-dev] [PATCH v1] net/tap: fix promiscuous rules double insersions

[dpdk-dev] [PATCH v1] net/tap: fix promiscuous rules double insersions

[Ophir Munk]

Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
---
Full Commit message will follow immediately in v2

 drivers/net/tap/tap_flow.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/net/tap/tap_flow.c b/drivers/net/tap/tap_flow.c
index 65657f0..d1f4a52 100644
--- a/drivers/net/tap/tap_flow.c
+++ b/drivers/net/tap/tap_flow.c
@@ -123,6 +123,7 @@ enum key_status_e {
 };
 
 #define ISOLATE_HANDLE 1
+#define REMOTE_PROMISCUOUS_HANDLE 2
 
 struct rte_flow {
 	LIST_ENTRY(rte_flow) next; /* Pointer to the next rte_flow structure */
@@ -1692,9 +1693,15 @@ int tap_flow_implicit_create(struct pmd_internals *pmd,
 	 * The ISOLATE rule is always present and must have a static handle, as
 	 * the action is changed whether the feature is enabled (DROP) or
 	 * disabled (PASSTHRU).
+	 * There is just one REMOTE_PROMISCUOUS rule in all cases. It should
+	 * have a static handle such that adding it twice will fail with EEXIST
+	 * with any kernel version. Remark: old kernels may falsely accept the
+	 * same REMOTE_PREMISCUOUS rules if they had different handles.
 	 */
 	if (idx == TAP_ISOLATE)
 		remote_flow->msg.t.tcm_handle = ISOLATE_HANDLE;
+	else if (idx == TAP_REMOTE_PROMISC)
+		remote_flow->msg.t.tcm_handle = REMOTE_PROMISCUOUS_HANDLE;
 	else
 		tap_flow_set_handle(remote_flow);
 	if (priv_flow_process(pmd, attr, items, actions, NULL,
@@ -1709,12 +1716,16 @@ int tap_flow_implicit_create(struct pmd_internals *pmd,
 	}
 	err = tap_nl_recv_ack(pmd->nlsk_fd);
 	if (err < 0) {
+		/* Silently ignore re-entering remote promiscuous rule */
+		if (errno == EEXIST && idx == TAP_REMOTE_PROMISC)
+			goto success;
 		RTE_LOG(ERR, PMD,
 			"Kernel refused TC filter rule creation (%d): %s\n",
 			errno, strerror(errno));
 		goto fail;
 	}
 	LIST_INSERT_HEAD(&pmd->implicit_flows, remote_flow, next);
+success:
 	return 0;
 fail:
 	if (remote_flow)
-- 
2.7.4

[dpdk-dev] [PATCH v2] net/tap: fix promiscuous rules double insersions

[Ophir Munk]

Running testpmd command "port stop all" followed by command "port start
all" may result in a TAP error:
PMD: Kernel refused TC filter rule creation (17): File exists

Root cause analysis: during the execution of "port start all" command
testpmd calls rte_eth_promiscuous_enable() while during the execution
of "port stop all" command testpmd does not call
rte_eth_promiscuous_enable().
As a result the TAP PMD is trying to add tc (traffic control command)
promiscuous rules to the remote netvsc device consecutively. From the
kernel point of view it is seen as an attempt to add the same rule more
than once. In recent kernels (e.g. version 4.13) this attempt is rejected
with a "File exists" error. In less recent kernels (e.g. version 4.4) the
same rule may have been accepted twice successfully, which is undesirable.

In the corrupted code every tc promiscuous rule included a different
handle number parameter. If instead an identical handle number parameter is
used for all tc promiscuous rules - all kernels will reject the second
rule with a "File exists" error, which is easy to identify and to silently
ignore.

Fixes: 2bc06869cd94 ("net/tap: add remote netdevice traffic capture")
Cc: stable@dpdk.org

Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
---
v2: add detailed commit message

 drivers/net/tap/tap_flow.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/net/tap/tap_flow.c b/drivers/net/tap/tap_flow.c
index 65657f0..d1f4a52 100644
--- a/drivers/net/tap/tap_flow.c
+++ b/drivers/net/tap/tap_flow.c
@@ -123,6 +123,7 @@ enum key_status_e {
 };
 
 #define ISOLATE_HANDLE 1
+#define REMOTE_PROMISCUOUS_HANDLE 2
 
 struct rte_flow {
 	LIST_ENTRY(rte_flow) next; /* Pointer to the next rte_flow structure */
@@ -1692,9 +1693,15 @@ int tap_flow_implicit_create(struct pmd_internals *pmd,
 	 * The ISOLATE rule is always present and must have a static handle, as
 	 * the action is changed whether the feature is enabled (DROP) or
 	 * disabled (PASSTHRU).
+	 * There is just one REMOTE_PROMISCUOUS rule in all cases. It should
+	 * have a static handle such that adding it twice will fail with EEXIST
+	 * with any kernel version. Remark: old kernels may falsely accept the
+	 * same REMOTE_PREMISCUOUS rules if they had different handles.
 	 */
 	if (idx == TAP_ISOLATE)
 		remote_flow->msg.t.tcm_handle = ISOLATE_HANDLE;
+	else if (idx == TAP_REMOTE_PROMISC)
+		remote_flow->msg.t.tcm_handle = REMOTE_PROMISCUOUS_HANDLE;
 	else
 		tap_flow_set_handle(remote_flow);
 	if (priv_flow_process(pmd, attr, items, actions, NULL,
@@ -1709,12 +1716,16 @@ int tap_flow_implicit_create(struct pmd_internals *pmd,
 	}
 	err = tap_nl_recv_ack(pmd->nlsk_fd);
 	if (err < 0) {
+		/* Silently ignore re-entering remote promiscuous rule */
+		if (errno == EEXIST && idx == TAP_REMOTE_PROMISC)
+			goto success;
 		RTE_LOG(ERR, PMD,
 			"Kernel refused TC filter rule creation (%d): %s\n",
 			errno, strerror(errno));
 		goto fail;
 	}
 	LIST_INSERT_HEAD(&pmd->implicit_flows, remote_flow, next);
+success:
 	return 0;
 fail:
 	if (remote_flow)
-- 
2.7.4

Re: [dpdk-dev] [PATCH v2] net/tap: fix promiscuous rules double insersions

[Pascal Mazon]

Hi Ophir,

Typo in title: s/insersions/insertions/

I'm ok on principle, I have just a few comments inline.

Regards,
Pascal

On 13/02/2018 19:35, Ophir Munk wrote:
> Running testpmd command "port stop all" followed by command "port start
> all" may result in a TAP error:
> PMD: Kernel refused TC filter rule creation (17): File exists
>
> Root cause analysis: during the execution of "port start all" command
> testpmd calls rte_eth_promiscuous_enable() while during the execution
> of "port stop all" command testpmd does not call
> rte_eth_promiscuous_enable().
Shouldn't it be rte_eth_promiscuous_disable()?
> As a result the TAP PMD is trying to add tc (traffic control command)
> promiscuous rules to the remote netvsc device consecutively. From the
> kernel point of view it is seen as an attempt to add the same rule more
> than once. In recent kernels (e.g. version 4.13) this attempt is rejected
> with a "File exists" error. In less recent kernels (e.g. version 4.4) the
> same rule may have been accepted twice successfully, which is undesirable.
>
> In the corrupted code every tc promiscuous rule included a different
> handle number parameter. If instead an identical handle number parameter is
> used for all tc promiscuous rules - all kernels will reject the second
> rule with a "File exists" error, which is easy to identify and to silently
> ignore.
>
> Fixes: 2bc06869cd94 ("net/tap: add remote netdevice traffic capture")
> Cc: stable@dpdk.org
>
> Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
> ---
> v2: add detailed commit message
>
>  drivers/net/tap/tap_flow.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/net/tap/tap_flow.c b/drivers/net/tap/tap_flow.c
> index 65657f0..d1f4a52 100644
> --- a/drivers/net/tap/tap_flow.c
> +++ b/drivers/net/tap/tap_flow.c
> @@ -123,6 +123,7 @@ enum key_status_e {
>  };
>  
>  #define ISOLATE_HANDLE 1
> +#define REMOTE_PROMISCUOUS_HANDLE 2
>  
>  struct rte_flow {
>  	LIST_ENTRY(rte_flow) next; /* Pointer to the next rte_flow structure */
> @@ -1692,9 +1693,15 @@ int tap_flow_implicit_create(struct pmd_internals *pmd,
>  	 * The ISOLATE rule is always present and must have a static handle, as
>  	 * the action is changed whether the feature is enabled (DROP) or
>  	 * disabled (PASSTHRU).
> +	 * There is just one REMOTE_PROMISCUOUS rule in all cases. It should
> +	 * have a static handle such that adding it twice will fail with EEXIST
> +	 * with any kernel version. Remark: old kernels may falsely accept the
> +	 * same REMOTE_PREMISCUOUS rules if they had different handles.
s/PREMISCUOUS/PROMISCUOUS/
>  	 */
>  	if (idx == TAP_ISOLATE)
>  		remote_flow->msg.t.tcm_handle = ISOLATE_HANDLE;
> +	else if (idx == TAP_REMOTE_PROMISC)
> +		remote_flow->msg.t.tcm_handle = REMOTE_PROMISCUOUS_HANDLE;
>  	else
>  		tap_flow_set_handle(remote_flow);
>  	if (priv_flow_process(pmd, attr, items, actions, NULL,
> @@ -1709,12 +1716,16 @@ int tap_flow_implicit_create(struct pmd_internals *pmd,
>  	}
>  	err = tap_nl_recv_ack(pmd->nlsk_fd);
>  	if (err < 0) {
> +		/* Silently ignore re-entering remote promiscuous rule */
> +		if (errno == EEXIST && idx == TAP_REMOTE_PROMISC)
> +			goto success;
>  		RTE_LOG(ERR, PMD,
>  			"Kernel refused TC filter rule creation (%d): %s\n",
>  			errno, strerror(errno));
>  		goto fail;
>  	}
>  	LIST_INSERT_HEAD(&pmd->implicit_flows, remote_flow, next);
Are we sure the previous rule is still in the registered implicit flows?
> +success:
>  	return 0;
>  fail:
>  	if (remote_flow)

Re: [dpdk-dev] [PATCH v2] net/tap: fix promiscuous rules double insersions

[Ophir Munk]

Please see inline.
I will send updated v3

> -----Original Message-----
> From: Pascal Mazon [mailto:pascal.mazon@6wind.com]
> Sent: Wednesday, February 14, 2018 10:51 AM
> To: Ophir Munk <ophirmu@mellanox.com>; dev@dpdk.org
> Cc: Thomas Monjalon <thomas@monjalon.net>; Olga Shern
> <olgas@mellanox.com>; stable@dpdk.org
> Subject: Re: [PATCH v2] net/tap: fix promiscuous rules double insersions
> 
> Hi Ophir,
> 
> Typo in title: s/insersions/insertions/
> 

Fixed in v3

> I'm ok on principle, I have just a few comments inline.
> 
> Regards,
> Pascal
> 
> On 13/02/2018 19:35, Ophir Munk wrote:
> > Running testpmd command "port stop all" followed by command "port
> > start all" may result in a TAP error:
> > PMD: Kernel refused TC filter rule creation (17): File exists
> >
> > Root cause analysis: during the execution of "port start all" command
> > testpmd calls rte_eth_promiscuous_enable() while during the execution
> > of "port stop all" command testpmd does not call
> > rte_eth_promiscuous_enable().
> Shouldn't it be rte_eth_promiscuous_disable()?

Yes it should. Fixed in v3

> > As a result the TAP PMD is trying to add tc (traffic control command)
> > promiscuous rules to the remote netvsc device consecutively. From the
> > kernel point of view it is seen as an attempt to add the same rule
> > more than once. In recent kernels (e.g. version 4.13) this attempt is
> > rejected with a "File exists" error. In less recent kernels (e.g.
> > version 4.4) the same rule may have been accepted twice successfully,
> which is undesirable.
> >
> > In the corrupted code every tc promiscuous rule included a different
> > handle number parameter. If instead an identical handle number
> > parameter is used for all tc promiscuous rules - all kernels will
> > reject the second rule with a "File exists" error, which is easy to
> > identify and to silently ignore.
> >
> > Fixes: 2bc06869cd94 ("net/tap: add remote netdevice traffic capture")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
> > ---
> > v2: add detailed commit message
> >
> >  drivers/net/tap/tap_flow.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/net/tap/tap_flow.c b/drivers/net/tap/tap_flow.c
> > index 65657f0..d1f4a52 100644
> > --- a/drivers/net/tap/tap_flow.c
> > +++ b/drivers/net/tap/tap_flow.c
> > @@ -123,6 +123,7 @@ enum key_status_e {  };
> >
> >  #define ISOLATE_HANDLE 1
> > +#define REMOTE_PROMISCUOUS_HANDLE 2
> >
> >  struct rte_flow {
> >  	LIST_ENTRY(rte_flow) next; /* Pointer to the next rte_flow structure
> > */ @@ -1692,9 +1693,15 @@ int tap_flow_implicit_create(struct
> pmd_internals *pmd,
> >  	 * The ISOLATE rule is always present and must have a static handle,
> as
> >  	 * the action is changed whether the feature is enabled (DROP) or
> >  	 * disabled (PASSTHRU).
> > +	 * There is just one REMOTE_PROMISCUOUS rule in all cases. It
> should
> > +	 * have a static handle such that adding it twice will fail with EEXIST
> > +	 * with any kernel version. Remark: old kernels may falsely accept the
> > +	 * same REMOTE_PREMISCUOUS rules if they had different handles.
> s/PREMISCUOUS/PROMISCUOUS/
> >  	 */
> >  	if (idx == TAP_ISOLATE)
> >  		remote_flow->msg.t.tcm_handle = ISOLATE_HANDLE;
> > +	else if (idx == TAP_REMOTE_PROMISC)
> > +		remote_flow->msg.t.tcm_handle =
> REMOTE_PROMISCUOUS_HANDLE;
> >  	else
> >  		tap_flow_set_handle(remote_flow);
> >  	if (priv_flow_process(pmd, attr, items, actions, NULL, @@ -1709,12
> > +1716,16 @@ int tap_flow_implicit_create(struct pmd_internals *pmd,
> >  	}
> >  	err = tap_nl_recv_ack(pmd->nlsk_fd);
> >  	if (err < 0) {
> > +		/* Silently ignore re-entering remote promiscuous rule */
> > +		if (errno == EEXIST && idx == TAP_REMOTE_PROMISC)
> > +			goto success;
> >  		RTE_LOG(ERR, PMD,
> >  			"Kernel refused TC filter rule creation (%d): %s\n",
> >  			errno, strerror(errno));
> >  		goto fail;
> >  	}
> >  	LIST_INSERT_HEAD(&pmd->implicit_flows, remote_flow, next);
> Are we sure the previous rule is still in the registered implicit flows?

I will run tests to verify that.

> > +success:
> >  	return 0;
> >  fail:
> >  	if (remote_flow)

[dpdk-dev] [PATCH v3] net/tap: fix promiscuous rules double insertions

[Ophir Munk]

Running testpmd command "port stop all" followed by command "port start
all" may result in a TAP error:
PMD: Kernel refused TC filter rule creation (17): File exists

Root cause analysis: during the execution of "port start all" command
testpmd calls rte_eth_promiscuous_enable() while during the execution
of "port stop all" command testpmd does not call
rte_eth_promiscuous_disable().
As a result the TAP PMD is trying to add tc (traffic control command)
promiscuous rules to the remote netvsc device consecutively. From the
kernel point of view it is seen as an attempt to add the same rule more
than once. In recent kernels (e.g. version 4.13) this attempt is rejected
with a "File exists" error. In less recent kernels (e.g. version 4.4) the
same rule may have been successfully accepted twice, which is undesirable.

In the corrupted code every tc promiscuous rule included a different
handle number parameter. If instead an identical handle number is
used for all tc promiscuous rules - all kernels will reject the second
identical rule with a "File exists" error, which is easy to identify and
to silently ignore.

Fixes: 2bc06869cd94 ("net/tap: add remote netdevice traffic capture")
Cc: stable@dpdk.org

Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
---
v1: initial version
v2: add detailed commit message
v3: textual fixes to commit message and code comments

 drivers/net/tap/tap_flow.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/net/tap/tap_flow.c b/drivers/net/tap/tap_flow.c
index 65657f0..551b2d8 100644
--- a/drivers/net/tap/tap_flow.c
+++ b/drivers/net/tap/tap_flow.c
@@ -123,6 +123,7 @@ enum key_status_e {
 };
 
 #define ISOLATE_HANDLE 1
+#define REMOTE_PROMISCUOUS_HANDLE 2
 
 struct rte_flow {
 	LIST_ENTRY(rte_flow) next; /* Pointer to the next rte_flow structure */
@@ -1692,9 +1693,15 @@ int tap_flow_implicit_create(struct pmd_internals *pmd,
 	 * The ISOLATE rule is always present and must have a static handle, as
 	 * the action is changed whether the feature is enabled (DROP) or
 	 * disabled (PASSTHRU).
+	 * There is just one REMOTE_PROMISCUOUS rule in all cases. It should
+	 * have a static handle such that adding it twice will fail with EEXIST
+	 * with any kernel version. Remark: old kernels may falsely accept the
+	 * same REMOTE_PROMISCUOUS rules if they had different handles.
 	 */
 	if (idx == TAP_ISOLATE)
 		remote_flow->msg.t.tcm_handle = ISOLATE_HANDLE;
+	else if (idx == TAP_REMOTE_PROMISC)
+		remote_flow->msg.t.tcm_handle = REMOTE_PROMISCUOUS_HANDLE;
 	else
 		tap_flow_set_handle(remote_flow);
 	if (priv_flow_process(pmd, attr, items, actions, NULL,
@@ -1709,12 +1716,16 @@ int tap_flow_implicit_create(struct pmd_internals *pmd,
 	}
 	err = tap_nl_recv_ack(pmd->nlsk_fd);
 	if (err < 0) {
+		/* Silently ignore re-entering remote promiscuous rule */
+		if (errno == EEXIST && idx == TAP_REMOTE_PROMISC)
+			goto success;
 		RTE_LOG(ERR, PMD,
 			"Kernel refused TC filter rule creation (%d): %s\n",
 			errno, strerror(errno));
 		goto fail;
 	}
 	LIST_INSERT_HEAD(&pmd->implicit_flows, remote_flow, next);
+success:
 	return 0;
 fail:
 	if (remote_flow)
-- 
2.7.4

Re: [dpdk-dev] [PATCH v3] net/tap: fix promiscuous rules double insertions

[Pascal Mazon]

Good job. Looks ok to me.

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

On 14/02/2018 12:32, Ophir Munk wrote:
> Running testpmd command "port stop all" followed by command "port start
> all" may result in a TAP error:
> PMD: Kernel refused TC filter rule creation (17): File exists
>
> Root cause analysis: during the execution of "port start all" command
> testpmd calls rte_eth_promiscuous_enable() while during the execution
> of "port stop all" command testpmd does not call
> rte_eth_promiscuous_disable().
> As a result the TAP PMD is trying to add tc (traffic control command)
> promiscuous rules to the remote netvsc device consecutively. From the
> kernel point of view it is seen as an attempt to add the same rule more
> than once. In recent kernels (e.g. version 4.13) this attempt is rejected
> with a "File exists" error. In less recent kernels (e.g. version 4.4) the
> same rule may have been successfully accepted twice, which is undesirable.
>
> In the corrupted code every tc promiscuous rule included a different
> handle number parameter. If instead an identical handle number is
> used for all tc promiscuous rules - all kernels will reject the second
> identical rule with a "File exists" error, which is easy to identify and
> to silently ignore.
>
> Fixes: 2bc06869cd94 ("net/tap: add remote netdevice traffic capture")
> Cc: stable@dpdk.org
>
> Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
> ---
> v1: initial version
> v2: add detailed commit message
> v3: textual fixes to commit message and code comments
>
>  drivers/net/tap/tap_flow.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/net/tap/tap_flow.c b/drivers/net/tap/tap_flow.c
> index 65657f0..551b2d8 100644
> --- a/drivers/net/tap/tap_flow.c
> +++ b/drivers/net/tap/tap_flow.c
> @@ -123,6 +123,7 @@ enum key_status_e {
>  };
>  
>  #define ISOLATE_HANDLE 1
> +#define REMOTE_PROMISCUOUS_HANDLE 2
>  
>  struct rte_flow {
>  	LIST_ENTRY(rte_flow) next; /* Pointer to the next rte_flow structure */
> @@ -1692,9 +1693,15 @@ int tap_flow_implicit_create(struct pmd_internals *pmd,
>  	 * The ISOLATE rule is always present and must have a static handle, as
>  	 * the action is changed whether the feature is enabled (DROP) or
>  	 * disabled (PASSTHRU).
> +	 * There is just one REMOTE_PROMISCUOUS rule in all cases. It should
> +	 * have a static handle such that adding it twice will fail with EEXIST
> +	 * with any kernel version. Remark: old kernels may falsely accept the
> +	 * same REMOTE_PROMISCUOUS rules if they had different handles.
>  	 */
>  	if (idx == TAP_ISOLATE)
>  		remote_flow->msg.t.tcm_handle = ISOLATE_HANDLE;
> +	else if (idx == TAP_REMOTE_PROMISC)
> +		remote_flow->msg.t.tcm_handle = REMOTE_PROMISCUOUS_HANDLE;
>  	else
>  		tap_flow_set_handle(remote_flow);
>  	if (priv_flow_process(pmd, attr, items, actions, NULL,
> @@ -1709,12 +1716,16 @@ int tap_flow_implicit_create(struct pmd_internals *pmd,
>  	}
>  	err = tap_nl_recv_ack(pmd->nlsk_fd);
>  	if (err < 0) {
> +		/* Silently ignore re-entering remote promiscuous rule */
> +		if (errno == EEXIST && idx == TAP_REMOTE_PROMISC)
> +			goto success;
>  		RTE_LOG(ERR, PMD,
>  			"Kernel refused TC filter rule creation (%d): %s\n",
>  			errno, strerror(errno));
>  		goto fail;
>  	}
>  	LIST_INSERT_HEAD(&pmd->implicit_flows, remote_flow, next);
> +success:
>  	return 0;
>  fail:
>  	if (remote_flow)

Re: [dpdk-dev] [PATCH v2] net/tap: fix promiscuous rules double insersions

[Ophir Munk]

Hi,
Regarding your question:
> Are we sure the previous rule is still in the registered implicit flows?

It is confirmed. 

After running several "port stop/start" commands in testpmd I am executing 
testpmd> flow isolate <port id> 1 
and notice that promiscuous rule is removed from remote device.

Regards,
Ophir

> -----Original Message-----
> From: Ophir Munk
> Sent: Wednesday, February 14, 2018 1:24 PM
> To: 'Pascal Mazon' <pascal.mazon@6wind.com>; dev@dpdk.org
> Cc: Thomas Monjalon <thomas@monjalon.net>; Olga Shern
> <olgas@mellanox.com>; stable@dpdk.org
> Subject: RE: [PATCH v2] net/tap: fix promiscuous rules double insersions
> 
> Please see inline.
> I will send updated v3
> 
> > -----Original Message-----
> > From: Pascal Mazon [mailto:pascal.mazon@6wind.com]
> > Sent: Wednesday, February 14, 2018 10:51 AM
> > To: Ophir Munk <ophirmu@mellanox.com>; dev@dpdk.org
> > Cc: Thomas Monjalon <thomas@monjalon.net>; Olga Shern
> > <olgas@mellanox.com>; stable@dpdk.org
> > Subject: Re: [PATCH v2] net/tap: fix promiscuous rules double
> > insersions
> >
> > Hi Ophir,
> >
> > Typo in title: s/insersions/insertions/
> >
> 
> Fixed in v3
> 
> > I'm ok on principle, I have just a few comments inline.
> >
> > Regards,
> > Pascal
> >
> > On 13/02/2018 19:35, Ophir Munk wrote:
> > > Running testpmd command "port stop all" followed by command "port
> > > start all" may result in a TAP error:
> > > PMD: Kernel refused TC filter rule creation (17): File exists
> > >
> > > Root cause analysis: during the execution of "port start all"
> > > command testpmd calls rte_eth_promiscuous_enable() while during the
> > > execution of "port stop all" command testpmd does not call
> > > rte_eth_promiscuous_enable().
> > Shouldn't it be rte_eth_promiscuous_disable()?
> 
> Yes it should. Fixed in v3
> 
> > > As a result the TAP PMD is trying to add tc (traffic control
> > > command) promiscuous rules to the remote netvsc device
> > > consecutively. From the kernel point of view it is seen as an
> > > attempt to add the same rule more than once. In recent kernels (e.g.
> > > version 4.13) this attempt is rejected with a "File exists" error. In less
> recent kernels (e.g.
> > > version 4.4) the same rule may have been accepted twice
> > > successfully,
> > which is undesirable.
> > >
> > > In the corrupted code every tc promiscuous rule included a different
> > > handle number parameter. If instead an identical handle number
> > > parameter is used for all tc promiscuous rules - all kernels will
> > > reject the second rule with a "File exists" error, which is easy to
> > > identify and to silently ignore.
> > >
> > > Fixes: 2bc06869cd94 ("net/tap: add remote netdevice traffic
> > > capture")
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
> > > ---
> > > v2: add detailed commit message
> > >
> > >  drivers/net/tap/tap_flow.c | 11 +++++++++++
> > >  1 file changed, 11 insertions(+)
> > >
> > > diff --git a/drivers/net/tap/tap_flow.c b/drivers/net/tap/tap_flow.c
> > > index 65657f0..d1f4a52 100644
> > > --- a/drivers/net/tap/tap_flow.c
> > > +++ b/drivers/net/tap/tap_flow.c
> > > @@ -123,6 +123,7 @@ enum key_status_e {  };
> > >
> > >  #define ISOLATE_HANDLE 1
> > > +#define REMOTE_PROMISCUOUS_HANDLE 2
> > >
> > >  struct rte_flow {
> > >  	LIST_ENTRY(rte_flow) next; /* Pointer to the next rte_flow
> > > structure */ @@ -1692,9 +1693,15 @@ int
> > > tap_flow_implicit_create(struct
> > pmd_internals *pmd,
> > >  	 * The ISOLATE rule is always present and must have a static
> > > handle,
> > as
> > >  	 * the action is changed whether the feature is enabled (DROP) or
> > >  	 * disabled (PASSTHRU).
> > > +	 * There is just one REMOTE_PROMISCUOUS rule in all cases. It
> > should
> > > +	 * have a static handle such that adding it twice will fail with EEXIST
> > > +	 * with any kernel version. Remark: old kernels may falsely accept the
> > > +	 * same REMOTE_PREMISCUOUS rules if they had different handles.
> > s/PREMISCUOUS/PROMISCUOUS/
> > >  	 */
> > >  	if (idx == TAP_ISOLATE)
> > >  		remote_flow->msg.t.tcm_handle = ISOLATE_HANDLE;
> > > +	else if (idx == TAP_REMOTE_PROMISC)
> > > +		remote_flow->msg.t.tcm_handle =
> > REMOTE_PROMISCUOUS_HANDLE;
> > >  	else
> > >  		tap_flow_set_handle(remote_flow);
> > >  	if (priv_flow_process(pmd, attr, items, actions, NULL, @@ -1709,12
> > > +1716,16 @@ int tap_flow_implicit_create(struct pmd_internals *pmd,
> > >  	}
> > >  	err = tap_nl_recv_ack(pmd->nlsk_fd);
> > >  	if (err < 0) {
> > > +		/* Silently ignore re-entering remote promiscuous rule */
> > > +		if (errno == EEXIST && idx == TAP_REMOTE_PROMISC)
> > > +			goto success;
> > >  		RTE_LOG(ERR, PMD,
> > >  			"Kernel refused TC filter rule creation (%d): %s\n",
> > >  			errno, strerror(errno));
> > >  		goto fail;
> > >  	}
> > >  	LIST_INSERT_HEAD(&pmd->implicit_flows, remote_flow, next);
> > Are we sure the previous rule is still in the registered implicit flows?
> 
> I will run tests to verify that.
> 
> > > +success:
> > >  	return 0;
> > >  fail:
> > >  	if (remote_flow)

Re: [dpdk-dev] [PATCH v3] net/tap: fix promiscuous rules double insertions

[Thomas Monjalon]

14/02/2018 14:13, Pascal Mazon:
> On 14/02/2018 12:32, Ophir Munk wrote:
> > Running testpmd command "port stop all" followed by command "port start
> > all" may result in a TAP error:
> > PMD: Kernel refused TC filter rule creation (17): File exists
> >
> > Root cause analysis: during the execution of "port start all" command
> > testpmd calls rte_eth_promiscuous_enable() while during the execution
> > of "port stop all" command testpmd does not call
> > rte_eth_promiscuous_disable().
> > As a result the TAP PMD is trying to add tc (traffic control command)
> > promiscuous rules to the remote netvsc device consecutively. From the
> > kernel point of view it is seen as an attempt to add the same rule more
> > than once. In recent kernels (e.g. version 4.13) this attempt is rejected
> > with a "File exists" error. In less recent kernels (e.g. version 4.4) the
> > same rule may have been successfully accepted twice, which is undesirable.
> >
> > In the corrupted code every tc promiscuous rule included a different
> > handle number parameter. If instead an identical handle number is
> > used for all tc promiscuous rules - all kernels will reject the second
> > identical rule with a "File exists" error, which is easy to identify and
> > to silently ignore.
> >
> > Fixes: 2bc06869cd94 ("net/tap: add remote netdevice traffic capture")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
> Acked-by: Pascal Mazon <pascal.mazon@6wind.com>

Applied, thanks