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

[Leon Romanovsky <leon@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3738 bytes --]

On Tue, Nov 28, 2017 at 03:03:39PM -0600, Daniel Jurgens wrote:
> On 11/28/2017 2:38 PM, Don Dutile wrote:
> > On 11/27/2017 06:28 PM, Don Dutile wrote:
> >> 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@mellanox.com>
> >>>>>
> >>>>>
> >>>>> -    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
> >>
> >> -
> > Is someone doing a v3? I didn't see an email today w/another patch version.
> > ... at least I wasn't directly cc'd on one anyhow... off to check linux-rdma folder...
> > nothing there... and I don't find a v3 in Leon's tree either.
> >
> >
> I sent a fix-up patch to Leon, but it seems he didn't get to it today. I'll resend this afternoon.

Thanks Daniel for handling this.

>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]