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:   "Laba, SlawomirX" <slawomirx.laba@intel.com>
To:     ivecera <ivecera@redhat.com>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>
CC:     "Keller, Jacob E" <jacob.e.keller@intel.com>,
        "Piotrowski, Patryk" <patryk.piotrowski@intel.com>,
        Vitaly Grinberg <vgrinber@redhat.com>,
        "Brandeburg, Jesse" <jesse.brandeburg@intel.com>,
        "Nguyen, Anthony L" <anthony.l.nguyen@intel.com>,
        "David S. Miller" <davem@davemloft.net>,
        "Eric Dumazet" <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Jeff Kirsher <jeffrey.t.kirsher@intel.com>,
        "moderated list:INTEL ETHERNET DRIVERS" 
        <intel-wired-lan@lists.osuosl.org>,
        open list <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH net v2] iavf: Detach device during reset task
Thread-Topic: [PATCH net v2] iavf: Detach device during reset task
Thread-Index: AQHYvEjhCJvnRrVNjEyX8LmyQxwL263H6JFw
Date:   Tue, 30 Aug 2022 20:49:54 +0000
Message-ID: <DM6PR11MB31134AD7D5D1CB5382A5052887799@DM6PR11MB3113.namprd11.prod.outlook.com>
References: <20220830081627.1205872-1-ivecera@redhat.com>
In-Reply-To: <20220830081627.1205872-1-ivecera@redhat.com>
Accept-Language: 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?Z5YR1I1Ve6bFDYYEkptn0BgHbIhpqj81znq0/yHqo81BywcLMdy6YFkjGi/D?=
 =?us-ascii?Q?JzCqnnZi1cqp6q7+fOaVHrKH/9qgXOIhtWwNYcSQiC3mWKEK/UulO8ilfL9D?=
 =?us-ascii?Q?kKml7MjUcyNbrukHFUTYFt7FAa8T5t5LrGXcgzp/pYqpMOjKm+b8Z7cdEbQE?=
 =?us-ascii?Q?aBDcvDK9kqF9ZrsLej9rBhDak3NYvEVSrwvvaqtroD49Y0nrQv5r0dK5+Onf?=
 =?us-ascii?Q?FYTl/nWSNjk4ZLeN/cVc889aRfn5+55kPNyp9LfgQDsEaik//rlISVkvy71P?=
 =?us-ascii?Q?LfdB9C4/kcRpivVa136hHoqJe5/A4UBqm543uDnKZyxHDsLXhKl9H95eUgv2?=
 =?us-ascii?Q?wWvVfvANOkROqMW1mEyyd84yi3qdfBbY+Ix8TglDQ7xlZr5ZSnUd8StuhBfw?=
 =?us-ascii?Q?bb1gTV3oD8ThXWZ4+RysJMFOjrwpvEZ0BjQk5IGJhFEfengA412fWEBUttvW?=
 =?us-ascii?Q?UQJ4ny3zQ5vvYynAvcCNrqcxjKtZV+99n2+xNl/HULDvXsOsTL50JjwzwfSf?=
 =?us-ascii?Q?dZ48WyoP9zYNmt5jw7uxY9u4PQafMV92T8MZA7RHCKqduQFuqxxnYcEYJYfL?=
 =?us-ascii?Q?OezCkt9vNVMFuL1SzMApVLI5CV7wpCYO+1D8ifVuJDscTu5NTcKJmKF/lpu9?=
 =?us-ascii?Q?VfhShFabsAvIbhyC3OBLxs7oxkSVXvI3xmcJObKbJ3b6r+TwEgIuDtlNnYfo?=
 =?us-ascii?Q?wEkDpqJyJgo++tGJ3+AdJMzI6LxKf8nT/FnLxEmsr/in2Y53W0m5vI5VzvSL?=
 =?us-ascii?Q?jMB2oZ7MZaplj1kSXBykKJUt83QWdOkCmqKnc3wowtt2xJs7P3rSrhQ2O8hr?=
 =?us-ascii?Q?qQGL5RQgM7UY41qxK+3+BGcs1NaIf7QBKNfYbhZ+AIQxsINo8LMdpPTiPsfq?=
 =?us-ascii?Q?h6JSS5GGWvcHCFLYTY3NB+O50SxQOlod4QKAGJLtnol1dSmBVnKeSmZMNGoS?=
 =?us-ascii?Q?b7lvc/oHGvK6r27zh7b8CwfRizGmIeO5uIMy6plaALWuat/yypm8Tt8Ltp3t?=
 =?us-ascii?Q?WOq9mc1WxQRgfRlmmplz5FVuEgqkrnRGzlr/5fLbG96uJWh82tO4CNMQtFU5?=
 =?us-ascii?Q?R9IKTM3+WEWjoV/44j+KnhozVXcLPjO8g2B0z/59rvK1gxBMmJvUzkslRwjj?=
 =?us-ascii?Q?oCxQHVGqJJfHHzhT99PE2A/duCpmOQf2oaiaGiJDgjs/espJdNV8AQMU0th6?=
 =?us-ascii?Q?gSqwJ5/haRuQCgD0740jKqfS85DJUExrb7hh7tfIY3ldYEKaxWW4QJyiZqd/?=
 =?us-ascii?Q?a9SBo+2zoYF1cTBQiANojhz9/jGYyPaT9he1IGWr+PetTbhw7EBhNmKeoKBy?=
 =?us-ascii?Q?7oGmbb/3E9mx3A97BD3wWVdMhs8gLtcG3BnOF2hJVwjHnBiboqqcv4ri5bvX?=
 =?us-ascii?Q?PsUcyBYgKtY2me6yR7DTeBdnxtE3RG92xKCe3H36VC6I1NeKdB0fSt5xWbay?=
 =?us-ascii?Q?Rl3SuMv2TeNmja8FQBsbxoXQ9FVhr+ZrOGObO9ekzlzg78HnOX4RMmCaLPHC?=
 =?us-ascii?Q?K/bVoxA/u4UuTH4lhFftd3aPDHph3kHK2vfAA0VKlmXM8vSgMD9esyEvnm0T?=
 =?us-ascii?Q?pl0cKUqTc6kpQFYa4lTW3W4/HSG71UTNtDk2yiQv?=
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: DM6PR11MB3113.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 7c53c1a2-df10-4fc4-fa8d-08da8ac93111
X-MS-Exchange-CrossTenant-originalarrivaltime: 30 Aug 2022 20:49:54.2042
 (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: /kIBcHNJFJMRikp8I3Ir358KQkfCgVmGaNdOGqhhX67z+j9+SvSxdH4Egn7x19VNITIc7HQDOVu2C53kLgn2FO5n2Jul4EViWJUYVtq4CWI=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY4PR11MB1608
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-7.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_HI,
        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: Ivan Vecera <ivecera@redhat.com>
> Sent: Tuesday, August 30, 2022 10:16 AM
> Subject: [PATCH net v2] iavf: Detach device during reset task
>=20
> iavf_reset_task() takes crit_lock at the beginning and holds it during wh=
ole call.
> The function subsequently calls
> iavf_init_interrupt_scheme() that grabs RTNL. Problem occurs when userspa=
ce
> initiates during the reset task any ndo callback that runs under RTNL lik=
e
> iavf_open() because some of that functions tries to take crit_lock. This =
leads to
> classic A-B B-A deadlock scenario.
>=20
> To resolve this situation the device should be detached in
> iavf_reset_task() prior taking crit_lock to avoid subsequent ndos running=
 under
> RTNL and reattach the device at the end.
>=20
> Fixes: 62fe2a865e6d ("i40evf: add missing rtnl_lock() around
> i40evf_set_interrupt_capability")
> Cc: Jacob Keller <jacob.e.keller@intel.com>
> Cc: Patryk Piotrowski <patryk.piotrowski@intel.com>
> Cc: SlawomirX Laba <slawomirx.laba@intel.com>
> Tested-by: Vitaly Grinberg <vgrinber@redhat.com>
> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
>
> @@ -2884,7 +2889,7 @@ static void iavf_reset_task(struct work_struct *wor=
k)
>  		if (adapter->state !=3D __IAVF_REMOVE)
>  			queue_work(iavf_wq, &adapter->reset_task);
>=20
> -		return;
> +		goto reset_finish;
>  	}
>=20
>  	while (!mutex_trylock(&adapter->client_lock))

Ivan, what do you think about this flow [1]? Shouldn't it also goto reset_f=
inish label?

	if (i =3D=3D IAVF_RESET_WAIT_COMPLETE_COUNT) {
		dev_err(&adapter->pdev->dev, "Reset never finished (%x)\n",
			reg_val);
		iavf_disable_vf(adapter);
		mutex_unlock(&adapter->client_lock);
		mutex_unlock(&adapter->crit_lock);
		return; /* Do not attempt to reinit. It's dead, Jim. */
	}

I am concerned that if the reset never finishes and iavf goes into disabled=
 state, and then for example if driver reload operation is performed, bad t=
hings can happen.

[1] https://elixir.bootlin.com/linux/v6.0-rc3/source/drivers/net/ethernet/i=
ntel/iavf/iavf_main.c#L2939