On 2/25/19 4:18 PM, Shaobo wrote:
> Hello everyone,
>
> In function `_mlx4_ib_create_qp`, `create_qp_common` is called in case `IB_QPT_UD` with qp being NULL. In this function, there exists a code segment as follows,
>
> ```
> if (init_attr->create_flags & IB_QP_CREATE_NETIF_QP) {
> if (dev->steering_support ==
> MLX4_STEERING_MODE_DEVICE_MANAGED)
> qp->flags |= MLX4_IB_QP_NETIF;
> else
> goto err;
This is an error in the code, should be fixed.
> }
> ```
> So it's possible that the function returns with `err` being 0 (previously set by `set_rq_size`). This leads to NULL pointer dereference at line 1515 in qp.c because `qp` remains to be NULL while `err` returned is 0. I was wondering if some `if` statement checking the `err` variable is required here or it's omitted by purpose. Please let me know if it makes sense.
The way the code looks today, this error can't be hit, I'll explain:
MLX4_IB_QP_NETIF = IB_QP_CREATE_NETIF_QP (see drivers/infiniband/hw/mlx4/mlx4_ib.h for that).
The only place in the kernel that passes this flag is in:
drivers/infiniband/ulp/ipoib/ipoib_verbs.c
*I care only about the kernel because of this check in _mlx4_ib_create_qp()
/*
* We only support LSO, vendor flag1, and multicast loopback blocking,
* and only for kernel UD QPs.
*/
if (init_attr->create_flags & ~(MLX4_IB_QP_LSO |
MLX4_IB_QP_BLOCK_MULTICAST_LOOPBACK |
MLX4_IB_SRIOV_TUNNEL_QP |
MLX4_IB_SRIOV_SQP |
MLX4_IB_QP_NETIF |
MLX4_IB_QP_CREATE_ROCE_V2_GSI))
return ERR_PTR(-EINVAL);
Ok, so lets see what IPoIB does (ipoib_verbs.c):
if (priv->hca_caps & IB_DEVICE_MANAGED_FLOW_STEERING)
init_attr.create_flags |= IB_QP_CREATE_NETIF_QP;
hca_caps is set here (ipoib_main.c:1841):
priv->hca_caps = priv->ca->attrs.device_cap_flags;
priv->ca = ib device the IPoIB interface is created on.
so back to mlx4, lets see when it sets IB_DEVICE_MANAGED_FLOW_STEERING, this is done in
drivers/infiniband/hw/mlx4/main.c:506
if (dev->steering_support == MLX4_STEERING_MODE_DEVICE_MANAGED)
props->device_cap_flags |= IB_DEVICE_MANAGED_FLOW_STEERING;
so as the code stands today, it can't lead to a null pointer dereference but the reason way
isn't that clear :)
>
> Best,
> Shaobo
Mark
