netdev - RE: [PATCH net 1/6] ice: avoid bonding causing auxiliary plug/unplug under RTNL lock

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 14 Feb 2023 22:24:04 +0000
From:   "Ertman, David M" <david.m.ertman@...el.com>
To:     Leon Romanovsky <leonro@...dia.com>,
        "Nguyen, Anthony L" <anthony.l.nguyen@...el.com>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        poros <poros@...hat.com>, ivecera <ivecera@...hat.com>,
        "Saleem, Shiraz" <shiraz.saleem@...el.com>,
        "Ismail, Mustafa" <mustafa.ismail@...el.com>,
        "jgg@...dia.com" <jgg@...dia.com>,
        "linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
        Jaroslav Pulchart <jaroslav.pulchart@...ddata.com>,
        Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>,
        "G, GurucharanX" <gurucharanx.g@...el.com>
Subject: RE: [PATCH net 1/6] ice: avoid bonding causing auxiliary plug/unplug
 under RTNL lock

> -----Original Message-----
> From: Leon Romanovsky <leonro@...dia.com>
> Sent: Wednesday, February 1, 2023 1:50 AM
> Subject: Re: [PATCH net 1/6] ice: avoid bonding causing auxiliary plug/unplug
> under RTNL lock
> 
> On Tue, Jan 31, 2023 at 01:36:58PM -0800, Tony Nguyen wrote:
> > From: Dave Ertman <david.m.ertman@...el.com>
> >
> > RDMA is not supported in ice on a PF that has been added to a bonded
> > interface. To enforce this, when an interface enters a bond, we unplug
> > the auxiliary device that supports RDMA functionality.  This unplug
> > currently happens in the context of handling the netdev bonding event.
> > This event is sent to the ice driver under RTNL context.  This is causing
> > a deadlock where the RDMA driver is waiting for the RTNL lock to complete
> > the removal.
> >
> > Defer the unplugging/re-plugging of the auxiliary device to the service
> > task so that it is not performed under the RTNL lock context.
> >
> > Reported-by: Jaroslav Pulchart <jaroslav.pulchart@...ddata.com>
> > Link: https://lore.kernel.org/linux-rdma/68b14b11-d0c7-65c9-4eeb-
> 0487c95e395d@...mhuis.info/
> > Fixes: 5cb1ebdbc434 ("ice: Fix race condition during interface enslave")
> > Fixes: 4eace75e0853 ("RDMA/irdma: Report the correct link speed")
> > Signed-off-by: Dave Ertman <david.m.ertman@...el.com>
> > Reviewed-by: Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>
> > Tested-by: Gurucharan G <gurucharanx.g@...el.com> (A Contingent
> worker at Intel)
> > Signed-off-by: Tony Nguyen <anthony.l.nguyen@...el.com>
> > ---
> >  drivers/net/ethernet/intel/ice/ice.h      | 14 +++++---------
> >  drivers/net/ethernet/intel/ice/ice_main.c | 17 +++++++----------
> >  2 files changed, 12 insertions(+), 19 deletions(-)
> 
> <...>
> 
> > index 5f86e4111fa9..055494dbcce0 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_main.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> > @@ -2290,18 +2290,15 @@ static void ice_service_task(struct work_struct
> *work)
> >  		}
> >  	}
> >
> > -	if (test_bit(ICE_FLAG_PLUG_AUX_DEV, pf->flags)) {
> > -		/* Plug aux device per request */
> > +	/* Plug aux device per request */
> > +	if (test_and_clear_bit(ICE_FLAG_PLUG_AUX_DEV, pf->flags))
> 
> Very interesting pattern. You are not holding any locks while running
> ice_service_task() and clear bits before you actually performed requested
> operation.
> 
> How do you protect from races while testing bits in other places of ice
> driver?

Leon,

Thanks for the review and sorry for the late reply, got sidetracked into another project.

Your review caused us to re-evaluate the plug/unplug flow, and since these bits are only set/cleared in
the bonding event flow, and the UNPLUG bit set clears the PLUG bit, we attain the desired outcome
in all cases if we swap the order that we evaluate the bits in the service task.

Any multi-event situation that happens between or during service task will be handled in the expected way.

DaveE

> 
> Thanks
> 
> >  		ice_plug_aux_dev(pf);
> >
> > -		/* Mark plugging as done but check whether unplug was
> > -		 * requested during ice_plug_aux_dev() call
> > -		 * (e.g. from ice_clear_rdma_cap()) and if so then
> > -		 * plug aux device.
> > -		 */
> > -		if (!test_and_clear_bit(ICE_FLAG_PLUG_AUX_DEV, pf-
> >flags))
> > -			ice_unplug_aux_dev(pf);
> > -	}
> > +	/* unplug aux dev per request, if an unplug request came in
> > +	 * while processing a plug request, this will handle it
> > +	 */
> > +	if (test_and_clear_bit(ICE_FLAG_UNPLUG_AUX_DEV, pf->flags))
> > +		ice_unplug_aux_dev(pf);
> >
> >  	if (test_and_clear_bit(ICE_FLAG_MTU_CHANGED, pf->flags)) {
> >  		struct iidc_event *event;
> > --
> > 2.38.1
> >