Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Message-ID: <6603e0480feea2e7a28a865705da52bb99679a35.camel@redhat.com>
Subject: Re: [PATCH net 1/7] net: hns3: fix add VLAN fail issue
From:   Paolo Abeni <pabeni@redhat.com>
To:     Jijie Shao <shaojijie@huawei.com>, yisen.zhuang@huawei.com,
        salil.mehta@huawei.com, davem@davemloft.net, edumazet@google.com,
        kuba@kernel.org
Cc:     shenjian15@huawei.com, wangjie125@huawei.com,
        liuyonglong@huawei.com, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Date:   Thu, 02 Nov 2023 11:31:22 +0100
In-Reply-To: <20231028025917.314305-2-shaojijie@huawei.com>
References: <20231028025917.314305-1-shaojijie@huawei.com>
         <20231028025917.314305-2-shaojijie@huawei.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.4 (3.46.4-1.fc37) 
MIME-Version: 1.0
Precedence: bulk

On Sat, 2023-10-28 at 10:59 +0800, Jijie Shao wrote:
> From: Jian Shen <shenjian15@huawei.com>
>=20
> 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.
>=20
>      user                        hns3 driver
>=20
>                                            periodic task
>                                                 =E2=94=82
>   add vlan 10 =E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80 hns3_vlan_rx_=
add_vid        =E2=94=82
>        =E2=94=82             (suppose success)          =E2=94=82
>        =E2=94=82                                        =E2=94=82
>   del vlan 10 =E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80  hns3_vlan_rx=
_kill_vid      =E2=94=82
>        =E2=94=82           (suppose fail,add to         =E2=94=82
>        =E2=94=82             vlan_del_fail_bmap)        =E2=94=82
>        =E2=94=82                                        =E2=94=82
>   add vlan 10 =E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80 hns3_vlan_rx_=
add_vid        =E2=94=82
>                      (suppose success)          =E2=94=82
>                                        foreach vlan_del_fail_bmp
>                                             del vlan 10
>=20
> Fixes: fe4144d47eef ("net: hns3: sync VLAN filter entries when kill VLAN =
ID failed")
> Signed-off-by: Jian Shen <shenjian15@huawei.com>
> Signed-off-by: Jijie Shao <shaojijie@huawei.com>
> ---
>  .../hisilicon/hns3/hns3pf/hclge_main.c        | 21 +++++++++++++------
>  .../hisilicon/hns3/hns3vf/hclgevf_main.c      | 11 ++++++++--
>  2 files changed, 24 insertions(+), 8 deletions(-)
>=20
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c b/dr=
ivers/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 hclg=
e_vport *vport, u16 vlan_id,
>  	struct hclge_vport_vlan_cfg *vlan, *tmp;
>  	struct hclge_dev *hdev =3D vport->back;
> =20
> -	mutex_lock(&hdev->vport_lock);
> -
>  	list_for_each_entry_safe(vlan, tmp, &vport->vlan_list, node) {
>  		if (vlan->vlan_id =3D=3D vlan_id) {
>  			if (is_write_tbl && vlan->hd_tbl_status)
> @@ -10042,8 +10040,6 @@ static void hclge_rm_vport_vlan_table(struct hclg=
e_vport *vport, u16 vlan_id,
>  			break;
>  		}
>  	}
> -
> -	mutex_unlock(&hdev->vport_lock);
>  }
> =20
>  void hclge_rm_vport_all_vlan_table(struct hclge_vport *vport, bool is_de=
l_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);
> =20
>  	/* 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 *ha=
ndle, __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