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>, mschmidt <mschmidt@redhat.com>
CC:     "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        poros <poros@redhat.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] ice: Fix race during aux device (un)plugging
Thread-Topic: [PATCH net] ice: Fix race during aux device (un)plugging
Thread-Index: AQHYUB40uNYOIOS5/U2OD0pQhU2OnKzw0wiAgABNhwCAACCwkA==
Date:   Fri, 15 Apr 2022 17:53:44 +0000
Message-ID: <MW5PR11MB5811D6D5245206C5A37B803DDDEE9@MW5PR11MB5811.namprd11.prod.outlook.com>
References: <20220414163907.1456925-1-ivecera@redhat.com>
        <CADEbmW3eUAnvn4gvNxqjCMmO333-=OdObGhDXkrTbDwn0YkJDw@mail.gmail.com>
 <20220415174932.6c85d5ab@ceranb>
In-Reply-To: <20220415174932.6c85d5ab@ceranb>
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?4KfeuYQjQ3VaKd1Ozv3RBRJ9xS1JwD7BltgE6nNIdbytlzG1HHgrOST7ZZwC?=
 =?us-ascii?Q?Ov5zM8lVq0XM6X6NPMdS0cWMDUqVqXS9cXpKGY58Fj5VKr5D2dZioAtrKZyJ?=
 =?us-ascii?Q?gatTvEoHH5oc+dkCT32wZjb5lRr4OjYy/M3RPHfV5x9MsC9UI7gEELHJx3S6?=
 =?us-ascii?Q?EApNYJQpAcFbjyhX25xZBXoJegxq4uy/onEzTI5yNPR9LrJE3t+/UjbPtbDH?=
 =?us-ascii?Q?xBMmmf5A/iHO5bIeOF0qXJw6bfPGmfUkZWJcm2K+PWu2OlExb0J840w4BFuK?=
 =?us-ascii?Q?3yHEoKTThYL/LUvGksTgnjD85C5k6ez5tpU6Bl3SlAvzSc8jNXTmv0HdEDrh?=
 =?us-ascii?Q?fd1BLcbllooRJ3wot1SyW0COilbmyspE9P19LL4+c8faYmMERfNLLaxSq9FL?=
 =?us-ascii?Q?CBdusb8LgMAeaasVOWBheGv20midnxU3/1Z3su3DeHkO5eR36hHEmADBfta/?=
 =?us-ascii?Q?BgZw+6N6sipGg0Hr92xAy2m+YyUHgPcl6zNFbmmk9iAmudPkR3OWGLEE48N8?=
 =?us-ascii?Q?u3/NH0TOR8Ph4l0bttSels4WHI3jsB6Km5auV0Jf0NN5bSVeHKe5RFxgJD8e?=
 =?us-ascii?Q?aH2oxnNyZvM5F2QaBZ0tsGfADQkVQ2jBVtm2VRzABOAhblCe0OEXLg0Gny7y?=
 =?us-ascii?Q?zHBSP24dayaqOkEofBsn8iHBNFpMzqAUx2agQnSEqAoM5tqwbW6DOWEG1lbV?=
 =?us-ascii?Q?KUpCjeTXOMQ9bMjHE/lvspVfgCyohvIbTYWr+4o/pJDgD/aNDObhi1Vz5lgr?=
 =?us-ascii?Q?OrCVIkCFsjyuDwrtsbavPml7kTcTr3UBJjf81IJofmxkb/yvD6XMveFHDzb2?=
 =?us-ascii?Q?f4a2hrxJFv5ach0zrIi5Doe2XbxpQOfng3RPu9o/kxfhe9vrbz589zM36oki?=
 =?us-ascii?Q?5piEk8RF1vNMVSvYv3H+k/j5HATJti0BPZAuzSWb86SK5Q2/SHwWggxkmvCh?=
 =?us-ascii?Q?cU5HVKb8p9fiSZ9QdmJvp+OZ0zYpLoUe9hz+PsjmpnnEouEKBmaQt/k+0afe?=
 =?us-ascii?Q?OzowT3wMd4pXWMB/6+EOBmDW7jptanjhLGFCikR3KbEX6B8rwsVi0UJGsjsS?=
 =?us-ascii?Q?PstdK1jUPwdAcEDoGVTf2HDAUGYMWCEgmgpVdzvMrhWeXR0L7zBD4+/A6W1O?=
 =?us-ascii?Q?AX5E7Uljbj0y3iaScDV2QK7hkTIRnl+HhceoF1myZqAIPMTzlSVbTW29z7PW?=
 =?us-ascii?Q?pvgPeK97HZlFKY3+t1w0qbhOsFFHWlsNYZb4eNQY6ttnNhp5PI7zccdmdrJi?=
 =?us-ascii?Q?RaTke2aD46SJfUM6U3NrK3QTV1+6r7mkNNPxZu9d4/vVTxMYaL6kEg7G6+ba?=
 =?us-ascii?Q?ihDuhHXIRtizvLWXzrWB2aGFlsV48bHFDsNBQ2ive6GHNgdaCnVnNBuNrcOP?=
 =?us-ascii?Q?ZX7UL6sIhhIweMcnDziatIYTouw97dbxl74P0NOaRczp/+Z40UxPniOfT6Sz?=
 =?us-ascii?Q?YxuGw3y7zq7aw+N+anRqZq4189D5kSJ7pSHtvWZzL0oMZZ8AsYDcRoyBZMA9?=
 =?us-ascii?Q?EE3RMQrHH5Wf+Lb8YMYWk6/dZ5ikeAzzmQC8nSOT9XyeDNRzNBQjTDn7PVJi?=
 =?us-ascii?Q?D4K7jjhjaBj+iFXssK0QOWu6N7K46Sgp39Q04NVplSKVFiRHFoeskuzacxGE?=
 =?us-ascii?Q?dfI3Wnb7J8SLiEdxjPG+TzRw3BDrzNkpaYQW0Rcu78YgVpWkqcaLq0x/Ln2r?=
 =?us-ascii?Q?YTkU+UYT1rac4yGNEkUFCkJuLJFG7XlNKLIgID603sHKsr4a5vs4pYsmaB+4?=
 =?us-ascii?Q?L1aDusPeTUsexXJBGTH0MEy3qeMm1JQ=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: 2b455c40-c21e-4cf6-3fe1-08da1f08e256
