diff mbox series

net/bonding: fix crash when stopping mode 4 port

Message ID 1541690802-16121-1-git-send-email-radu.nicolau@intel.com (mailing list archive)
State Accepted, archived
Delegated to: Ferruh Yigit
Headers
Series net/bonding: fix crash when stopping mode 4 port |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/mellanox-Performance-Testing success Performance Testing PASS
ci/intel-Performance-Testing success Performance Testing PASS

Commit Message

Radu Nicolau Nov. 8, 2018, 3:26 p.m. UTC 
  When stopping a bonded port all slaves are deactivated. Attempting
to deactivate a slave that was never activated will result in a segfault
when mode 4 is used.

Fixes: 7486331308f6 ("net/bonding: stop and deactivate slaves on stop")
Cc: stable@dpdk.org

Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
---
 drivers/net/bonding/rte_eth_bond_pmd.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

Comments

Ferruh Yigit Nov. 9, 2018, 8:49 p.m. UTC | #1 
On 11/8/2018 3:26 PM, Radu Nicolau wrote:
> When stopping a bonded port all slaves are deactivated. Attempting
> to deactivate a slave that was never activated will result in a segfault
> when mode 4 is used.
> 
> Fixes: 7486331308f6 ("net/bonding: stop and deactivate slaves on stop")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
> ---
>  drivers/net/bonding/rte_eth_bond_pmd.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
> index 1a6d8e4..2661620 100644
> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> @@ -2181,9 +2181,14 @@ bond_ethdev_stop(struct rte_eth_dev *eth_dev)
>  
>  	internals->link_status_polling_enabled = 0;
>  	for (i = 0; i < internals->slave_count; i++) {
> -		internals->slaves[i].last_link_status = 0;
> -		rte_eth_dev_stop(internals->slaves[i].port_id);
> -		deactivate_slave(eth_dev, internals->slaves[i].port_id);
> +		uint16_t slave_id = internals->slaves[i].port_id;
> +		if (find_slave_by_id(internals->active_slaves,
> +				internals->active_slave_count, slave_id) !=
> +						internals->active_slave_count) {
> +			internals->slaves[i].last_link_status = 0;
> +			rte_eth_dev_stop(slave_id);
> +			deactivate_slave(eth_dev, slave_id);
> +		}
>  	}
>  }

Hi Chas, Declan,

Any comment on patch? I think we should have this for rc3.
Chas Williams Nov. 9, 2018, 9:40 p.m. UTC | #2 
On 11/08/2018 10:26 AM, Radu Nicolau wrote:
> When stopping a bonded port all slaves are deactivated. Attempting
> to deactivate a slave that was never activated will result in a segfault
> when mode 4 is used.
> 
> Fixes: 7486331308f6 ("net/bonding: stop and deactivate slaves on stop")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>

Acked-by: Chas Williams <chas3@att.com>

> ---
>   drivers/net/bonding/rte_eth_bond_pmd.c | 11 ++++++++---
>   1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
> index 1a6d8e4..2661620 100644
> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> @@ -2181,9 +2181,14 @@ bond_ethdev_stop(struct rte_eth_dev *eth_dev)
>   
>   	internals->link_status_polling_enabled = 0;
>   	for (i = 0; i < internals->slave_count; i++) {
> -		internals->slaves[i].last_link_status = 0;
> -		rte_eth_dev_stop(internals->slaves[i].port_id);
> -		deactivate_slave(eth_dev, internals->slaves[i].port_id);
> +		uint16_t slave_id = internals->slaves[i].port_id;
> +		if (find_slave_by_id(internals->active_slaves,
> +				internals->active_slave_count, slave_id) !=
> +						internals->active_slave_count) {
> +			internals->slaves[i].last_link_status = 0;
> +			rte_eth_dev_stop(slave_id);
> +			deactivate_slave(eth_dev, slave_id);

Just some commentary. I believe that when this is stopping the bonding
driver is still watching for link status changes. You can't know when
the LSC thread was scheduled to run so something could still activate
a slave after this has deactivated the slaves.

> +		}
>   	}
>   }
>   
>
Ferruh Yigit Nov. 9, 2018, 10:18 p.m. UTC | #3 
On 11/9/2018 9:40 PM, Chas Williams wrote:
> 
> 
> On 11/08/2018 10:26 AM, Radu Nicolau wrote:
>> When stopping a bonded port all slaves are deactivated. Attempting
>> to deactivate a slave that was never activated will result in a segfault
>> when mode 4 is used.
>>
>> Fixes: 7486331308f6 ("net/bonding: stop and deactivate slaves on stop")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
> 
> Acked-by: Chas Williams <chas3@att.com>

Applied to dpdk-next-net/master, thanks.
diff mbox series

Patch

diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index 1a6d8e4..2661620 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -2181,9 +2181,14 @@  bond_ethdev_stop(struct rte_eth_dev *eth_dev)
 
 	internals->link_status_polling_enabled = 0;
 	for (i = 0; i < internals->slave_count; i++) {
-		internals->slaves[i].last_link_status = 0;
-		rte_eth_dev_stop(internals->slaves[i].port_id);
-		deactivate_slave(eth_dev, internals->slaves[i].port_id);
+		uint16_t slave_id = internals->slaves[i].port_id;
+		if (find_slave_by_id(internals->active_slaves,
+				internals->active_slave_count, slave_id) !=
+						internals->active_slave_count) {
+			internals->slaves[i].last_link_status = 0;
+			rte_eth_dev_stop(slave_id);
+			deactivate_slave(eth_dev, slave_id);
+		}
 	}
 }