Skip to contentSkip to conversation

RDMA/efa: Clear the admin command buffer prior to its submission

PATCH Applied Gal Pressman 2 participants 5 messages Nov 12, 2019 – Nov 14, 2019

Subsystems (2)
InfiniBand Amazon RDMA efa driver
lore
Share & tools
lore

Gal Pressman

author
#0 lore raw

PATCH 1/1 RDMA/efa: Clear the admin command buffer prior to its submission

We cannot rely on the entry memcpy as we only copy the actual size of the command, the rest of the bytes must be memset to zero.

Fixes 0420e542569b ("RDMA/efa: Implement functions that submit and complete admin commands")
Reviewed-by Daniel Kranzdorf <[email protected]>
Reviewed-by Firas JahJah <[email protected]>
Signed-off-by Gal Pressman <[email protected]>
---
 drivers/infiniband/hw/efa/efa_com.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/infiniband/hw/efa/efa_com.c +4 -1
index 3c412bc5b94f..0778f4f7dccd 100644
@@ -317,6 +317,7 @@ static struct efa_comp_ctx *__efa_com_submit_admin_cmd(struct efa_com_admin_queu
 						       struct efa_admin_acq_entry *comp,
 						       size_t comp_size_in_bytes)
 {
+	struct efa_admin_aq_entry *aqe;
 	struct efa_comp_ctx *comp_ctx;
 	u16 queue_size_mask;
 	u16 cmd_id;

@@ -350,7 +351,9 @@ static struct efa_comp_ctx *__efa_com_submit_admin_cmd(struct efa_com_admin_queu
 
 	reinit_completion(&comp_ctx->wait_event);
 
-	memcpy(&aq->sq.entries[pi], cmd, cmd_size_in_bytes);
+	aqe = &aq->sq.entries[pi];
+	memset(aqe, 0, sizeof(*aqe));
+	memcpy(aqe, cmd, cmd_size_in_bytes);
 
 	aq->sq.pc++;
 	atomic64_inc(&aq->stats.submitted_cmd);
Signature · 1 line
2.23.0
#1 lore raw
Quoted 10 lines · Gal Pressman

We cannot rely on the entry memcpy as we only copy the actual size of the command, the rest of the bytes must be memset to zero.

Fixes 0420e542569b ("RDMA/efa: Implement functions that submit and complete admin commands")
Reviewed-by Daniel Kranzdorf <[email protected]>
Reviewed-by Firas JahJah <[email protected]>
Signed-off-by Gal Pressman <[email protected]>
---
 drivers/infiniband/hw/efa/efa_com.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

This is quite late in the -rc cycle for such a vauge description. What is the user visible impact of passing non-zero memory beyond the command length?

Jason

author
#2 lore raw
Quoted 12 lines · Jason Gunthorpe
Quoted 10 lines · Gal Pressman

We cannot rely on the entry memcpy as we only copy the actual size of the command, the rest of the bytes must be memset to zero.

Fixes 0420e542569b ("RDMA/efa: Implement functions that submit and complete admin commands")
Reviewed-by Daniel Kranzdorf <[email protected]>
Reviewed-by Firas JahJah <[email protected]>
Signed-off-by Gal Pressman <[email protected]>
---
 drivers/infiniband/hw/efa/efa_com.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
Quoted 3 lines

This is quite late in the -rc cycle for such a vauge description. What is the user visible impact of passing non-zero memory beyond the command length?

Currently providing non-zero memory will not have any user visible impact. However, since admin commands are extendable (in a backwards compatible way) everything beyond the size of the command must be cleared to prevent issues in the future.

Applied
#3 lore raw
In reply to Gal Pressman
Quoted 10 lines · Gal Pressman

We cannot rely on the entry memcpy as we only copy the actual size of the command, the rest of the bytes must be memset to zero.

Fixes 0420e542569b ("RDMA/efa: Implement functions that submit and complete admin commands")
Reviewed-by Daniel Kranzdorf <[email protected]>
Reviewed-by Firas JahJah <[email protected]>
Signed-off-by Gal Pressman <[email protected]>
---
 drivers/infiniband/hw/efa/efa_com.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

This isn't really -rc material since the device will have to be compatible with these old kernels for quite some time.

Applied to for-next with the better description

Thanks,
Jason

author
#4 lore raw
Quoted 12 lines · Jason Gunthorpe
Quoted 10 lines · Gal Pressman

We cannot rely on the entry memcpy as we only copy the actual size of the command, the rest of the bytes must be memset to zero.

Fixes 0420e542569b ("RDMA/efa: Implement functions that submit and complete admin commands")
Reviewed-by Daniel Kranzdorf <[email protected]>
Reviewed-by Firas JahJah <[email protected]>
Signed-off-by Gal Pressman <[email protected]>
---
 drivers/infiniband/hw/efa/efa_com.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
Quoted 2 lines

This isn't really -rc material since the device will have to be compatible with these old kernels for quite some time.

That's why I pushed it to -rc, this bug fix must be applied to these kernels (5.2, 5.3) as well through stable.

If we're trying to avoid pushing this change late in the -rc cycle, I'm fine with this patch going through -next and backported to 5.4 stable as well.