Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   "Jankowski, Konrad0" <konrad0.jankowski@intel.com>
To:     Lin Ma <linma@zju.edu.cn>,
        "Brandeburg, Jesse" <jesse.brandeburg@intel.com>,
        "Nguyen, Anthony L" <anthony.l.nguyen@intel.com>,
        "davem@davemloft.net" <davem@davemloft.net>,
        "edumazet@google.com" <edumazet@google.com>,
        "kuba@kernel.org" <kuba@kernel.org>,
        "pabeni@redhat.com" <pabeni@redhat.com>,
        "ast@kernel.org" <ast@kernel.org>,
        "daniel@iogearbox.net" <daniel@iogearbox.net>,
        "hawk@kernel.org" <hawk@kernel.org>,
        "john.fastabend@gmail.com" <john.fastabend@gmail.com>,
        "intel-wired-lan@lists.osuosl.org" <intel-wired-lan@lists.osuosl.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "bpf@vger.kernel.org" <bpf@vger.kernel.org>
Subject: RE: [Intel-wired-lan] [PATCH v3] igb: Add lock to avoid data race
Thread-Topic: [Intel-wired-lan] [PATCH v3] igb: Add lock to avoid data race
Thread-Index: AQHYq8LJ/nnQsBEYNUqoCgAWEQWOLa2xX9pg
Date:   Tue, 16 Aug 2022 10:35:05 +0000
Message-ID: <DM8PR11MB5621A0C7FD7E20ABE4187137AB6B9@DM8PR11MB5621.namprd11.prod.outlook.com>
References: <20220809073542.3390-1-linma@zju.edu.cn>
In-Reply-To: <20220809073542.3390-1-linma@zju.edu.cn>
Accept-Language: pl-PL, en-US
Content-Language: en-US
dlp-version: 11.6.500.17
dlp-product: dlpe-windows
dlp-reaction: no-action
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?jwCsWB7ZA7ASu54f3XZ8UHwAIWGATtebGROWiDYrKBcfenvnO0mV/WMLdSDj?=
 =?us-ascii?Q?ixxXOs4zMi6QAyrVd6HIFVvfdWZlcAhjNS4V43maJiqfzXb08Sev6j0GbePk?=
 =?us-ascii?Q?7rQyxYzoZvYN+gmkvGb5wQJBLIqX5fR0DTv10jVkQS4nv6b3OjJQTWLNknhz?=
 =?us-ascii?Q?WcHv2Xx5fLu7rLFV0l3qGrWYPcrlFtTHGLb4V/Bi61Rg/C2jxntmW8OLvODX?=
 =?us-ascii?Q?uWM3ZZbisXVx0LzSYQunRSdBOzc9NfimsGM93CiPhyKdOFdP37fc5QUgeaSz?=
 =?us-ascii?Q?COZhZizy+ffr5qqg8hYQRmOAHeIMbtXDIMX2DnTPZ1OO207f7lJ2B2PcyKnK?=
 =?us-ascii?Q?pxYW6A2o6f9OI8zy6bhhnw/NSLa/HcoK0q7T8PYBrC2hihkvyJnt15sfy2pf?=
 =?us-ascii?Q?xmaHoFixZjAPbi6AJ+/d6x/6aiMnxZQMROudgYZnsYm7zXqxbk2cbNWhO0sy?=
 =?us-ascii?Q?SFhYISJ0pjahzEnJ1b87lIt5httbGwMVap33RzA+pSjti0FnR++RaZYXiqzo?=
 =?us-ascii?Q?lWVDcUm1NaeHDpKgZRQPdj2AofderaB0R4iR6axu3nZWaLHpoFVzEBbcmOH+?=
 =?us-ascii?Q?24JmwAJbrlIHk5L2bYXingApCcjbaGHey1fIeejVS8LvkTG62rD8pgwVJRQk?=
 =?us-ascii?Q?Y7oHd6e9LpmPI3t5CwID+j6AUhGNZ/DtE+zoNA18BC/c3X1nUpfItJrG51z/?=
 =?us-ascii?Q?Zv+UoMUARMHRSI3yF0MNubCqs58mExXw2JHbPbkGbGDCpyyf2LJraflzmPL4?=
 =?us-ascii?Q?Atu+ewXADir6AhLYZbTwYSG2vaVxfQvdY/95zLS8dmYIYt22L6GRvygADdqE?=
 =?us-ascii?Q?xh0UFozXJUMYAGsGdecuFV3e8MSLwZ/wzH/n1dpyq4iFILAztxEg0ZyMGFdh?=
 =?us-ascii?Q?zMpgggYtXhWVlD8LGux2TqfZjkArLTX6cfvIKoak8xYiGB6z/NlN2tRd+sPE?=
 =?us-ascii?Q?wATt1UOVF7RCrposob478wshZOkf9jMF/vjM/6aGl3h6hp76VKmtOw3zJ+nL?=
 =?us-ascii?Q?vmRRySJAuGY4Fl39nXOHKRyvoepyINchDiz9NDNEj1Lv4w0qxAGY8GP3W9vL?=
 =?us-ascii?Q?rxbo79wgJOF9HQhBVDsK7AEsQYrNCIZvBlV+Mrba1hfkMTZXfJZwPIoA+x1D?=
 =?us-ascii?Q?VH0K0aETVfcCH8L+Wf73RFbN9FJ/hzBNlCWDF8Ah4lvykPZQx/aglXQbTG8+?=
 =?us-ascii?Q?rsPciVi/Vlj6UvOlQlJrh6bpNI+nJFHrJohQu5i0o7Vb2nan/J7/mQAQyNSi?=
 =?us-ascii?Q?Psn4TWkWZLfAbKiA+e0PgaOE5/hdp0I1iKSRdgUF4e797D6l7vRiRtp8vtFV?=
 =?us-ascii?Q?n/KpaaYwYkAuAWkc+7jGL+ET/E1hBsx7ajBJvtc/KZTxj68cioztJHuKEeE1?=
 =?us-ascii?Q?hT6tTVobig8fHAZnuJuCQoOXw80BLbQgs+4xC/ZaCfM3bL9l/yM3wkOSdZcr?=
 =?us-ascii?Q?ZAzmps3wgHbcggTsa8yOZfmEhSAsGuMm+ZHq5rDXsY/DA60OsETlKM2wG5m3?=
 =?us-ascii?Q?bhH8yg+Uzy5BYmE4SdU1FRmr1TGdJsHJUtlrYvLhAWmurX/hrzzsMDVjB0hn?=
 =?us-ascii?Q?rpDiG/Sbaw+PMD8W7SzjhuWvpRxoQCivCCeYxnvwuO1oWIz5qOY0h6fxtjHH?=
 =?us-ascii?Q?xg=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: DM8PR11MB5621.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 845c022c-64dc-4aa7-38fa-08da7f72fbb9
