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

[Hal Rosenstock]

On Thu, 2005-12-08 at 10:25, Michael S. Tsirkin wrote:
> 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".

You're right.

> > 		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:

Couldn't the callback be rescheduled for some time later where the
allocation might succeed ?

> lets at least be careful to avoid a crash or memory leak.

Agreed.

-- Hal