diff mbox series

[1/3] net/enic: restrict several handlers to primary process

Message ID 20190906065020.21206-2-hyonkim@cisco.com (mailing list archive)
State Changes Requested, archived
Delegated to: Ferruh Yigit
Headers
Series net/enic: fix multi-process isseus |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/iol-dpdk_compile_ovs success Compile Testing PASS
ci/iol-dpdk_compile success Compile Testing PASS
ci/iol-dpdk_compile_spdk success Compile Testing PASS
ci/intel-Performance success Performance Testing PASS
ci/mellanox-Performance success Performance Testing PASS

Commit Message

Hyong Youb Kim (hyonkim) Sept. 6, 2019, 6:50 a.m. UTC 
  These eth_dev_ops handlers should run only in the primary process.
- filter_ctrl
- reta_update
- rss_hash_update
- set_mc_addr_list
- udp_tunnel_port_add
- udp_tunnel_port_del

Fixes: c2fec27b5cb0 ("net/enic: allow to change RSS settings")
Fixes: 8d496995346c ("net/enic: support multicast filtering")
Fixes: 8a4efd17410c ("net/enic: add handlers to add/delete vxlan port number")
Cc: stable@dpdk.org

Reported-by: Dirk-Holger Lenz <dirk.lenz@ng4t.com>
Signed-off-by: Hyong Youb Kim <hyonkim@cisco.com>
Tested-by: Dirk-Holger Lenz <dirk.lenz@ng4t.com>
Reviewed-by: John Daley <johndale@cisco.com>
---
 drivers/net/enic/enic_ethdev.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Ferruh Yigit Oct. 9, 2019, 8:01 a.m. UTC | #1 