X-MS-Exchange-CrossTenant-originalarrivaltime: 16 Aug 2022 10:35:05.1940
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: QgGR+iVvekl7JkGx4U8QvgbNNQxi65WQqYo8qhVf8Ld4m4pc0s5ZKlBQWQG4MA5j7hVyqunXMcWP9BBPBRX5Q/m7s5PwWTOdmu7Ju3WdhMY=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN9PR11MB5545
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Lin Ma
> Sent: Tuesday, August 9, 2022 9:36 AM
> To: Brandeburg, Jesse <jesse.brandeburg@intel.com>; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>; davem@davemloft.net;
> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
> ast@kernel.org; daniel@iogearbox.net; hawk@kernel.org;
> john.fastabend@gmail.com; intel-wired-lan@lists.osuosl.org;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> bpf@vger.kernel.org
> Cc: Lin Ma <linma@zju.edu.cn>
> Subject: [Intel-wired-lan] [PATCH v3] igb: Add lock to avoid data race
>=20
> The commit c23d92b80e0b ("igb: Teardown SR-IOV before
> unregister_netdev()") places the unregister_netdev() call after the
> igb_disable_sriov() call to avoid functionality issue.
>=20
> However, it introduces several race conditions when detaching a device.
> For example, when .remove() is called, the below interleaving leads to us=
e-
> after-free.
>=20
>  (FREE from device detaching)      |   (USE from netdev core)
> igb_remove                         |  igb_ndo_get_vf_config
>  igb_disable_sriov                 |  vf >=3D adapter->vfs_allocated_coun=
t?
>   kfree(adapter->vf_data)          |
>   adapter->vfs_allocated_count =3D 0 |
>                                    |    memcpy(... adapter->vf_data[vf]
>=20
> Moreover, the igb_disable_sriov() also suffers from data race with the
> requests from VF driver.
>=20
>  (FREE from device detaching)      |   (USE from requests)
> igb_remove                         |  igb_msix_other
>  igb_disable_sriov                 |   igb_msg_task
>   kfree(adapter->vf_data)          |    vf < adapter->vfs_allocated_count
>   adapter->vfs_allocated_count =3D 0 |
>=20
> To this end, this commit first eliminates the data races from netdev core=
 by
> using rtnl_lock (similar to commit 719479230893 ("dpaa2-eth: add MAC/PHY
> support through phylink")). And then adds a spinlock to eliminate races f=
rom
> driver requests. (similar to commit 1e53834ce541
> ("ixgbe: Add locking to prevent panic when setting sriov_numvfs to zero")
>=20
>=20
> Fixes: c23d92b80e0b ("igb: Teardown SR-IOV before unregister_netdev()")
> Signed-off-by: Lin Ma <linma@zju.edu.cn>
> ---
> V2 -> V3:  make the commit message much clear
> V1 -> V2:  fix typo in title idb -> igb
> V0 -> V1:  change title from "Add rtnl_lock" to "Add lock"
>            add additional spinlock as suggested by Jakub, according to
>            1e53834ce541 ("ixgbe: Add locking to prevent panic when settin=
g
>            sriov_numvfs to zero")
>=20
>  drivers/net/ethernet/intel/igb/igb.h      |  2 ++
>  drivers/net/ethernet/intel/igb/igb_main.c | 12 +++++++++++-
>  2 files changed, 13 insertions(+), 1 deletion(-)
>=20
> diff --git a/drivers/net/ethernet/intel/igb/igb.h
> b/drivers/net/ethernet/intel/igb/igb.h
> index 2d3daf022651..015b78144114 100644
> --- a/drivers/net/ethernet/intel/igb/igb.h
> +++ b/drivers/net/ethernet/intel/igb/igb.h
> @@ -664,6 +664,8 @@ struct igb_adapter {

Tested-by: Konrad Jankowski <konrad0.jankowski@intel.com>