RDMA/core: Fix pkey and port assignment in get_new_pps

PATCH Applied Leon Romanovsky 4 participants 7 messages Feb 27, 2020 – Feb 28, 2020 InfiniBand

lore

Share & tools

lore

Leon Romanovsky

author

2020-02-27 12:57 UTC #0 lore raw

PATCH 1/1 RDMA/core: Fix pkey and port assignment in get_new_pps

From: Maor Gottlieb <[email protected]>

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 <[email protected]>

Fixes 1dd017882e01 ("RDMA/core: Fix protection fault in get_pkey_idx_qp_list")

Signed-off-by Maor Gottlieb <[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---
 drivers/infiniband/core/security.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

drivers/infiniband/core/security.c +8 -4

index b9a36ea244d4..2d5608315dc8 100644
@@ -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;

Signature · 1 line

2.24.1

Marciniszyn, Mike

2020-02-27 14:28 UTC #1 lore raw

Quoted 41 lines

From: Maor Gottlieb <[email protected]>
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 <[email protected]>
Fixes 1dd017882e01 ("RDMA/core: Fix protection fault in
get_pkey_idx_qp_list")
Signed-off-by Maor Gottlieb <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
 drivers/infiniband/core/security.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)
drivers/infiniband/core/security.c +4 -3
index b9a36ea244d4..2d5608315dc8 100644
@@ -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-
Quoted 1 line
main.pkey_index :
drivers/infiniband/core/security.c +4 -1
-						      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 &

Quoted 3 lines

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

Leon Romanovsky

author

2020-02-27 19:01 UTC #2 lore raw

Quoted 50 lines · Mike

Quoted 41 lines
From: Maor Gottlieb <[email protected]>
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 <[email protected]>
Fixes 1dd017882e01 ("RDMA/core: Fix protection fault in
get_pkey_idx_qp_list")
Signed-off-by Maor Gottlieb <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
 drivers/infiniband/core/security.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)
drivers/infiniband/core/security.c +4 -3
index b9a36ea244d4..2d5608315dc8 100644
@@ -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-
Quoted 1 line
main.pkey_index :
drivers/infiniband/core/security.c +4 -1
-						      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 &
Quoted 3 lines
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.

Quoted 7 lines

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?

Quoted 14 lines

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

Marciniszyn, Mike

2020-02-27 22:49 UTC #3 lore raw

Quoted 58 lines · Mike

Quoted 15 lines
Quoted 15 lines
From: Maor Gottlieb <[email protected]>
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 <[email protected]>
Fixes 1dd017882e01 ("RDMA/core: Fix protection fault in
get_pkey_idx_qp_list")
Signed-off-by Maor Gottlieb <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
 drivers/infiniband/core/security.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)
Quoted 6 lines
drivers/infiniband/core/security.c static struct ib_ports_pkeys
Quoted 6 lines
drivers/infiniband/core/security.c +0 -0
index b9a36ea244d4..2d5608315dc8 100644
@@ -340,11 +340,15 @@ static struct ib_ports_pkeys
*get_new_pps(const
Quoted 14 lines
Quoted 14 lines
drivers/infiniband/core/security.c +4 -3
 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-
Quoted 1 line
main.pkey_index :
Quoted 11 lines
Quoted 9 lines
drivers/infiniband/core/security.c +4 -1
-						      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
Quoted 3 lines
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.
Quoted 5 lines
The sequence I'm concerned about is:
0x71 attr mask with both pkey index and port.

Quoted 8 lines

Quoted 1 line
A ulp decides to change the pkey index and does an 0x51 modify without
setting the unit.
Quoted 2 lines
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

Jason Gunthorpe

2020-02-28 15:02 UTC #4 lore raw

Quoted 81 lines · Mike

Quoted 56 lines · Mike
Quoted 14 lines
Quoted 14 lines
From: Maor Gottlieb <[email protected]>
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 <[email protected]>
Fixes 1dd017882e01 ("RDMA/core: Fix protection fault in
get_pkey_idx_qp_list")
Signed-off-by Maor Gottlieb <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
 drivers/infiniband/core/security.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)
Quoted 5 lines
drivers/infiniband/core/security.c static struct ib_ports_pkeys
Quoted 5 lines
drivers/infiniband/core/security.c +1 -0
index b9a36ea244d4..2d5608315dc8 100644
+++ b/drivers/infiniband/core/security.c
@@ -340,11 +340,15 @@ static struct ib_ports_pkeys
*get_new_pps(const
Quoted 14 lines
Quoted 14 lines
drivers/infiniband/core/security.c +4 -3
 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-
Quoted 1 line
main.pkey_index :
Quoted 11 lines
Quoted 9 lines
drivers/infiniband/core/security.c +4 -1
-						      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
Quoted 3 lines
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.
Quoted 5 lines
The sequence I'm concerned about is:
0x71 attr mask with both pkey index and port.
Quoted 8 lines
Quoted 1 line
A ulp decides to change the pkey index and does an 0x51 modify without
setting the unit.
Quoted 2 lines
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;
	}

Quoted 1 line

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

Marciniszyn, Mike

Tested-by

2020-02-28 15:12 UTC #5 lore raw

Quoted 105 lines

-----Original Message-----
From: Jason Gunthorpe <[email protected]>
Sent: Friday, February 28, 2020 10:03 AM
To: Marciniszyn, Mike <[email protected]>
Cc: Leon Romanovsky <[email protected]>; Doug Ledford <[email protected]>; Maor Gottlieb <[email protected]>; linux- [email protected]
Subject: Re: [PATCH rdma-rc] RDMA/core: Fix pkey and port assignment in get_new_pps
Quoted 21 lines · Mike
Quoted 21 lines · Mike
Quoted 14 lines
Quoted 14 lines
From: Maor Gottlieb <[email protected]>
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 <[email protected]>
Fixes 1dd017882e01 ("RDMA/core: Fix protection fault in
get_pkey_idx_qp_list")
Signed-off-by Maor Gottlieb <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
 drivers/infiniband/core/security.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)
Quoted 5 lines
drivers/infiniband/core/security.c static struct ib_ports_pkeys
Quoted 5 lines
drivers/infiniband/core/security.c +1 -0
index b9a36ea244d4..2d5608315dc8 100644
+++ b/drivers/infiniband/core/security.c
@@ -340,11 +340,15 @@ static struct ib_ports_pkeys
*get_new_pps(const
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
drivers/infiniband/core/security.c static struct ib_ports_pkeys *get_new_pps(const struct ib_qp *qp,
Quoted 6 lines
drivers/infiniband/core/security.c +0 -2
 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-
Quoted 8 lines
Quoted 4 lines
Quoted 4 lines
drivers/infiniband/core/security.c static struct ib_ports_pkeys *get_new_pps(const struct ib_qp *qp,
Quoted 4 lines
drivers/infiniband/core/security.c +3 -0
 >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;
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
drivers/infiniband/core/security.c +1 -1
+
 	if (qp_attr_mask & IB_QP_PKEY_INDEX)
-		new_pps->main.pkey_index = (qp_pps) ? qp_pps-
Quoted 9 lines
Quoted 2 lines
Quoted 2 lines
drivers/infiniband/core/security.c
Quoted 2 lines
Quoted 1 line
main.pkey_index :
drivers/infiniband/core/security.c +0 -1
-						      qp_attr-
pkey_index;
Quoted 3 lines
Quoted 3 lines
drivers/infiniband/core/security.c static struct ib_ports_pkeys *get_new_pps(const struct ib_qp *qp,
Quoted 3 lines
drivers/infiniband/core/security.c +3 -0
+		new_pps->main.pkey_index = qp_attr->pkey_index;
+	else if (qp_pps)
+		new_pps->main.pkey_index = qp_pps-
main.pkey_index;
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
drivers/infiniband/core/security.c +1 -0
+
 	if ((qp_attr_mask & IB_QP_PKEY_INDEX) && (qp_attr_mask
&
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 3 lines
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
Quoted 11 lines
Quoted 11 lines
correct unit
Quoted 3 lines
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.
Quoted 6 lines
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
Quoted 3 lines
Quoted 3 lines
setting the unit.
Quoted 2 lines
I see new_pps->main.state being returned as 0 and
port_pkey_list_insert()
Quoted 3 lines
Quoted 3 lines
will early out.
I see, maybe we can store the main.state in qps and restore it from
there?
Quoted 7 lines
Quoted 1 line
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))
Quoted 2 lines
		new_pps->main.state = IB_PORT_PKEY_VALID;
	 else if ((qp_attr_mask & (IB_QP_PKEY_INDEX | IB_QP_PORT)) &&
qp_pps) {
Quoted 9 lines
                             /*
		 * 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;
	}

Quoted 5 lines

Quoted 1 line
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 <[email protected]>

Mike

Jason Gunthorpe

Applied

2020-02-28 15:23 UTC #6 lore raw

In reply to Leon Romanovsky

Quoted 5 lines · Leon Romanovsky

From: Maor Gottlieb <[email protected]>
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.

Quoted 7 lines

Cc <[email protected]>
Fixes 1dd017882e01 ("RDMA/core: Fix protection fault in get_pkey_idx_qp_list")
Signed-off-by Maor Gottlieb <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
 drivers/infiniband/core/security.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

Applied to for-rc, thanks

Jason