Subject: Re: [PATCH 3/3] dsa: fix freeing of sparse port allocation

From: Florian Fainelli <[email-masked]>
Date: Mon, 25 Mar 2013 20:55:33 +0100

Le lundi 25 mars 2013 17:03:23, David Laight a écrit :
> > If we have defined a sparse port allocation which is non-contiguous and
> > contains gaps, the code freeing port_names will just stop when it
> > encouters a first NULL port_names, which is not right, we should iterate
> > over all possible number of ports (DSA_MAX_PORTS) until we are done.
> 
> ...
> 
> >  		port_index = 0;
> > 
> > -		while (pd->chip[i].port_names &&
> > -			pd->chip[i].port_names[++port_index])
> > -			kfree(pd->chip[i].port_names[port_index]);
> > +		while (port_index < DSA_MAX_PORTS) {
> > +			if (pd->chip[i].port_names[port_index])
> > +				kfree(pd->chip[i].port_names[port_index]);
> > +			port_index++;
> > +		}
> 
> A couple of 'odd' differences:
> 
> The old code checked pd->chip[i].port_names (as if it
> might be a pointer) that is absent from the new version.
> (If it is separately allocated it is leaked).
> 
> The new code tests and frees pd->chip[i].port_names[0]
> whereas the old code ignored the 0th entry.

The old code was wrong, it was off by one for the first array index, and would 
stop whenever it encountered the first NULL port_names[index] so we would not 
skip other these and free the possibly non-NULL next one.

I think the current code is now correct, but thanks for the review!
--
Florian