netdev - Re: [PATCH net 1/7] net: hns3: fix add VLAN fail issue

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6603e0480feea2e7a28a865705da52bb99679a35.camel@redhat.com>
Date: Thu, 02 Nov 2023 11:31:22 +0100
From: Paolo Abeni <pabeni@...hat.com>
To: Jijie Shao <shaojijie@...wei.com>, yisen.zhuang@...wei.com, 
 salil.mehta@...wei.com, davem@...emloft.net, edumazet@...gle.com,
 kuba@...nel.org
Cc: shenjian15@...wei.com, wangjie125@...wei.com, liuyonglong@...wei.com, 
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net 1/7] net: hns3: fix add VLAN fail issue

On Sat, 2023-10-28 at 10:59 +0800, Jijie Shao wrote:
> From: Jian Shen <shenjian15@...wei.com>
> 
> The hclge_sync_vlan_filter is called in periodic task,
> trying to remove VLAN from vlan_del_fail_bmap. It can
> be concurrence with VLAN adding operation from user.
> So once user failed to delete a VLAN id, and add it
> again soon, it may be removed by the periodic task,
> which may cause the software configuration being
> inconsistent with hardware. So add mutex handling
> to avoid this.
> 
>      user                        hns3 driver
> 
>                                            periodic task
>                                                 │
>   add vlan 10 ───── hns3_vlan_rx_add_vid        │
>        │             (suppose success)          │
>        │                                        │
>   del vlan 10 ─────  hns3_vlan_rx_kill_vid      │
>        │           (suppose fail,add to         │
>        │             vlan_del_fail_bmap)        │
>        │                                        │
>   add vlan 10 ───── hns3_vlan_rx_add_vid        │
>                      (suppose success)          │
>                                        foreach vlan_del_fail_bmp
>                                             del vlan 10
> 
> Fixes: fe4144d47eef ("net: hns3: sync VLAN filter entries when kill VLAN ID failed")
> Signed-off-by: Jian Shen <shenjian15@...wei.com>
> Signed-off-by: Jijie Shao <shaojijie@...wei.com>
> ---
>  .../hisilicon/hns3/hns3pf/hclge_main.c        | 21 +++++++++++++------
>  .../hisilicon/hns3/hns3vf/hclgevf_main.c      | 11 ++++++++--
>  2 files changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> index c42574e29747..a3230ac928a9 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> @@ -10026,8 +10026,6 @@ static void hclge_rm_vport_vlan_table(struct hclge_vport *vport, u16 vlan_id,
>  	struct hclge_vport_vlan_cfg *vlan, *tmp;
>  	struct hclge_dev *hdev = vport->back;
>  
> -	mutex_lock(&hdev->vport_lock);
> -
>  	list_for_each_entry_safe(vlan, tmp, &vport->vlan_list, node) {
>  		if (vlan->vlan_id == vlan_id) {
>  			if (is_write_tbl && vlan->hd_tbl_status)
> @@ -10042,8 +10040,6 @@ static void hclge_rm_vport_vlan_table(struct hclge_vport *vport, u16 vlan_id,
>  			break;
>  		}
>  	}
> -
> -	mutex_unlock(&hdev->vport_lock);
>  }
>  
>  void hclge_rm_vport_all_vlan_table(struct hclge_vport *vport, bool is_del_list)
> @@ -10452,11 +10448,16 @@ int hclge_set_vlan_filter(struct hnae3_handle *handle, __be16 proto,
>  	 * handle mailbox. Just record the vlan id, and remove it after
>  	 * reset finished.
>  	 */
> +	mutex_lock(&hdev->vport_lock);
>  	if ((test_bit(HCLGE_STATE_RST_HANDLING, &hdev->state) ||
>  	     test_bit(HCLGE_STATE_RST_FAIL, &hdev->state)) && is_kill) {
>  		set_bit(vlan_id, vport->vlan_del_fail_bmap);
> +		mutex_unlock(&hdev->vport_lock);
>  		return -EBUSY;
> +	} else if (!is_kill && test_bit(vlan_id, vport->vlan_del_fail_bmap)) {
> +		clear_bit(vlan_id, vport->vlan_del_fail_bmap);
>  	}
> +	mutex_unlock(&hdev->vport_lock);
>  
>  	/* when port base vlan enabled, we use port base vlan as the vlan
>  	 * filter entry. In this case, we don't update vlan filter table
> @@ -10481,7 +10482,9 @@ int hclge_set_vlan_filter(struct hnae3_handle *handle, __be16 proto,
>  		 * and try to remove it from hw later, to be consistence
>  		 * with stack
>  		 */
> +		mutex_lock(&hdev->vport_lock);
>  		set_bit(vlan_id, vport->vlan_del_fail_bmap);
> +		mutex_unlock(&hdev->vport_lock);

It looks like that the 'hclge_rm_vport_vlan_table()' call a few lines
above will now happen with the vport_lock unlocked.

That looks racy and would deserve at least a comment explaining why
it's safe.

Thanks,

Paolo