[Intel-wired-lan] [PATCH net v1] i40e: Fix crash when rebuild failed in i40e_xdp_setup

[Intel-wired-lan] [PATCH net v1] i40e: Fix crash when rebuild failed in i40e_xdp_setup

[Andrii Staikov]

From: Sylwester Dziedziuch <sylwesterx.dziedziuch@intel.com>

When attaching XDP program on i40e driver there was a
reset and rebuild of the interface to reconfigure the
queues for XDP operation. If one of the steps of rebuild
failed then the interface was left in incorrect state
that could lead to a crash. If rebuild failed while
getting capabilities from HW such crash occurs:

capability discovery failed, err I40E_ERR_ADMIN_QUEUE_TIMEOUT aq_err OK
BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
Call Trace:
? i40e_reconfig_rss_queues+0x120/0x120 [i40e]
  dev_xdp_install+0x70/0x100
  dev_xdp_attach+0x1d7/0x530
  dev_change_xdp_fd+0x1f4/0x230
  do_setlink+0x45f/0xf30
  ? irq_work_interrupt+0xa/0x20
  ? __nla_validate_parse+0x12d/0x1a0
  rtnl_setlink+0xb5/0x120
  rtnetlink_rcv_msg+0x2b1/0x360
  ? sock_has_perm+0x80/0xa0
  ? rtnl_calcit.isra.42+0x120/0x120
  netlink_rcv_skb+0x4c/0x120
  netlink_unicast+0x196/0x230
  netlink_sendmsg+0x204/0x3d0
  sock_sendmsg+0x4c/0x50
  __sys_sendto+0xee/0x160
  ? handle_mm_fault+0xc1/0x1e0
  ? syscall_trace_enter+0x1fb/0x2c0
  ? __sys_setsockopt+0xd6/0x1d0
  __x64_sys_sendto+0x24/0x30
  do_syscall_64+0x5b/0x1a0
  entry_SYSCALL_64_after_hwframe+0x65/0xca
  RIP: 0033:0x7f3535d99781

Fix this by removing reset and rebuild from i40e_xdp_setup
and replace it by interface down, reconfigure queues and
interface up. This way if any step fails the interface
will remain in a correct state.

Fixes: 0c8493d90b6b ("i40e: add XDP support for pass and drop actions")
Title: i40e: Fix crash when rebuild failed in i40e_xdp_setup
Change-type: DefectResolution
Signed-off-by: Sylwester Dziedziuch <sylwesterx.dziedziuch@intel.com>
Signed-off-by: Andrii Staikov <andrii.staikov@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_main.c | 155 ++++++++++++++------
 1 file changed, 112 insertions(+), 43 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 4b3b6e5b612d..68b343a7b77e 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -50,6 +50,8 @@ static int i40e_veb_get_bw_info(struct i40e_veb *veb);
 static int i40e_get_capabilities(struct i40e_pf *pf,
 				 enum i40e_admin_queue_opc list_type);
 static bool i40e_is_total_port_shutdown_enabled(struct i40e_pf *pf);
+static struct i40e_vsi *i40e_vsi_reinit_setup(struct i40e_vsi *vsi,
+					      bool is_xdp);
 
 /* i40e_pci_tbl - PCI Device ID Table
  *
@@ -3563,11 +3565,17 @@ static int i40e_configure_rx_ring(struct i40e_ring *ring)
 	/* clear the context structure first */
 	memset(&rx_ctx, 0, sizeof(rx_ctx));
 
