[PATCH rdma-rc] RDMA/core: Fix pkey and port assignment in get_new_pps

[PATCH rdma-rc] RDMA/core: Fix pkey and port assignment in get_new_pps

[Leon Romanovsky]

From: Maor Gottlieb <maorg@mellanox.com>

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@vger.kernel.org>
Fixes: 1dd017882e01 ("RDMA/core: Fix protection fault in get_pkey_idx_qp_list")
Signed-off-by: Maor Gottlieb <maorg@mellanox.com>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
 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
--- a/drivers/infiniband/core/security.c
+++ 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;
 
-- 
2.24.1

RE: [PATCH rdma-rc] RDMA/core: Fix pkey and port assignment in get_new_pps

[Marciniszyn, Mike]

> 
> From: Maor Gottlieb <maorg@mellanox.com>
> 
> 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@vger.kernel.org>
> Fixes: 1dd017882e01 ("RDMA/core: Fix protection fault in
> get_pkey_idx_qp_list")
> Signed-off-by: Maor Gottlieb <maorg@mellanox.com>
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> ---
>  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
> --- a/drivers/infiniband/core/security.c
> +++ 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 asked this exact question in https://marc.info/?l=linux-rdma&m=158278763015030&w=2.

I also asked about the logical or, and you answered that pointing to an additional patch.

You but didn't address the main.state being 0 and losing the pkey_index update in the 0x51 modify.

Mike

Re: [PATCH rdma-rc] RDMA/core: Fix pkey and port assignment in get_new_pps

[Leon Romanovsky]

On Thu, Feb 27, 2020 at 02:07:10PM +0000, Marciniszyn, Mike wrote:
> >
> > From: Maor Gottlieb <maorg@mellanox.com>
> >
> > 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@vger.kernel.org>
> > Fixes: 1dd017882e01 ("RDMA/core: Fix protection fault in
> > get_pkey_idx_qp_list")
> > Signed-off-by: Maor Gottlieb <maorg@mellanox.com>
> > Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> > ---
> >  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
> > --- a/drivers/infiniband/core/security.c
> > +++ 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 asked this exact question in https://marc.info/?l=linux-rdma&m=158278763015030&w=2.
>
> I also asked about the logical or, and you answered that pointing to an additional patch.
>
> You but didn't address the main.state being 0 and losing the pkey_index update in the 0x51 modify.
>
> Mike
>
>
>
>
>
>

RE: [PATCH rdma-rc] RDMA/core: Fix pkey and port assignment in get_new_pps

[Marciniszyn, Mike]

> On Thu, Feb 27, 2020 at 02:07:10PM +0000, Marciniszyn, Mike wrote:
> > >
> > > From: Maor Gottlieb <maorg@mellanox.com>
> > >
> > > 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@vger.kernel.org>
> > > Fixes: 1dd017882e01 ("RDMA/core: Fix protection fault in
> > > get_pkey_idx_qp_list")
> > > Signed-off-by: Maor Gottlieb <maorg@mellanox.com>
> > > Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> > > ---
> > >  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
> > > --- a/drivers/infiniband/core/security.c
> > > +++ 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.

Right now, all of our upstream testing (5.6 rc3+, stables) is dead because of 1dd017882e01.

Mike

Re: [PATCH rdma-rc] RDMA/core: Fix pkey and port assignment in get_new_pps

[Jason Gunthorpe]

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@mellanox.com>
> > > >
> > > > 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@vger.kernel.org>
> > > > Fixes: 1dd017882e01 ("RDMA/core: Fix protection fault in
> > > > get_pkey_idx_qp_list")
> > > > Signed-off-by: Maor Gottlieb <maorg@mellanox.com>
> > > > Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> > > >  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 :)

Jason

RE: [PATCH rdma-rc] RDMA/core: Fix pkey and port assignment in get_new_pps

[Marciniszyn, Mike]

> -----Original Message-----
> From: Jason Gunthorpe <jgg@mellanox.com>
> Sent: Friday, February 28, 2020 10:03 AM
> To: Marciniszyn, Mike <mike.marciniszyn@intel.com>
> Cc: Leon Romanovsky <leon@kernel.org>; Doug Ledford
> <dledford@redhat.com>; Maor Gottlieb <maorg@mellanox.com>; linux-
> rdma@vger.kernel.org
> 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@mellanox.com>
> > > > >
> > > > > 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@vger.kernel.org>
> > > > > Fixes: 1dd017882e01 ("RDMA/core: Fix protection fault in
> > > > > get_pkey_idx_qp_list")
> > > > > Signed-off-by: Maor Gottlieb <maorg@mellanox.com>
> > > > > Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> > > > >  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@intel.com>

Mike

Re: [PATCH rdma-rc] RDMA/core: Fix pkey and port assignment in get_new_pps

[Jason Gunthorpe]

On Thu, Feb 27, 2020 at 02:57:28PM +0200, Leon Romanovsky wrote:
> From: Maor Gottlieb <maorg@mellanox.com>
> 
> 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@vger.kernel.org>
> Fixes: 1dd017882e01 ("RDMA/core: Fix protection fault in get_pkey_idx_qp_list")
> Signed-off-by: Maor Gottlieb <maorg@mellanox.com>
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> ---
>  drivers/infiniband/core/security.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)

Applied to for-rc, thanks

Jason