IB/mlx5: Prevent allocating UMEM and UCTX as some general object

PATCH Applied Leon Romanovsky 4 participants 7 messages Dec 19, 2018 – Dec 21, 2018

Subsystems (5)

Networking Network drivers InfiniBand Mellanox mlx5 core +1 more subsystems

lore

Share & tools

lore

Leon Romanovsky

author

2018-12-19 14:28 UTC #0 lore raw

PATCH 1/1 IB/mlx5: Prevent allocating UMEM and UCTX as some general object

From: Yishai Hadas <[email protected]>

The driver needs to prevent a user space application to create a UMEM and UCTX via the general object command.

The UMEM must go through the kernel UMEM_REG method to prevent the user from setting physical addresses by himself. The UCTX is some internal kernel object and shouldn't be exposed.

As of not being any more part of the general object the caps bits were moved to be some log_xxx indication in the general HCA caps, 0 means not supported.

The firmware code was adapted as well to match the above.

Signed-off-by Yishai Hadas <[email protected]>

Reviewed-by Achiad Shochat <[email protected]>

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

---
 drivers/infiniband/hw/mlx5/devx.c | 34 ++++++++---------
 drivers/infiniband/hw/mlx5/main.c |  3 +-
 include/linux/mlx5/mlx5_ifc.h     | 62 +++++++++++++++++++++----------
 3 files changed, 58 insertions(+), 41 deletions(-)

drivers/infiniband/hw/mlx5/devx.c +15 ~~-19~~

