On Thu, 2018-12-06 at 21:29 +0000, Parav Pandit wrote:
> > -----Original Message-----
> > From: Doug Ledford <dledford@xxxxxxxxxx>
> > Sent: Thursday, December 6, 2018 3:12 PM
> > To: Parav Pandit <parav@xxxxxxxxxxxx>; Leon Romanovsky
> > <leon@xxxxxxxxxx>
> > Cc: Jason Gunthorpe <jgg@xxxxxxxxxxxx>; Mark Zhang
> > <markz@xxxxxxxxxxxx>; RDMA mailing list <linux-rdma@xxxxxxxxxxxxxxx>
> > Subject: Re: [PATCH rdma-rc] IB/core: Don't match bond master filter for
> > closed slaves
> >
> > On Thu, 2018-12-06 at 16:01 -0500, Doug Ledford wrote:
> > > On Thu, 2018-12-06 at 20:58 +0000, Parav Pandit wrote:
> > > > > -----Original Message-----
> > > > > From: Doug Ledford <dledford@xxxxxxxxxx>
> > > > > Sent: Thursday, December 6, 2018 2:55 PM
> > > > > To: Parav Pandit <parav@xxxxxxxxxxxx>; Leon Romanovsky
> > > > > <leon@xxxxxxxxxx>
> > > > > Cc: Jason Gunthorpe <jgg@xxxxxxxxxxxx>; Mark Zhang
> > > > > <markz@xxxxxxxxxxxx>; RDMA mailing list
> > > > > <linux-rdma@xxxxxxxxxxxxxxx>
> > > > > Subject: Re: [PATCH rdma-rc] IB/core: Don't match bond master
> > > > > filter for closed slaves
> > > > >
> > > > > On Thu, 2018-12-06 at 19:10 +0000, Parav Pandit wrote:
> > > > > > > -----Original Message-----
> > > > > > > From: Leon Romanovsky <leon@xxxxxxxxxx>
> > > > > > > Sent: Thursday, December 6, 2018 12:48 PM
> > > > > > > To: Doug Ledford <dledford@xxxxxxxxxx>
> > > > > > > Cc: Jason Gunthorpe <jgg@xxxxxxxxxxxx>; Mark Zhang
> > > > > > > <markz@xxxxxxxxxxxx>; RDMA mailing list
> > > > > > > <linux-rdma@xxxxxxxxxxxxxxx>; Parav Pandit
> > > > > > > <parav@xxxxxxxxxxxx>
> > > > > > > Subject: Re: [PATCH rdma-rc] IB/core: Don't match bond master
> > > > > > > filter for closed slaves
> > > > > > >
> > > > > > > On Thu, Dec 06, 2018 at 12:19:30PM -0500, Doug Ledford wrote:
> > > > > > > > On Wed, 2018-12-05 at 15:50 +0200, Leon Romanovsky wrote:
> > > > > > > > > From: Mark Zhang <markz@xxxxxxxxxxxx>
> > > > > > > > >
> > > > > > > > > The "modprobe -r mlx4_en" will close bond slaves and in
> > > > > > > > > such case the rdma_dev would be NULL, so no match should be
> > returned.
> > > > > > > > I'm not groking your commit message. What happens without
> > > > > > > > this
> > > > > patch?
> > > > > > > We will see calltrace due to the fact that NULL is passed down
> > > > > > > the stack through rdma_is_upper_dev_rcu() upto
> > > > > netdev_walk_all_upper_dev_rcu().
> > > > > > I see this patch being sent for -rc which I was not sure when I
> > reviewed.
> > > > > > Commit message needs to change as below.
> > > > > >
> > > > > > When mlx4_en driver is unloaded, and configured to use bond
> > > > > > devices,
> > > > > get_netdev() callback returns NULL because associated netdev is
> > > > > unregistered.
> > > > > > Therefore, similar to other filter() routines, fix
> > > > > is_upper_ndev_bond_master_filter() to handle NULL netdev.
> > > > > > Without this check, a call trace is observed.
> > > > >
> > > > > Does it catch a WARN_ON later on, or does it oops?
> > > > >
> > > > It is NULL reference oops.
> > >
> > > Yep, I just got through following the trail. It oopses in
> > > netdev_next_upper_dev_rcu().
> > >
> > > Ok, patch applied to for-rc with commit message fixups.
> > >
> >
> > I didn't like the commit message as it stood, so I changed it to this.
> > Let me know if there are any objections, it's still changeable as long as it's in
> > my wip branch:
> >
> > IB/core: Fix oops in netdev_next_upper_dev_rcu()
> >
> > On removal of the mlx4_en module, the proper order of removal is to
> > remove the roce devices, and then remove the net devices the roce
> In current code, mlx4_ib and mlx4_en are layered on top of mlx4_core.
Agree.
> So mlx4_en can get unloaded
Agree.
> even with roce devices created by mlx4_ib exist.
Are you sure about this part? I'm not seeing any RoCE devices after I
unload mlx4_en. I am, however, seeing the parent ibdevice mlx4_0 and it
has two ports, but 1 port is registered and active and the other port
exists, but is unregistered from things like the verbs layer and stuff
like that. It seems it would be more accurate to say that there are no
longer any registered RoCE devices, but the parent ibdevice, which is a
dual port device, still has a stub for the RoCE device with all of the
capabilities and status set to defaults/down, and which has no attached
netdev device. We need to filter this down/default/unregistered device
port from results or we get the oops referenced.
> This leads to condition where get_netdev() returns NULL.
>
> So we should modify the commit message a bit.
> How about below text,
>
> In bonding configuration of netdevices for using mlx4_en and mlx4_ib in dual port mode,
> get_netdev() returns NULL when associated netdevice is unregistered for a port of rdma device.
> A filter function is_upper_ndev_bond_master_filter () needs to consider NULL return value by get_netdev().
> Otherwise, it results into a NULL reference oops.
>
> To avoid such oops, perform NULL reference check.
>
> > devices were paired on top of. If the netdevice happens to be part
> > of a bond, then during netdevice removal, after the roce device is
> > already gone, a call to is_upper_ndev_bond_master_filter() is made.
> > We can't allow the slave netdevice to succeed at the match if the
> > rdma_dev is already gone, otherwise the mentioned oops occurs.
> >
> > Fixes: 408f1242d940 ("IB/core: Delete lower netdevice default GID entries
> > in bonding scenario")
> > Signed-off-by: Mark Zhang <markz@xxxxxxxxxxxx>
> > Reviewed-by: Parav Pandit <parav@xxxxxxxxxxxx>
> > Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> > Signed-off-by: Doug Ledford <dledford@xxxxxxxxxx>
> >
> >
> > --
> > Doug Ledford <dledford@xxxxxxxxxx>
> > GPG KeyID: B826A3330E572FDD
> > Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD
--
Doug Ledford <dledford@xxxxxxxxxx>
GPG KeyID: B826A3330E572FDD
Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD
Attachment:
signature.asc
Description: This is a digitally signed message part
