Re: [PATCH for-rc 5/7] Revert "IB/ipoib: Update broadcast object if PKey value was changed in index 0"

[Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 2789 bytes --]

On Tue, Sep 26, 2017 at 06:06:22AM -0700, Dennis Dalessandro wrote:
> From: Alex Estrin <alex.estrin-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>
> commit 9a9b8112699d will cause core to fail UD QP from being destroyed
> on ipoib unload, therefore cause resources leakage.
> On pkey change event above patch modifies mgid before calling underlying
> driver to detach it from QP. Drivers' detach_mcast() will fail to find
> modified mgid it was never given to attach in a first place.
> Core qp->usecnt will never go down, so ib_destroy_qp() will fail.
>
> IPoIB driver actually does take care of new broadcast mgid based on new
> pkey by destroying an old mcast object in ipoib_mcast_dev_flush())
> ....
> 	if (priv->broadcast) {
> 		rb_erase(&priv->broadcast->rb_node, &priv->multicast_tree);
> 		list_add_tail(&priv->broadcast->list, &remove_list);
> 		priv->broadcast = NULL;
> 	}
> ...
>
> then in restarted ipoib_macst_join_task() creating a new broadcast mcast
> object, sending join request and on completion tells the driver to attach
> to reinitialized QP:
> ...
> if (!priv->broadcast) {
> ...
> 	broadcast = ipoib_mcast_alloc(dev, 0);
> ...
> 	memcpy(broadcast->mcmember.mgid.raw, priv->dev->broadcast + 4,
> 	       sizeof (union ib_gid));
> 	priv->broadcast = broadcast;
> ...
>
> Fixes: 9a9b8112699d ("IB/ipoib: Update broadcast object if PKey value was changed in index 0")
> Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Reviewed-by: Mike Marciniszyn <mike.marciniszyn-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Reviewed-by: Dennis Dalessandro <dennis.dalessandro-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Alex Estrin <alex.estrin-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Dennis Dalessandro <dennis.dalessandro-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/infiniband/ulp/ipoib/ipoib_ib.c |   13 -------------
>  1 files changed, 0 insertions(+), 13 deletions(-)
>

I consulted with Feras about it and it is his reply.

From Feras:
After applying the patch in question, the following scenario may happens:

    PKEY change event occurs, it calls update_parent_pkey (which updates the device and the priv broadcast)
    and then brings the device down. The flow of taking the device down flushes the multicast groups and
    the broadcast group. Since the patch changes the broadcast address, the flow fails to detach from the
    unattached group causing syndrome.

Mistakenly we thought that that patch will fix other issue with VM migrations when pkey change in index 0,
in this case the interface state was down so we did not hit this issue.

Anyway, your revert is correct.

Thanks,
Reviewed-by: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]