From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <konstantin.ananyev@intel.com>
Received: from mga04.intel.com (mga04.intel.com [192.55.52.120])
 by dpdk.org (Postfix) with ESMTP id D052C5942;
 Mon, 15 Apr 2019 14:21:12 +0200 (CEST)
X-Amp-Result: SKIPPED(no attachment in message)
X-Amp-File-Uploaded: False
Received: from fmsmga008.fm.intel.com ([10.253.24.58])
 by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384;
 15 Apr 2019 05:21:10 -0700
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.60,353,1549958400"; d="scan'208";a="140800632"
Received: from irsmsx101.ger.corp.intel.com ([163.33.3.153])
 by fmsmga008.fm.intel.com with ESMTP; 15 Apr 2019 05:21:10 -0700
Received: from irsmsx105.ger.corp.intel.com ([169.254.7.31]) by
 IRSMSX101.ger.corp.intel.com ([169.254.1.115]) with mapi id 14.03.0415.000;
 Mon, 15 Apr 2019 13:21:09 +0100
From: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>
To: wangyunjian <wangyunjian@huawei.com>, "dev@dpdk.org" <dev@dpdk.org>
CC: "xudingke@huawei.com" <xudingke@huawei.com>, "stable@dpdk.org"
 <stable@dpdk.org>
Thread-Topic: [dpdk-dev] [PATCH] net/i40e: fix crash when calling
 i40e_vsi_delete_mac
Thread-Index: AQHU8d8IjuCfzE+MZEaWqavVP6qkOaY9JZow
Date: Mon, 15 Apr 2019 12:21:08 +0000
Message-ID: <2601191342CEEE43887BDE71AB9772580148A97E3D@irsmsx105.ger.corp.intel.com>
References: <1555149291-12432-1-git-send-email-wangyunjian@huawei.com>
In-Reply-To: <1555149291-12432-1-git-send-email-wangyunjian@huawei.com>
Accept-Language: en-IE, en-US
Content-Language: en-US
X-MS-Has-Attach: 
X-MS-TNEF-Correlator: 
x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiZTg3MjU4ZjEtNGU1Yy00ZjZiLWIxYmEtNjczZTFkNzYyOWZkIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiMXliU0JUeWZ1bXpHdWczRWoyWnc2ZHhsTVg2YjNDYXpYYWlveXRDbjBMUnV0YTZuQ0J4V3ppXC9MWFFDbkxZaE4ifQ==
x-ctpclassification: CTP_NT
dlp-product: dlpe-windows
dlp-version: 11.0.600.7
dlp-reaction: no-action
x-originating-ip: [163.33.239.182]
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Subject: Re: [dpdk-dev] [PATCH] net/i40e: fix crash when
	calling	i40e_vsi_delete_mac
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Mon, 15 Apr 2019 12:21:13 -0000

Hi,

> Now the macvlan filter list may be accessed in the same time by two
> different threads and may cause a lot of optional errors. This patch
> protects the macvlan filter access with a spinlock.
>=20
> Call Trace:
>   #1  0x00007ffb4cbe2e3c in i40e_vsi_delete_mac (vsi=3Dvsi@entry=3D
>       0x400052804b40, addr=3Daddr@entry=3D0x7ffb47672244) at /usr/src/
>       debug/dpdk-18.11/drivers/net/i40e/i40e_ethdev.c:7266
>   #2  0x00007ffb4cbe342b in i40e_set_default_mac_addr (dev=3D<optimized o=
ut>,
>       mac_addr=3D0x400052a6618d) at /usr/src/debug/dpdk-18.11/drivers/net=
/
> 	  i40e/i40e_ethdev.c:11893
>   #3  0x00007ffb4f569d4a in rte_eth_dev_default_mac_addr_set (port_id=3D
>       <optimized out>, addr=3Daddr@entry=3D0x400052a6618d) at /usr/src/de=
bug/
> 	  dpdk-18.11/lib/librte_ethdev/rte_ethdev.c:3366
>   #4  0x00007ffb4d0bb403 in mac_address_slaves_update (bonded_eth_dev=3D
>       bonded_eth_dev@entry=3D0xacf8c0 <rte_eth_devices>) at /usr/src/debu=
g/
> 	  dpdk-18.11/drivers/net/bonding/rte_eth_bond_pmd.c:1854
>   #5  0x00007ffb4d0bd221 in bond_ethdev_lsc_event_callback (port_id=3D
>       <optimized out>, type=3D<optimized out>, param=3D<optimized out>,
>       ret_param=3D <optimized out>) at /usr/src/debug/dpdk-18.11/drivers/
>       net/bonding/rte_eth_bond_pmd.c:3076
>   #6  0x00007ffb4f56aa09 in _rte_eth_dev_callback_process (dev=3Ddev@entr=
y=3D
>       0xad3940 <rte_eth_devices+16512>, event=3Devent@entry=3D
>       RTE_ETH_EVENT_INTR_LSC, ret_param=3Dret_param@entry=3D0x0)
>       at /usr/src/debug/dpdk-18.11/lib/librte_ethdev/rte_ethdev.c:3699
>   #7  0x00007ffb4cbb99f1 in i40e_dev_handle_aq_msg (dev=3Ddev@entry=3D0xa=
d3940
>       <rte_eth_devices+16512>) at /usr/src/debug/dpdk-18.11/drivers/net/
>       i40e/i40e_ethdev.c:6573
>   #8  0x00007ffb4cbdfbed in i40e_dev_alarm_handler (param=3D0xad3940
>       <rte_eth_devices+16512>) at /usr/src/debug/dpdk-18.11/drivers/net/
>       i40e/i40e_ethdev.c:6681
>   #9  0x00007ffb4fb9766f in eal_alarm_callback (arg=3D<optimized out>) at
>       /usr/src/debug/dpdk-18.11/lib/librte_eal/linuxapp/eal/eal_alarm.c:9=
0
>   #10 0x00007ffb4fb95dd2 in eal_intr_process_interrupts (nfds=3D<optimize=
d
>       out>, events=3D<optimized out>) at /usr/src/debug/dpdk-18.11/lib/
>       librte_eal/linuxapp/eal/eal_interrupts.c:886
>   #11 eal_intr_handle_interrupts (totalfds=3D<optimized out>, pfd=3D20) a=
t
>       /usr/src/debug/dpdk-18.11/lib/librte_eal/linuxapp/eal/
>       eal_interrupts.c:946
>   #12 eal_intr_thread_main (arg=3D<optimized out>) at /usr/src/debug/
>       dpdk-18.11/lib/librte_eal/linuxapp/eal/eal_interrupts.c:1035
>   #13 0x00007ffb4b208dd5 in start_thread () from /usr/lib64/libpthread.so=
.0
>   #14 0x00007ffb4981659d in clone () from /usr/lib64/libc.so.6

