***SPAM*** Re: [ofa-general] [PATCH] IPoIB: Prevent address handles leak.

[Yossi Etigin]

I think the problem is that multicast is not really flushed when the
interface is downed. Therefore, a join can start after the device was
brought down, and the fix below will not reap the ah's.

How about reaping all remaining ah's after multicast device is really
flushed, that is in ipoib_ib_dev_cleanup(), which is called when
ib_ipoib is unloaded? This way you can have only a limited amount of 
non-reaped dead ah when interface is down (until the reap task is back
on), and you can be certain that all of them will be reaped when module
is unloaded.

--Yossi

Vladimir Sokolovsky wrote:
> In case of removing ib_ipoib module ipoib_ib_dev_stop() function will be
> called and all address handles (ah) in dead_ahs list will be reaped.
> But some ah will be added to the dead list after ipoib_ib_dev_stop done
> by ipoib_mcast_free. These ahs will not be freed.
> 
> The solution here is to wait till multicast_list will be empty. So, all
> ahs will be added to dead_ahs list.
> 
> Signed-off-by: Vladimir Sokolovsky <vlad at mellanox.co.il>
> ---
> Roland,
> There may be some extremely slight window for leaking address handles
> still, since the multicast list is emptied in ipoib_mcast_dev_flush() before
> it calls ipoib_mcast_free() (which adds address handles to the dead list).
> 
> However, this seems to be the best compromise that I can see without
> a lot of nasty (and possibly buggy) changes.
> 
>  drivers/infiniband/ulp/ipoib/ipoib_ib.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/infiniband/ulp/ipoib/ipoib_ib.c b/drivers/infiniband/ulp/ipoib/ipoib_ib.c
> index 66cafa2..6cc0c59 100644
> --- a/drivers/infiniband/ulp/ipoib/ipoib_ib.c
> +++ b/drivers/infiniband/ulp/ipoib/ipoib_ib.c
> @@ -863,7 +863,7 @@ timeout:
>  
>  	begin = jiffies;
>  
> -	while (!list_empty(&priv->dead_ahs)) {
> +	while (!list_empty(&priv->dead_ahs) || !list_empty(&priv->multicast_list)) {
>  		__ipoib_reap_ah(dev);
>  
>  		if (time_after(jiffies, begin + HZ)) {