From: "Zhang, Qi Z" <qi.z.zhang@intel.com>
To: "Ye, MingjinX" <mingjinx.ye@intel.com>, "dev@dpdk.org" <dev@dpdk.org>
Cc: "Yang, Qiming" <qiming.yang@intel.com>,
"Zhou, YidingX" <yidingx.zhou@intel.com>,
"stable@dpdk.org" <stable@dpdk.org>
Subject: RE: [PATCH v5] net/ice: fix crash on closing representor ports
Date: Mon, 6 Nov 2023 12:05:35 +0000 [thread overview]
Message-ID: <DM4PR11MB59943473433BD5689E523A2FD7AAA@DM4PR11MB5994.namprd11.prod.outlook.com> (raw)
In-Reply-To: <20231106100001.40213-1-mingjinx.ye@intel.com>
> -----Original Message-----
> From: Ye, MingjinX <mingjinx.ye@intel.com>
> Sent: Monday, November 6, 2023 6:00 PM
> To: dev@dpdk.org
> Cc: Yang, Qiming <qiming.yang@intel.com>; Zhou, YidingX
> <yidingx.zhou@intel.com>; Ye, MingjinX <mingjinx.ye@intel.com>;
> stable@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>
> Subject: [PATCH v5] net/ice: fix crash on closing representor ports
>
> The data resource in struct rte_eth_dev is cleared and points to NULL when
> the DCF port is closed.
>
> If the DCF representor port is closed after the DCF port is closed, a
> segmentation fault occurs because the representor port accesses the data
> resource released by the DCF port.
>
> This patch fixes this issue by synchronizing the state of DCF ports and
> representor ports to the peer in real time when their state changes.
>
> Fixes: 5674465a32c8 ("net/ice: add DCF VLAN handling")
> Fixes: da9cdcd1f372 ("net/ice: fix crash on representor port closing")
> Fixes: 7564d5509611 ("net/ice: add DCF hardware initialization")
> Fixes: c7e1a1a3bfeb ("net/ice: refactor DCF VLAN handling")
> Fixes: 1a86f4dbdf42 ("net/ice: support DCF device reset")
These fixlines does not make sense to me, I believe the issue comes from when we enabled port representor for DCF,
A patch expose the issue does not imply it cause the issue.
> Cc: stable@dpdk.org
>
> Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
> ---
> v2: Reformat code to remove unneeded fixlines.
> ---
> v3: New solution.
> ---
> v4: Optimize v2 patch.
> ---
> v5: optimization.
> ---
> drivers/net/ice/ice_dcf_ethdev.c | 30 ++++++++++++--
> drivers/net/ice/ice_dcf_ethdev.h | 3 ++
> drivers/net/ice/ice_dcf_vf_representor.c | 50 ++++++++++++++++++++++--
> 3 files changed, 77 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/ice/ice_dcf_ethdev.c b/drivers/net/ice/ice_dcf_ethdev.c
> index 065ec728c2..eea24ee3a9 100644
> --- a/drivers/net/ice/ice_dcf_ethdev.c
> +++ b/drivers/net/ice/ice_dcf_ethdev.c
> @@ -1618,6 +1618,26 @@ ice_dcf_free_repr_info(struct ice_dcf_adapter
> *dcf_adapter)
> }
> }
>
> +int
> +ice_dcf_handle_vf_repr_close(struct ice_dcf_adapter *dcf_adapter,
> + uint16_t vf_id)
> +{
> + struct ice_dcf_repr_info *vf_rep_info;
> +
> + if (dcf_adapter->num_reprs >= vf_id) {
> + PMD_DRV_LOG(ERR, "Invalid VF id: %d", vf_id);
> + return -1;
> + }
> +
> + if (!dcf_adapter->repr_infos)
> + return 0;
> +
> + vf_rep_info = &dcf_adapter->repr_infos[vf_id];
> + vf_rep_info->vf_rep_eth_dev = NULL;
> +
> + return 0;
> +}
> +
> static int
> ice_dcf_init_repr_info(struct ice_dcf_adapter *dcf_adapter) { @@ -1641,11
> +1661,10 @@ ice_dcf_dev_close(struct rte_eth_dev *dev)
> if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> return 0;
>
> + ice_dcf_vf_repr_notify_all(adapter, false);
> (void)ice_dcf_dev_stop(dev);
>
> ice_free_queues(dev);
> -
> - ice_dcf_free_repr_info(adapter);
> ice_dcf_uninit_parent_adapter(dev);
> ice_dcf_uninit_hw(dev, &adapter->real_hw);
>
> @@ -1835,7 +1854,7 @@ ice_dcf_dev_reset(struct rte_eth_dev *dev)
> ice_dcf_reset_hw(dev, hw);
> }
>
> - ret = ice_dcf_dev_uninit(dev);
> + ret = ice_dcf_dev_close(dev);
> if (ret)
> return ret;
>
> @@ -1938,12 +1957,17 @@ ice_dcf_dev_init(struct rte_eth_dev *eth_dev)
> }
>
> dcf_config_promisc(adapter, false, false);
> + ice_dcf_vf_repr_notify_all(adapter, true);
> +
> return 0;
> }
>
> static int
> ice_dcf_dev_uninit(struct rte_eth_dev *eth_dev) {
> + struct ice_dcf_adapter *adapter = eth_dev->data->dev_private;
> +
> + ice_dcf_free_repr_info(adapter);
> ice_dcf_dev_close(eth_dev);
>
> return 0;
> diff --git a/drivers/net/ice/ice_dcf_ethdev.h
> b/drivers/net/ice/ice_dcf_ethdev.h
> index 4baaec4b8b..6dcbaac5eb 100644
> --- a/drivers/net/ice/ice_dcf_ethdev.h
> +++ b/drivers/net/ice/ice_dcf_ethdev.h
> @@ -60,6 +60,7 @@ struct ice_dcf_vf_repr {
> struct rte_ether_addr mac_addr;
> uint16_t switch_domain_id;
> uint16_t vf_id;
> + bool dcf_valid;
>
> struct ice_dcf_vlan outer_vlan_info; /* DCF always handle outer VLAN
> */ }; @@ -80,6 +81,8 @@ int ice_dcf_vf_repr_init(struct rte_eth_dev
> *vf_rep_eth_dev, void *init_param); int ice_dcf_vf_repr_uninit(struct
> rte_eth_dev *vf_rep_eth_dev); int ice_dcf_vf_repr_init_vlan(struct
> rte_eth_dev *vf_rep_eth_dev); void ice_dcf_vf_repr_stop_all(struct
> ice_dcf_adapter *dcf_adapter);
> +void ice_dcf_vf_repr_notify_all(struct ice_dcf_adapter *dcf_adapter,
> +bool valid); int ice_dcf_handle_vf_repr_close(struct ice_dcf_adapter
> +*dcf_adapter, uint16_t vf_id);
> bool ice_dcf_adminq_need_retry(struct ice_adapter *ad);
>
> #endif /* _ICE_DCF_ETHDEV_H_ */
> diff --git a/drivers/net/ice/ice_dcf_vf_representor.c
> b/drivers/net/ice/ice_dcf_vf_representor.c
> index b9fcfc80ad..6c342798ac 100644
> --- a/drivers/net/ice/ice_dcf_vf_representor.c
> +++ b/drivers/net/ice/ice_dcf_vf_representor.c
> @@ -50,9 +50,32 @@ ice_dcf_vf_repr_dev_stop(struct rte_eth_dev *dev)
> return 0;
> }
>
> +static bool
This function return Boolean, but it never be checked.
> +ice_dcf_vf_repr_set_dcf_valid(struct rte_eth_dev *dev, bool valid) {
> + struct ice_dcf_vf_repr *repr = dev->data->dev_private;
> +
> + if (!repr)
> + return false;
Is this check necessary? If repr = null, means the representor port already be closed, the function will not be called for that port.
> +
> + repr->dcf_valid = valid;
> +
> + return true;
> +}
> +
> static int
> ice_dcf_vf_repr_dev_close(struct rte_eth_dev *dev) {
> + struct ice_dcf_vf_repr *repr = dev->data->dev_private;
> + struct ice_dcf_adapter *dcf_adapter;
> +
> + if (repr->dcf_valid) {
> + dcf_adapter = repr->dcf_eth_dev->data->dev_private;
> + ice_dcf_handle_vf_repr_close(dcf_adapter, repr->vf_id);
> + /* Commitment to not access DCF port resources */
> + repr->dcf_valid = false;
This is not necessary, as you already called ice_dcf_handle_vf_repr_close to notify dcf.
And the repr will be freed in ice_dcf_vf_repr_uninit anyway.
> + }
> +
> return ice_dcf_vf_repr_uninit(dev);
> }
>
> @@ -111,14 +134,15 @@ ice_dcf_vf_repr_link_update(__rte_unused struct
> rte_eth_dev *ethdev, static __rte_always_inline struct ice_dcf_hw *
> ice_dcf_vf_repr_hw(struct ice_dcf_vf_repr *repr) {
> - struct ice_dcf_adapter *dcf_adapter =
> - repr->dcf_eth_dev->data->dev_private;
> + struct ice_dcf_adapter *dcf_adapter;
>
> - if (!dcf_adapter) {
> + if (!repr->dcf_valid) {
> PMD_DRV_LOG(ERR, "DCF for VF representor has been
> released\n");
> return NULL;
> }
>
> + dcf_adapter = repr->dcf_eth_dev->data->dev_private;
> +
> return &dcf_adapter->real_hw;
> }
>
> @@ -414,6 +438,7 @@ ice_dcf_vf_repr_init(struct rte_eth_dev
> *vf_rep_eth_dev, void *init_param)
> repr->dcf_eth_dev = param->dcf_eth_dev;
> repr->switch_domain_id = param->switch_domain_id;
> repr->vf_id = param->vf_id;
> + repr->dcf_valid = true;
> repr->outer_vlan_info.port_vlan_ena = false;
> repr->outer_vlan_info.stripping_ena = false;
> repr->outer_vlan_info.tpid = RTE_ETHER_TYPE_VLAN; @@ -488,3
> +513,22 @@ ice_dcf_vf_repr_stop_all(struct ice_dcf_adapter *dcf_adapter)
> vf_rep_eth_dev->data->dev_started = 0;
> }
> }
> +
> +void
> +ice_dcf_vf_repr_notify_all(struct ice_dcf_adapter *dcf_adapter, bool
> +valid) {
> + uint16_t vf_id;
> + struct rte_eth_dev *vf_rep_eth_dev;
> +
> + if (!dcf_adapter->repr_infos)
> + return;
> +
> + for (vf_id = 0; vf_id < dcf_adapter->real_hw.num_vfs; vf_id++) {
> + vf_rep_eth_dev = dcf_adapter-
> >repr_infos[vf_id].vf_rep_eth_dev;
> +
> + if (!vf_rep_eth_dev)
> + continue;
> +
> + ice_dcf_vf_repr_set_dcf_valid(vf_rep_eth_dev, valid);
> + }
> +}
> --
> 2.25.1
next prev parent reply other threads:[~2023-11-06 12:05 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-26 9:51 [PATCH] net/iavf: " Mingjin Ye
2023-10-27 7:17 ` David Marchand
2023-10-30 2:35 ` [PATCH v2] " Mingjin Ye
2023-10-30 8:44 ` [PATCH v2] net/ice: " Mingjin Ye
2023-11-01 1:11 ` Zhang, Qi Z
2023-11-01 10:13 ` [PATCH v3] " Mingjin Ye
2023-11-01 10:48 ` Zhang, Qi Z
2023-11-02 2:38 ` Ye, MingjinX
2023-11-02 3:59 ` Zhang, Qi Z
2023-11-02 10:11 ` [PATCH v4] " Mingjin Ye
2023-11-06 1:23 ` Zhang, Qi Z
2023-11-06 10:00 ` [PATCH v5] " Mingjin Ye
2023-11-06 12:05 ` Zhang, Qi Z [this message]
2023-11-07 10:12 ` [PATCH v6] " Mingjin Ye
2023-11-07 12:18 ` Zhang, Qi Z
2023-11-08 11:39 ` [PATCH v7] " Mingjin Ye
2023-11-09 0:26 ` Zhang, Qi Z
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=DM4PR11MB59943473433BD5689E523A2FD7AAA@DM4PR11MB5994.namprd11.prod.outlook.com \
--to=qi.z.zhang@intel.com \
--cc=dev@dpdk.org \
--cc=mingjinx.ye@intel.com \
--cc=qiming.yang@intel.com \
--cc=stable@dpdk.org \
--cc=yidingx.zhou@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).