X-MS-Exchange-CrossTenant-originalarrivaltime: 15 Apr 2022 17:53:44.3380
 (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: A5y/2Op0sqmQvN+nfM0eLsIOPQR8MUGzI9GtiKv2tCMbKn3UrR9YY+jBXXpnJGfeOwynAxhBfK6tSn0eM+mMTxt3/YIEErZIlw7bPVYCyvw=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR11MB3100
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,
        RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,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: Friday, April 15, 2022 8:50 AM
> To: mschmidt <mschmidt@redhat.com>
> Cc: netdev@vger.kernel.org; poros <poros@redhat.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: Re: [PATCH net] ice: Fix race during aux device (un)plugging
>=20
> On Fri, 15 Apr 2022 13:12:03 +0200
> Michal Schmidt <mschmidt@redhat.com> wrote:
>=20
> > On Thu, Apr 14, 2022 at 6:39 PM Ivan Vecera <ivecera@redhat.com> wrote:
> >
> > > 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 and can causes a crash when ice_send_event_to_aux()
> > > call occurs during these operations because that function depends
> > > on non-NULL value of pf->adev and does not assume that aux device
> > > is half-initialized or half-destroyed.
> > >
> > > Modify affected functions so pf->adev field is set after aux device
> > > init and prior aux device destroy.
> > >
> > [...]
> >
> > > @@ -320,12 +319,14 @@ int ice_plug_aux_dev(struct ice_pf *pf)
> > >   */
> > >  void ice_unplug_aux_dev(struct ice_pf *pf)
> > >  {
> > > -       if (!pf->adev)
> > > +       struct auxiliary_device *adev =3D pf->adev;
> > > +
> > > +       if (!adev)
> > >                 return;
> > >
> > > -       auxiliary_device_delete(pf->adev);
> > > -       auxiliary_device_uninit(pf->adev);
> > >         pf->adev =3D NULL;
> > > +       auxiliary_device_delete(adev);
> > > +       auxiliary_device_uninit(adev);
> > >  }
> > >
> >
> > Hi Ivan,
> > What prevents ice_unplug_aux_dev() from running immediately after
> > ice_send_event_to_aux() gets past its "if (!pf->adev)" test ?
> > Michal
>=20
> ice_send_event_to_aux() takes aux device lock. ice_unplug_aux_dev()
> calls auxiliary_device_delete() that calls device_del(). device_del()
> takes device_lock() prior kill_device(). So if ice_send_event_to_aux()
> is in progress then device_del() waits for its completion.
>=20
> Thanks,
> Ivan

Thanks for the patch Ivan!

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