Re: [PATCH rdma-rc v2] IB/core: Only enforce security for InfiniBand

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 11/27/2017 05:58 PM, Daniel Jurgens wrote:
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.

yup, I considered that as well.
wasn't sure if lockdep checking code would not like the fact that a mutex_lock() could be taken,
but if new_pps == NULL after the get call(it may always succeed, but an analyzer may not conclude the same),
that the mutex_unlock() wouldn't be called.

the double, same-condition if-check with the fcn call in btwn seems like it ought to be
restructured differently so the mutex lock/unlock pairs are contained neatly in a single if-clause,
and the new_pps alloc & use would be similarly containted.

-dd

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



[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Photo]     [Yosemite News]     [Yosemite Photos]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux