On 11/27/2017 4:03 PM, Don Dutile wrote:
> On 11/27/2017 06:25 AM, Leon Romanovsky wrote:
>> From: Daniel Jurgens <danielj@xxxxxxxxxxxx>
>>
>>
>> - if (pps_change && !special_qp) {
>> + if (pps_change && !special_qp && real_qp->qp_sec) {
>> mutex_lock(&real_qp->qp_sec->mutex);
>> new_pps = get_new_pps(real_qp,
>> qp_attr,
>> @@ -600,7 +627,7 @@ int ib_security_modify_qp(struct ib_qp *qp,
>> qp_attr_mask,
>> udata);
>>
>> - if (pps_change && !special_qp) {
>> + if (pps_change && !special_qp && real_qp->qp_sec) {
>> /* Clean up the lists and free the appropriate
>> * ports_pkeys structure.
>> */
>>
> This patch breaks the kernel build on RHEL b/c it generates
> a warning in the second if (pps_change && !special_qp && real_qp->qp_sec) {}
> that new_pps may not be assigned. ... build warnings in RHEL kernel == build failure (on x86).
>
> That's b/c the patch adds real_qp->qp_sec to if's conditions,
> and the compiler cannot determine if real_qp->qp_sec cannot be modified
> between the first check like it, above, which sets the value of new_pps,
> and the second check that uses it, because real_qp is passed into the device->modify()
> function call btwn those two if() check's.
>
> The code needs to do something like this in the first if-check:
> .....
> bool new_pps_gotten = false;
> ....
>
> if (pps_change && !special_qp && real_qp->qp_sec) {
> mutex_lock(&real_qp->qp_sec->mutex);
> new_pps = get_new_pps(real_qp,
> qp_attr,
> qp_attr_mask);
> new_pps_gotten = true;
> ....
> }
> ....
>
> and change the second if check to be:
>
> if (new_pps_gotten) {
> * Clean up the lists and free the appropriate
> .....
>
Thanks Don, I think it's better to initialize new_pps to NULL, vs introducing a new variable. Also, there needs to be a check of new_pps after getting it.