From: Mark Zhang <[email protected]>
Only UD QP type is allowed to join multicast.
This patch also fixes an uninit-value error: the ib->rec.qkey field is
accessed without being initialized. This is because multicast join was
allowed for all port spaces, even these that omit qkey.
=====================================================
BUG: KMSAN: uninit-value in cma_set_qkey drivers/infiniband/core/cma.c:510 [inline]
BUG: KMSAN: uninit-value in cma_make_mc_event+0xb73/0xe00 drivers/infiniband/core/cma.c:4570
cma_set_qkey drivers/infiniband/core/cma.c:510 [inline]
cma_make_mc_event+0xb73/0xe00 drivers/infiniband/core/cma.c:4570
cma_iboe_join_multicast drivers/infiniband/core/cma.c:4782 [inline]
rdma_join_multicast+0x2b83/0x30a0 drivers/infiniband/core/cma.c:4814
ucma_process_join+0xa76/0xf60 drivers/infiniband/core/ucma.c:1479
ucma_join_multicast+0x1e3/0x250 drivers/infiniband/core/ucma.c:1546
ucma_write+0x639/0x6d0 drivers/infiniband/core/ucma.c:1732
vfs_write+0x8ce/0x2030 fs/read_write.c:588
ksys_write+0x28c/0x520 fs/read_write.c:643
__do_sys_write fs/read_write.c:655 [inline]
__se_sys_write fs/read_write.c:652 [inline]
__ia32_sys_write+0xdb/0x120 fs/read_write.c:652
do_syscall_32_irqs_on arch/x86/entry/common.c:114 [inline]
__do_fast_syscall_32+0x96/0xf0 arch/x86/entry/common.c:180
do_fast_syscall_32+0x34/0x70 arch/x86/entry/common.c:205
do_SYSENTER_32+0x1b/0x20 arch/x86/entry/common.c:248
entry_SYSENTER_compat_after_hwframe+0x4d/0x5c
Local variable ib.i created at:
cma_iboe_join_multicast drivers/infiniband/core/cma.c:4737 [inline]
rdma_join_multicast+0x586/0x30a0 drivers/infiniband/core/cma.c:4814
ucma_process_join+0xa76/0xf60 drivers/infiniband/core/ucma.c:1479
CPU: 0 PID: 29874 Comm: syz-executor.3 Not tainted 5.16.0-rc3-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
=====================================================
Fixes: b5de0c60cc30 ("RDMA/cma: Fix use after free race in roce multicast join")
Reported-by: [email protected]
Signed-off-by: Mark Zhang <[email protected]>
Signed-off-by: Leon Romanovsky <[email protected]>
---
drivers/infiniband/core/cma.c | 32 ++++++++++++++++++--------------
1 file changed, 18 insertions(+), 14 deletions(-)
diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index fabca5e51e3d..3e315fc0ac16 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -496,22 +496,11 @@ static inline unsigned short cma_family(struct rdma_id_private *id_priv)
return id_priv->id.route.addr.src_addr.ss_family;
}
-static int cma_set_qkey(struct rdma_id_private *id_priv, u32 qkey)
+static int cma_set_default_qkey(struct rdma_id_private *id_priv)
{
struct ib_sa_mcmember_rec rec;
int ret = 0;
- if (id_priv->qkey) {
- if (qkey && id_priv->qkey != qkey)
- return -EINVAL;
- return 0;
- }
-
- if (qkey) {
- id_priv->qkey = qkey;
- return 0;
- }
-
switch (id_priv->id.ps) {
case RDMA_PS_UDP:
case RDMA_PS_IB:
@@ -528,9 +517,22 @@ static int cma_set_qkey(struct rdma_id_private *id_priv, u32 qkey)
default:
break;
}
+
return ret;
}
+static int cma_set_qkey(struct rdma_id_private *id_priv, u32 qkey)
+{
+ if (!qkey)
+ return cma_set_default_qkey(id_priv);
+
+ if (id_priv->qkey && (id_priv->qkey != qkey))
+ return -EINVAL;
+
+ id_priv->qkey = qkey;
+ return 0;
+}
+
static void cma_translate_ib(struct sockaddr_ib *sib, struct rdma_dev_addr *dev_addr)
{
dev_addr->dev_type = ARPHRD_INFINIBAND;
@@ -4762,8 +4764,7 @@ static int cma_iboe_join_multicast(struct rdma_id_private *id_priv,
cma_iboe_set_mgid(addr, &ib.rec.mgid, gid_type);
ib.rec.pkey = cpu_to_be16(0xffff);
- if (id_priv->id.ps == RDMA_PS_UDP)
- ib.rec.qkey = cpu_to_be32(RDMA_UDP_QKEY);
+ ib.rec.qkey = cpu_to_be32(RDMA_UDP_QKEY);
if (dev_addr->bound_dev_if)
ndev = dev_get_by_index(dev_addr->net, dev_addr->bound_dev_if);
@@ -4815,6 +4816,9 @@ int rdma_join_multicast(struct rdma_cm_id *id, struct sockaddr *addr,
READ_ONCE(id_priv->state) != RDMA_CM_ADDR_RESOLVED))
return -EINVAL;
+ if (id_priv->id.qp_type != IB_QPT_UD)
+ return -EINVAL;
+
mc = kzalloc(sizeof(*mc), GFP_KERNEL);
if (!mc)
return -ENOMEM;
--
2.35.1
On Mon, Apr 04, 2022 at 05:52:18PM +0300, Leon Romanovsky wrote:
> -static int cma_set_qkey(struct rdma_id_private *id_priv, u32 qkey)
> +static int cma_set_default_qkey(struct rdma_id_private *id_priv)
> {
> struct ib_sa_mcmember_rec rec;
> int ret = 0;
>
> - if (id_priv->qkey) {
> - if (qkey && id_priv->qkey != qkey)
> - return -EINVAL;
> - return 0;
> - }
> -
> - if (qkey) {
> - id_priv->qkey = qkey;
> - return 0;
> - }
> -
> switch (id_priv->id.ps) {
> case RDMA_PS_UDP:
> case RDMA_PS_IB:
> @@ -528,9 +517,22 @@ static int cma_set_qkey(struct rdma_id_private *id_priv, u32 qkey)
> default:
> break;
> }
> +
> return ret;
> }
>
> +static int cma_set_qkey(struct rdma_id_private *id_priv, u32 qkey)
> +{
> + if (!qkey)
> + return cma_set_default_qkey(id_priv);
This should be called in the couple of places that are actually
allowed to set a default qkey. We have some confusion about when that
is supposed to happen and when a 0 qkey can be presented.
But isn't this not the same? The original behavior was to make the
set_default a NOP if the id_priv already had a qkey:
- if (id_priv->qkey) {
- if (qkey && id_priv->qkey != qkey)
But that is gone now?
I got this:
diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index 3e315fc0ac16cb..ef980ea7153e51 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -1102,7 +1102,7 @@ static int cma_ib_init_qp_attr(struct rdma_id_private *id_priv,
*qp_attr_mask = IB_QP_STATE | IB_QP_PKEY_INDEX | IB_QP_PORT;
if (id_priv->id.qp_type == IB_QPT_UD) {
- ret = cma_set_qkey(id_priv, 0);
+ ret = cma_set_default_qkey(id_priv);
if (ret)
return ret;
@@ -4430,14 +4430,10 @@ int rdma_accept(struct rdma_cm_id *id, struct rdma_conn_param *conn_param)
if (rdma_cap_ib_cm(id->device, id->port_num)) {
if (id->qp_type == IB_QPT_UD) {
- if (conn_param)
- ret = cma_send_sidr_rep(id_priv, IB_SIDR_SUCCESS,
- conn_param->qkey,
- conn_param->private_data,
- conn_param->private_data_len);
- else
- ret = cma_send_sidr_rep(id_priv, IB_SIDR_SUCCESS,
- 0, NULL, 0);
+ ret = cma_send_sidr_rep(id_priv, IB_SIDR_SUCCESS,
+ conn_param->qkey,
+ conn_param->private_data,
+ conn_param->private_data_len);
} else {
if (conn_param)
ret = cma_accept_ib(id_priv, conn_param);
@@ -4685,7 +4681,7 @@ static int cma_join_ib_multicast(struct rdma_id_private *id_priv,
if (ret)
return ret;
- ret = cma_set_qkey(id_priv, 0);
+ ret = cma_set_default_qkey(id_priv);
if (ret)
return ret;
> static void cma_translate_ib(struct sockaddr_ib *sib, struct rdma_dev_addr *dev_addr)
> {
> dev_addr->dev_type = ARPHRD_INFINIBAND;
> @@ -4762,8 +4764,7 @@ static int cma_iboe_join_multicast(struct rdma_id_private *id_priv,
> cma_iboe_set_mgid(addr, &ib.rec.mgid, gid_type);
>
> ib.rec.pkey = cpu_to_be16(0xffff);
> - if (id_priv->id.ps == RDMA_PS_UDP)
> - ib.rec.qkey = cpu_to_be32(RDMA_UDP_QKEY);
> + ib.rec.qkey = cpu_to_be32(RDMA_UDP_QKEY);
Why isn't this symetrical with the IB side:
ret = cma_set_default_qkey(id_priv);
if (ret)
return ret;
rec.qkey = cpu_to_be32(id_priv->qkey);
??
It fells like set_default_qkey() is the right thing to do incase the
qkey was already set by something, just as IB does it.
> @@ -4815,6 +4816,9 @@ int rdma_join_multicast(struct rdma_cm_id *id, struct sockaddr *addr,
> READ_ONCE(id_priv->state) != RDMA_CM_ADDR_RESOLVED))
> return -EINVAL;
>
> + if (id_priv->id.qp_type != IB_QPT_UD)
> + return -EINVAL;
> +
This makes sense
Jason
On Fri, Apr 08, 2022 at 03:24:40PM -0300, Jason Gunthorpe wrote:
> On Mon, Apr 04, 2022 at 05:52:18PM +0300, Leon Romanovsky wrote:
> > -static int cma_set_qkey(struct rdma_id_private *id_priv, u32 qkey)
> > +static int cma_set_default_qkey(struct rdma_id_private *id_priv)
> > {
> > struct ib_sa_mcmember_rec rec;
> > int ret = 0;
> >
> > - if (id_priv->qkey) {
> > - if (qkey && id_priv->qkey != qkey)
> > - return -EINVAL;
> > - return 0;
> > - }
> > -
> > - if (qkey) {
> > - id_priv->qkey = qkey;
> > - return 0;
> > - }
> > -
> > switch (id_priv->id.ps) {
> > case RDMA_PS_UDP:
> > case RDMA_PS_IB:
> > @@ -528,9 +517,22 @@ static int cma_set_qkey(struct rdma_id_private *id_priv, u32 qkey)
> > default:
> > break;
> > }
> > +
> > return ret;
> > }
> >
> > +static int cma_set_qkey(struct rdma_id_private *id_priv, u32 qkey)
> > +{
> > + if (!qkey)
> > + return cma_set_default_qkey(id_priv);
>
> This should be called in the couple of places that are actually
> allowed to set a default qkey. We have some confusion about when that
> is supposed to happen and when a 0 qkey can be presented.
>
> But isn't this not the same? The original behavior was to make the
> set_default a NOP if the id_priv already had a qkey:
>
> - if (id_priv->qkey) {
> - if (qkey && id_priv->qkey != qkey)
>
> But that is gone now?
When I reviewed, I got an impression what once we create id_priv and set
qkey to default values, we won't hit this if (..).
>
> I got this:
>
> diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
> index 3e315fc0ac16cb..ef980ea7153e51 100644
> --- a/drivers/infiniband/core/cma.c
> +++ b/drivers/infiniband/core/cma.c
> @@ -1102,7 +1102,7 @@ static int cma_ib_init_qp_attr(struct rdma_id_private *id_priv,
> *qp_attr_mask = IB_QP_STATE | IB_QP_PKEY_INDEX | IB_QP_PORT;
>
> if (id_priv->id.qp_type == IB_QPT_UD) {
> - ret = cma_set_qkey(id_priv, 0);
> + ret = cma_set_default_qkey(id_priv);
This is ok
> if (ret)
> return ret;
>
> @@ -4430,14 +4430,10 @@ int rdma_accept(struct rdma_cm_id *id, struct rdma_conn_param *conn_param)
>
> if (rdma_cap_ib_cm(id->device, id->port_num)) {
> if (id->qp_type == IB_QPT_UD) {
> - if (conn_param)
> - ret = cma_send_sidr_rep(id_priv, IB_SIDR_SUCCESS,
> - conn_param->qkey,
> - conn_param->private_data,
> - conn_param->private_data_len);
> - else
> - ret = cma_send_sidr_rep(id_priv, IB_SIDR_SUCCESS,
> - 0, NULL, 0);
> + ret = cma_send_sidr_rep(id_priv, IB_SIDR_SUCCESS,
> + conn_param->qkey,
> + conn_param->private_data,
> + conn_param->private_data_len);
It is ok too and we have many other places with not-possible "if (conn_param)".
> } else {
> if (conn_param)
> ret = cma_accept_ib(id_priv, conn_param);
> @@ -4685,7 +4681,7 @@ static int cma_join_ib_multicast(struct rdma_id_private *id_priv,
> if (ret)
> return ret;
>
> - ret = cma_set_qkey(id_priv, 0);
> + ret = cma_set_default_qkey(id_priv);
> if (ret)
> return ret;
>
>
> > static void cma_translate_ib(struct sockaddr_ib *sib, struct rdma_dev_addr *dev_addr)
> > {
> > dev_addr->dev_type = ARPHRD_INFINIBAND;
> > @@ -4762,8 +4764,7 @@ static int cma_iboe_join_multicast(struct rdma_id_private *id_priv,
> > cma_iboe_set_mgid(addr, &ib.rec.mgid, gid_type);
> >
> > ib.rec.pkey = cpu_to_be16(0xffff);
> > - if (id_priv->id.ps == RDMA_PS_UDP)
> > - ib.rec.qkey = cpu_to_be32(RDMA_UDP_QKEY);
> > + ib.rec.qkey = cpu_to_be32(RDMA_UDP_QKEY);
>
> Why isn't this symetrical with the IB side:
>
> ret = cma_set_default_qkey(id_priv);
> if (ret)
> return ret;
> rec.qkey = cpu_to_be32(id_priv->qkey);
>
>
> ??
The original code didn't touch id_priv.
>
> It fells like set_default_qkey() is the right thing to do incase the
> qkey was already set by something, just as IB does it.
>
> > @@ -4815,6 +4816,9 @@ int rdma_join_multicast(struct rdma_cm_id *id, struct sockaddr *addr,
> > READ_ONCE(id_priv->state) != RDMA_CM_ADDR_RESOLVED))
> > return -EINVAL;
> >
> > + if (id_priv->id.qp_type != IB_QPT_UD)
> > + return -EINVAL;
> > +
>
> This makes sense
>
> Jason
On Tue, 12 Apr 2022, Jason Gunthorpe wrote:
> The only places setting non-default qkeys are SIDR, maybe nobody uses
> SIDR with multicast.
IP port numbers provided are ignored by the RDMA subsytem when doing
multicast joins. Thus no need to do SIDRs with RDMA multicast.
Some middleware solutions (like LLM by IBM and Confinity) are creating
their own custom MGID because of this problem. The custom MGID will then
contain the port number.
On the subject of this patch: There is a usecase for Multicast with
IBV_QPT_RAW_PACKET too. A multicast join is required to redirect traffic
for a multicast group to the raw socket.
On Sun, Apr 10, 2022 at 03:03:24PM +0300, Leon Romanovsky wrote:
> On Fri, Apr 08, 2022 at 03:24:40PM -0300, Jason Gunthorpe wrote:
> > On Mon, Apr 04, 2022 at 05:52:18PM +0300, Leon Romanovsky wrote:
> > > -static int cma_set_qkey(struct rdma_id_private *id_priv, u32 qkey)
> > > +static int cma_set_default_qkey(struct rdma_id_private *id_priv)
> > > {
> > > struct ib_sa_mcmember_rec rec;
> > > int ret = 0;
> > >
> > > - if (id_priv->qkey) {
> > > - if (qkey && id_priv->qkey != qkey)
> > > - return -EINVAL;
> > > - return 0;
> > > - }
> > > -
> > > - if (qkey) {
> > > - id_priv->qkey = qkey;
> > > - return 0;
> > > - }
> > > -
> > > switch (id_priv->id.ps) {
> > > case RDMA_PS_UDP:
> > > case RDMA_PS_IB:
> > > @@ -528,9 +517,22 @@ static int cma_set_qkey(struct rdma_id_private *id_priv, u32 qkey)
> > > default:
> > > break;
> > > }
> > > +
> > > return ret;
> > > }
> > >
> > > +static int cma_set_qkey(struct rdma_id_private *id_priv, u32 qkey)
> > > +{
> > > + if (!qkey)
> > > + return cma_set_default_qkey(id_priv);
> >
> > This should be called in the couple of places that are actually
> > allowed to set a default qkey. We have some confusion about when that
> > is supposed to happen and when a 0 qkey can be presented.
> >
> > But isn't this not the same? The original behavior was to make the
> > set_default a NOP if the id_priv already had a qkey:
> >
> > - if (id_priv->qkey) {
> > - if (qkey && id_priv->qkey != qkey)
> >
> > But that is gone now?
>
> When I reviewed, I got an impression what once we create id_priv and set
> qkey to default values, we won't hit this if (..).
We don't set qkey during create, so I'm not so sure..
The only places setting non-default qkeys are SIDR, maybe nobody uses
SIDR with multicast.
> > > static void cma_translate_ib(struct sockaddr_ib *sib, struct rdma_dev_addr *dev_addr)
> > > {
> > > dev_addr->dev_type = ARPHRD_INFINIBAND;
> > > @@ -4762,8 +4764,7 @@ static int cma_iboe_join_multicast(struct rdma_id_private *id_priv,
> > > cma_iboe_set_mgid(addr, &ib.rec.mgid, gid_type);
> > >
> > > ib.rec.pkey = cpu_to_be16(0xffff);
> > > - if (id_priv->id.ps == RDMA_PS_UDP)
> > > - ib.rec.qkey = cpu_to_be32(RDMA_UDP_QKEY);
> > > + ib.rec.qkey = cpu_to_be32(RDMA_UDP_QKEY);
> >
> > Why isn't this symetrical with the IB side:
> >
> > ret = cma_set_default_qkey(id_priv);
> > if (ret)
> > return ret;
> > rec.qkey = cpu_to_be32(id_priv->qkey);
> >
> >
> > ??
>
> The original code didn't touch id_priv.
I know, but I think that is a mistake, we should make it symmetric
Jason
On Tue, Apr 12, 2022 at 05:01:36PM +0200, Christoph Lameter wrote:
> On Tue, 12 Apr 2022, Jason Gunthorpe wrote:
>
> > The only places setting non-default qkeys are SIDR, maybe nobody uses
> > SIDR with multicast.
>
>
> IP port numbers provided are ignored by the RDMA subsytem when doing
> multicast joins. Thus no need to do SIDRs with RDMA multicast.
>
> Some middleware solutions (like LLM by IBM and Confinity) are creating
> their own custom MGID because of this problem. The custom MGID will then
> contain the port number.
>
> On the subject of this patch: There is a usecase for Multicast with
> IBV_QPT_RAW_PACKET too. A multicast join is required to redirect traffic
> for a multicast group to the raw socket.
The qp_type in rdma-cm is a little bit misleading and represents
communication QP. It can be or RC or UD, which is hard coded in
almost all rdma-cm code.
The one of the places that receive it from the user space is ucma_get_qp_type()
for RDMA_PS_IB flow, but librdmacm set it to RC too.
790
791 int rdma_create_id(struct rdma_event_channel *channel,
792 struct rdma_cm_id **id, void *context,
793 enum rdma_port_space ps)
794 {
795 enum ibv_qp_type qp_type;
796
797 qp_type = (ps == RDMA_PS_IPOIB || ps == RDMA_PS_UDP) ?
798 IBV_QPT_UD : IBV_QPT_RC;
799 return rdma_create_id2(channel, id, context, ps, qp_type);
800 }
801
Thanks
On Tue, Apr 12, 2022 at 11:11:34AM -0300, Jason Gunthorpe wrote:
> On Sun, Apr 10, 2022 at 03:03:24PM +0300, Leon Romanovsky wrote:
> > On Fri, Apr 08, 2022 at 03:24:40PM -0300, Jason Gunthorpe wrote:
> > > On Mon, Apr 04, 2022 at 05:52:18PM +0300, Leon Romanovsky wrote:
> > > > -static int cma_set_qkey(struct rdma_id_private *id_priv, u32 qkey)
> > > > +static int cma_set_default_qkey(struct rdma_id_private *id_priv)
> > > > {
> > > > struct ib_sa_mcmember_rec rec;
> > > > int ret = 0;
> > > >
> > > > - if (id_priv->qkey) {
> > > > - if (qkey && id_priv->qkey != qkey)
> > > > - return -EINVAL;
> > > > - return 0;
> > > > - }
> > > > -
> > > > - if (qkey) {
> > > > - id_priv->qkey = qkey;
> > > > - return 0;
> > > > - }
> > > > -
> > > > switch (id_priv->id.ps) {
> > > > case RDMA_PS_UDP:
> > > > case RDMA_PS_IB:
> > > > @@ -528,9 +517,22 @@ static int cma_set_qkey(struct rdma_id_private *id_priv, u32 qkey)
> > > > default:
> > > > break;
> > > > }
> > > > +
> > > > return ret;
> > > > }
> > > >
> > > > +static int cma_set_qkey(struct rdma_id_private *id_priv, u32 qkey)
> > > > +{
> > > > + if (!qkey)
> > > > + return cma_set_default_qkey(id_priv);
> > >
> > > This should be called in the couple of places that are actually
> > > allowed to set a default qkey. We have some confusion about when that
> > > is supposed to happen and when a 0 qkey can be presented.
> > >
> > > But isn't this not the same? The original behavior was to make the
> > > set_default a NOP if the id_priv already had a qkey:
> > >
> > > - if (id_priv->qkey) {
> > > - if (qkey && id_priv->qkey != qkey)
> > >
> > > But that is gone now?
> >
> > When I reviewed, I got an impression what once we create id_priv and set
> > qkey to default values, we won't hit this if (..).
>
> We don't set qkey during create, so I'm not so sure..
>
> The only places setting non-default qkeys are SIDR, maybe nobody uses
> SIDR with multicast.
>
>
> > > > static void cma_translate_ib(struct sockaddr_ib *sib, struct rdma_dev_addr *dev_addr)
> > > > {
> > > > dev_addr->dev_type = ARPHRD_INFINIBAND;
> > > > @@ -4762,8 +4764,7 @@ static int cma_iboe_join_multicast(struct rdma_id_private *id_priv,
> > > > cma_iboe_set_mgid(addr, &ib.rec.mgid, gid_type);
> > > >
> > > > ib.rec.pkey = cpu_to_be16(0xffff);
> > > > - if (id_priv->id.ps == RDMA_PS_UDP)
> > > > - ib.rec.qkey = cpu_to_be32(RDMA_UDP_QKEY);
> > > > + ib.rec.qkey = cpu_to_be32(RDMA_UDP_QKEY);
> > >
> > > Why isn't this symetrical with the IB side:
> > >
> > > ret = cma_set_default_qkey(id_priv);
> > > if (ret)
> > > return ret;
> > > rec.qkey = cpu_to_be32(id_priv->qkey);
> > >
> > >
> > > ??
> >
> > The original code didn't touch id_priv.
>
> I know, but I think that is a mistake, we should make it symmetric
ok, I added it to regression.
Thanks
>
> Jason