Re: [PATCH-v4 6/9] Bluetooth: Cleanup blkcipher on SMP termination

[Brian Gix <bgix@codeaurora.org>]

Hi Gustavo

On 11/21/2011 7:48 AM, Gustavo Padovan wrote:
[...]
>>   void smp_chan_destroy(struct l2cap_conn *conn)
>>   {
>> -	kfree(conn->smp_chan);
>> +	struct smp_chan *smp = conn->smp_chan;
>> +
>> +	if (smp&&  !IS_ERR(smp->tfm))
>> +		crypto_free_blkcipher(smp->tfm);
>
> smp->tfm doesn't say what you want, if its allocation failed its value is
> still 0. And I don't think we need to check for smp == NULL. It can't be null
> at this point.
>
> 	Gustavo

OK, so I see that smp_>tfm will never equal an Error, so that check is 
meaningless.

However, there are conditions where it can in fact be NULL, 
particularily if the user aborts the paring process, and looking at the 
blkcipher's FREE code, I am not confident that it couldn't get past the 
"unlikely" NULL checks with all compilation parameters, so we should 
keep at least a NULL check here.

As far as NULL checking "smp", I think we can avoid it, but only if we 
make sure that we clear the HCI_CONN_LE_SMP_PEND bit within the destroy 
function itself.  A cursory look at the usage doesn't fill me with 
confidence that the flag bit, and the allocation are rigorously kept in 
sync.

-- 
Brian Gix
bgix@codeaurora.org
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum