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@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
> >>
> >> -
> > 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@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Attachment:
signature.asc
Description: PGP signature