On 9/6/2019 7:50 AM, Hyong Youb Kim wrote:
> These eth_dev_ops handlers should run only in the primary process.
> - filter_ctrl
> - reta_update
> - rss_hash_update
> - set_mc_addr_list
> - udp_tunnel_port_add
> - udp_tunnel_port_del
> 
> Fixes: c2fec27b5cb0 ("net/enic: allow to change RSS settings")
> Fixes: 8d496995346c ("net/enic: support multicast filtering")
> Fixes: 8a4efd17410c ("net/enic: add handlers to add/delete vxlan port number")
> Cc: stable@dpdk.org
> 
> Reported-by: Dirk-Holger Lenz <dirk.lenz@ng4t.com>
> Signed-off-by: Hyong Youb Kim <hyonkim@cisco.com>
> Tested-by: Dirk-Holger Lenz <dirk.lenz@ng4t.com>
> Reviewed-by: John Daley <johndale@cisco.com>
> ---
>  drivers/net/enic/enic_ethdev.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/net/enic/enic_ethdev.c b/drivers/net/enic/enic_ethdev.c
> index 06dc67122..85d785e62 100644
> --- a/drivers/net/enic/enic_ethdev.c
> +++ b/drivers/net/enic/enic_ethdev.c
> @@ -129,6 +129,8 @@ enicpmd_dev_filter_ctrl(struct rte_eth_dev *dev,
>  {
>  	int ret = 0;
>  
> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> +		return -E_RTE_SECONDARY;
>  	ENICPMD_FUNC_TRACE();
>  
>  	switch (filter_type) {

I remember we have similar talk with John in the past about these secondary
application checks in ethdev_ops.

I would like to understand why these checks required only in enic, can you
please describe your use case?
Is there any reason secondary application can't change configuration of the
device, or are you updating your driver to work with specific application?
Hyong Youb Kim (hyonkim) Oct. 9, 2019, 8:48 a.m. UTC | #2 
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Wednesday, October 9, 2019 5:02 PM
> To: Hyong Youb Kim (hyonkim) <hyonkim@cisco.com>
> Cc: dev@dpdk.org; John Daley (johndale) <johndale@cisco.com>; Dirk-
> Holger Lenz <dirk.lenz@ng4t.com>; stable@dpdk.org
> Subject: Re: [PATCH 1/3] net/enic: restrict several handlers to primary
> process
> 
> On 9/6/2019 7:50 AM, Hyong Youb Kim wrote:
> > These eth_dev_ops handlers should run only in the primary process.
> > - filter_ctrl
> > - reta_update
> > - rss_hash_update
> > - set_mc_addr_list
> > - udp_tunnel_port_add
> > - udp_tunnel_port_del
> >
> > Fixes: c2fec27b5cb0 ("net/enic: allow to change RSS settings")
> > Fixes: 8d496995346c ("net/enic: support multicast filtering")
> > Fixes: 8a4efd17410c ("net/enic: add handlers to add/delete vxlan port
> number")
> > Cc: stable@dpdk.org
> >
> > Reported-by: Dirk-Holger Lenz <dirk.lenz@ng4t.com>
> > Signed-off-by: Hyong Youb Kim <hyonkim@cisco.com>
> > Tested-by: Dirk-Holger Lenz <dirk.lenz@ng4t.com>
> > Reviewed-by: John Daley <johndale@cisco.com>
> > ---
> >  drivers/net/enic/enic_ethdev.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/drivers/net/enic/enic_ethdev.c
> b/drivers/net/enic/enic_ethdev.c
> > index 06dc67122..85d785e62 100644
> > --- a/drivers/net/enic/enic_ethdev.c
> > +++ b/drivers/net/enic/enic_ethdev.c
> > @@ -129,6 +129,8 @@ enicpmd_dev_filter_ctrl(struct rte_eth_dev *dev,
> >  {
> >  	int ret = 0;
> >
> > +	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> > +		return -E_RTE_SECONDARY;
> >  	ENICPMD_FUNC_TRACE();
> >
> >  	switch (filter_type) {
> 
> I remember we have similar talk with John in the past about these secondary
> application checks in ethdev_ops.
> 
> I would like to understand why these checks required only in enic, can you
> please describe your use case?
> Is there any reason secondary application can't change configuration of the
> device, or are you updating your driver to work with specific application?

Hi,

No fundamental reasons why secondary processes cannot run these
handlers. These checks are to make it clear that it is not safe to do
so at the moment. It is a software limitation.

The firmware API (devcmd) we use to configure NIC settings assumes one
user executing one command at a time. And, many of the handlers in the
driver also assume primary process. The firmware itself has
enough checks to prevent concurrent devcmd attempts from corrupting
its internal state. But, host processes can get confused. For example,
process A gets process B's results, or overwrites B's devcmd, etc.

I believe these issues are all fixable in the driver. We could use
locks in shared memory to serialize devcmd (though risky), fix
handlers that assume primary process, and so on. It is a to-do item for
this driver and would require its own patch series (e.g. allow
secondary processes to run X, Y, Z safely)..

Thanks.
-Hyong
Ferruh Yigit Oct. 9, 2019, 9:28 a.m. UTC | #3 
On 10/9/2019 9:48 AM, Hyong Youb Kim (hyonkim) wrote:
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> Sent: Wednesday, October 9, 2019 5:02 PM
>> To: Hyong Youb Kim (hyonkim) <hyonkim@cisco.com>
>> Cc: dev@dpdk.org; John Daley (johndale) <johndale@cisco.com>; Dirk-
>> Holger Lenz <dirk.lenz@ng4t.com>; stable@dpdk.org
>> Subject: Re: [PATCH 1/3] net/enic: restrict several handlers to primary
>> process
>>
>> On 9/6/2019 7:50 AM, Hyong Youb Kim wrote:
>>> These eth_dev_ops handlers should run only in the primary process.
>>> - filter_ctrl
>>> - reta_update
>>> - rss_hash_update
>>> - set_mc_addr_list
>>> - udp_tunnel_port_add
>>> - udp_tunnel_port_del
>>>
>>> Fixes: c2fec27b5cb0 ("net/enic: allow to change RSS settings")
>>> Fixes: 8d496995346c ("net/enic: support multicast filtering")
>>> Fixes: 8a4efd17410c ("net/enic: add handlers to add/delete vxlan port
>> number")
>>> Cc: stable@dpdk.org
>>>
>>> Reported-by: Dirk-Holger Lenz <dirk.lenz@ng4t.com>
>>> Signed-off-by: Hyong Youb Kim <hyonkim@cisco.com>
>>> Tested-by: Dirk-Holger Lenz <dirk.lenz@ng4t.com>
>>> Reviewed-by: John Daley <johndale@cisco.com>
>>> ---
>>>  drivers/net/enic/enic_ethdev.c | 12 ++++++++++++
>>>  1 file changed, 12 insertions(+)
>>>
>>> diff --git a/drivers/net/enic/enic_ethdev.c
>> b/drivers/net/enic/enic_ethdev.c
>>> index 06dc67122..85d785e62 100644
>>> --- a/drivers/net/enic/enic_ethdev.c
>>> +++ b/drivers/net/enic/enic_ethdev.c
>>> @@ -129,6 +129,8 @@ enicpmd_dev_filter_ctrl(struct rte_eth_dev *dev,
>>>  {
>>>  	int ret = 0;
>>>
>>> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
>>> +		return -E_RTE_SECONDARY;
>>>  	ENICPMD_FUNC_TRACE();
>>>
>>>  	switch (filter_type) {
>>
>> I remember we have similar talk with John in the past about these secondary
>> application checks in ethdev_ops.
>>
>> I would like to understand why these checks required only in enic, can you
>> please describe your use case?
>> Is there any reason secondary application can't change configuration of the
>> device, or are you updating your driver to work with specific application?
> 
> Hi,
> 
> No fundamental reasons why secondary processes cannot run these
> handlers. These checks are to make it clear that it is not safe to do
> so at the moment. It is a software limitation.
> 
> The firmware API (devcmd) we use to configure NIC settings assumes one
> user executing one command at a time. And, many of the handlers in the
> driver also assume primary process. The firmware itself has
> enough checks to prevent concurrent devcmd attempts from corrupting
> its internal state. But, host processes can get confused. For example,
> process A gets process B's results, or overwrites B's devcmd, etc.
> 
> I believe these issues are all fixable in the driver. We could use
> locks in shared memory to serialize devcmd (though risky), fix
> handlers that assume primary process, and so on. It is a to-do item for
> this driver and would require its own patch series (e.g. allow
> secondary processes to run X, Y, Z safely)..

What you have described is valid concern for all drivers, that synchronization
has been pushed to the application level.

I don't see the point of just putting protection to only one driver.

And as a alternative, what do you think about having a check in the prob for the
secondary process and assign a subset of the ethdev_ops in that case? This makes
 more clear what is supported in the secondary process, and it prevents putting
secondary process checks everywhere.
Hyong Youb Kim (hyonkim) Oct. 9, 2019, 9:38 a.m. UTC | #4 
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Wednesday, October 9, 2019 6:28 PM
> To: Hyong Youb Kim (hyonkim) <hyonkim@cisco.com>
> Cc: dev@dpdk.org; John Daley (johndale) <johndale@cisco.com>; Dirk-
> Holger Lenz <dirk.lenz@ng4t.com>; stable@dpdk.org
> Subject: Re: [dpdk-stable] [PATCH 1/3] net/enic: restrict several handlers to
> primary process
> 
> On 10/9/2019 9:48 AM, Hyong Youb Kim (hyonkim) wrote:
> >> -----Original Message-----
> >> From: Ferruh Yigit <ferruh.yigit@intel.com>
> >> Sent: Wednesday, October 9, 2019 5:02 PM
> >> To: Hyong Youb Kim (hyonkim) <hyonkim@cisco.com>
> >> Cc: dev@dpdk.org; John Daley (johndale) <johndale@cisco.com>; Dirk-
> >> Holger Lenz <dirk.lenz@ng4t.com>; stable@dpdk.org
> >> Subject: Re: [PATCH 1/3] net/enic: restrict several handlers to primary
> >> process
> >>
> >> On 9/6/2019 7:50 AM, Hyong Youb Kim wrote:
> >>> These eth_dev_ops handlers should run only in the primary process.
> >>> - filter_ctrl
> >>> - reta_update
> >>> - rss_hash_update
> >>> - set_mc_addr_list
> >>> - udp_tunnel_port_add
> >>> - udp_tunnel_port_del
> >>>
> >>> Fixes: c2fec27b5cb0 ("net/enic: allow to change RSS settings")
> >>> Fixes: 8d496995346c ("net/enic: support multicast filtering")
> >>> Fixes: 8a4efd17410c ("net/enic: add handlers to add/delete vxlan port
> >> number")
> >>> Cc: stable@dpdk.org
> >>>
> >>> Reported-by: Dirk-Holger Lenz <dirk.lenz@ng4t.com>
> >>> Signed-off-by: Hyong Youb Kim <hyonkim@cisco.com>
> >>> Tested-by: Dirk-Holger Lenz <dirk.lenz@ng4t.com>
> >>> Reviewed-by: John Daley <johndale@cisco.com>
> >>> ---
> >>>  drivers/net/enic/enic_ethdev.c | 12 ++++++++++++
> >>>  1 file changed, 12 insertions(+)
> >>>
> >>> diff --git a/drivers/net/enic/enic_ethdev.c
> >> b/drivers/net/enic/enic_ethdev.c
> >>> index 06dc67122..85d785e62 100644
> >>> --- a/drivers/net/enic/enic_ethdev.c
> >>> +++ b/drivers/net/enic/enic_ethdev.c
> >>> @@ -129,6 +129,8 @@ enicpmd_dev_filter_ctrl(struct rte_eth_dev
> *dev,
> >>>  {
> >>>  	int ret = 0;
> >>>
> >>> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> >>> +		return -E_RTE_SECONDARY;
> >>>  	ENICPMD_FUNC_TRACE();
> >>>
> >>>  	switch (filter_type) {
> >>
> >> I remember we have similar talk with John in the past about these
> secondary
> >> application checks in ethdev_ops.
> >>
> >> I would like to understand why these checks required only in enic, can you
> >> please describe your use case?
> >> Is there any reason secondary application can't change configuration of
> the
> >> device, or are you updating your driver to work with specific application?
> >
> > Hi,
> >
> > No fundamental reasons why secondary processes cannot run these
> > handlers. These checks are to make it clear that it is not safe to do
> > so at the moment. It is a software limitation.
> >
> > The firmware API (devcmd) we use to configure NIC settings assumes one
> > user executing one command at a time. And, many of the handlers in the
> > driver also assume primary process. The firmware itself has
> > enough checks to prevent concurrent devcmd attempts from corrupting
> > its internal state. But, host processes can get confused. For example,
> > process A gets process B's results, or overwrites B's devcmd, etc.
> >
> > I believe these issues are all fixable in the driver. We could use
> > locks in shared memory to serialize devcmd (though risky), fix
> > handlers that assume primary process, and so on. It is a to-do item for
> > this driver and would require its own patch series (e.g. allow
> > secondary processes to run X, Y, Z safely)..
> 
> What you have described is valid concern for all drivers, that synchronization
> has been pushed to the application level.
> 
> I don't see the point of just putting protection to only one driver.
> 
> And as a alternative, what do you think about having a check in the prob for
> the
> secondary process and assign a subset of the ethdev_ops in that case? This
> makes
>  more clear what is supported in the secondary process, and it prevents
> putting
> secondary process checks everywhere.

Hi,

Okay, that sounds reasonable. Could you drop this one patch and apply
the rest in the series? I may not have time to properly re-do this one
in this cycle..

Thanks!
-Hyong
Ferruh Yigit Oct. 9, 2019, 5:17 p.m. UTC | #5 
On 10/9/2019 10:38 AM, Hyong Youb Kim (hyonkim) wrote:
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> Sent: Wednesday, October 9, 2019 6:28 PM
>> To: Hyong Youb Kim (hyonkim) <hyonkim@cisco.com>
>> Cc: dev@dpdk.org; John Daley (johndale) <johndale@cisco.com>; Dirk-
>> Holger Lenz <dirk.lenz@ng4t.com>; stable@dpdk.org
>> Subject: Re: [dpdk-stable] [PATCH 1/3] net/enic: restrict several handlers to
>> primary process
>>
>> On 10/9/2019 9:48 AM, Hyong Youb Kim (hyonkim) wrote:
>>>> -----Original Message-----
>>>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>>>> Sent: Wednesday, October 9, 2019 5:02 PM
>>>> To: Hyong Youb Kim (hyonkim) <hyonkim@cisco.com>
>>>> Cc: dev@dpdk.org; John Daley (johndale) <johndale@cisco.com>; Dirk-
>>>> Holger Lenz <dirk.lenz@ng4t.com>; stable@dpdk.org
>>>> Subject: Re: [PATCH 1/3] net/enic: restrict several handlers to primary
>>>> process
>>>>
>>>> On 9/6/2019 7:50 AM, Hyong Youb Kim wrote:
>>>>> These eth_dev_ops handlers should run only in the primary process.
>>>>> - filter_ctrl
>>>>> - reta_update
>>>>> - rss_hash_update
>>>>> - set_mc_addr_list
>>>>> - udp_tunnel_port_add
>>>>> - udp_tunnel_port_del
>>>>>
>>>>> Fixes: c2fec27b5cb0 ("net/enic: allow to change RSS settings")
>>>>> Fixes: 8d496995346c ("net/enic: support multicast filtering")
>>>>> Fixes: 8a4efd17410c ("net/enic: add handlers to add/delete vxlan port
>>>> number")
>>>>> Cc: stable@dpdk.org
>>>>>
>>>>> Reported-by: Dirk-Holger Lenz <dirk.lenz@ng4t.com>
>>>>> Signed-off-by: Hyong Youb Kim <hyonkim@cisco.com>
>>>>> Tested-by: Dirk-Holger Lenz <dirk.lenz@ng4t.com>
>>>>> Reviewed-by: John Daley <johndale@cisco.com>
>>>>> ---
>>>>>  drivers/net/enic/enic_ethdev.c | 12 ++++++++++++
>>>>>  1 file changed, 12 insertions(+)
>>>>>
>>>>> diff --git a/drivers/net/enic/enic_ethdev.c
>>>> b/drivers/net/enic/enic_ethdev.c
>>>>> index 06dc67122..85d785e62 100644
>>>>> --- a/drivers/net/enic/enic_ethdev.c
>>>>> +++ b/drivers/net/enic/enic_ethdev.c
>>>>> @@ -129,6 +129,8 @@ enicpmd_dev_filter_ctrl(struct rte_eth_dev
>> *dev,
>>>>>  {
>>>>>  	int ret = 0;
>>>>>
>>>>> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
>>>>> +		return -E_RTE_SECONDARY;
>>>>>  	ENICPMD_FUNC_TRACE();
>>>>>
>>>>>  	switch (filter_type) {
>>>>
>>>> I remember we have similar talk with John in the past about these
>> secondary
>>>> application checks in ethdev_ops.
>>>>
>>>> I would like to understand why these checks required only in enic, can you
>>>> please describe your use case?
>>>> Is there any reason secondary application can't change configuration of
>> the
>>>> device, or are you updating your driver to work with specific application?
>>>
>>> Hi,
>>>
>>> No fundamental reasons why secondary processes cannot run these
>>> handlers. These checks are to make it clear that it is not safe to do
>>> so at the moment. It is a software limitation.
>>>
>>> The firmware API (devcmd) we use to configure NIC settings assumes one
>>> user executing one command at a time. And, many of the handlers in the
>>> driver also assume primary process. The firmware itself has
>>> enough checks to prevent concurrent devcmd attempts from corrupting
>>> its internal state. But, host processes can get confused. For example,
>>> process A gets process B's results, or overwrites B's devcmd, etc.
>>>
>>> I believe these issues are all fixable in the driver. We could use
>>> locks in shared memory to serialize devcmd (though risky), fix
>>> handlers that assume primary process, and so on. It is a to-do item for
>>> this driver and would require its own patch series (e.g. allow
>>> secondary processes to run X, Y, Z safely)..
>>
>> What you have described is valid concern for all drivers, that synchronization
>> has been pushed to the application level.
>>
>> I don't see the point of just putting protection to only one driver.
>>
>> And as a alternative, what do you think about having a check in the prob for
>> the
>> secondary process and assign a subset of the ethdev_ops in that case? This
>> makes
>>  more clear what is supported in the secondary process, and it prevents
>> putting
>> secondary process checks everywhere.
> 
> Hi,
> 
> Okay, that sounds reasonable. Could you drop this one patch and apply
> the rest in the series? I may not have time to properly re-do this one
> in this cycle..

OK, will mark 1/3 as "Change Requested" and continue with others. Thanks.
diff mbox series

Patch

diff --git a/drivers/net/enic/enic_ethdev.c b/drivers/net/enic/enic_ethdev.c
index 06dc67122..85d785e62 100644
--- a/drivers/net/enic/enic_ethdev.c
+++ b/drivers/net/enic/enic_ethdev.c
@@ -129,6 +129,8 @@  enicpmd_dev_filter_ctrl(struct rte_eth_dev *dev,
 {
 	int ret = 0;
 
+	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+		return -E_RTE_SECONDARY;
 	ENICPMD_FUNC_TRACE();
 
 	switch (filter_type) {
@@ -710,6 +712,8 @@  static int enicpmd_set_mc_addr_list(struct rte_eth_dev *eth_dev,
 	uint32_t i, j;
 	int ret;
 
+	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+		return -E_RTE_SECONDARY;
 	ENICPMD_FUNC_TRACE();
 
 	/* Validate the given addresses first */
@@ -826,6 +830,8 @@  static int enicpmd_dev_rss_reta_update(struct rte_eth_dev *dev,
 	union vnic_rss_cpu rss_cpu;
 	uint16_t i, idx, shift;
 
+	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+		return -E_RTE_SECONDARY;
 	ENICPMD_FUNC_TRACE();
 	if (reta_size != ENIC_RSS_RETA_SIZE) {
 		dev_err(enic, "reta_update: wrong reta_size. given=%u"
@@ -854,6 +860,8 @@  static int enicpmd_dev_rss_hash_update(struct rte_eth_dev *dev,
 {
 	struct enic *enic = pmd_priv(dev);
 
+	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+		return -E_RTE_SECONDARY;
 	ENICPMD_FUNC_TRACE();
 	return enic_set_rss_conf(enic, rss_conf);
 }
@@ -986,6 +994,8 @@  static int enicpmd_dev_udp_tunnel_port_add(struct rte_eth_dev *eth_dev,
 	struct enic *enic = pmd_priv(eth_dev);
 	int ret;
 
+	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+		return -E_RTE_SECONDARY;
 	ENICPMD_FUNC_TRACE();
 	ret = udp_tunnel_common_check(enic, tnl);
 	if (ret)
@@ -1008,6 +1018,8 @@  static int enicpmd_dev_udp_tunnel_port_del(struct rte_eth_dev *eth_dev,
 	struct enic *enic = pmd_priv(eth_dev);
 	int ret;
 
+	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+		return -E_RTE_SECONDARY;
 	ENICPMD_FUNC_TRACE();
 	ret = udp_tunnel_common_check(enic, tnl);
 	if (ret)