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:   "Ertman, David M" <david.m.ertman@intel.com>
To:     ivecera <ivecera@redhat.com>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>
CC:     poros <poros@redhat.com>, mschmidt <mschmidt@redhat.com>,
        Leon Romanovsky <leonro@nvidia.com>,
        "Brandeburg, Jesse" <jesse.brandeburg@intel.com>,
        "Nguyen, Anthony L" <anthony.l.nguyen@intel.com>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        "Saleem, Shiraz" <shiraz.saleem@intel.com>,
        "moderated list:INTEL ETHERNET DRIVERS" 
        <intel-wired-lan@lists.osuosl.org>,
        open list <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH net v4] ice: Fix race during aux device (un)plugging
Thread-Topic: [PATCH net v4] ice: Fix race during aux device (un)plugging
Thread-Index: AQHYVvvS7+rYQkRlfEGFSsJ/CUUOEa0AyVKw
Date:   Mon, 25 Apr 2022 15:58:55 +0000
Message-ID: <MW5PR11MB5811DF21387703272C67E9B6DDF89@MW5PR11MB5811.namprd11.prod.outlook.com>
References: <20220423102021.4056946-1-ivecera@redhat.com>
In-Reply-To: <20220423102021.4056946-1-ivecera@redhat.com>
Accept-Language: en-US
Content-Language: en-US
dlp-product: dlpe-windows
dlp-reaction: no-action
dlp-version: 11.6.401.20
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?KOgFc0W7mZD1i6tsp+3BhN8Af1ue9BJv155u/CHLfLV1/7j5LjiIJuwU57rJ?=
 =?us-ascii?Q?qc+YxALh6tmk65vlSNJGbaQsyCydtofTSUnWZaRY5acBnmJwdB2Gh63lv/yQ?=
 =?us-ascii?Q?DIPhrlKEapLJ36v9SSSfMoxu2IuD7/dK/nQ7p+V4yQlc5t2g28siN0fxsInZ?=
 =?us-ascii?Q?SScCIUzkm1V12SwezzFQG5bZb7+1aWPV9R6EYFytsL9FIazApS/WiyCLPT8+?=
 =?us-ascii?Q?sE3CzgfC/9hkA4Jd1DRRviSRaEHvKxVZT4kB5LBjZw6tfi11DpgxJfQVHDDR?=
 =?us-ascii?Q?2vqjcMKEoI1Em69sI4yXrCRlFjXl8Rb9jfHwl2E3GPDW/7rr4wjWPut+Rbbt?=
 =?us-ascii?Q?3qdO2NgQrTKId+XHZww5a/O4IXBQ1ALDLK1/mZ+nwSOvVPt/MYmIdLI8EUWw?=
 =?us-ascii?Q?kqyKZi59dijNm5xCcxV4sFMPDSdasDVJpOcjZ1hp2+ooU5dOoWx1kancGLk9?=
 =?us-ascii?Q?KSuhNocPrwuWoE3RH/PXiJNGvP854awll/PMZQnQYXh5c/VOH6IOr0j6SwR6?=
 =?us-ascii?Q?/hikqIMi5J0q56D+sOravnDyMAz0icI9obSdOqLj0R64/mCEEyqIpBb8xJhA?=
 =?us-ascii?Q?Daf803RYLXVosTMyFnC4/FbBpEWCKNBDLqODNXEFklhDYS2WqsbHO2hkd/BS?=
 =?us-ascii?Q?RaNbra1hNisUk9/z+3b/TzDmWZDCh++kG8qf6b1y9ydCm+yz55ICNCW+Nr8n?=
 =?us-ascii?Q?Iw0Gds8qufMv71L2IiJP+Cj/+Kzt2/S1fVNruPtgUT25qbsl0i+aRTGJu3xO?=
 =?us-ascii?Q?xnLpnYV0uRVheKiYerNX7naQGXLsPu8ssD0zWx53mOmRF83HU1cTvdkGS37T?=
 =?us-ascii?Q?vD/cPfasQygOkegi2lGKssGub9mTPJwt5T9DFRQwTm3C4vFH0rheK4RAy5Bp?=
 =?us-ascii?Q?CBI+iDBIz6yM7kwhHWs6/JEI4ngOpqqGdeJwurdLasmJngiHGJR2O3CuViwa?=
 =?us-ascii?Q?xX7gwIQKhZO95x/AWpJaqgdwIMr96UZvAKU2J+zD927240LSUliRjOM3ZVYz?=
 =?us-ascii?Q?dMGGTFiMopTo2R3SwsxNoGLBcj+ZOXluQGWQDQq5/syW+b0eQd7cOPocukMi?=
 =?us-ascii?Q?heADE0Hk9die9VHmZLpppAuydva4SDdgeNcuE1sEkVv5cj9stNe9HXTeL18i?=
 =?us-ascii?Q?giTrlHD31/RN7OMgQEAQYt/kq1/8Or8qzpzrkCtc0ORFIJRQanRRbgNyqc9j?=
 =?us-ascii?Q?QpM026t7MkTG2iEVbLfO0r4i968wXqbREG5nxYgoYIn58PX8Rf3qFjxAt9Yn?=
 =?us-ascii?Q?i7qhl78BXTqF5rz0mogGNDHCJU8RxkhNuHaAr8eVU2/RnhI9MZ4aZPnamt6Y?=
 =?us-ascii?Q?afh7S/lbkM4/YHTxiRbKF5rC8QpTgP0FVOigtN6UTYeUzvQPnLM8QOkpMQHz?=
 =?us-ascii?Q?dIvx44pZbBGXRoe78bNZ5Um+vO58n0KKx6HT2JpLhuehjfW6JovvjtIbly0M?=
 =?us-ascii?Q?0OYvYbE40XPnlkj5l3Iql0xS/JeFZWSc7s/h/HBwdNiyPAqLxobfu/n/Yyaz?=
 =?us-ascii?Q?VF1CiWWOQILxjIIwVpS48hb/dVxjSXiGPq0VNCLSbP+ZoxGpUvOQBnMnvP0/?=
 =?us-ascii?Q?uq0pJzcHrFoehb8D9tjz64zltjhGFvn3tA8MUoBUn3DCZM7tQiimR4IrlC5r?=
 =?us-ascii?Q?vykBNrHkwbGdwkPWOBDMC7dGeGX/ux4LB5zkPpE4BwXL2bQLb+OtwOBDAhpL?=
 =?us-ascii?Q?I942+HIF+oPDO62ZxKJawhzlAj15RPqoF1Ji8D2dKYUBVpLzFz9TbgXHn2Js?=
 =?us-ascii?Q?ALwk8nY6/hYVDx1joiKiznFROzV37uE=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: MW5PR11MB5811.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 760af7dd-718f-406d-43e3-08da26d4a259