That is not specific to i40e or macvlan filter.
If inside your app several threads concurrently access/modify NIC config,
then you need to provide some synchronization mechanism for them.
DPDK ethdev API (as most others) on itself doesn't provide any synchronizat=
ion,
leaving it up to the upper layer to choose the most appropriate one.
Konstantin

>=20
> Fixes: 4861cde46116 ("i40e: new poll mode driver")
> Cc: stable@dpdk.org
>=20
> Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
> ---
>  drivers/net/i40e/i40e_ethdev.c  | 28 +++++++++++++++++++++++++---
>  drivers/net/i40e/i40e_ethdev.h  |  1 +
>  drivers/net/i40e/i40e_pf.c      |  6 ++++++
>  drivers/net/i40e/rte_pmd_i40e.c | 12 ++++++++++++
>  4 files changed, 44 insertions(+), 3 deletions(-)
>=20
> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethde=
v.c
> index 5b01dc1..e4f6818 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -4036,8 +4036,9 @@ static int i40e_dev_xstats_get_names(__rte_unused s=
truct rte_eth_dev *dev,
>  		vsi =3D pf->main_vsi;
>  	else
>  		vsi =3D pf->vmdq[pool - 1].vsi;
> -
> +	rte_spinlock_lock(&vsi->mac_list_lock);
>  	ret =3D i40e_vsi_add_mac(vsi, &mac_filter);
> +	rte_spinlock_unlock(&vsi->mac_list_lock);
>  	if (ret !=3D I40E_SUCCESS) {
>  		PMD_DRV_LOG(ERR, "Failed to add MACVLAN filter");
>  		return -ENODEV;
> @@ -4075,7 +4076,9 @@ static int i40e_dev_xstats_get_names(__rte_unused s=
truct rte_eth_dev *dev,
>  				}
>  				vsi =3D pf->vmdq[i - 1].vsi;
>  			}
> +			rte_spinlock_lock(&vsi->mac_list_lock);
>  			ret =3D i40e_vsi_delete_mac(vsi, macaddr);
> +			rte_spinlock_unlock(&vsi->mac_list_lock);
>=20
>  			if (ret) {
>  				PMD_DRV_LOG(ERR, "Failed to remove MACVLAN filter");
> @@ -4138,7 +4141,9 @@ static int i40e_dev_xstats_get_names(__rte_unused s=
truct rte_eth_dev *dev,
>  				 ETHER_ADDR_LEN);
>=20
>  		mac_filter.filter_type =3D filter->filter_type;
> +		rte_spinlock_lock(&vf->vsi->mac_list_lock);
>  		ret =3D i40e_vsi_add_mac(vf->vsi, &mac_filter);
> +		rte_spinlock_unlock(&vf->vsi->mac_list_lock);
>  		if (ret !=3D I40E_SUCCESS) {
>  			PMD_DRV_LOG(ERR, "Failed to add MAC filter.");
>  			return -1;
> @@ -4147,7 +4152,9 @@ static int i40e_dev_xstats_get_names(__rte_unused s=
truct rte_eth_dev *dev,
>  	} else {
>  		rte_memcpy(hw->mac.addr, hw->mac.perm_addr,
>  				ETHER_ADDR_LEN);
> +		rte_spinlock_lock(&vf->vsi->mac_list_lock);
>  		ret =3D i40e_vsi_delete_mac(vf->vsi, &filter->mac_addr);
> +		rte_spinlock_unlock(&vf->vsi->mac_list_lock);
>  		if (ret !=3D I40E_SUCCESS) {
>  			PMD_DRV_LOG(ERR, "Failed to delete MAC filter.");
>  			return -1;
> @@ -5266,9 +5273,11 @@ static int i40e_pf_config_vf_rxq_number(struct rte=
_eth_dev *dev)
>  	}
>=20
>  	/* Remove all macvlan filters of the VSI */
> +	rte_spinlock_lock(&vsi->mac_list_lock);
>  	i40e_vsi_remove_all_macvlan_filter(vsi);
>  	TAILQ_FOREACH_SAFE(f, &vsi->mac_list, next, temp)
>  		rte_free(f);
> +	rte_spinlock_unlock(&vsi->mac_list_lock);
>=20
>  	if (vsi->type !=3D I40E_VSI_MAIN &&
>  	    ((vsi->type !=3D I40E_VSI_SRIOV) ||
> @@ -5346,7 +5355,9 @@ static int i40e_pf_config_vf_rxq_number(struct rte_=
eth_dev *dev)
>  		rte_memcpy(&mac->addr_bytes, hw->mac.perm_addr,
>  				ETH_ADDR_LEN);
>  		f->mac_info.filter_type =3D RTE_MACVLAN_PERFECT_MATCH;
> +		rte_spinlock_lock(&vsi->mac_list_lock);
>  		TAILQ_INSERT_TAIL(&vsi->mac_list, f, next);
> +		rte_spinlock_unlock(&vsi->mac_list_lock);
>  		vsi->mac_num++;
>=20
>  		return ret;
> @@ -5354,7 +5365,10 @@ static int i40e_pf_config_vf_rxq_number(struct rte=
_eth_dev *dev)
>  	rte_memcpy(&filter.mac_addr,
>  		(struct ether_addr *)(hw->mac.perm_addr), ETH_ADDR_LEN);
>  	filter.filter_type =3D RTE_MACVLAN_PERFECT_MATCH;
> -	return i40e_vsi_add_mac(vsi, &filter);
> +	rte_spinlock_lock(&vsi->mac_list_lock);
> +	ret =3D i40e_vsi_add_mac(vsi, &filter);
> +	rte_spinlock_unlock(&vsi->mac_list_lock);
> +	return ret;
>  }
>=20
>  /*
> @@ -5521,6 +5535,7 @@ struct i40e_vsi *
>  		return NULL;
>  	}
>  	TAILQ_INIT(&vsi->mac_list);
> +	rte_spinlock_init(&vsi->mac_list_lock);
>  	vsi->type =3D type;
>  	vsi->adapter =3D I40E_PF_TO_ADAPTER(pf);
>  	vsi->max_macaddrs =3D I40E_NUM_MACADDR_MAX;
> @@ -5816,8 +5831,9 @@ struct i40e_vsi *
>  	/* MAC/VLAN configuration */
>  	rte_memcpy(&filter.mac_addr, &broadcast, ETHER_ADDR_LEN);
>  	filter.filter_type =3D RTE_MACVLAN_PERFECT_MATCH;
> -
> +	rte_spinlock_lock(&vsi->mac_list_lock);
>  	ret =3D i40e_vsi_add_mac(vsi, &filter);
> +	rte_spinlock_unlock(&vsi->mac_list_lock);
>  	if (ret !=3D I40E_SUCCESS) {
>  		PMD_DRV_LOG(ERR, "Failed to add MACVLAN filter");
>  		goto fail_msix_alloc;
> @@ -5866,6 +5882,7 @@ struct i40e_vsi *
>  	i =3D 0;
>=20
>  	/* Remove all existing mac */
> +	rte_spinlock_lock(&vsi->mac_list_lock);
>  	TAILQ_FOREACH_SAFE(f, &vsi->mac_list, next, temp) {
>  		mac_filter[i] =3D f->mac_info;
>  		ret =3D i40e_vsi_delete_mac(vsi, &f->mac_info.mac_addr);
> @@ -5889,6 +5906,7 @@ struct i40e_vsi *
>  	}
>=20
>  DONE:
> +	rte_spinlock_unlock(&vsi->mac_list_lock);
>  	rte_free(mac_filter);
>  	return ret;
>  }
> @@ -11953,12 +11971,14 @@ static int i40e_set_default_mac_addr(struct rte=
_eth_dev *dev,
>  		return -EINVAL;
>  	}
>=20
> +	rte_spinlock_lock(&vsi->mac_list_lock);
>  	TAILQ_FOREACH(f, &vsi->mac_list, next) {
>  		if (is_same_ether_addr(&pf->dev_addr, &f->mac_info.mac_addr))
>  			break;
>  	}
>=20
>  	if (f =3D=3D NULL) {
> +		rte_spinlock_unlock(&vsi->mac_list_lock);
>  		PMD_DRV_LOG(ERR, "Failed to find filter for default mac");
>  		return -EIO;
>  	}
> @@ -11966,11 +11986,13 @@ static int i40e_set_default_mac_addr(struct rte=
_eth_dev *dev,
>  	mac_filter =3D f->mac_info;
>  	ret =3D i40e_vsi_delete_mac(vsi, &mac_filter.mac_addr);
>  	if (ret !=3D I40E_SUCCESS) {
> +		rte_spinlock_unlock(&vsi->mac_list_lock);
>  		PMD_DRV_LOG(ERR, "Failed to delete mac filter");
>  		return -EIO;
>  	}
>  	memcpy(&mac_filter.mac_addr, mac_addr, ETH_ADDR_LEN);
>  	ret =3D i40e_vsi_add_mac(vsi, &mac_filter);
> +	rte_spinlock_unlock(&vsi->mac_list_lock);
>  	if (ret !=3D I40E_SUCCESS) {
>  		PMD_DRV_LOG(ERR, "Failed to add mac filter");
>  		return -EIO;
> diff --git a/drivers/net/i40e/i40e_ethdev.h b/drivers/net/i40e/i40e_ethde=
v.h
> index 930eb9a..060be26 100644
> --- a/drivers/net/i40e/i40e_ethdev.h
> +++ b/drivers/net/i40e/i40e_ethdev.h
> @@ -368,6 +368,7 @@ struct i40e_vsi {
>  	uint16_t mac_num;        /* Total mac number */
>  	uint32_t vfta[I40E_VFTA_SIZE];        /* VLAN bitmap */
>  	struct i40e_mac_filter_list mac_list; /* macvlan filter list */
> +	rte_spinlock_t mac_list_lock; /* macvlan filter list lock*/
>  	/* specific VSI-defined parameters, SRIOV stored the vf_id */
>  	uint32_t user_param;
>  	uint16_t seid;           /* The seid of VSI itself */
> diff --git a/drivers/net/i40e/i40e_pf.c b/drivers/net/i40e/i40e_pf.c
> index 91be450..faa80cc 100644
> --- a/drivers/net/i40e/i40e_pf.c
> +++ b/drivers/net/i40e/i40e_pf.c
> @@ -845,11 +845,14 @@
>  		mac =3D (struct ether_addr *)(addr_list->list[i].addr);
>  		rte_memcpy(&filter.mac_addr, mac, ETHER_ADDR_LEN);
>  		filter.filter_type =3D RTE_MACVLAN_PERFECT_MATCH;
> +		rte_spinlock_lock(&vf->vsi->mac_list_lock);
>  		if (is_zero_ether_addr(mac) ||
>  		    i40e_vsi_add_mac(vf->vsi, &filter)) {
> +			rte_spinlock_unlock(&vf->vsi->mac_list_lock);
>  			ret =3D I40E_ERR_INVALID_MAC_ADDR;
>  			goto send_msg;
>  		}
> +		rte_spinlock_unlock(&vf->vsi->mac_list_lock);
>  	}
>=20
>  send_msg:
> @@ -887,11 +890,14 @@
>=20
>  	for (i =3D 0; i < addr_list->num_elements; i++) {
>  		mac =3D (struct ether_addr *)(addr_list->list[i].addr);
> +		rte_spinlock_lock(&vf->vsi->mac_list_lock);
>  		if(is_zero_ether_addr(mac) ||
>  			i40e_vsi_delete_mac(vf->vsi, mac)) {
> +			rte_spinlock_unlock(&vf->vsi->mac_list_lock);
>  			ret =3D I40E_ERR_INVALID_MAC_ADDR;
>  			goto send_msg;
>  		}
> +		rte_spinlock_unlock(&vf->vsi->mac_list_lock);
>  	}
>=20
>  send_msg:
> diff --git a/drivers/net/i40e/rte_pmd_i40e.c b/drivers/net/i40e/rte_pmd_i=
40e.c
> index 7ae78e4..c5983c9 100644
> --- a/drivers/net/i40e/rte_pmd_i40e.c
> +++ b/drivers/net/i40e/rte_pmd_i40e.c
> @@ -360,7 +360,9 @@
>  	}
>=20
>  	/* remove all the MAC and VLAN first */
> +	rte_spinlock_lock(&vsi->mac_list_lock);
>  	ret =3D i40e_vsi_rm_mac_filter(vsi);
> +	rte_spinlock_unlock(&vsi->mac_list_lock);
>  	if (ret) {
>  		PMD_INIT_LOG(ERR, "Failed to remove MAC filters.");
>  		return ret;
> @@ -390,7 +392,9 @@
>  	}
>=20
>  	/* add all the MAC and VLAN back */
> +	rte_spinlock_lock(&vsi->mac_list_lock);
>  	ret =3D i40e_vsi_restore_mac_filter(vsi);
> +	rte_spinlock_unlock(&vsi->mac_list_lock);
>  	if (ret)
>  		return ret;
>  	if (vsi->vlan_anti_spoof_on || vsi->vlan_filter_on) {
> @@ -563,10 +567,12 @@
>  	ether_addr_copy(mac_addr, &vf->mac_addr);
>=20
>  	/* Remove all existing mac */
> +	rte_spinlock_lock(&vsi->mac_list_lock);
>  	TAILQ_FOREACH_SAFE(f, &vsi->mac_list, next, temp)
>  		if (i40e_vsi_delete_mac(vsi, &f->mac_info.mac_addr)
>  				!=3D I40E_SUCCESS)
>  			PMD_DRV_LOG(WARNING, "Delete MAC failed");
> +	rte_spinlock_unlock(&vf->vsi->mac_list_lock);
>=20
>  	return 0;
>  }
> @@ -609,7 +615,9 @@
>  		ether_addr_copy(&null_mac_addr, &vf->mac_addr);
>=20
>  	/* Remove the mac */
> +	rte_spinlock_lock(&vsi->mac_list_lock);
>  	i40e_vsi_delete_mac(vsi, mac_addr);
> +	rte_spinlock_unlock(&vsi->mac_list_lock);
>=20
>  	return 0;
>  }
> @@ -764,6 +772,7 @@ int rte_pmd_i40e_set_vf_broadcast(uint16_t port, uint=
16_t vf_id,
>  		return -EINVAL;
>  	}
>=20
> +	rte_spinlock_lock(&vsi->mac_list_lock);
>  	if (on) {
>  		rte_memcpy(&filter.mac_addr, &broadcast, ETHER_ADDR_LEN);
>  		filter.filter_type =3D RTE_MACVLAN_PERFECT_MATCH;
> @@ -771,6 +780,7 @@ int rte_pmd_i40e_set_vf_broadcast(uint16_t port, uint=
16_t vf_id,
>  	} else {
>  		ret =3D i40e_vsi_delete_mac(vsi, &broadcast);
>  	}
> +	rte_spinlock_unlock(&vsi->mac_list_lock);
>=20
>  	if (ret !=3D I40E_SUCCESS && ret !=3D I40E_ERR_PARAM) {
>  		ret =3D -ENOTSUP;
> @@ -2388,7 +2398,9 @@ int rte_pmd_i40e_ptype_mapping_replace(uint16_t por=
t,
>=20
>  	mac_filter.filter_type =3D RTE_MACVLAN_PERFECT_MATCH;
>  	ether_addr_copy(mac_addr, &mac_filter.mac_addr);
> +	rte_spinlock_lock(&vf->vsi->mac_list_lock);
>  	ret =3D i40e_vsi_add_mac(vsi, &mac_filter);
> +	rte_spinlock_unlock(&vf->vsi->mac_list_lock);
>  	if (ret !=3D I40E_SUCCESS) {
>  		PMD_DRV_LOG(ERR, "Failed to add MAC filter.");
>  		return -1;
> --
> 1.8.3.1
>=20

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from dpdk.org (dpdk.org [92.243.14.124])
	by dpdk.space (Postfix) with ESMTP id 5FBC4A00E6
	for <public@inbox.dpdk.org>; Mon, 15 Apr 2019 14:21:18 +0200 (CEST)
Received: from [92.243.14.124] (localhost [127.0.0.1])
	by dpdk.org (Postfix) with ESMTP id 108CA1B1EF;
	Mon, 15 Apr 2019 14:21:15 +0200 (CEST)
Received: from mga04.intel.com (mga04.intel.com [192.55.52.120])
 by dpdk.org (Postfix) with ESMTP id D052C5942;
 Mon, 15 Apr 2019 14:21:12 +0200 (CEST)
X-Amp-Result: SKIPPED(no attachment in message)
X-Amp-File-Uploaded: False
Received: from fmsmga008.fm.intel.com ([10.253.24.58])
 by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384;
 15 Apr 2019 05:21:10 -0700
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.60,353,1549958400"; d="scan'208";a="140800632"
Received: from irsmsx101.ger.corp.intel.com ([163.33.3.153])
 by fmsmga008.fm.intel.com with ESMTP; 15 Apr 2019 05:21:10 -0700
Received: from irsmsx105.ger.corp.intel.com ([169.254.7.31]) by
 IRSMSX101.ger.corp.intel.com ([169.254.1.115]) with mapi id 14.03.0415.000;
 Mon, 15 Apr 2019 13:21:09 +0100
From: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>
To: wangyunjian <wangyunjian@huawei.com>, "dev@dpdk.org" <dev@dpdk.org>
CC: "xudingke@huawei.com" <xudingke@huawei.com>, "stable@dpdk.org"
 <stable@dpdk.org>
Thread-Topic: [dpdk-dev] [PATCH] net/i40e: fix crash when calling
 i40e_vsi_delete_mac
Thread-Index: AQHU8d8IjuCfzE+MZEaWqavVP6qkOaY9JZow
Date: Mon, 15 Apr 2019 12:21:08 +0000
Message-ID:
 <2601191342CEEE43887BDE71AB9772580148A97E3D@irsmsx105.ger.corp.intel.com>
References: <1555149291-12432-1-git-send-email-wangyunjian@huawei.com>
In-Reply-To: <1555149291-12432-1-git-send-email-wangyunjian@huawei.com>
Accept-Language: en-IE, en-US
Content-Language: en-US
X-MS-Has-Attach: 
X-MS-TNEF-Correlator: 
x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiZTg3MjU4ZjEtNGU1Yy00ZjZiLWIxYmEtNjczZTFkNzYyOWZkIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiMXliU0JUeWZ1bXpHdWczRWoyWnc2ZHhsTVg2YjNDYXpYYWlveXRDbjBMUnV0YTZuQ0J4V3ppXC9MWFFDbkxZaE4ifQ==
x-ctpclassification: CTP_NT
dlp-product: dlpe-windows
dlp-version: 11.0.600.7
dlp-reaction: no-action
x-originating-ip: [163.33.239.182]
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Subject: Re: [dpdk-dev] [PATCH] net/i40e: fix crash when
	calling	i40e_vsi_delete_mac
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>
Message-ID: <20190415122108.yQ9Mls2RrU51USv4jbL4YMFuDyGnFz5igy9vDJD_gKQ@z>

Hi,

> Now the macvlan filter list may be accessed in the same time by two
> different threads and may cause a lot of optional errors. This patch
> protects the macvlan filter access with a spinlock.
>=20
> Call Trace:
>   #1  0x00007ffb4cbe2e3c in i40e_vsi_delete_mac (vsi=3Dvsi@entry=3D
>       0x400052804b40, addr=3Daddr@entry=3D0x7ffb47672244) at /usr/src/
>       debug/dpdk-18.11/drivers/net/i40e/i40e_ethdev.c:7266
>   #2  0x00007ffb4cbe342b in i40e_set_default_mac_addr (dev=3D<optimized o=
ut>,
>       mac_addr=3D0x400052a6618d) at /usr/src/debug/dpdk-18.11/drivers/net=
/
> 	  i40e/i40e_ethdev.c:11893
>   #3  0x00007ffb4f569d4a in rte_eth_dev_default_mac_addr_set (port_id=3D
>       <optimized out>, addr=3Daddr@entry=3D0x400052a6618d) at /usr/src/de=
bug/
> 	  dpdk-18.11/lib/librte_ethdev/rte_ethdev.c:3366
>   #4  0x00007ffb4d0bb403 in mac_address_slaves_update (bonded_eth_dev=3D
>       bonded_eth_dev@entry=3D0xacf8c0 <rte_eth_devices>) at /usr/src/debu=
g/
> 	  dpdk-18.11/drivers/net/bonding/rte_eth_bond_pmd.c:1854
>   #5  0x00007ffb4d0bd221 in bond_ethdev_lsc_event_callback (port_id=3D
>       <optimized out>, type=3D<optimized out>, param=3D<optimized out>,
>       ret_param=3D <optimized out>) at /usr/src/debug/dpdk-18.11/drivers/
>       net/bonding/rte_eth_bond_pmd.c:3076
>   #6  0x00007ffb4f56aa09 in _rte_eth_dev_callback_process (dev=3Ddev@entr=
y=3D
>       0xad3940 <rte_eth_devices+16512>, event=3Devent@entry=3D
>       RTE_ETH_EVENT_INTR_LSC, ret_param=3Dret_param@entry=3D0x0)
>       at /usr/src/debug/dpdk-18.11/lib/librte_ethdev/rte_ethdev.c:3699
>   #7  0x00007ffb4cbb99f1 in i40e_dev_handle_aq_msg (dev=3Ddev@entry=3D0xa=
d3940
>       <rte_eth_devices+16512>) at /usr/src/debug/dpdk-18.11/drivers/net/
>       i40e/i40e_ethdev.c:6573
>   #8  0x00007ffb4cbdfbed in i40e_dev_alarm_handler (param=3D0xad3940
>       <rte_eth_devices+16512>) at /usr/src/debug/dpdk-18.11/drivers/net/
>       i40e/i40e_ethdev.c:6681
>   #9  0x00007ffb4fb9766f in eal_alarm_callback (arg=3D<optimized out>) at
>       /usr/src/debug/dpdk-18.11/lib/librte_eal/linuxapp/eal/eal_alarm.c:9=
0
>   #10 0x00007ffb4fb95dd2 in eal_intr_process_interrupts (nfds=3D<optimize=
d
>       out>, events=3D<optimized out>) at /usr/src/debug/dpdk-18.11/lib/
>       librte_eal/linuxapp/eal/eal_interrupts.c:886
>   #11 eal_intr_handle_interrupts (totalfds=3D<optimized out>, pfd=3D20) a=
t
>       /usr/src/debug/dpdk-18.11/lib/librte_eal/linuxapp/eal/
>       eal_interrupts.c:946
>   #12 eal_intr_thread_main (arg=3D<optimized out>) at /usr/src/debug/
>       dpdk-18.11/lib/librte_eal/linuxapp/eal/eal_interrupts.c:1035
>   #13 0x00007ffb4b208dd5 in start_thread () from /usr/lib64/libpthread.so=
.0
>   #14 0x00007ffb4981659d in clone () from /usr/lib64/libc.so.6

That is not specific to i40e or macvlan filter.
If inside your app several threads concurrently access/modify NIC config,
then you need to provide some synchronization mechanism for them.
DPDK ethdev API (as most others) on itself doesn't provide any synchronizat=
ion,
leaving it up to the upper layer to choose the most appropriate one.
Konstantin

>=20
> Fixes: 4861cde46116 ("i40e: new poll mode driver")
> Cc: stable@dpdk.org
>=20
> Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
> ---
>  drivers/net/i40e/i40e_ethdev.c  | 28 +++++++++++++++++++++++++---
>  drivers/net/i40e/i40e_ethdev.h  |  1 +
>  drivers/net/i40e/i40e_pf.c      |  6 ++++++
>  drivers/net/i40e/rte_pmd_i40e.c | 12 ++++++++++++
>  4 files changed, 44 insertions(+), 3 deletions(-)
>=20
> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethde=
v.c
> index 5b01dc1..e4f6818 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -4036,8 +4036,9 @@ static int i40e_dev_xstats_get_names(__rte_unused s=
truct rte_eth_dev *dev,
>  		vsi =3D pf->main_vsi;
>  	else
>  		vsi =3D pf->vmdq[pool - 1].vsi;
> -
> +	rte_spinlock_lock(&vsi->mac_list_lock);
>  	ret =3D i40e_vsi_add_mac(vsi, &mac_filter);
> +	rte_spinlock_unlock(&vsi->mac_list_lock);
>  	if (ret !=3D I40E_SUCCESS) {
>  		PMD_DRV_LOG(ERR, "Failed to add MACVLAN filter");
>  		return -ENODEV;
> @@ -4075,7 +4076,9 @@ static int i40e_dev_xstats_get_names(__rte_unused s=
truct rte_eth_dev *dev,
>  				}
>  				vsi =3D pf->vmdq[i - 1].vsi;
>  			}
> +			rte_spinlock_lock(&vsi->mac_list_lock);
>  			ret =3D i40e_vsi_delete_mac(vsi, macaddr);
> +			rte_spinlock_unlock(&vsi->mac_list_lock);
>=20
>  			if (ret) {
>  				PMD_DRV_LOG(ERR, "Failed to remove MACVLAN filter");
> @@ -4138,7 +4141,9 @@ static int i40e_dev_xstats_get_names(__rte_unused s=
truct rte_eth_dev *dev,
>  				 ETHER_ADDR_LEN);
>=20
>  		mac_filter.filter_type =3D filter->filter_type;
> +		rte_spinlock_lock(&vf->vsi->mac_list_lock);
>  		ret =3D i40e_vsi_add_mac(vf->vsi, &mac_filter);
> +		rte_spinlock_unlock(&vf->vsi->mac_list_lock);
>  		if (ret !=3D I40E_SUCCESS) {
>  			PMD_DRV_LOG(ERR, "Failed to add MAC filter.");
>  			return -1;
> @@ -4147,7 +4152,9 @@ static int i40e_dev_xstats_get_names(__rte_unused s=
truct rte_eth_dev *dev,
>  	} else {
>  		rte_memcpy(hw->mac.addr, hw->mac.perm_addr,
>  				ETHER_ADDR_LEN);
> +		rte_spinlock_lock(&vf->vsi->mac_list_lock);
>  		ret =3D i40e_vsi_delete_mac(vf->vsi, &filter->mac_addr);
> +		rte_spinlock_unlock(&vf->vsi->mac_list_lock);
>  		if (ret !=3D I40E_SUCCESS) {
>  			PMD_DRV_LOG(ERR, "Failed to delete MAC filter.");
>  			return -1;
> @@ -5266,9 +5273,11 @@ static int i40e_pf_config_vf_rxq_number(struct rte=
_eth_dev *dev)
>  	}
>=20
>  	/* Remove all macvlan filters of the VSI */
> +	rte_spinlock_lock(&vsi->mac_list_lock);
>  	i40e_vsi_remove_all_macvlan_filter(vsi);
>  	TAILQ_FOREACH_SAFE(f, &vsi->mac_list, next, temp)
>  		rte_free(f);
> +	rte_spinlock_unlock(&vsi->mac_list_lock);
>=20
>  	if (vsi->type !=3D I40E_VSI_MAIN &&
>  	    ((vsi->type !=3D I40E_VSI_SRIOV) ||
> @@ -5346,7 +5355,9 @@ static int i40e_pf_config_vf_rxq_number(struct rte_=
eth_dev *dev)
>  		rte_memcpy(&mac->addr_bytes, hw->mac.perm_addr,
>  				ETH_ADDR_LEN);
>  		f->mac_info.filter_type =3D RTE_MACVLAN_PERFECT_MATCH;
> +		rte_spinlock_lock(&vsi->mac_list_lock);
>  		TAILQ_INSERT_TAIL(&vsi->mac_list, f, next);
> +		rte_spinlock_unlock(&vsi->mac_list_lock);
>  		vsi->mac_num++;
>=20
>  		return ret;
> @@ -5354,7 +5365,10 @@ static int i40e_pf_config_vf_rxq_number(struct rte=
_eth_dev *dev)
>  	rte_memcpy(&filter.mac_addr,
>  		(struct ether_addr *)(hw->mac.perm_addr), ETH_ADDR_LEN);
>  	filter.filter_type =3D RTE_MACVLAN_PERFECT_MATCH;
> -	return i40e_vsi_add_mac(vsi, &filter);
> +	rte_spinlock_lock(&vsi->mac_list_lock);
> +	ret =3D i40e_vsi_add_mac(vsi, &filter);
> +	rte_spinlock_unlock(&vsi->mac_list_lock);
> +	return ret;
>  }
>=20
>  /*
> @@ -5521,6 +5535,7 @@ struct i40e_vsi *
>  		return NULL;
>  	}
>  	TAILQ_INIT(&vsi->mac_list);
> +	rte_spinlock_init(&vsi->mac_list_lock);
>  	vsi->type =3D type;
>  	vsi->adapter =3D I40E_PF_TO_ADAPTER(pf);
>  	vsi->max_macaddrs =3D I40E_NUM_MACADDR_MAX;
> @@ -5816,8 +5831,9 @@ struct i40e_vsi *
>  	/* MAC/VLAN configuration */
>  	rte_memcpy(&filter.mac_addr, &broadcast, ETHER_ADDR_LEN);
>  	filter.filter_type =3D RTE_MACVLAN_PERFECT_MATCH;
> -
> +	rte_spinlock_lock(&vsi->mac_list_lock);
>  	ret =3D i40e_vsi_add_mac(vsi, &filter);
> +	rte_spinlock_unlock(&vsi->mac_list_lock);
>  	if (ret !=3D I40E_SUCCESS) {
>  		PMD_DRV_LOG(ERR, "Failed to add MACVLAN filter");
>  		goto fail_msix_alloc;
> @@ -5866,6 +5882,7 @@ struct i40e_vsi *
>  	i =3D 0;
>=20
>  	/* Remove all existing mac */
> +	rte_spinlock_lock(&vsi->mac_list_lock);
>  	TAILQ_FOREACH_SAFE(f, &vsi->mac_list, next, temp) {
>  		mac_filter[i] =3D f->mac_info;
>  		ret =3D i40e_vsi_delete_mac(vsi, &f->mac_info.mac_addr);
> @@ -5889,6 +5906,7 @@ struct i40e_vsi *
>  	}
>=20
>  DONE:
> +	rte_spinlock_unlock(&vsi->mac_list_lock);
>  	rte_free(mac_filter);
>  	return ret;
>  }
> @@ -11953,12 +11971,14 @@ static int i40e_set_default_mac_addr(struct rte=
_eth_dev *dev,
>  		return -EINVAL;
>  	}
>=20
> +	rte_spinlock_lock(&vsi->mac_list_lock);
>  	TAILQ_FOREACH(f, &vsi->mac_list, next) {
>  		if (is_same_ether_addr(&pf->dev_addr, &f->mac_info.mac_addr))
>  			break;
>  	}
>=20
>  	if (f =3D=3D NULL) {
> +		rte_spinlock_unlock(&vsi->mac_list_lock);
>  		PMD_DRV_LOG(ERR, "Failed to find filter for default mac");
>  		return -EIO;
>  	}
> @@ -11966,11 +11986,13 @@ static int i40e_set_default_mac_addr(struct rte=
_eth_dev *dev,
>  	mac_filter =3D f->mac_info;
>  	ret =3D i40e_vsi_delete_mac(vsi, &mac_filter.mac_addr);
>  	if (ret !=3D I40E_SUCCESS) {
> +		rte_spinlock_unlock(&vsi->mac_list_lock);
>  		PMD_DRV_LOG(ERR, "Failed to delete mac filter");
>  		return -EIO;
>  	}
>  	memcpy(&mac_filter.mac_addr, mac_addr, ETH_ADDR_LEN);
>  	ret =3D i40e_vsi_add_mac(vsi, &mac_filter);
> +	rte_spinlock_unlock(&vsi->mac_list_lock);
>  	if (ret !=3D I40E_SUCCESS) {
>  		PMD_DRV_LOG(ERR, "Failed to add mac filter");
>  		return -EIO;
> diff --git a/drivers/net/i40e/i40e_ethdev.h b/drivers/net/i40e/i40e_ethde=
v.h
> index 930eb9a..060be26 100644
> --- a/drivers/net/i40e/i40e_ethdev.h
> +++ b/drivers/net/i40e/i40e_ethdev.h
> @@ -368,6 +368,7 @@ struct i40e_vsi {
>  	uint16_t mac_num;        /* Total mac number */
>  	uint32_t vfta[I40E_VFTA_SIZE];        /* VLAN bitmap */
>  	struct i40e_mac_filter_list mac_list; /* macvlan filter list */
> +	rte_spinlock_t mac_list_lock; /* macvlan filter list lock*/
>  	/* specific VSI-defined parameters, SRIOV stored the vf_id */
>  	uint32_t user_param;
>  	uint16_t seid;           /* The seid of VSI itself */
> diff --git a/drivers/net/i40e/i40e_pf.c b/drivers/net/i40e/i40e_pf.c
> index 91be450..faa80cc 100644
> --- a/drivers/net/i40e/i40e_pf.c
> +++ b/drivers/net/i40e/i40e_pf.c
> @@ -845,11 +845,14 @@
>  		mac =3D (struct ether_addr *)(addr_list->list[i].addr);
>  		rte_memcpy(&filter.mac_addr, mac, ETHER_ADDR_LEN);
>  		filter.filter_type =3D RTE_MACVLAN_PERFECT_MATCH;
> +		rte_spinlock_lock(&vf->vsi->mac_list_lock);
>  		if (is_zero_ether_addr(mac) ||
>  		    i40e_vsi_add_mac(vf->vsi, &filter)) {
> +			rte_spinlock_unlock(&vf->vsi->mac_list_lock);
>  			ret =3D I40E_ERR_INVALID_MAC_ADDR;
>  			goto send_msg;
>  		}
> +		rte_spinlock_unlock(&vf->vsi->mac_list_lock);
>  	}
>=20
>  send_msg:
> @@ -887,11 +890,14 @@
>=20
>  	for (i =3D 0; i < addr_list->num_elements; i++) {
>  		mac =3D (struct ether_addr *)(addr_list->list[i].addr);
> +		rte_spinlock_lock(&vf->vsi->mac_list_lock);
>  		if(is_zero_ether_addr(mac) ||
>  			i40e_vsi_delete_mac(vf->vsi, mac)) {
> +			rte_spinlock_unlock(&vf->vsi->mac_list_lock);
>  			ret =3D I40E_ERR_INVALID_MAC_ADDR;
>  			goto send_msg;
>  		}
> +		rte_spinlock_unlock(&vf->vsi->mac_list_lock);
>  	}
>=20
>  send_msg:
> diff --git a/drivers/net/i40e/rte_pmd_i40e.c b/drivers/net/i40e/rte_pmd_i=
40e.c
> index 7ae78e4..c5983c9 100644
> --- a/drivers/net/i40e/rte_pmd_i40e.c
> +++ b/drivers/net/i40e/rte_pmd_i40e.c
> @@ -360,7 +360,9 @@
>  	}
>=20
>  	/* remove all the MAC and VLAN first */
> +	rte_spinlock_lock(&vsi->mac_list_lock);
>  	ret =3D i40e_vsi_rm_mac_filter(vsi);
> +	rte_spinlock_unlock(&vsi->mac_list_lock);
>  	if (ret) {
>  		PMD_INIT_LOG(ERR, "Failed to remove MAC filters.");
>  		return ret;
> @@ -390,7 +392,9 @@
>  	}
>=20
>  	/* add all the MAC and VLAN back */
> +	rte_spinlock_lock(&vsi->mac_list_lock);
>  	ret =3D i40e_vsi_restore_mac_filter(vsi);
> +	rte_spinlock_unlock(&vsi->mac_list_lock);
>  	if (ret)
>  		return ret;
>  	if (vsi->vlan_anti_spoof_on || vsi->vlan_filter_on) {
> @@ -563,10 +567,12 @@
>  	ether_addr_copy(mac_addr, &vf->mac_addr);
>=20
>  	/* Remove all existing mac */
> +	rte_spinlock_lock(&vsi->mac_list_lock);
>  	TAILQ_FOREACH_SAFE(f, &vsi->mac_list, next, temp)
>  		if (i40e_vsi_delete_mac(vsi, &f->mac_info.mac_addr)
>  				!=3D I40E_SUCCESS)
>  			PMD_DRV_LOG(WARNING, "Delete MAC failed");
> +	rte_spinlock_unlock(&vf->vsi->mac_list_lock);
>=20
>  	return 0;
>  }
> @@ -609,7 +615,9 @@
>  		ether_addr_copy(&null_mac_addr, &vf->mac_addr);
>=20
>  	/* Remove the mac */
> +	rte_spinlock_lock(&vsi->mac_list_lock);
>  	i40e_vsi_delete_mac(vsi, mac_addr);
> +	rte_spinlock_unlock(&vsi->mac_list_lock);
>=20
>  	return 0;
>  }
> @@ -764,6 +772,7 @@ int rte_pmd_i40e_set_vf_broadcast(uint16_t port, uint=
16_t vf_id,
>  		return -EINVAL;
>  	}
>=20
> +	rte_spinlock_lock(&vsi->mac_list_lock);
>  	if (on) {
>  		rte_memcpy(&filter.mac_addr, &broadcast, ETHER_ADDR_LEN);
>  		filter.filter_type =3D RTE_MACVLAN_PERFECT_MATCH;
> @@ -771,6 +780,7 @@ int rte_pmd_i40e_set_vf_broadcast(uint16_t port, uint=
16_t vf_id,
>  	} else {
>  		ret =3D i40e_vsi_delete_mac(vsi, &broadcast);
>  	}
> +	rte_spinlock_unlock(&vsi->mac_list_lock);
>=20
>  	if (ret !=3D I40E_SUCCESS && ret !=3D I40E_ERR_PARAM) {
>  		ret =3D -ENOTSUP;
> @@ -2388,7 +2398,9 @@ int rte_pmd_i40e_ptype_mapping_replace(uint16_t por=
t,
>=20
>  	mac_filter.filter_type =3D RTE_MACVLAN_PERFECT_MATCH;
>  	ether_addr_copy(mac_addr, &mac_filter.mac_addr);
> +	rte_spinlock_lock(&vf->vsi->mac_list_lock);
>  	ret =3D i40e_vsi_add_mac(vsi, &mac_filter);
> +	rte_spinlock_unlock(&vf->vsi->mac_list_lock);
>  	if (ret !=3D I40E_SUCCESS) {
>  		PMD_DRV_LOG(ERR, "Failed to add MAC filter.");
>  		return -1;
> --
> 1.8.3.1
>=20