> -----Original Message-----
> From: Jason Gunthorpe <jgg@xxxxxxxxxxxx>
> Sent: Friday, February 28, 2020 10:03 AM
> To: Marciniszyn, Mike <mike.marciniszyn@xxxxxxxxx>
> Cc: Leon Romanovsky <leon@xxxxxxxxxx>; Doug Ledford
> <dledford@xxxxxxxxxx>; Maor Gottlieb <maorg@xxxxxxxxxxxx>; linux-
> rdma@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH rdma-rc] RDMA/core: Fix pkey and port assignment in
> get_new_pps
>
> On Thu, Feb 27, 2020 at 10:49:45PM +0000, Marciniszyn, Mike wrote:
> > > On Thu, Feb 27, 2020 at 02:07:10PM +0000, Marciniszyn, Mike wrote:
> > > > >
> > > > > From: Maor Gottlieb <maorg@xxxxxxxxxxxx>
> > > > >
> > > > > When port is part of the modify mask, then we should take
> > > > > it from the qp_attr and not from the old pps. Same for PKEY.
> > > > >
> > > > > Cc: <stable@xxxxxxxxxxxxxxx>
> > > > > Fixes: 1dd017882e01 ("RDMA/core: Fix protection fault in
> > > > > get_pkey_idx_qp_list")
> > > > > Signed-off-by: Maor Gottlieb <maorg@xxxxxxxxxxxx>
> > > > > Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> > > > > drivers/infiniband/core/security.c | 12 ++++++++----
> > > > > 1 file changed, 8 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/drivers/infiniband/core/security.c
> > > > > b/drivers/infiniband/core/security.c
> > > > > index b9a36ea244d4..2d5608315dc8 100644
> > > > > +++ b/drivers/infiniband/core/security.c
> > > > > @@ -340,11 +340,15 @@ static struct ib_ports_pkeys
> > > *get_new_pps(const
> > > > > struct ib_qp *qp,
> > > > > return NULL;
> > > > >
> > > > > if (qp_attr_mask & IB_QP_PORT)
> > > > > - new_pps->main.port_num =
> > > > > - (qp_pps) ? qp_pps->main.port_num :
> qp_attr-
> > > > > >port_num;
> > > > > + new_pps->main.port_num = qp_attr->port_num;
> > > > > + else if (qp_pps)
> > > > > + new_pps->main.port_num = qp_pps-
> >main.port_num;
> > > > > +
> > > > > if (qp_attr_mask & IB_QP_PKEY_INDEX)
> > > > > - new_pps->main.pkey_index = (qp_pps) ? qp_pps-
> > > > > >main.pkey_index :
> > > > > - qp_attr-
> >pkey_index;
> > > > > + new_pps->main.pkey_index = qp_attr->pkey_index;
> > > > > + else if (qp_pps)
> > > > > + new_pps->main.pkey_index = qp_pps-
> >main.pkey_index;
> > > > > +
> > > > > if ((qp_attr_mask & IB_QP_PKEY_INDEX) && (qp_attr_mask
> &
> > > > > IB_QP_PORT))
> > > > > new_pps->main.state = IB_PORT_PKEY_VALID;
> > > > >
> > > >
> > > > I agree with this aspect of the patch and it does fix the panic, because
> the
> > > correct unit
> > > > is gotten from qp_pps.
> > > >
> > > > My issue is that the new_pps->main.state will come back as 0, and the
> > > insert routine will drop any new pkey index update.
> > > >
> > > > The sequence I'm concerned about is:
> > > >
> > > > 0x71 attr mask with both pkey index and port.
> > > >
> > > > A ulp decides to change the pkey index and does an 0x51 modify
> without
> > > setting the unit.
> > > >
> > > > I see new_pps->main.state being returned as 0 and
> port_pkey_list_insert()
> > > will early out.
> > >
> > > I see, maybe we can store the main.state in qps and restore it from
> there?
> > >
> >
> > I don't think we need to go that far.
> >
> > I think all we need to do is
> >
> > if ((qp_attr_mask & IB_QP_PKEY_INDEX) && (qp_attr_mask &
> IB_QP_PORT))
> > new_pps->main.state = IB_PORT_PKEY_VALID;
> > else if ((qp_attr_mask & (IB_QP_PKEY_INDEX | IB_QP_PORT)) &&
> qp_pps) {
> > /*
> > * one of the attributes modified and already copied above.
> > *
> > * correct the state based on qp_pps state
> > */
> > if (qp_pps->main.state != IB_PORT_PKEY_NOT_VALID)
> > new_pps->main.state = IB_PORT_PKEY_VALID;
> > }
> >
> > I'm ok will a follow-up patch after Maor's patch to fix remaining issues.
>
> Are you then OK with the patch Maor posted? Please add a tag. I'm
> waiting for you :)
>
Yes, I'm ok with Maor's patch. It certainly fixes the panic.
I will try the above approach in a subsequent patch.
Tested-by: Mike Marciniszyn <mike.marciniszyn@xxxxxxxxx>
Mike
