On 11/22/2016 4:53 PM, James Morris wrote:
> On Tue, 22 Nov 2016, Dan Jurgens wrote:
>
>> +static int sel_pkey_sid_slow(u64 subnet_prefix, u16 pkey_num, u32 *sid)
>> +{
>> + int ret = -ENOMEM;
>> + struct sel_pkey *pkey;
>> + struct sel_pkey *new = NULL;
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&sel_pkey_lock, flags);
>> + pkey = sel_pkey_find(subnet_prefix, pkey_num);
>> + if (pkey) {
>> + *sid = pkey->psec.sid;
>> + spin_unlock_irqrestore(&sel_pkey_lock, flags);
>> + return 0;
>> + }
>> +
>> + ret = security_pkey_sid(subnet_prefix, pkey_num, sid);
>> + if (ret != 0)
>> + goto out;
>> +
>> + new = kzalloc(sizeof(*new), GFP_ATOMIC);
>> + if (!new)
>> + goto out;
>> +
>> + new->psec.subnet_prefix = subnet_prefix;
>> + new->psec.pkey = pkey_num;
>> + new->psec.sid = *sid;
>> + sel_pkey_insert(new);
>> +
>> +out:
>> + spin_unlock_irqrestore(&sel_pkey_lock, flags);
>> + if (unlikely(ret))
>> + kfree(new);
>> +
>> + return ret;
> The error handling is messed up here.
>
> You'll return 0 on kzalloc failure.
>
> Also, if security_pkey_sid fails, you'll attempt to kfree 'new', and you
> don't need an unlikely() in a slow path.
>
>
Right. I'll remove the if/kfree in the out path completely. There's no way ret becomes non-zero after the kzalloc. If the kzalloc fails I'll still have it return 0, the SID retrieved is valid, it just won't be added the cache so returning with an error is overkill.
Thank you for your review.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
