| Message ID | 0-v1-4608c5610afa+fb-uverbs_cmd_post_send_fix_jgg@nvidia.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | RDMA/rxe,siw: Restore uverbs_cmd_mask IB_USER_VERBS_CMD_POST_SEND | expand |
On 10/30/20 9:03 AM, Jason Gunthorpe wrote: > These two drivers open code the call to POST_SEND and do not use the > rdma-core wrapper to do it, thus their usages was missed during the audit. > > Both drivers use this as a doorbell to signal the kernel to start DMA. > > Fixes: 628c02bf38aa ("RDMA: Remove uverbs cmds from drivers that don't use them") > Reported-by: Bob Pearson <rpearsonhpe@gmail.com> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/infiniband/sw/rxe/rxe_verbs.c | 3 ++- > drivers/infiniband/sw/siw/siw_main.c | 2 ++ > 2 files changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.c b/drivers/infiniband/sw/rxe/rxe_verbs.c > index 7652d53af2c1d9..209c7b3fab97a2 100644 > --- a/drivers/infiniband/sw/rxe/rxe_verbs.c > +++ b/drivers/infiniband/sw/rxe/rxe_verbs.c > @@ -1142,7 +1142,8 @@ int rxe_register_device(struct rxe_dev *rxe, const char *ibdev_name) > dma_set_max_seg_size(&dev->dev, UINT_MAX); > dma_set_coherent_mask(&dev->dev, dma_get_required_mask(&dev->dev)); > > - dev->uverbs_cmd_mask |= BIT_ULL(IB_USER_VERBS_CMD_REQ_NOTIFY_CQ); > + dev->uverbs_cmd_mask |= BIT_ULL(IB_USER_VERBS_CMD_POST_SEND) | > + BIT_ULL(IB_USER_VERBS_CMD_REQ_NOTIFY_CQ); > > ib_set_device_ops(dev, &rxe_dev_ops); > err = ib_device_set_netdev(&rxe->ib_dev, rxe->ndev, 1); > diff --git a/drivers/infiniband/sw/siw/siw_main.c b/drivers/infiniband/sw/siw/siw_main.c > index e49faefdee923d..9cf596429dbf7d 100644 > --- a/drivers/infiniband/sw/siw/siw_main.c > +++ b/drivers/infiniband/sw/siw/siw_main.c > @@ -347,6 +347,8 @@ static struct siw_device *siw_device_create(struct net_device *netdev) > addr); > } > > + base_dev->uverbs_cmd_mask |= BIT_ULL(IB_USER_VERBS_CMD_POST_SEND); > + > base_dev->node_type = RDMA_NODE_RNIC; > memcpy(base_dev->node_desc, SIW_NODE_DESC_COMMON, > sizeof(SIW_NODE_DESC_COMMON)); > This the right short term fix but seriously why not just completely kill off uverbs_cmd_mask? If a driver sets a non-NULL value in ib_device_ops assume it was done for a reason and allow commands through. Is there any example of a driver using uverbs_cmd_mask to dynamically enable/disable a verb? I thought this was your plan when you mentioned this change a while back. Your comment about open coding the post send command is valid. I don't recall all the history that got us to where we are now but it looks like calling ibv_cmd_post_send could be made to work. Since wr_count will be zero a lot of extra work will not happen but there is still more work than needed. The only thing I would be worried about is that it assumes the wqes are struct ib_uverbs_send_wr's but at least for rxe they are expanded. Have to make sure there aren't any bad side effects now or in the future from e.g. sending the wrong wqe size to the kernel. Bob
On Fri, Oct 30, 2020 at 11:18:15AM -0500, Bob Pearson wrote: > This the right short term fix but seriously why not just completely > kill off uverbs_cmd_mask? If a driver sets a non-NULL value in > ib_device_ops assume it was done for a reason and allow commands > through. Is there any example of a driver using uverbs_cmd_mask to > dynamically enable/disable a verb? I thought this was your plan when > you mentioned this change a while back. The remaining ops are all shared with the kernel, so nearly all drivers set the post_send op but only a few are prepared for it to be called from userspace. It is an unfortuante side effect of co-mingling the kernel and user API. The only way out would be to split the remaining ops into user/kernel versions and require drivers to set the user op pointer. These would be effected: rdi->ibdev.uverbs_cmd_mask |= (1ull << IB_USER_VERBS_CMD_POLL_CQ) | (1ull << IB_USER_VERBS_CMD_REQ_NOTIFY_CQ) | (1ull << IB_USER_VERBS_CMD_POST_SEND) | (1ull << IB_USER_VERBS_CMD_POST_RECV) | (1ull << IB_USER_VERBS_CMD_POST_SRQ_RECV); I thought about doing it, maybe I should check again Jason
On Fri, Oct 30, 2020 at 11:03:05AM -0300, Jason Gunthorpe wrote: > These two drivers open code the call to POST_SEND and do not use the > rdma-core wrapper to do it, thus their usages was missed during the audit. > > Both drivers use this as a doorbell to signal the kernel to start DMA. > > Fixes: 628c02bf38aa ("RDMA: Remove uverbs cmds from drivers that don't use them") > Reported-by: Bob Pearson <rpearsonhpe@gmail.com> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/infiniband/sw/rxe/rxe_verbs.c | 3 ++- > drivers/infiniband/sw/siw/siw_main.c | 2 ++ > 2 files changed, 4 insertions(+), 1 deletion(-) Applied to for-next Jason
diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.c b/drivers/infiniband/sw/rxe/rxe_verbs.c index 7652d53af2c1d9..209c7b3fab97a2 100644 --- a/drivers/infiniband/sw/rxe/rxe_verbs.c +++ b/drivers/infiniband/sw/rxe/rxe_verbs.c @@ -1142,7 +1142,8 @@ int rxe_register_device(struct rxe_dev *rxe, const char *ibdev_name) dma_set_max_seg_size(&dev->dev, UINT_MAX); dma_set_coherent_mask(&dev->dev, dma_get_required_mask(&dev->dev)); - dev->uverbs_cmd_mask |= BIT_ULL(IB_USER_VERBS_CMD_REQ_NOTIFY_CQ); + dev->uverbs_cmd_mask |= BIT_ULL(IB_USER_VERBS_CMD_POST_SEND) | + BIT_ULL(IB_USER_VERBS_CMD_REQ_NOTIFY_CQ); ib_set_device_ops(dev, &rxe_dev_ops); err = ib_device_set_netdev(&rxe->ib_dev, rxe->ndev, 1); diff --git a/drivers/infiniband/sw/siw/siw_main.c b/drivers/infiniband/sw/siw/siw_main.c index e49faefdee923d..9cf596429dbf7d 100644 --- a/drivers/infiniband/sw/siw/siw_main.c +++ b/drivers/infiniband/sw/siw/siw_main.c @@ -347,6 +347,8 @@ static struct siw_device *siw_device_create(struct net_device *netdev) addr); } + base_dev->uverbs_cmd_mask |= BIT_ULL(IB_USER_VERBS_CMD_POST_SEND); + base_dev->node_type = RDMA_NODE_RNIC; memcpy(base_dev->node_desc, SIW_NODE_DESC_COMMON, sizeof(SIW_NODE_DESC_COMMON));
These two drivers open code the call to POST_SEND and do not use the rdma-core wrapper to do it, thus their usages was missed during the audit. Both drivers use this as a doorbell to signal the kernel to start DMA. Fixes: 628c02bf38aa ("RDMA: Remove uverbs cmds from drivers that don't use them") Reported-by: Bob Pearson <rpearsonhpe@gmail.com> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> --- drivers/infiniband/sw/rxe/rxe_verbs.c | 3 ++- drivers/infiniband/sw/siw/siw_main.c | 2 ++ 2 files changed, 4 insertions(+), 1 deletion(-)