From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chas Williams <3chas3@gmail.com> Subject: Re: [PATCH 2/2] net/bonding: avoid the next active slave going out of bound Date: Sat, 9 Feb 2019 08:17:18 -0500 Message-ID: References: <20190110102235.1238-1-hyonkim@cisco.com> <20190110102235.1238-3-hyonkim@cisco.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: dev@dpdk.org, stable@dpdk.org To: Hyong Youb Kim , Ferruh Yigit , Declan Doherty , Chas Williams Return-path: In-Reply-To: <20190110102235.1238-3-hyonkim@cisco.com> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 1/10/19 5:22 AM, Hyong Youb Kim wrote: > For bonding modes like broadcast that use bond_ethdev_rx_burst(), it > is fairly easy to produce a crash simply by bringing a slave port's > link down. When slave links go down, the driver on one thread reduces > active_slave_count via the LSC callback and deactivate_slave(). At the > same time, bond_ethdev_rx_burst() running on a forwarding thread may > increment active_slave (next active slave) beyond > active_slave_count. Here is a typical sequence of events. > > At time 0: > active_slave_count = 3 > active_slave = 2 > > At time 1: > A slave link goes down. > Thread 0 (main) reduces active_slave_count to 2. > > At time 2: > Thread 1 (forwarding) executes bond_ethdev_rx_burst(). > - Reads active_slave_count = 2. > - Increments active_slave at the end to 3. > > From this point on, everytime bond_ethdev_rx_burst() runs, > active_slave increments by one, eventually going well out of bound of > the active_slaves array and causing a crash. > > Make the rx burst function to first check that active_slave is within > bound. If not, reset it to 0 to avoid out-of-range array access. > > Fixes: e1110e977648 ("net/bonding: fix Rx slave fairness") > Cc: stable@dpdk.org > > Signed-off-by: Hyong Youb Kim Acked-by: Chas Williams > --- > drivers/net/bonding/rte_eth_bond_pmd.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c > index daf2440cd..bc2405e54 100644 > --- a/drivers/net/bonding/rte_eth_bond_pmd.c > +++ b/drivers/net/bonding/rte_eth_bond_pmd.c > @@ -68,6 +68,15 @@ bond_ethdev_rx_burst(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts) > internals = bd_rx_q->dev_private; > slave_count = internals->active_slave_count; > active_slave = internals->active_slave; > + /* > + * Reset the active slave index, in case active_slave goes out > + * of bound. It can hapen when slave links go down, and > + * another thread (LSC callback) shrinks the slave count. > + */ > + if (active_slave >= slave_count) { > + internals->active_slave = 0; > + active_slave = 0; > + } > > for (i = 0; i < slave_count && nb_pkts; i++) { > uint16_t num_rx_slave; > @@ -273,6 +282,11 @@ bond_ethdev_rx_burst_8023ad_fast_queue(void *queue, struct rte_mbuf **bufs, > active_slave = internals->active_slave; > memcpy(slaves, internals->active_slaves, > sizeof(internals->active_slaves[0]) * slave_count); > + /* active_slave may go out of bound. See bond_ethdev_rx_burst() */ > + if (active_slave >= slave_count) { > + internals->active_slave = 0; > + active_slave = 0; > + } > > for (i = 0; i < slave_count && nb_pkts; i++) { > uint16_t num_rx_slave; >