Received-SPF: pass (google.com: domain of linux-kernel+bounces-153526-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Mon, 22 Apr 2024 16:18:56 +0200
From: Jiri Pirko <jiri@resnulli.us>
To: Jijie Shao <shaojijie@huawei.com>
Cc: yisen.zhuang@huawei.com, salil.mehta@huawei.com, davem@davemloft.net,
	edumazet@google.com, kuba@kernel.org, pabeni@redhat.com,
	shenjian15@huawei.com, wangjie125@huawei.com,
	liuyonglong@huawei.com, chenhao418@huawei.com,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH net 7/7] net: hns3: fix kernel crash when devlink reload
 during vf initialization
Message-ID: <ZiZx0E5NlSL7nuLE@nanopsycho>
References: <20240422134327.3160587-1-shaojijie@huawei.com>
 <20240422134327.3160587-8-shaojijie@huawei.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240422134327.3160587-8-shaojijie@huawei.com>

Mon, Apr 22, 2024 at 03:43:27PM CEST, shaojijie@huawei.com wrote:
>From: Yonglong Liu <liuyonglong@huawei.com>
>
>The devlink reload process will access the hardware resources,
>but the register operation is done before the hardware is initialized.
>So, processing the devlink reload during initialization may lead to kernel
>crash. This patch fixes this by taking devl_lock during initialization.
>
>Fixes: cd6242991d2e ("net: hns3: add support for registering devlink for VF")
>Signed-off-by: Yonglong Liu <liuyonglong@huawei.com>
>Signed-off-by: Jijie Shao <shaojijie@huawei.com>
>---
> drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
>diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
>index 08db8e84be4e..3ee41943d15f 100644
>--- a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
>+++ b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
>@@ -2849,6 +2849,8 @@ static int hclgevf_init_hdev(struct hclgevf_dev *hdev)
> 	if (ret)
> 		goto err_devlink_init;
> 

Hmm, what if reload happens here? I think that you don't fix the issue,
rather just narrowing the race window.

Why don't you rather calle devlink_register at the end of this function?


Also, parallel to this patch, why don't you register devlink port of
flavour "virtual" for this VF?


>+	devl_lock(hdev->devlink);
>+
> 	ret = hclge_comm_cmd_queue_init(hdev->pdev, &hdev->hw.hw);
> 	if (ret)
> 		goto err_cmd_queue_init;
>@@ -2950,6 +2952,7 @@ static int hclgevf_init_hdev(struct hclgevf_dev *hdev)
> 	hclgevf_task_schedule(hdev, round_jiffies_relative(HZ));
> 	timer_setup(&hdev->reset_timer, hclgevf_reset_timer, 0);
> 
>+	devl_unlock(hdev->devlink);
> 	return 0;
> 
> err_config:
>@@ -2960,6 +2963,7 @@ static int hclgevf_init_hdev(struct hclgevf_dev *hdev)
> err_cmd_init:
> 	hclge_comm_cmd_uninit(hdev->ae_dev, &hdev->hw.hw);
> err_cmd_queue_init:
>+	devl_unlock(hdev->devlink);
> 	hclgevf_devlink_uninit(hdev);
> err_devlink_init:
> 	hclgevf_pci_uninit(hdev);
>-- 
>2.30.0
>
>