[openib-general] Re: [PATCH] core: fix user_mad memory leaks on timeout

[Michael S. Tsirkin]

Quoting r. Hal Rosenstock <halr at voltaire.com>:
> Subject: Re: [PATCH] core: fix user_mad memory leaks on timeout
> 
> On Thu, 2005-12-08 at 08:51, Michael S. Tsirkin wrote:
> > Dont leak packet if it had a timeout.
> > Dont leak timeout mad if queue_packet fails.
> > 
> > Signed-off-by: Jack Morgenstein <jackm at mellanox.co.il>
> > Signed-off-by: Michael S. Tsirkin <mst at mellanox.co.il>
> > 
> > Index: openib/drivers/infiniband/core/user_mad.c
> > ===================================================================
> > --- openib.orig/drivers/infiniband/core/user_mad.c	2005-12-08
> 15:40:41.000000000 +0200
> > +++ openib/drivers/infiniband/core/user_mad.c	2005-12-08
> 15:40:28.000000000 +0200
> > @@ -197,8 +197,8 @@ static void send_handler(struct ib_mad_a
> >  		memcpy(timeout->mad.data, packet->mad.data,
> >  		       sizeof (struct ib_mad_hdr));
> >  
> > -		if (!queue_packet(file, agent, timeout))
> > -				return;
> > +		if (queue_packet(file, agent, timeout))
> > +			kfree(timeout);
> 
> Yes, there appears to be a memory leak here but I don't think this fix
> is quite right as it has lost the return when the queue_packet succeeds.
> Isn't that still needed ?

No, the return here was wrong: we copied the packet
and we need to free it anyway
so falling through to kfree below is the correct behaviour.
Thats what I mean by "Dont leak packet if it had a timeout".

> 		if (!queue_packet(file, agent, timeout))
> 			return;
> 		kfree(timeout);
> 
> >  	}
> >  out:
> >  	kfree(packet);
> 
> Another point: on either failure to allocate the timeout MAD or failure
> to queue the timeout MAD, is simply throwing this away sufficient ? It
> seems to me that if this occurs, then the contract is broken and the
> client still needs to worry about its own timeout.
> 
> -- Hal
> 

Not much to do though, since allocating memory with gfp kernel fails:
lets at least be careful to avoid a crash or memory leak.

-- 
MST