X-MS-Exchange-CrossTenant-originalarrivaltime: 25 Apr 2022 15:58:55.2821
 (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: NEOH3s47cslzKrY6Je14crAm2ifkn0pEZ01R794GblICqPDjpfAo4S06mpqPMwg3SOUdK9G0X3580OoC7PDr1mPlq4E1T6xc4EbRVSSpblE=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY4PR11MB0037
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-5.0 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 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: Ivan Vecera <ivecera@redhat.com>
> Sent: Saturday, April 23, 2022 3:20 AM
> To: netdev@vger.kernel.org
> Cc: poros <poros@redhat.com>; mschmidt <mschmidt@redhat.com>; Leon
> Romanovsky <leonro@nvidia.com>; Brandeburg, Jesse
> <jesse.brandeburg@intel.com>; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>; David S. Miller <davem@davemloft.net>;
> Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>;
> Saleem, Shiraz <shiraz.saleem@intel.com>; Ertman, David M
> <david.m.ertman@intel.com>; moderated list:INTEL ETHERNET DRIVERS
> <intel-wired-lan@lists.osuosl.org>; open list <linux-kernel@vger.kernel.o=
rg>
> Subject: [PATCH net v4] ice: Fix race during aux device (un)plugging
>=20
> Function ice_plug_aux_dev() assigns pf->adev field too early prior
> aux device initialization and on other side ice_unplug_aux_dev()
> starts aux device deinit and at the end assigns NULL to pf->adev.
> This is wrong because pf->adev should always be non-NULL only when
> aux device is fully initialized and ready. This wrong order causes
> a crash when ice_send_event_to_aux() call occurs because that function
> depends on non-NULL value of pf->adev and does not assume that
> aux device is half-initialized or half-destroyed.
> After order correction the race window is tiny but it is still there,
> as Leon mentioned and manipulation with pf->adev needs to be protected
> by mutex.
>=20
> Fix (un-)plugging functions so pf->adev field is set after aux device
> init and prior aux device destroy and protect pf->adev assignment by
> new mutex. This mutex is also held during ice_send_event_to_aux()
> call to ensure that aux device is valid during that call.
> Note that device lock used ice_send_event_to_aux() needs to be kept
> to avoid race with aux drv unload.
>=20
> Reproducer:
> cycle=3D1
> while :;do
>         echo "#### Cycle: $cycle"
>=20
>         ip link set ens7f0 mtu 9000
>         ip link add bond0 type bond mode 1 miimon 100
>         ip link set bond0 up
>         ifenslave bond0 ens7f0
>         ip link set bond0 mtu 9000
>         ethtool -L ens7f0 combined 1
>         ip link del bond0
>         ip link set ens7f0 mtu 1500
>         sleep 1
>=20
>         let cycle++
> done
>=20
> In short when the device is added/removed to/from bond the aux device
> is unplugged/plugged. When MTU of the device is changed an event is
> sent to aux device asynchronously. This can race with (un)plugging
> operation and because pf->adev is set too early (plug) or too late
> (unplug) the function ice_send_event_to_aux() can touch uninitialized
> or destroyed fields. In the case of crash below pf->adev->dev.mutex.

--SNIP--

> Fixes: f9f5301e7e2d ("ice: Register auxiliary device to provide RDMA")
> Reviewed-by: Leon Romanovsky <leonro@nvidia.com>
> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
> ---
>  drivers/net/ethernet/intel/ice/ice.h      |  1 +
>  drivers/net/ethernet/intel/ice/ice_idc.c  | 25 +++++++++++++++--------
>  drivers/net/ethernet/intel/ice/ice_main.c |  2 ++
>  3 files changed, 20 insertions(+), 8 deletions(-)
>=20
> diff --git a/drivers/net/ethernet/intel/ice/ice.h
> b/drivers/net/ethernet/intel/ice/ice.h
> index 8ed3c9ab7ff7..a895e3a8e988 100644
> --- a/drivers/net/ethernet/intel/ice/ice.h
> +++ b/drivers/net/ethernet/intel/ice/ice.h
> @@ -540,6 +540,7 @@ struct ice_pf {
>  	struct mutex avail_q_mutex;	/* protects access to avail_[rx|tx]qs
> */
>  	struct mutex sw_mutex;		/* lock for protecting VSI alloc
> flow */
>  	struct mutex tc_mutex;		/* lock to protect TC changes
> */
> +	struct mutex adev_mutex;	/* lock to protect aux device access
> */
>  	u32 msg_enable;
>  	struct ice_ptp ptp;
>  	struct tty_driver *ice_gnss_tty_driver;
> diff --git a/drivers/net/ethernet/intel/ice/ice_idc.c
> b/drivers/net/ethernet/intel/ice/ice_idc.c
> index 25a436d342c2..3e3b2ed4cd5d 100644
> --- a/drivers/net/ethernet/intel/ice/ice_idc.c
> +++ b/drivers/net/ethernet/intel/ice/ice_idc.c
> @@ -37,14 +37,17 @@ void ice_send_event_to_aux(struct ice_pf *pf, struct
> iidc_event *event)
>  	if (WARN_ON_ONCE(!in_task()))
>  		return;
>=20
> +	mutex_lock(&pf->adev_mutex);
>  	if (!pf->adev)
> -		return;
> +		goto finish;
>=20
>  	device_lock(&pf->adev->dev);
>  	iadrv =3D ice_get_auxiliary_drv(pf);
>  	if (iadrv && iadrv->event_handler)
>  		iadrv->event_handler(pf, event);
>  	device_unlock(&pf->adev->dev);
> +finish:
> +	mutex_unlock(&pf->adev_mutex);
>  }
>=20
>  /**
> @@ -290,7 +293,6 @@ int ice_plug_aux_dev(struct ice_pf *pf)
>  		return -ENOMEM;
>=20
>  	adev =3D &iadev->adev;
> -	pf->adev =3D adev;
>  	iadev->pf =3D pf;
>=20
>  	adev->id =3D pf->aux_idx;
> @@ -300,18 +302,20 @@ int ice_plug_aux_dev(struct ice_pf *pf)
>=20
>  	ret =3D auxiliary_device_init(adev);
>  	if (ret) {
> -		pf->adev =3D NULL;
>  		kfree(iadev);
>  		return ret;
>  	}
>=20
>  	ret =3D auxiliary_device_add(adev);
>  	if (ret) {
> -		pf->adev =3D NULL;
>  		auxiliary_device_uninit(adev);
>  		return ret;
>  	}
>=20
> +	mutex_lock(&pf->adev_mutex);
> +	pf->adev =3D adev;
> +	mutex_unlock(&pf->adev_mutex);
> +
>  	return 0;
>  }
>=20
> @@ -320,12 +324,17 @@ int ice_plug_aux_dev(struct ice_pf *pf)
>   */
>  void ice_unplug_aux_dev(struct ice_pf *pf)
>  {
> -	if (!pf->adev)
> -		return;
> +	struct auxiliary_device *adev;
>=20
> -	auxiliary_device_delete(pf->adev);
> -	auxiliary_device_uninit(pf->adev);
> +	mutex_lock(&pf->adev_mutex);
> +	adev =3D pf->adev;
>  	pf->adev =3D NULL;
> +	mutex_unlock(&pf->adev_mutex);
> +
> +	if (adev) {
> +		auxiliary_device_delete(adev);
> +		auxiliary_device_uninit(adev);
> +	}
>  }
>=20
>  /**
> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c
> b/drivers/net/ethernet/intel/ice/ice_main.c
> index 5b1198859da7..2cbbf7abefc4 100644
> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> @@ -3769,6 +3769,7 @@ u16 ice_get_avail_rxq_count(struct ice_pf *pf)
>  static void ice_deinit_pf(struct ice_pf *pf)
>  {
>  	ice_service_task_stop(pf);
> +	mutex_destroy(&pf->adev_mutex);
>  	mutex_destroy(&pf->sw_mutex);
>  	mutex_destroy(&pf->tc_mutex);
>  	mutex_destroy(&pf->avail_q_mutex);
> @@ -3847,6 +3848,7 @@ static int ice_init_pf(struct ice_pf *pf)
>=20
>  	mutex_init(&pf->sw_mutex);
>  	mutex_init(&pf->tc_mutex);
> +	mutex_init(&pf->adev_mutex);
>=20
>  	INIT_HLIST_HEAD(&pf->aq_wait_list);
>  	spin_lock_init(&pf->aq_wait_lock);
> --
> 2.35.1

Reviewed-by: Dave Ertman <david.m.ertman@intel.com>