-	if (ring->vsi->type == I40E_VSI_MAIN)
-		xdp_rxq_info_unreg_mem_model(&ring->xdp_rxq);
+	if (ring->vsi->type == I40E_VSI_MAIN) {
+		if (!xdp_rxq_info_is_reg(&ring->xdp_rxq))
+			xdp_rxq_info_reg(&ring->xdp_rxq, ring->netdev,
+					 ring->queue_index,
+					 ring->q_vector->napi.napi_id);
+	}
 
 	ring->xsk_pool = i40e_xsk_pool(ring);
 	if (ring->xsk_pool) {
+		xdp_rxq_info_unreg_mem_model(&ring->xdp_rxq);
+
 		ring->rx_buf_len =
 		  xsk_pool_get_rx_frame_size(ring->xsk_pool);
 		/* For AF_XDP ZC, we disallow packets to span on
@@ -13269,6 +13277,42 @@ static netdev_features_t i40e_features_check(struct sk_buff *skb,
 	return features & ~(NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK);
 }
 
+/**
+ * i40e_vsi_assign_bpf_prog - set or clear bpf prog pointer on VSI
+ * @vsi: VSI to changed
+ * @prog: XDP program
+ **/
+static struct bpf_prog *i40e_vsi_assign_bpf_prog(struct i40e_vsi *vsi,
+						 struct bpf_prog *prog)
+{
+	struct bpf_prog *old_prog;
+	int i;
+
+	old_prog = xchg(&vsi->xdp_prog, prog);
+
+	for (i = 0; i < vsi->num_queue_pairs; i++)
+		WRITE_ONCE(vsi->rx_rings[i]->xdp_prog, vsi->xdp_prog);
+
+	if (old_prog)
+		bpf_prog_put(old_prog);
+
+	return old_prog;
+}
+
+/**
+ * i40e_vsi_rx_napi_schedule - Schedule napi on RX queues from VSI
+ * @vsi: VSI to schedule napi on
+ */
+static void i40e_vsi_rx_napi_schedule(struct i40e_vsi *vsi)
+{
+	int i;
+
+	for (i = 0; i < vsi->num_queue_pairs; i++)
+		if (vsi->xdp_rings[i]->xsk_pool)
+			(void)i40e_xsk_wakeup(vsi->netdev, i,
+					      XDP_WAKEUP_RX);
+}
+
 /**
  * i40e_xdp_setup - add/remove an XDP program
  * @vsi: VSI to changed
@@ -13279,10 +13323,11 @@ static int i40e_xdp_setup(struct i40e_vsi *vsi, struct bpf_prog *prog,
 			  struct netlink_ext_ack *extack)
 {
 	int frame_size = vsi->netdev->mtu + ETH_HLEN + ETH_FCS_LEN + VLAN_HLEN;
+	bool is_xdp_enabled = i40e_enabled_xdp_vsi(vsi);
+	bool if_running = netif_running(vsi->netdev);
 	struct i40e_pf *pf = vsi->back;
 	struct bpf_prog *old_prog;
-	bool need_reset;
-	int i;
+	int ret = 0;
 
 	/* Don't allow frames that span over multiple buffers */
 	if (frame_size > i40e_calculate_vsi_rx_buf_len(vsi)) {
@@ -13290,49 +13335,69 @@ static int i40e_xdp_setup(struct i40e_vsi *vsi, struct bpf_prog *prog,
 		return -EINVAL;
 	}
 
-	/* When turning XDP on->off/off->on we reset and rebuild the rings. */
-	need_reset = (i40e_enabled_xdp_vsi(vsi) != !!prog);
-
-	if (need_reset)
-		i40e_prep_for_reset(pf);
-
 	/* VSI shall be deleted in a moment, just return EINVAL */
 	if (test_bit(__I40E_IN_REMOVE, pf->state))
 		return -EINVAL;
 
-	old_prog = xchg(&vsi->xdp_prog, prog);
+	if (if_running && !test_and_set_bit(__I40E_VSI_DOWN, vsi->state))
+		i40e_down(vsi);
 
-	if (need_reset) {
-		if (!prog)
-			/* Wait until ndo_xsk_wakeup completes. */
-			synchronize_rcu();
-		i40e_reset_and_rebuild(pf, true, true);
+	old_prog = i40e_vsi_assign_bpf_prog(vsi, prog);
+
+	vsi = i40e_vsi_reinit_setup(vsi, true);
+
+	if (!vsi) {
+		NL_SET_ERR_MSG_MOD(extack, "Failed to reinitialize VSI during XDP setup");
+		goto err_vsi_setup;
 	}
 
-	if (!i40e_enabled_xdp_vsi(vsi) && prog) {
-		if (i40e_realloc_rx_bi_zc(vsi, true))
-			return -ENOMEM;
-	} else if (i40e_enabled_xdp_vsi(vsi) && !prog) {
-		if (i40e_realloc_rx_bi_zc(vsi, false))
-			return -ENOMEM;
+	/* allocate descriptors */
+	ret = i40e_vsi_setup_tx_resources(vsi);
+	if (ret) {
+		NL_SET_ERR_MSG_MOD(extack, "Failed to configure TX resources during XDP setup");
+		goto err_setup_tx;
+	}
+	ret = i40e_vsi_setup_rx_resources(vsi);
+	if (ret) {
+		NL_SET_ERR_MSG_MOD(extack, "Failed to configure RX resources during XDP setup");
+		goto err_setup_rx;
 	}
 
-	for (i = 0; i < vsi->num_queue_pairs; i++)
-		WRITE_ONCE(vsi->rx_rings[i]->xdp_prog, vsi->xdp_prog);
+	if (!is_xdp_enabled && prog)
+		ret = i40e_realloc_rx_bi_zc(vsi, true);
+	else if (is_xdp_enabled && !prog)
+		ret = i40e_realloc_rx_bi_zc(vsi, false);
 
-	if (old_prog)
-		bpf_prog_put(old_prog);
+	if (ret) {
+		NL_SET_ERR_MSG_MOD(extack, "Failed to reallocate RX resources during XDP setup");
+		goto err_realloc;
+	}
 
-	/* Kick start the NAPI context if there is an AF_XDP socket open
-	 * on that queue id. This so that receiving will start.
-	 */
-	if (need_reset && prog)
-		for (i = 0; i < vsi->num_queue_pairs; i++)
-			if (vsi->xdp_rings[i]->xsk_pool)
-				(void)i40e_xsk_wakeup(vsi->netdev, i,
-						      XDP_WAKEUP_RX);
+	if (if_running)
+		ret = i40e_up(vsi);
 
-	return 0;
+	if (ret) {
+		NL_SET_ERR_MSG_MOD(extack, "Failed to open VSI during XDP setup");
+		goto err_realloc;
+	} else if (prog) {
+		/* Kick start the NAPI context if there is an AF_XDP socket open
+		 * on that queue id. This so that receiving will start.
+		 */
+		i40e_vsi_rx_napi_schedule(vsi);
+	}
+
+	return ret;
+
+err_setup_rx:
+	i40e_vsi_free_rx_resources(vsi);
+err_setup_tx:
+	i40e_vsi_free_tx_resources(vsi);
+err_vsi_setup:
+	if (vsi == pf->vsi[pf->lan_vsi])
+		i40e_do_reset(pf, I40E_PF_RESET_FLAG, true);
+err_realloc:
+	i40e_vsi_assign_bpf_prog(vsi, old_prog);
+	return ret;
 }
 
 /**
@@ -14274,13 +14339,14 @@ static int i40e_vsi_setup_vectors(struct i40e_vsi *vsi)
 /**
  * i40e_vsi_reinit_setup - return and reallocate resources for a VSI
  * @vsi: pointer to the vsi.
+ * @is_xdp: flag indicating if this is reinit during XDP setup
  *
  * This re-allocates a vsi's queue resources.
  *
  * Returns pointer to the successfully allocated and configured VSI sw struct
  * on success, otherwise returns NULL on failure.
  **/
-static struct i40e_vsi *i40e_vsi_reinit_setup(struct i40e_vsi *vsi)
+static struct i40e_vsi *i40e_vsi_reinit_setup(struct i40e_vsi *vsi, bool is_xdp)
 {
 	u16 alloc_queue_pairs;
 	struct i40e_pf *pf;
@@ -14316,12 +14382,14 @@ static struct i40e_vsi *i40e_vsi_reinit_setup(struct i40e_vsi *vsi)
 	/* Update the FW view of the VSI. Force a reset of TC and queue
 	 * layout configurations.
 	 */
-	enabled_tc = pf->vsi[pf->lan_vsi]->tc_config.enabled_tc;
-	pf->vsi[pf->lan_vsi]->tc_config.enabled_tc = 0;
-	pf->vsi[pf->lan_vsi]->seid = pf->main_vsi_seid;
-	i40e_vsi_config_tc(pf->vsi[pf->lan_vsi], enabled_tc);
-	if (vsi->type == I40E_VSI_MAIN)
-		i40e_rm_default_mac_filter(vsi, pf->hw.mac.perm_addr);
+	if (!is_xdp) {
+		enabled_tc = pf->vsi[pf->lan_vsi]->tc_config.enabled_tc;
+		pf->vsi[pf->lan_vsi]->tc_config.enabled_tc = 0;
+		pf->vsi[pf->lan_vsi]->seid = pf->main_vsi_seid;
+		i40e_vsi_config_tc(pf->vsi[pf->lan_vsi], enabled_tc);
+		if (vsi->type == I40E_VSI_MAIN)
+			i40e_rm_default_mac_filter(vsi, pf->hw.mac.perm_addr);
+	}
 
 	/* assign it some queues */
 	ret = i40e_alloc_rings(vsi);
@@ -15087,7 +15155,8 @@ static int i40e_setup_pf_switch(struct i40e_pf *pf, bool reinit, bool lock_acqui
 		if (pf->lan_vsi == I40E_NO_VSI)
 			vsi = i40e_vsi_setup(pf, I40E_VSI_MAIN, uplink_seid, 0);
 		else if (reinit)
-			vsi = i40e_vsi_reinit_setup(pf->vsi[pf->lan_vsi]);
+			vsi = i40e_vsi_reinit_setup(pf->vsi[pf->lan_vsi],
+						    false);
 		if (!vsi) {
 			dev_info(&pf->pdev->dev, "setup of MAIN VSI failed\n");
 			i40e_cloud_filter_exit(pf);
-- 
2.25.1

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net v1] i40e: Fix crash when rebuild failed in i40e_xdp_setup

[Paul Menzel]

Dear Andrii, dear Sylwester,


Thank you for the patch.


Am 25.10.22 um 09:27 schrieb Andrii Staikov:
> From: Sylwester Dziedziuch <sylwesterx.dziedziuch@intel.com>

In the summary/title, maybe use:

… rebuild fails …

> When attaching XDP program on i40e driver there was a
> reset and rebuild of the interface to reconfigure the
> queues for XDP operation. If one of the steps of rebuild
> failed then the interface was left in incorrect state
> that could lead to a crash. If rebuild failed while
> getting capabilities from HW such crash occurs:

Please reflow for 75 characters per line.

> capability discovery failed, err I40E_ERR_ADMIN_QUEUE_TIMEOUT aq_err OK
> BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
> Call Trace:
> ? i40e_reconfig_rss_queues+0x120/0x120 [i40e]
>    dev_xdp_install+0x70/0x100
>    dev_xdp_attach+0x1d7/0x530
>    dev_change_xdp_fd+0x1f4/0x230
>    do_setlink+0x45f/0xf30
>    ? irq_work_interrupt+0xa/0x20
>    ? __nla_validate_parse+0x12d/0x1a0
>    rtnl_setlink+0xb5/0x120
>    rtnetlink_rcv_msg+0x2b1/0x360
>    ? sock_has_perm+0x80/0xa0
>    ? rtnl_calcit.isra.42+0x120/0x120
>    netlink_rcv_skb+0x4c/0x120
>    netlink_unicast+0x196/0x230
>    netlink_sendmsg+0x204/0x3d0
>    sock_sendmsg+0x4c/0x50
>    __sys_sendto+0xee/0x160
>    ? handle_mm_fault+0xc1/0x1e0
>    ? syscall_trace_enter+0x1fb/0x2c0
>    ? __sys_setsockopt+0xd6/0x1d0
>    __x64_sys_sendto+0x24/0x30
>    do_syscall_64+0x5b/0x1a0
>    entry_SYSCALL_64_after_hwframe+0x65/0xca
>    RIP: 0033:0x7f3535d99781
> 
> Fix this by removing reset and rebuild from i40e_xdp_setup
> and replace it by interface down, reconfigure queues and
> interface up. This way if any step fails the interface
> will remain in a correct state.

How much longer does this take?

Can you give a simple XDP to reproduce this?

> Fixes: 0c8493d90b6b ("i40e: add XDP support for pass and drop actions")
> Title: i40e: Fix crash when rebuild failed in i40e_xdp_setup
> Change-type: DefectResolution

The two tags above are not useful for the upstream Linux kernel, right?

> Signed-off-by: Sylwester Dziedziuch <sylwesterx.dziedziuch@intel.com>
> Signed-off-by: Andrii Staikov <andrii.staikov@intel.com>
> ---
>   drivers/net/ethernet/intel/i40e/i40e_main.c | 155 ++++++++++++++------
>   1 file changed, 112 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index 4b3b6e5b612d..68b343a7b77e 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -50,6 +50,8 @@ static int i40e_veb_get_bw_info(struct i40e_veb *veb);
>   static int i40e_get_capabilities(struct i40e_pf *pf,
>   				 enum i40e_admin_queue_opc list_type);
>   static bool i40e_is_total_port_shutdown_enabled(struct i40e_pf *pf);
> +static struct i40e_vsi *i40e_vsi_reinit_setup(struct i40e_vsi *vsi,
> +					      bool is_xdp);
>   
>   /* i40e_pci_tbl - PCI Device ID Table
>    *
> @@ -3563,11 +3565,17 @@ static int i40e_configure_rx_ring(struct i40e_ring *ring)
>   	/* clear the context structure first */
>   	memset(&rx_ctx, 0, sizeof(rx_ctx));
>   
> -	if (ring->vsi->type == I40E_VSI_MAIN)
> -		xdp_rxq_info_unreg_mem_model(&ring->xdp_rxq);
> +	if (ring->vsi->type == I40E_VSI_MAIN) {
> +		if (!xdp_rxq_info_is_reg(&ring->xdp_rxq))
> +			xdp_rxq_info_reg(&ring->xdp_rxq, ring->netdev,
> +					 ring->queue_index,
> +					 ring->q_vector->napi.napi_id);
> +	}
>   
>   	ring->xsk_pool = i40e_xsk_pool(ring);
>   	if (ring->xsk_pool) {
> +		xdp_rxq_info_unreg_mem_model(&ring->xdp_rxq);
> +
>   		ring->rx_buf_len =
>   		  xsk_pool_get_rx_frame_size(ring->xsk_pool);
>   		/* For AF_XDP ZC, we disallow packets to span on
> @@ -13269,6 +13277,42 @@ static netdev_features_t i40e_features_check(struct sk_buff *skb,
>   	return features & ~(NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK);
>   }
>   
> +/**
> + * i40e_vsi_assign_bpf_prog - set or clear bpf prog pointer on VSI
> + * @vsi: VSI to changed
> + * @prog: XDP program
> + **/
> +static struct bpf_prog *i40e_vsi_assign_bpf_prog(struct i40e_vsi *vsi,
> +						 struct bpf_prog *prog)
> +{
> +	struct bpf_prog *old_prog;
> +	int i;

Please check, if `unsigned int` can be used for loop variables. (You can 
check with `scripts/bloat-o-meter`.


Kind regards,

Paul


> +
> +	old_prog = xchg(&vsi->xdp_prog, prog);
> +
> +	for (i = 0; i < vsi->num_queue_pairs; i++)
> +		WRITE_ONCE(vsi->rx_rings[i]->xdp_prog, vsi->xdp_prog);
> +
> +	if (old_prog)
> +		bpf_prog_put(old_prog);
> +
> +	return old_prog;
> +}
> +
> +/**
> + * i40e_vsi_rx_napi_schedule - Schedule napi on RX queues from VSI
> + * @vsi: VSI to schedule napi on
> + */
> +static void i40e_vsi_rx_napi_schedule(struct i40e_vsi *vsi)
> +{
> +	int i;
> +
> +	for (i = 0; i < vsi->num_queue_pairs; i++)
> +		if (vsi->xdp_rings[i]->xsk_pool)
> +			(void)i40e_xsk_wakeup(vsi->netdev, i,
> +					      XDP_WAKEUP_RX);
> +}
> +
>   /**
>    * i40e_xdp_setup - add/remove an XDP program
>    * @vsi: VSI to changed
> @@ -13279,10 +13323,11 @@ static int i40e_xdp_setup(struct i40e_vsi *vsi, struct bpf_prog *prog,
>   			  struct netlink_ext_ack *extack)
>   {
>   	int frame_size = vsi->netdev->mtu + ETH_HLEN + ETH_FCS_LEN + VLAN_HLEN;
> +	bool is_xdp_enabled = i40e_enabled_xdp_vsi(vsi);
> +	bool if_running = netif_running(vsi->netdev);
>   	struct i40e_pf *pf = vsi->back;
>   	struct bpf_prog *old_prog;
> -	bool need_reset;
> -	int i;
> +	int ret = 0;
>   
>   	/* Don't allow frames that span over multiple buffers */
>   	if (frame_size > i40e_calculate_vsi_rx_buf_len(vsi)) {
> @@ -13290,49 +13335,69 @@ static int i40e_xdp_setup(struct i40e_vsi *vsi, struct bpf_prog *prog,
>   		return -EINVAL;
>   	}
>   
> -	/* When turning XDP on->off/off->on we reset and rebuild the rings. */
> -	need_reset = (i40e_enabled_xdp_vsi(vsi) != !!prog);
> -
> -	if (need_reset)
> -		i40e_prep_for_reset(pf);
> -
>   	/* VSI shall be deleted in a moment, just return EINVAL */
>   	if (test_bit(__I40E_IN_REMOVE, pf->state))
>   		return -EINVAL;
>   
> -	old_prog = xchg(&vsi->xdp_prog, prog);
> +	if (if_running && !test_and_set_bit(__I40E_VSI_DOWN, vsi->state))
> +		i40e_down(vsi);
>   
> -	if (need_reset) {
> -		if (!prog)
> -			/* Wait until ndo_xsk_wakeup completes. */
> -			synchronize_rcu();
> -		i40e_reset_and_rebuild(pf, true, true);
> +	old_prog = i40e_vsi_assign_bpf_prog(vsi, prog);
> +
> +	vsi = i40e_vsi_reinit_setup(vsi, true);
> +
> +	if (!vsi) {
> +		NL_SET_ERR_MSG_MOD(extack, "Failed to reinitialize VSI during XDP setup");
> +		goto err_vsi_setup;
>   	}
>   
> -	if (!i40e_enabled_xdp_vsi(vsi) && prog) {
> -		if (i40e_realloc_rx_bi_zc(vsi, true))
> -			return -ENOMEM;
> -	} else if (i40e_enabled_xdp_vsi(vsi) && !prog) {
> -		if (i40e_realloc_rx_bi_zc(vsi, false))
> -			return -ENOMEM;
> +	/* allocate descriptors */
> +	ret = i40e_vsi_setup_tx_resources(vsi);
> +	if (ret) {
> +		NL_SET_ERR_MSG_MOD(extack, "Failed to configure TX resources during XDP setup");
> +		goto err_setup_tx;
> +	}
> +	ret = i40e_vsi_setup_rx_resources(vsi);
> +	if (ret) {
> +		NL_SET_ERR_MSG_MOD(extack, "Failed to configure RX resources during XDP setup");
> +		goto err_setup_rx;
>   	}
>   
> -	for (i = 0; i < vsi->num_queue_pairs; i++)
> -		WRITE_ONCE(vsi->rx_rings[i]->xdp_prog, vsi->xdp_prog);
> +	if (!is_xdp_enabled && prog)
> +		ret = i40e_realloc_rx_bi_zc(vsi, true);
> +	else if (is_xdp_enabled && !prog)
> +		ret = i40e_realloc_rx_bi_zc(vsi, false);
>   
> -	if (old_prog)
> -		bpf_prog_put(old_prog);
> +	if (ret) {
> +		NL_SET_ERR_MSG_MOD(extack, "Failed to reallocate RX resources during XDP setup");
> +		goto err_realloc;
> +	}
>   
> -	/* Kick start the NAPI context if there is an AF_XDP socket open
> -	 * on that queue id. This so that receiving will start.
> -	 */
> -	if (need_reset && prog)
> -		for (i = 0; i < vsi->num_queue_pairs; i++)
> -			if (vsi->xdp_rings[i]->xsk_pool)
> -				(void)i40e_xsk_wakeup(vsi->netdev, i,
> -						      XDP_WAKEUP_RX);
> +	if (if_running)
> +		ret = i40e_up(vsi);
>   
> -	return 0;
> +	if (ret) {
> +		NL_SET_ERR_MSG_MOD(extack, "Failed to open VSI during XDP setup");
> +		goto err_realloc;
> +	} else if (prog) {
> +		/* Kick start the NAPI context if there is an AF_XDP socket open
> +		 * on that queue id. This so that receiving will start.
> +		 */
> +		i40e_vsi_rx_napi_schedule(vsi);
> +	}
> +
> +	return ret;
> +
> +err_setup_rx:
> +	i40e_vsi_free_rx_resources(vsi);
> +err_setup_tx:
> +	i40e_vsi_free_tx_resources(vsi);
> +err_vsi_setup:
> +	if (vsi == pf->vsi[pf->lan_vsi])
> +		i40e_do_reset(pf, I40E_PF_RESET_FLAG, true);
> +err_realloc:
> +	i40e_vsi_assign_bpf_prog(vsi, old_prog);
> +	return ret;
>   }
>   
>   /**
> @@ -14274,13 +14339,14 @@ static int i40e_vsi_setup_vectors(struct i40e_vsi *vsi)
>   /**
>    * i40e_vsi_reinit_setup - return and reallocate resources for a VSI
>    * @vsi: pointer to the vsi.
> + * @is_xdp: flag indicating if this is reinit during XDP setup
>    *
>    * This re-allocates a vsi's queue resources.
>    *
>    * Returns pointer to the successfully allocated and configured VSI sw struct
>    * on success, otherwise returns NULL on failure.
>    **/
> -static struct i40e_vsi *i40e_vsi_reinit_setup(struct i40e_vsi *vsi)
> +static struct i40e_vsi *i40e_vsi_reinit_setup(struct i40e_vsi *vsi, bool is_xdp)
>   {
>   	u16 alloc_queue_pairs;
>   	struct i40e_pf *pf;
> @@ -14316,12 +14382,14 @@ static struct i40e_vsi *i40e_vsi_reinit_setup(struct i40e_vsi *vsi)
>   	/* Update the FW view of the VSI. Force a reset of TC and queue
>   	 * layout configurations.
>   	 */
> -	enabled_tc = pf->vsi[pf->lan_vsi]->tc_config.enabled_tc;
> -	pf->vsi[pf->lan_vsi]->tc_config.enabled_tc = 0;
> -	pf->vsi[pf->lan_vsi]->seid = pf->main_vsi_seid;
> -	i40e_vsi_config_tc(pf->vsi[pf->lan_vsi], enabled_tc);
> -	if (vsi->type == I40E_VSI_MAIN)
> -		i40e_rm_default_mac_filter(vsi, pf->hw.mac.perm_addr);
> +	if (!is_xdp) {
> +		enabled_tc = pf->vsi[pf->lan_vsi]->tc_config.enabled_tc;
> +		pf->vsi[pf->lan_vsi]->tc_config.enabled_tc = 0;
> +		pf->vsi[pf->lan_vsi]->seid = pf->main_vsi_seid;
> +		i40e_vsi_config_tc(pf->vsi[pf->lan_vsi], enabled_tc);
> +		if (vsi->type == I40E_VSI_MAIN)
> +			i40e_rm_default_mac_filter(vsi, pf->hw.mac.perm_addr);
> +	}
>   
>   	/* assign it some queues */
>   	ret = i40e_alloc_rings(vsi);
> @@ -15087,7 +15155,8 @@ static int i40e_setup_pf_switch(struct i40e_pf *pf, bool reinit, bool lock_acqui
>   		if (pf->lan_vsi == I40E_NO_VSI)
>   			vsi = i40e_vsi_setup(pf, I40E_VSI_MAIN, uplink_seid, 0);
>   		else if (reinit)
> -			vsi = i40e_vsi_reinit_setup(pf->vsi[pf->lan_vsi]);
> +			vsi = i40e_vsi_reinit_setup(pf->vsi[pf->lan_vsi],
> +						    false);
>   		if (!vsi) {
>   			dev_info(&pf->pdev->dev, "setup of MAIN VSI failed\n");
>   			i40e_cloud_filter_exit(pf);
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net v1] i40e: Fix crash when rebuild failed in i40e_xdp_setup

[Staikov, Andrii]

Hi Paul,

Thank you for the response on the path.
I have a few questions about it

-----Original Message-----
From: Paul Menzel <pmenzel@molgen.mpg.de> 
Sent: Tuesday, October 25, 2022 10:41 AM
To: Staikov, Andrii <andrii.staikov@intel.com>; Dziedziuch, SylwesterX <sylwesterx.dziedziuch@intel.com>
Cc: intel-wired-lan@lists.osuosl.org
Subject: Re: [Intel-wired-lan] [PATCH net v1] i40e: Fix crash when rebuild failed in i40e_xdp_setup

Dear Andrii, dear Sylwester,


Thank you for the patch.
I have a few questions to your response:

1. In the summary/title, maybe use: … rebuild fails …
Do you mean in Subject?

2. Please reflow for 75 characters per line.
From what I see, none of the lines (except Subject after change, it's 76 :) ), exceeds 75 characters limit. Could you specify what you meant, please?

3. Your question: How much longer does this take?
Answer: It shouldn't take any longer actually as we do not do the reset and rebuild anymore. It should be a bit faster but it's hard to say by how much as it depends on hardware type, present configuration (how many VFs are spawned), etc.
Should I put it somewhere in the commit message?

4. Please, provide information how to use the script you mentioned and what it does?

Regards,
Staikov Andrii

Am 25.10.22 um 09:27 schrieb Andrii Staikov:
> From: Sylwester Dziedziuch <sylwesterx.dziedziuch@intel.com>

In the summary/title, maybe use:

… rebuild fails …

> When attaching XDP program on i40e driver there was a reset and 
> rebuild of the interface to reconfigure the queues for XDP operation. 
> If one of the steps of rebuild failed then the interface was left in 
> incorrect state that could lead to a crash. If rebuild failed while 
> getting capabilities from HW such crash occurs:

Please reflow for 75 characters per line.

> capability discovery failed, err I40E_ERR_ADMIN_QUEUE_TIMEOUT aq_err 
> OK
> BUG: unable to handle kernel NULL pointer dereference at 
> 0000000000000000 Call Trace:
> ? i40e_reconfig_rss_queues+0x120/0x120 [i40e]
>    dev_xdp_install+0x70/0x100
>    dev_xdp_attach+0x1d7/0x530
>    dev_change_xdp_fd+0x1f4/0x230
>    do_setlink+0x45f/0xf30
>    ? irq_work_interrupt+0xa/0x20
>    ? __nla_validate_parse+0x12d/0x1a0
>    rtnl_setlink+0xb5/0x120
>    rtnetlink_rcv_msg+0x2b1/0x360
>    ? sock_has_perm+0x80/0xa0
>    ? rtnl_calcit.isra.42+0x120/0x120
>    netlink_rcv_skb+0x4c/0x120
>    netlink_unicast+0x196/0x230
>    netlink_sendmsg+0x204/0x3d0
>    sock_sendmsg+0x4c/0x50
>    __sys_sendto+0xee/0x160
>    ? handle_mm_fault+0xc1/0x1e0
>    ? syscall_trace_enter+0x1fb/0x2c0
>    ? __sys_setsockopt+0xd6/0x1d0
>    __x64_sys_sendto+0x24/0x30
>    do_syscall_64+0x5b/0x1a0
>    entry_SYSCALL_64_after_hwframe+0x65/0xca
>    RIP: 0033:0x7f3535d99781
> 
> Fix this by removing reset and rebuild from i40e_xdp_setup and replace 
> it by interface down, reconfigure queues and interface up. This way if 
> any step fails the interface will remain in a correct state.

How much longer does this take?

Can you give a simple XDP to reproduce this?

> Fixes: 0c8493d90b6b ("i40e: add XDP support for pass and drop 
> actions")
> Title: i40e: Fix crash when rebuild failed in i40e_xdp_setup
> Change-type: DefectResolution

The two tags above are not useful for the upstream Linux kernel, right?

> Signed-off-by: Sylwester Dziedziuch <sylwesterx.dziedziuch@intel.com>
> Signed-off-by: Andrii Staikov <andrii.staikov@intel.com>
> ---
>   drivers/net/ethernet/intel/i40e/i40e_main.c | 155 ++++++++++++++------
>   1 file changed, 112 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c 
> b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index 4b3b6e5b612d..68b343a7b77e 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -50,6 +50,8 @@ static int i40e_veb_get_bw_info(struct i40e_veb *veb);
>   static int i40e_get_capabilities(struct i40e_pf *pf,
>   				 enum i40e_admin_queue_opc list_type);
>   static bool i40e_is_total_port_shutdown_enabled(struct i40e_pf *pf);
> +static struct i40e_vsi *i40e_vsi_reinit_setup(struct i40e_vsi *vsi,
> +					      bool is_xdp);
>   
>   /* i40e_pci_tbl - PCI Device ID Table
>    *
> @@ -3563,11 +3565,17 @@ static int i40e_configure_rx_ring(struct i40e_ring *ring)
>   	/* clear the context structure first */
>   	memset(&rx_ctx, 0, sizeof(rx_ctx));
>   
> -	if (ring->vsi->type == I40E_VSI_MAIN)
> -		xdp_rxq_info_unreg_mem_model(&ring->xdp_rxq);
> +	if (ring->vsi->type == I40E_VSI_MAIN) {
> +		if (!xdp_rxq_info_is_reg(&ring->xdp_rxq))
> +			xdp_rxq_info_reg(&ring->xdp_rxq, ring->netdev,
> +					 ring->queue_index,
> +					 ring->q_vector->napi.napi_id);
> +	}
>   
>   	ring->xsk_pool = i40e_xsk_pool(ring);
>   	if (ring->xsk_pool) {
> +		xdp_rxq_info_unreg_mem_model(&ring->xdp_rxq);
> +
>   		ring->rx_buf_len =
>   		  xsk_pool_get_rx_frame_size(ring->xsk_pool);
>   		/* For AF_XDP ZC, we disallow packets to span on @@ -13269,6 
> +13277,42 @@ static netdev_features_t i40e_features_check(struct sk_buff *skb,
>   	return features & ~(NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK);
>   }
>   
> +/**
> + * i40e_vsi_assign_bpf_prog - set or clear bpf prog pointer on VSI
> + * @vsi: VSI to changed
> + * @prog: XDP program
> + **/
> +static struct bpf_prog *i40e_vsi_assign_bpf_prog(struct i40e_vsi *vsi,
> +						 struct bpf_prog *prog)
> +{
> +	struct bpf_prog *old_prog;
> +	int i;

Please check, if `unsigned int` can be used for loop variables. (You can check with `scripts/bloat-o-meter`.


Kind regards,

Paul


> +
> +	old_prog = xchg(&vsi->xdp_prog, prog);
> +
> +	for (i = 0; i < vsi->num_queue_pairs; i++)
> +		WRITE_ONCE(vsi->rx_rings[i]->xdp_prog, vsi->xdp_prog);
> +
> +	if (old_prog)
> +		bpf_prog_put(old_prog);
> +
> +	return old_prog;
> +}
> +
> +/**
> + * i40e_vsi_rx_napi_schedule - Schedule napi on RX queues from VSI
> + * @vsi: VSI to schedule napi on
> + */
> +static void i40e_vsi_rx_napi_schedule(struct i40e_vsi *vsi)
> +{
> +	int i;
> +
> +	for (i = 0; i < vsi->num_queue_pairs; i++)
> +		if (vsi->xdp_rings[i]->xsk_pool)
> +			(void)i40e_xsk_wakeup(vsi->netdev, i,
> +					      XDP_WAKEUP_RX);
> +}
> +
>   /**
>    * i40e_xdp_setup - add/remove an XDP program
>    * @vsi: VSI to changed
> @@ -13279,10 +13323,11 @@ static int i40e_xdp_setup(struct i40e_vsi *vsi, struct bpf_prog *prog,
>   			  struct netlink_ext_ack *extack)
>   {
>   	int frame_size = vsi->netdev->mtu + ETH_HLEN + ETH_FCS_LEN + VLAN_HLEN;
> +	bool is_xdp_enabled = i40e_enabled_xdp_vsi(vsi);
> +	bool if_running = netif_running(vsi->netdev);
>   	struct i40e_pf *pf = vsi->back;
>   	struct bpf_prog *old_prog;
> -	bool need_reset;
> -	int i;
> +	int ret = 0;
>   
>   	/* Don't allow frames that span over multiple buffers */
>   	if (frame_size > i40e_calculate_vsi_rx_buf_len(vsi)) {
> @@ -13290,49 +13335,69 @@ static int i40e_xdp_setup(struct i40e_vsi *vsi, struct bpf_prog *prog,
>   		return -EINVAL;
>   	}
>   
> -	/* When turning XDP on->off/off->on we reset and rebuild the rings. */
> -	need_reset = (i40e_enabled_xdp_vsi(vsi) != !!prog);
> -
> -	if (need_reset)
> -		i40e_prep_for_reset(pf);
> -
>   	/* VSI shall be deleted in a moment, just return EINVAL */
>   	if (test_bit(__I40E_IN_REMOVE, pf->state))
>   		return -EINVAL;
>   
> -	old_prog = xchg(&vsi->xdp_prog, prog);
> +	if (if_running && !test_and_set_bit(__I40E_VSI_DOWN, vsi->state))
> +		i40e_down(vsi);
>   
> -	if (need_reset) {
> -		if (!prog)
> -			/* Wait until ndo_xsk_wakeup completes. */
> -			synchronize_rcu();
> -		i40e_reset_and_rebuild(pf, true, true);
> +	old_prog = i40e_vsi_assign_bpf_prog(vsi, prog);
> +
> +	vsi = i40e_vsi_reinit_setup(vsi, true);
> +
> +	if (!vsi) {
> +		NL_SET_ERR_MSG_MOD(extack, "Failed to reinitialize VSI during XDP setup");
> +		goto err_vsi_setup;
>   	}
>   
> -	if (!i40e_enabled_xdp_vsi(vsi) && prog) {
> -		if (i40e_realloc_rx_bi_zc(vsi, true))
> -			return -ENOMEM;
> -	} else if (i40e_enabled_xdp_vsi(vsi) && !prog) {
> -		if (i40e_realloc_rx_bi_zc(vsi, false))
> -			return -ENOMEM;
> +	/* allocate descriptors */
> +	ret = i40e_vsi_setup_tx_resources(vsi);
> +	if (ret) {
> +		NL_SET_ERR_MSG_MOD(extack, "Failed to configure TX resources during XDP setup");
> +		goto err_setup_tx;
> +	}
> +	ret = i40e_vsi_setup_rx_resources(vsi);
> +	if (ret) {
> +		NL_SET_ERR_MSG_MOD(extack, "Failed to configure RX resources during XDP setup");
> +		goto err_setup_rx;
>   	}
>   
> -	for (i = 0; i < vsi->num_queue_pairs; i++)
> -		WRITE_ONCE(vsi->rx_rings[i]->xdp_prog, vsi->xdp_prog);
> +	if (!is_xdp_enabled && prog)
> +		ret = i40e_realloc_rx_bi_zc(vsi, true);
> +	else if (is_xdp_enabled && !prog)
> +		ret = i40e_realloc_rx_bi_zc(vsi, false);
>   
> -	if (old_prog)
> -		bpf_prog_put(old_prog);
> +	if (ret) {
> +		NL_SET_ERR_MSG_MOD(extack, "Failed to reallocate RX resources during XDP setup");
> +		goto err_realloc;
> +	}
>   
> -	/* Kick start the NAPI context if there is an AF_XDP socket open
> -	 * on that queue id. This so that receiving will start.
> -	 */
> -	if (need_reset && prog)
> -		for (i = 0; i < vsi->num_queue_pairs; i++)
> -			if (vsi->xdp_rings[i]->xsk_pool)
> -				(void)i40e_xsk_wakeup(vsi->netdev, i,
> -						      XDP_WAKEUP_RX);
> +	if (if_running)
> +		ret = i40e_up(vsi);
>   
> -	return 0;
> +	if (ret) {
> +		NL_SET_ERR_MSG_MOD(extack, "Failed to open VSI during XDP setup");
> +		goto err_realloc;
> +	} else if (prog) {
> +		/* Kick start the NAPI context if there is an AF_XDP socket open
> +		 * on that queue id. This so that receiving will start.
> +		 */
> +		i40e_vsi_rx_napi_schedule(vsi);
> +	}
> +
> +	return ret;
> +
> +err_setup_rx:
> +	i40e_vsi_free_rx_resources(vsi);
> +err_setup_tx:
> +	i40e_vsi_free_tx_resources(vsi);
> +err_vsi_setup:
> +	if (vsi == pf->vsi[pf->lan_vsi])
> +		i40e_do_reset(pf, I40E_PF_RESET_FLAG, true);
> +err_realloc:
> +	i40e_vsi_assign_bpf_prog(vsi, old_prog);
> +	return ret;
>   }
>   
>   /**
> @@ -14274,13 +14339,14 @@ static int i40e_vsi_setup_vectors(struct i40e_vsi *vsi)
>   /**
>    * i40e_vsi_reinit_setup - return and reallocate resources for a VSI
>    * @vsi: pointer to the vsi.
> + * @is_xdp: flag indicating if this is reinit during XDP setup
>    *
>    * This re-allocates a vsi's queue resources.
>    *
>    * Returns pointer to the successfully allocated and configured VSI sw struct
>    * on success, otherwise returns NULL on failure.
>    **/
> -static struct i40e_vsi *i40e_vsi_reinit_setup(struct i40e_vsi *vsi)
> +static struct i40e_vsi *i40e_vsi_reinit_setup(struct i40e_vsi *vsi, bool is_xdp)
>   {
>   	u16 alloc_queue_pairs;
>   	struct i40e_pf *pf;
> @@ -14316,12 +14382,14 @@ static struct i40e_vsi *i40e_vsi_reinit_setup(struct i40e_vsi *vsi)
>   	/* Update the FW view of the VSI. Force a reset of TC and queue
>   	 * layout configurations.
>   	 */
> -	enabled_tc = pf->vsi[pf->lan_vsi]->tc_config.enabled_tc;
> -	pf->vsi[pf->lan_vsi]->tc_config.enabled_tc = 0;
> -	pf->vsi[pf->lan_vsi]->seid = pf->main_vsi_seid;
> -	i40e_vsi_config_tc(pf->vsi[pf->lan_vsi], enabled_tc);
> -	if (vsi->type == I40E_VSI_MAIN)
> -		i40e_rm_default_mac_filter(vsi, pf->hw.mac.perm_addr);
> +	if (!is_xdp) {
> +		enabled_tc = pf->vsi[pf->lan_vsi]->tc_config.enabled_tc;
> +		pf->vsi[pf->lan_vsi]->tc_config.enabled_tc = 0;
> +		pf->vsi[pf->lan_vsi]->seid = pf->main_vsi_seid;
> +		i40e_vsi_config_tc(pf->vsi[pf->lan_vsi], enabled_tc);
> +		if (vsi->type == I40E_VSI_MAIN)
> +			i40e_rm_default_mac_filter(vsi, pf->hw.mac.perm_addr);
> +	}
>   
>   	/* assign it some queues */
>   	ret = i40e_alloc_rings(vsi);
> @@ -15087,7 +15155,8 @@ static int i40e_setup_pf_switch(struct i40e_pf *pf, bool reinit, bool lock_acqui
>   		if (pf->lan_vsi == I40E_NO_VSI)
>   			vsi = i40e_vsi_setup(pf, I40E_VSI_MAIN, uplink_seid, 0);
>   		else if (reinit)
> -			vsi = i40e_vsi_reinit_setup(pf->vsi[pf->lan_vsi]);
> +			vsi = i40e_vsi_reinit_setup(pf->vsi[pf->lan_vsi],
> +						    false);
>   		if (!vsi) {
>   			dev_info(&pf->pdev->dev, "setup of MAIN VSI failed\n");
>   			i40e_cloud_filter_exit(pf);
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net v1] i40e: Fix crash when rebuild failed in i40e_xdp_setup

[Alexander Lobakin]

From: Andrii Staikov <andrii.staikov@intel.com>
Date: Tue, 25 Oct 2022 09:27:05 +0200

> From: Sylwester Dziedziuch <sylwesterx.dziedziuch@intel.com>
> 
> When attaching XDP program on i40e driver there was a
> reset and rebuild of the interface to reconfigure the
> queues for XDP operation. If one of the steps of rebuild
> failed then the interface was left in incorrect state
> that could lead to a crash. If rebuild failed while
> getting capabilities from HW such crash occurs:
> 
> capability discovery failed, err I40E_ERR_ADMIN_QUEUE_TIMEOUT aq_err OK
> BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
> Call Trace:
> ? i40e_reconfig_rss_queues+0x120/0x120 [i40e]
>   dev_xdp_install+0x70/0x100
>   dev_xdp_attach+0x1d7/0x530
>   dev_change_xdp_fd+0x1f4/0x230
>   do_setlink+0x45f/0xf30
>   ? irq_work_interrupt+0xa/0x20
>   ? __nla_validate_parse+0x12d/0x1a0
>   rtnl_setlink+0xb5/0x120
>   rtnetlink_rcv_msg+0x2b1/0x360
>   ? sock_has_perm+0x80/0xa0
>   ? rtnl_calcit.isra.42+0x120/0x120
>   netlink_rcv_skb+0x4c/0x120
>   netlink_unicast+0x196/0x230
>   netlink_sendmsg+0x204/0x3d0
>   sock_sendmsg+0x4c/0x50
>   __sys_sendto+0xee/0x160
>   ? handle_mm_fault+0xc1/0x1e0
>   ? syscall_trace_enter+0x1fb/0x2c0
>   ? __sys_setsockopt+0xd6/0x1d0
>   __x64_sys_sendto+0x24/0x30
>   do_syscall_64+0x5b/0x1a0
>   entry_SYSCALL_64_after_hwframe+0x65/0xca
>   RIP: 0033:0x7f3535d99781
> 
> Fix this by removing reset and rebuild from i40e_xdp_setup
> and replace it by interface down, reconfigure queues and
> interface up. This way if any step fails the interface
> will remain in a correct state.
> 
> Fixes: 0c8493d90b6b ("i40e: add XDP support for pass and drop actions")
> Title: i40e: Fix crash when rebuild failed in i40e_xdp_setup
> Change-type: DefectResolution
> Signed-off-by: Sylwester Dziedziuch <sylwesterx.dziedziuch@intel.com>
> Signed-off-by: Andrii Staikov <andrii.staikov@intel.com>
> ---
>  drivers/net/ethernet/intel/i40e/i40e_main.c | 155 ++++++++++++++------
>  1 file changed, 112 insertions(+), 43 deletions(-)

[...]

-	old_prog = xchg(&vsi->xdp_prog, prog);
+	if (if_running && !test_and_set_bit(__I40E_VSI_DOWN, vsi->state))
+		i40e_down(vsi);

Please don't. Unconditional down-up was removed from i40e, we've
made XDP hotswap in ice in our recent patch queue and now you're
trying to effectively revert all that ._.
You don't need any interface actions when there is a prog and you
want to install a new one as well, just RCU the pointers and that's
it.

[...]

> -- 
> 2.25.1

Thanks,
Olek
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net v1] i40e: Fix crash when rebuild failed in i40e_xdp_setup

[Staikov, Andrii]

>From: Andrii Staikov <andrii.staikov@intel.com>
>Date: Tue, 25 Oct 2022 09:27:05 +0200
>
>> From: Sylwester Dziedziuch <sylwesterx.dziedziuch@intel.com>
>> 
>> When attaching XDP program on i40e driver there was a reset and 
>> rebuild of the interface to reconfigure the queues for XDP operation. 
>> If one of the steps of rebuild failed then the interface was left in 
>> incorrect state that could lead to a crash. If rebuild failed while 
>> getting capabilities from HW such crash occurs:
>> 
>> capability discovery failed, err I40E_ERR_ADMIN_QUEUE_TIMEOUT aq_err 
>> OK
>> BUG: unable to handle kernel NULL pointer dereference at 
>> 0000000000000000 Call Trace:
>> ? i40e_reconfig_rss_queues+0x120/0x120 [i40e]
>>   dev_xdp_install+0x70/0x100
>>   dev_xdp_attach+0x1d7/0x530
>>   dev_change_xdp_fd+0x1f4/0x230
>>   do_setlink+0x45f/0xf30
>>   ? irq_work_interrupt+0xa/0x20
>>   ? __nla_validate_parse+0x12d/0x1a0
>>   rtnl_setlink+0xb5/0x120
>>   rtnetlink_rcv_msg+0x2b1/0x360
>>   ? sock_has_perm+0x80/0xa0
>>   ? rtnl_calcit.isra.42+0x120/0x120
>>   netlink_rcv_skb+0x4c/0x120
>>   netlink_unicast+0x196/0x230
>>   netlink_sendmsg+0x204/0x3d0
>>   sock_sendmsg+0x4c/0x50
>>   __sys_sendto+0xee/0x160
>>   ? handle_mm_fault+0xc1/0x1e0
>>   ? syscall_trace_enter+0x1fb/0x2c0
>>   ? __sys_setsockopt+0xd6/0x1d0
>>   __x64_sys_sendto+0x24/0x30
>>   do_syscall_64+0x5b/0x1a0
>>   entry_SYSCALL_64_after_hwframe+0x65/0xca
>>   RIP: 0033:0x7f3535d99781
>> 
>> Fix this by removing reset and rebuild from i40e_xdp_setup and replace 
>> it by interface down, reconfigure queues and interface up. This way if 
>> any step fails the interface will remain in a correct state.
>> 
>> Fixes: 0c8493d90b6b ("i40e: add XDP support for pass and drop 
>> actions")
>> Title: i40e: Fix crash when rebuild failed in i40e_xdp_setup
>> Change-type: DefectResolution
>> Signed-off-by: Sylwester Dziedziuch <sylwesterx.dziedziuch@intel.com>
>> Signed-off-by: Andrii Staikov <andrii.staikov@intel.com>
>> ---
>>  drivers/net/ethernet/intel/i40e/i40e_main.c | 155 
>> ++++++++++++++------
>>  1 file changed, 112 insertions(+), 43 deletions(-)
>
>[...]
>
>-	old_prog = xchg(&vsi->xdp_prog, prog);
>+	if (if_running && !test_and_set_bit(__I40E_VSI_DOWN, vsi->state))
>+		i40e_down(vsi);
>
>Please don't. Unconditional down-up was removed from i40e, we've made XDP hotswap in ice in our recent patch queue and now you're trying to effectively revert all that ._.
>You don't need any interface actions when there is a prog and you want to install a new one as well, just RCU the pointers and that's it.
>

Can you please elaborate on this. I am not an author of this commit so it's hard for me to grasp what you are talking about.
Are you suggestion just leaving old_prog = xchg(&vsi->xdp_prog, prog); instread of the part that was added?

>[...]
>
>> --
>> 2.25.1
>
>Thanks,
>Olek
>

Regards,
Staikov Andrii
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net v1] i40e: Fix crash when rebuild failed in i40e_xdp_setup

[Paul Menzel]

Dear Andrii,


Am 26.10.22 um 16:19 schrieb Staikov, Andrii:

> Thank you for the response on the path.
> I have a few questions about it

Thank you for your response.

> -----Original Message-----
> From: Paul Menzel <pmenzel@molgen.mpg.de>
> Sent: Tuesday, October 25, 2022 10:41 AM
> To: Staikov, Andrii <andrii.staikov@intel.com>; Dziedziuch, SylwesterX <sylwesterx.dziedziuch@intel.com>
> Cc: intel-wired-lan@lists.osuosl.org
> Subject: Re: [Intel-wired-lan] [PATCH net v1] i40e: Fix crash when rebuild failed in i40e_xdp_setup
> 
> Dear Andrii, dear Sylwester,
> 
> 
> Thank you for the patch.
> I have a few questions to your response:
> 
> 1. In the summary/title, maybe use: … rebuild fails …
> Do you mean in Subject?

Yes, when the patch is sent as an email, it’s the subject there, 
including the list and other tags ([Intel-wired-lan] [PATCH net v1]).

> 2. Please reflow for 75 characters per line.
>  From what I see, none of the lines (except Subject after change, it's 76 :) ), exceeds 75 characters limit. Could you specify what you meant, please?

The lines in the body are below 60 characters, making it too narrow in 
my opinion. Reflowing for 75 characters also saves a line.

> 3. Your question: How much longer does this take?
> Answer: It shouldn't take any longer actually as we do not do the
> reset and rebuild anymore. It should be a bit faster but it's hard to
> say by how much as it depends on hardware type, present configuration
> (how many VFs are spawned), etc. Should I put it somewhere in the
> commit message?

Thank you for the explanation, I should have seen that myself. A small 
note might be nice, but I guess it’s not necessary.

> 4. Please, provide information how to use the script you mentioned and what it does?
You build the object file once with `int i`, save it somewhere, and then 
with `unsigned i`, and provide both files to the script.


Kind regards,

Paul


PS: Is it possible for you to use an email client, which is able to 
quote properly? (I know, your company email account probably makes this 
hard.)
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net v1] i40e: Fix crash when rebuild failed in i40e_xdp_setup

[Alexander Lobakin]

> From: "Staikov, Andrii" <andrii.staikov@intel.com>
> Date: Mon, 31 Oct 2022 08:30:59 +0000
> 
> >From: Andrii Staikov <andrii.staikov@intel.com>
> >Date: Tue, 25 Oct 2022 09:27:05 +0200
> >
> >> From: Sylwester Dziedziuch <sylwesterx.dziedziuch@intel.com>

[...]

> >> Fixes: 0c8493d90b6b ("i40e: add XDP support for pass and drop 
> >> actions")
> >> Title: i40e: Fix crash when rebuild failed in i40e_xdp_setup
> >> Change-type: DefectResolution

Those two tags shouldn't be here BTW, only Fixes and SoBs.

> >> Signed-off-by: Sylwester Dziedziuch <sylwesterx.dziedziuch@intel.com>
> >> Signed-off-by: Andrii Staikov <andrii.staikov@intel.com>
> >> ---
> >>  drivers/net/ethernet/intel/i40e/i40e_main.c | 155 
> >> ++++++++++++++------
> >>  1 file changed, 112 insertions(+), 43 deletions(-)
> >
> >[...]
> >
> >-	old_prog = xchg(&vsi->xdp_prog, prog);
> >+	if (if_running && !test_and_set_bit(__I40E_VSI_DOWN, vsi->state))
> >+		i40e_down(vsi);
> >
> >Please don't. Unconditional down-up was removed from i40e, we've made XDP hotswap in ice in our recent patch queue and now you're trying to effectively revert all that ._.
> >You don't need any interface actions when there is a prog and you want to install a new one as well, just RCU the pointers and that's it.
> >
> 
> Can you please elaborate on this. I am not an author of this commit so it's hard for me to grasp what you are talking about.
> Are you suggestion just leaving old_prog = xchg(&vsi->xdp_prog, prog); instread of the part that was added?

if (!!prog == !!old_prog), you shouldn't do any
resets/reallocatiions/etc, please leave it just how it was before
(when they were being done conditionally on (need_reset)).
Otherwise, you'll kill prog hotswapping by trying to fix this splat,
which would be a major regression.

> 
> >[...]
> >
> >> --
> >> 2.25.1
> >
> >Thanks,
> >Olek
> >
> 
> Regards,
> Staikov Andrii

Thanks,
Olek
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net v1] i40e: Fix crash when rebuild failed in i40e_xdp_setup

[Staikov, Andrii]

>> From: "Staikov, Andrii" <andrii.staikov@intel.com>
>> Date: Mon, 31 Oct 2022 08:30:59 +0000
>> 
>> >From: Andrii Staikov <andrii.staikov@intel.com>
>> >Date: Tue, 25 Oct 2022 09:27:05 +0200
>> >
>> >> From: Sylwester Dziedziuch <sylwesterx.dziedziuch@intel.com>
>
>[...]
>
>> >> Fixes: 0c8493d90b6b ("i40e: add XDP support for pass and drop
>> >> actions")
>> >> Title: i40e: Fix crash when rebuild failed in i40e_xdp_setup
>> >> Change-type: DefectResolution
>
>Those two tags shouldn't be here BTW, only Fixes and SoBs.
>
>> >> Signed-off-by: Sylwester Dziedziuch 
>> >> <sylwesterx.dziedziuch@intel.com>
>> >> Signed-off-by: Andrii Staikov <andrii.staikov@intel.com>
>> >> ---
>> >>  drivers/net/ethernet/intel/i40e/i40e_main.c | 155
>> >> ++++++++++++++------
>> >>  1 file changed, 112 insertions(+), 43 deletions(-)
>> >
>> >[...]
>> >
>> >-	old_prog = xchg(&vsi->xdp_prog, prog);
>> >+	if (if_running && !test_and_set_bit(__I40E_VSI_DOWN, vsi->state))
>> >+		i40e_down(vsi);
>> >
>> >Please don't. Unconditional down-up was removed from i40e, we've made XDP hotswap in ice in our recent patch queue and now you're trying to effectively revert all that ._.
>> >You don't need any interface actions when there is a prog and you want to install a new one as well, just RCU the pointers and that's it.
>> >
>> 
>> Can you please elaborate on this. I am not an author of this commit so it's hard for me to grasp what you are talking about.
>> Are you suggestion just leaving old_prog = xchg(&vsi->xdp_prog, prog); instread of the part that was added?
>
>if (!!prog == !!old_prog), you shouldn't do any resets/reallocatiions/etc, please leave it just how it was before (when they were being done conditionally on (need_reset)).
>Otherwise, you'll kill prog hotswapping by trying to fix this splat, which would be a major regression.

Sorry, but it's still not clear for me what changes you are asking for.
In your email you are refering to this part only:
-	old_prog = xchg(&vsi->xdp_prog, prog);
+	if (if_running && !test_and_set_bit(__I40E_VSI_DOWN, vsi->state))
+		i40e_down(vsi);

Are you requesting reverting changes of the whole i40e_xdp_setup function or some specific ones related to (need_reset)?

>> 
>> >[...]
>> >
>> >> --
>> >> 2.25.1
>> >
>> >Thanks,
>> >Olek
>> >
>> 
>> Regards,
>> Staikov Andrii
>
>Thanks,
>Olek
>

Regards,
Staikov Andrii
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net v1] i40e: Fix crash when rebuild failed in i40e_xdp_setup

[Alexander Lobakin]

From: "Staikov, Andrii" <andrii.staikov@intel.com>
Date: Wed, 2 Nov 2022 14:04:43 +0000

> >> From: "Staikov, Andrii" <andrii.staikov@intel.com>
> >> Date: Mon, 31 Oct 2022 08:30:59 +0000
> >> 
> >> >From: Andrii Staikov <andrii.staikov@intel.com>
> >> >Date: Tue, 25 Oct 2022 09:27:05 +0200
> >> >
> >> >> From: Sylwester Dziedziuch <sylwesterx.dziedziuch@intel.com>
> >
> >[...]
> >
> >> >> Fixes: 0c8493d90b6b ("i40e: add XDP support for pass and drop
> >> >> actions")
> >> >> Title: i40e: Fix crash when rebuild failed in i40e_xdp_setup
> >> >> Change-type: DefectResolution
> >
> >Those two tags shouldn't be here BTW, only Fixes and SoBs.
> >
> >> >> Signed-off-by: Sylwester Dziedziuch 
> >> >> <sylwesterx.dziedziuch@intel.com>
> >> >> Signed-off-by: Andrii Staikov <andrii.staikov@intel.com>
> >> >> ---
> >> >>  drivers/net/ethernet/intel/i40e/i40e_main.c | 155
> >> >> ++++++++++++++------
> >> >>  1 file changed, 112 insertions(+), 43 deletions(-)
> >> >
> >> >[...]
> >> >
> >> >-	old_prog = xchg(&vsi->xdp_prog, prog);
> >> >+	if (if_running && !test_and_set_bit(__I40E_VSI_DOWN, vsi->state))
> >> >+		i40e_down(vsi);
> >> >
> >> >Please don't. Unconditional down-up was removed from i40e, we've made XDP hotswap in ice in our recent patch queue and now you're trying to effectively revert all that ._.
> >> >You don't need any interface actions when there is a prog and you want to install a new one as well, just RCU the pointers and that's it.
> >> >
> >> 
> >> Can you please elaborate on this. I am not an author of this commit so it's hard for me to grasp what you are talking about.
> >> Are you suggestion just leaving old_prog = xchg(&vsi->xdp_prog, prog); instread of the part that was added?
> >
> >if (!!prog == !!old_prog), you shouldn't do any resets/reallocatiions/etc, please leave it just how it was before (when they were being done conditionally on (need_reset)).
> >Otherwise, you'll kill prog hotswapping by trying to fix this splat, which would be a major regression.
> 
> Sorry, but it's still not clear for me what changes you are asking for.
> In your email you are refering to this part only:
> -	old_prog = xchg(&vsi->xdp_prog, prog);
> +	if (if_running && !test_and_set_bit(__I40E_VSI_DOWN, vsi->state))
> +		i40e_down(vsi);
> 
> Are you requesting reverting changes of the whole i40e_xdp_setup function or some specific ones related to (need_reset)?

It's a reference to that you now perform resets unconditionally, not
to a particular piece of code. I guess the best and quickest way for
you is to contact the author and let him fix this and publish to the
upstream MLs.

> 
> >> 
> >> >[...]
> >> >
> >> >> --
> >> >> 2.25.1
> >> >
> >> >Thanks,
> >> >Olek
> >> >
> >> 
> >> Regards,
> >> Staikov Andrii
> >
> >Thanks,
> >Olek
> >
> 
> Regards,
> Staikov Andrii

Thanks,
Olek
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net v1] i40e: Fix crash when rebuild failed in i40e_xdp_setup

[Staikov, Andrii]

>From: "Staikov, Andrii" <andrii.staikov@intel.com>
>Date: Wed, 2 Nov 2022 14:04:43 +0000
>
>> >> From: "Staikov, Andrii" <andrii.staikov@intel.com>
>> >> Date: Mon, 31 Oct 2022 08:30:59 +0000
>> >> 
>> >> >From: Andrii Staikov <andrii.staikov@intel.com>
>> >> >Date: Tue, 25 Oct 2022 09:27:05 +0200
>> >> >
>> >> >> From: Sylwester Dziedziuch <sylwesterx.dziedziuch@intel.com>
>> >
>> >[...]
>> >
>> >> >> Fixes: 0c8493d90b6b ("i40e: add XDP support for pass and drop
>> >> >> actions")
>> >> >> Title: i40e: Fix crash when rebuild failed in i40e_xdp_setup
>> >> >> Change-type: DefectResolution
>> >
>> >Those two tags shouldn't be here BTW, only Fixes and SoBs.
>> >
>> >> >> Signed-off-by: Sylwester Dziedziuch 
>> >> >> <sylwesterx.dziedziuch@intel.com>
>> >> >> Signed-off-by: Andrii Staikov <andrii.staikov@intel.com>
>> >> >> ---
>> >> >>  drivers/net/ethernet/intel/i40e/i40e_main.c | 155
>> >> >> ++++++++++++++------
>> >> >>  1 file changed, 112 insertions(+), 43 deletions(-)
>> >> >
>> >> >[...]
>> >> >
>> >> >-	old_prog = xchg(&vsi->xdp_prog, prog);
>> >> >+	if (if_running && !test_and_set_bit(__I40E_VSI_DOWN, vsi->state))
>> >> >+		i40e_down(vsi);
>> >> >
>> >> >Please don't. Unconditional down-up was removed from i40e, we've made XDP hotswap in ice in our recent patch queue and now you're trying to effectively revert all that ._.
>> >> >You don't need any interface actions when there is a prog and you want to install a new one as well, just RCU the pointers and that's it.
>> >> >
>> >> 
>> >> Can you please elaborate on this. I am not an author of this commit so it's hard for me to grasp what you are talking about.
>> >> Are you suggestion just leaving old_prog = xchg(&vsi->xdp_prog, prog); instread of the part that was added?
>> >
>> >if (!!prog == !!old_prog), you shouldn't do any resets/reallocatiions/etc, please leave it just how it was before (when they were being done conditionally on (need_reset)).
>> >Otherwise, you'll kill prog hotswapping by trying to fix this splat, which would be a major regression.
>> 
>> Sorry, but it's still not clear for me what changes you are asking for.
>> In your email you are refering to this part only:
>> -	old_prog = xchg(&vsi->xdp_prog, prog);
>> +	if (if_running && !test_and_set_bit(__I40E_VSI_DOWN, vsi->state))
>> +		i40e_down(vsi);
>> 
>> Are you requesting reverting changes of the whole i40e_xdp_setup function or some specific ones related to (need_reset)?
>
>It's a reference to that you now perform resets unconditionally, not to a particular piece of code. I guess the best and quickest way for you is to contact the author and let him fix this and publish to the upstream MLs.

Unfortunately, there is no possibility to contact the author of the commit because he is not working for Intel anymore. In addition, it seems to me that some of the changes in i40e_xdp_setup() function indeed fix the issue. The commit is mostly targeted to this function. However, I don't know how it may correspond with the changes you are requiring to make, so it's hard for me to grasp what should stay and what should be removed.

>> 
>> >> 
>> >> >[...]
>> >> >
>> >> >> --
>> >> >> 2.25.1
>> >> >
>> >> >Thanks,
>> >> >Olek
>> >> >
>> >> 
>> >> Regards,
>> >> Staikov Andrii
>> >
>> >Thanks,
>> >Olek
>> >
>> 
>> Regards,
>> Staikov Andrii
>
>Thanks,
>Olek
>

Regards,
Staikov Andrii
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan