| Message ID | 20191112092608.46964-1-galpress@amazon.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 64c264872b8879e2ab9017eefe9514d4c045c60e |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | [for-rc] RDMA/efa: Clear the admin command buffer prior to its submission | expand |
On Tue, Nov 12, 2019 at 11:26:08AM +0200, Gal Pressman wrote: > 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 <dkkranzd@amazon.com> > Reviewed-by: Firas JahJah <firasj@amazon.com> > Signed-off-by: Gal Pressman <galpress@amazon.com> > --- > 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
On 13/11/2019 2:17, Jason Gunthorpe wrote: > On Tue, Nov 12, 2019 at 11:26:08AM +0200, Gal Pressman wrote: >> 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 <dkkranzd@amazon.com> >> Reviewed-by: Firas JahJah <firasj@amazon.com> >> Signed-off-by: Gal Pressman <galpress@amazon.com> >> --- >> 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? 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.
On Tue, Nov 12, 2019 at 11:26:08AM +0200, Gal Pressman wrote: > 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 <dkkranzd@amazon.com> > Reviewed-by: Firas JahJah <firasj@amazon.com> > Signed-off-by: Gal Pressman <galpress@amazon.com> > --- > 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
On 14/11/2019 17:59, Jason Gunthorpe wrote: > On Tue, Nov 12, 2019 at 11:26:08AM +0200, Gal Pressman wrote: >> 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 <dkkranzd@amazon.com> >> Reviewed-by: Firas JahJah <firasj@amazon.com> >> Signed-off-by: Gal Pressman <galpress@amazon.com> >> --- >> 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. 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.
diff --git a/drivers/infiniband/hw/efa/efa_com.c b/drivers/infiniband/hw/efa/efa_com.c index 3c412bc5b94f..0778f4f7dccd 100644 --- a/drivers/infiniband/hw/efa/efa_com.c +++ b/drivers/infiniband/hw/efa/efa_com.c @@ -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);
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 <dkkranzd@amazon.com> Reviewed-by: Firas JahJah <firasj@amazon.com> Signed-off-by: Gal Pressman <galpress@amazon.com> --- drivers/infiniband/hw/efa/efa_com.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)