index 5271469aad10..dcc7c974173f 100644
@@ -51,26 +51,21 @@ int mlx5_ib_devx_create(struct mlx5_ib_dev *dev, bool is_user)
 {
 	u32 in[MLX5_ST_SZ_DW(create_uctx_in)] = {0};
 	u32 out[MLX5_ST_SZ_DW(general_obj_out_cmd_hdr)] = {0};
-	u64 general_obj_types;
-	void *hdr, *uctx;
+	void *uctx;
 	int err;
 	u16 uid;
 	u32 cap = 0;
 
-	hdr = MLX5_ADDR_OF(create_uctx_in, in, hdr);
-	uctx = MLX5_ADDR_OF(create_uctx_in, in, uctx);
-
-	general_obj_types = MLX5_CAP_GEN_64(dev->mdev, general_obj_types);
-	if (!(general_obj_types & MLX5_GENERAL_OBJ_TYPES_CAP_UCTX) ||
-	    !(general_obj_types & MLX5_GENERAL_OBJ_TYPES_CAP_UMEM))
+	/* 0 means not supported */
+	if (!MLX5_CAP_GEN(dev->mdev, log_max_uctx))
 		return -EINVAL;
 
+	uctx = MLX5_ADDR_OF(create_uctx_in, in, uctx);
 	if (is_user && capable(CAP_NET_RAW) &&
 	    (MLX5_CAP_GEN(dev->mdev, uctx_cap) & MLX5_UCTX_CAP_RAW_TX))
 		cap |= MLX5_UCTX_CAP_RAW_TX;
 
-	MLX5_SET(general_obj_in_cmd_hdr, hdr, opcode, MLX5_CMD_OP_CREATE_GENERAL_OBJECT);
-	MLX5_SET(general_obj_in_cmd_hdr, hdr, obj_type, MLX5_OBJ_TYPE_UCTX);
+	MLX5_SET(create_uctx_in, in, opcode, MLX5_CMD_OP_CREATE_UCTX);
 	MLX5_SET(uctx, uctx, cap, cap);
 
 	err = mlx5_cmd_exec(dev->mdev, in, sizeof(in), out, sizeof(out));

@@ -83,12 +78,11 @@ int mlx5_ib_devx_create(struct mlx5_ib_dev *dev, bool is_user)
 
 void mlx5_ib_devx_destroy(struct mlx5_ib_dev *dev, u16 uid)
 {
-	u32 in[MLX5_ST_SZ_DW(general_obj_in_cmd_hdr)] = {0};
+	u32 in[MLX5_ST_SZ_DW(destroy_uctx_in)] = {0};
 	u32 out[MLX5_ST_SZ_DW(general_obj_out_cmd_hdr)] = {0};
 
-	MLX5_SET(general_obj_in_cmd_hdr, in, opcode, MLX5_CMD_OP_DESTROY_GENERAL_OBJECT);
-	MLX5_SET(general_obj_in_cmd_hdr, in, obj_type, MLX5_OBJ_TYPE_UCTX);
-	MLX5_SET(general_obj_in_cmd_hdr, in, obj_id, uid);
+	MLX5_SET(destroy_uctx_in, in, opcode, MLX5_CMD_OP_DESTROY_UCTX);
+	MLX5_SET(destroy_uctx_in, in, uid, uid);
 
 	mlx5_cmd_exec(dev->mdev, in, sizeof(in), out, sizeof(out));
 }

@@ -861,6 +855,10 @@ static void devx_obj_build_destroy_cmd(void *in, void *out, void *din,
 		MLX5_SET(general_obj_in_cmd_hdr, din, obj_type, obj_type);
 		break;
 
+	case MLX5_CMD_OP_CREATE_UMEM:
+		MLX5_SET(general_obj_in_cmd_hdr, din, opcode,
+			 MLX5_CMD_OP_DESTROY_UMEM);
+		break;
 	case MLX5_CMD_OP_CREATE_MKEY:
 		MLX5_SET(general_obj_in_cmd_hdr, din, opcode, MLX5_CMD_OP_DESTROY_MKEY);
 		break;

@@ -1234,8 +1232,7 @@ static void devx_umem_reg_cmd_build(struct mlx5_ib_dev *dev,
 	umem = MLX5_ADDR_OF(create_umem_in, cmd->in, umem);
 	mtt = (__be64 *)MLX5_ADDR_OF(umem, umem, mtt);
 
-	MLX5_SET(general_obj_in_cmd_hdr, cmd->in, opcode, MLX5_CMD_OP_CREATE_GENERAL_OBJECT);
-	MLX5_SET(general_obj_in_cmd_hdr, cmd->in, obj_type, MLX5_OBJ_TYPE_UMEM);
+	MLX5_SET(create_umem_in, cmd->in, opcode, MLX5_CMD_OP_CREATE_UMEM);
 	MLX5_SET64(umem, umem, num_of_mtt, obj->ncont);
 	MLX5_SET(umem, umem, log_page_size, obj->page_shift -
 					    MLX5_ADAPTER_PAGE_SHIFT);

@@ -1274,7 +1271,7 @@ static int UVERBS_HANDLER(MLX5_IB_METHOD_DEVX_UMEM_REG)(
 
 	devx_umem_reg_cmd_build(dev, obj, &cmd);
 
-	MLX5_SET(general_obj_in_cmd_hdr, cmd.in, uid, c->devx_uid);
+	MLX5_SET(create_umem_in, cmd.in, uid, c->devx_uid);
 	err = mlx5_cmd_exec(dev->mdev, cmd.in, cmd.inlen, cmd.out,
 			    sizeof(cmd.out));
 	if (err)

@@ -1445,8 +1442,7 @@ static bool devx_is_supported(struct ib_device *device)
 {
 	struct mlx5_ib_dev *dev = to_mdev(device);
 
-	return !dev->rep && MLX5_CAP_GEN_64(dev->mdev, general_obj_types) &
-				    MLX5_GENERAL_OBJ_TYPES_CAP_UCTX;
+	return !dev->rep && MLX5_CAP_GEN(dev->mdev, log_max_uctx);
 }
 
 const struct uapi_definition mlx5_ib_devx_defs[] = {

drivers/infiniband/hw/mlx5/main.c +1 -2

index f918a15f1152..6584e638387a 100644
@@ -5120,8 +5120,7 @@ static int mlx5_ib_alloc_counters(struct mlx5_ib_dev *dev)
 	int i;
 	bool is_shared;
 
-	is_shared = MLX5_CAP_GEN_64(dev->mdev, general_obj_types) &
-				MLX5_GENERAL_OBJ_TYPES_CAP_UCTX;
+	is_shared = MLX5_CAP_GEN(dev->mdev, log_max_uctx) != 0;
 
 	for (i = 0; i < dev->num_ports; i++) {
 		err = __mlx5_ib_alloc_counters(dev, &dev->port[i].cnts);

include/linux/mlx5/mlx5_ifc.h +42 ~~-20~~

101 lines not on this page · raw message

Signature · 1 line

2.19.1

Jason Gunthorpe

2018-12-19 19:14 UTC #1 lore raw

Quoted 9 lines · Leon Romanovsky

From: Yishai Hadas <[email protected]>
The driver needs to prevent a user space application to create a UMEM and UCTX via the general object command.
The UMEM must go through the kernel UMEM_REG method to prevent the user from setting physical addresses by himself. The UCTX is some internal kernel object and shouldn't be exposed.

Quoted 5 lines

As of not being any more part of the general object the caps bits were moved to be some log_xxx indication in the general HCA caps, 0 means not supported.
The firmware code was adapted as well to match the above.

This commit message is a bit wonky.. how about

IB/mlx5: Use the correct commands for UMEM and UCTX allocation

During testing the command format was changed to close a security hole. Revise the driver to use the command format that will actually be supported in GA firmware.

Both the UMEM and UCTX are intended only for use by the kernel and cannot be executed using a general command.

Since the UMEM and CTX are not part of the general object the caps bits were moved to be some log_xxx location in the general HCA caps.

Quoted 3 lines

Signed-off-by Yishai Hadas <[email protected]>
Reviewed-by Achiad Shochat <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>

Also add a fixes line please, any kernel with the devx needs this patch to work with GA firmware.

Quoted 4 lines

 drivers/infiniband/hw/mlx5/devx.c | 34 ++++++++---------
 drivers/infiniband/hw/mlx5/main.c |  3 +-
 include/linux/mlx5/mlx5_ifc.h     | 62 +++++++++++++++++++++----------
 3 files changed, 58 insertions(+), 41 deletions(-)

Otherwise the patch looks fine, please apply to the shared branch..

Thanks,
Jason

Leon Romanovsky

author

2018-12-20 06:21 UTC #2 lore raw

Quoted 24 lines · Jason Gunthorpe

Quoted 9 lines · Leon Romanovsky
From: Yishai Hadas <[email protected]>
The driver needs to prevent a user space application to create a UMEM and UCTX via the general object command.
The UMEM must go through the kernel UMEM_REG method to prevent the user from setting physical addresses by himself. The UCTX is some internal kernel object and shouldn't be exposed.
Quoted 5 lines
As of not being any more part of the general object the caps bits were moved to be some log_xxx indication in the general HCA caps, 0 means not supported.
The firmware code was adapted as well to match the above.
This commit message is a bit wonky.. how about
IB/mlx5: Use the correct commands for UMEM and UCTX allocation
During testing the command format was changed to close a security hole. Revise the driver to use the command format that will actually be supported in GA firmware.

Quoted 5 lines

Both the UMEM and UCTX are intended only for use by the kernel and cannot be executed using a general command.
Since the UMEM and CTX are not part of the general object the caps bits were moved to be some log_xxx location in the general HCA caps.

I'm fine with this description too.

Quoted 7 lines

Quoted 4 lines
Signed-off-by Yishai Hadas <[email protected]>
Reviewed-by Achiad Shochat <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
Also add a fixes line please, any kernel with the devx needs this patch to work with GA firmware.

Fixes a8b92ca1b0e5 ("IB/mlx5: Introduce DEVX")

Quoted 7 lines

Quoted 5 lines

 drivers/infiniband/hw/mlx5/devx.c | 34 ++++++++---------
 drivers/infiniband/hw/mlx5/main.c |  3 +-
 include/linux/mlx5/mlx5_ifc.h     | 62 +++++++++++++++++++++----------
 3 files changed, 58 insertions(+), 41 deletions(-)

Otherwise the patch looks fine, please apply to the shared branch..

Jason,

I have procedural question. This patch is based on rdma-next and there is diversion in both devx.c and main.c in those areas, enough do not apply cleanly. Will it be easier if you take this patch to rdma-next, instead me applying different patch to mlx5-next and you hitting merge conflict later on while trying to merge it into rdma-next?

Thanks

Quoted 3 lines

Thanks,
Jason

Jason Gunthorpe

2018-12-20 17:02 UTC #3 lore raw

Quoted 47 lines · Leon Romanovsky

Quoted 24 lines · Jason Gunthorpe
Quoted 9 lines · Leon Romanovsky
From: Yishai Hadas <[email protected]>
The driver needs to prevent a user space application to create a UMEM and UCTX via the general object command.
The UMEM must go through the kernel UMEM_REG method to prevent the user from setting physical addresses by himself. The UCTX is some internal kernel object and shouldn't be exposed.
Quoted 5 lines
As of not being any more part of the general object the caps bits were moved to be some log_xxx indication in the general HCA caps, 0 means not supported.
The firmware code was adapted as well to match the above.
This commit message is a bit wonky.. how about
IB/mlx5: Use the correct commands for UMEM and UCTX allocation
During testing the command format was changed to close a security hole. Revise the driver to use the command format that will actually be supported in GA firmware.
Quoted 5 lines
Both the UMEM and UCTX are intended only for use by the kernel and cannot be executed using a general command.
Since the UMEM and CTX are not part of the general object the caps bits were moved to be some log_xxx location in the general HCA caps.
I'm fine with this description too.
Quoted 7 lines
Quoted 4 lines
Signed-off-by Yishai Hadas <[email protected]>
Reviewed-by Achiad Shochat <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
Also add a fixes line please, any kernel with the devx needs this patch to work with GA firmware.
Fixes a8b92ca1b0e5 ("IB/mlx5: Introduce DEVX")
Quoted 4 lines
Quoted 4 lines
 drivers/infiniband/hw/mlx5/devx.c | 34 ++++++++---------
 drivers/infiniband/hw/mlx5/main.c |  3 +-
 include/linux/mlx5/mlx5_ifc.h     | 62 +++++++++++++++++++++--------

Leon Romanovsky

author

2018-12-20 17:13 UTC #4 lore raw

Quoted 54 lines · Jason Gunthorpe

Quoted 53 lines · Leon Romanovsky
Quoted 24 lines · Jason Gunthorpe
Quoted 9 lines · Leon Romanovsky
From: Yishai Hadas <[email protected]>
The driver needs to prevent a user space application to create a UMEM and UCTX via the general object command.
The UMEM must go through the kernel UMEM_REG method to prevent the user from setting physical addresses by himself. The UCTX is some internal kernel object and shouldn't be exposed.
Quoted 5 lines
As of not being any more part of the general object the caps bits were moved to be some log_xxx indication in the general HCA caps, 0 means not supported.
The firmware code was adapted as well to match the above.
This commit message is a bit wonky.. how about
IB/mlx5: Use the correct commands for UMEM and UCTX allocation
During testing the command format was changed to close a security hole. Revise the driver to use the command format that will actually be supported in GA firmware.
Quoted 5 lines
Both the UMEM and UCTX are intended only for use by the kernel and cannot be executed using a general command.
Since the UMEM and CTX are not part of the general object the caps bits were moved to be some log_xxx location in the general HCA caps.
I'm fine with this description too.
Quoted 7 lines
Quoted 4 lines
Signed-off-by Yishai Hadas <[email protected]>
Reviewed-by Achiad Shochat <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
Also add a fixes line please, any kernel with the devx needs this patch to work with GA firmware.
Fixes a8b92ca1b0e5 ("IB/mlx5: Introduce DEVX")
Quoted 7 lines
Quoted 5 lines
 drivers/infiniband/hw/mlx5/devx.c | 34 ++++++++---------
 drivers/infiniband/hw/mlx5/main.c |  3 +-
 include/linux/mlx5/mlx5_ifc.h     | 62 +++++++++++++++++++++----------
 3 files changed, 58 insertions(+), 41 deletions(-)
Otherwise the patch looks fine, please apply to the shared branch..
Jason,

Quoted 7 lines

Quoted 5 lines
I have procedural question. This patch is based on rdma-next and there is diversion in both devx.c and main.c in those areas, enough do not apply cleanly. Will it be easier if you take this patch to rdma-next, instead me applying different patch to mlx5-next and you hitting merge conflict later on while trying to merge it into rdma-next?
Sure, but it has the unsplit _ifc update...

If it was possible, I wouldn't bother you. This change should be as one piece otherwise we will have failed to build patch.

Quoted 3 lines

I think we are at the end of the cycle so this probably won't make a conflict - lets just go in RDMA then.

From what I see in queues, you are right, merge conflict is unlikely to happen.

Thanks

Quoted 2 lines

Jason

Jason Gunthorpe

Applied

2018-12-20 21:01 UTC #5 lore raw

In reply to Leon Romanovsky

Quoted 15 lines · Leon Romanovsky

From: Yishai Hadas <[email protected]>
The driver needs to prevent a user space application to create a UMEM and UCTX via the general object command.
The UMEM must go through the kernel UMEM_REG method to prevent the user from setting physical addresses by himself. The UCTX is some internal kernel object and shouldn't be exposed.
As of not being any more part of the general object the caps bits were moved to be some log_xxx indication in the general HCA caps, 0 means not supported.
The firmware code was adapted as well to match the above.

Quoted 8 lines

Signed-off-by Yishai Hadas <[email protected]>
Reviewed-by Achiad Shochat <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
 drivers/infiniband/hw/mlx5/devx.c | 34 ++++++++---------
 drivers/infiniband/hw/mlx5/main.c |  3 +-
 include/linux/mlx5/mlx5_ifc.h     | 62 +++++++++++++++++++++----------
 3 files changed, 58 insertions(+), 41 deletions(-)

Applied to for-next

Thanks,
Jason

Achiad Shochat

Applied

2018-12-21 09:29 UTC #6 lore raw

Great, thanks

-----Original Message-----
From: Jason Gunthorpe <[email protected]>
Sent: Thursday, December 20, 2018 11:02 PM
To: Leon Romanovsky <[email protected]>
Cc: Doug Ledford <[email protected]>; Yishai Hadas <[email protected]>; RDMA mailing list <[email protected]>; Achiad Shochat <[email protected]>; Saeed Mahameed <[email protected]>; linux-netdev <[email protected]>; Leon Romanovsky <[email protected]>
Subject: Re: [PATCH mlx5-next] IB/mlx5: Prevent allocating UMEM and UCTX as some general object

Quoted 15 lines · Leon Romanovsky

From: Yishai Hadas <[email protected]>
The driver needs to prevent a user space application to create a UMEM and UCTX via the general object command.
The UMEM must go through the kernel UMEM_REG method to prevent the user from setting physical addresses by himself. The UCTX is some internal kernel object and shouldn't be exposed.
As of not being any more part of the general object the caps bits were moved to be some log_xxx indication in the general HCA caps, 0 means not supported.
The firmware code was adapted as well to match the above.

Quoted 8 lines

Signed-off-by Yishai Hadas <[email protected]>
Reviewed-by Achiad Shochat <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
 drivers/infiniband/hw/mlx5/devx.c | 34 ++++++++---------  
drivers/infiniband/hw/mlx5/main.c |  3 +-
 include/linux/mlx5/mlx5_ifc.h     | 62 +++++++++++++++++++++----------
 3 files changed, 58 insertions(+), 41 deletions(-)

Applied to for-next

Thanks,
Jason