| Message ID | 1876bacbbcb6f82af3948e5c37a09da6ea3fcae5.1641474841.git.leonro@nvidia.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | [rdma-rc] RDMA/cma: Clear all multicast request fields | expand |
On Thu, Jan 06, 2022 at 03:15:07PM +0200, Leon Romanovsky wrote: > From: Leon Romanovsky <leonro@nvidia.com> > > The ib->rec.qkey field is accessed without being initialized. > Clear the ib_sa_multicast struct to fix the following syzkaller error/. > > ===================================================== > 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: syzbot+8fcbb77276d43cc8b693@syzkaller.appspotmail.com > Signed-off-by: Leon Romanovsky <leonro@nvidia.com> > drivers/infiniband/core/cma.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c > index 69c9a12dd14e..9c53e1e7de50 100644 > +++ b/drivers/infiniband/core/cma.c > @@ -4737,7 +4737,7 @@ static int cma_iboe_join_multicast(struct rdma_id_private *id_priv, > int err = 0; > struct sockaddr *addr = (struct sockaddr *)&mc->addr; > struct net_device *ndev = NULL; > - struct ib_sa_multicast ib; > + struct ib_sa_multicast ib = {}; > enum ib_gid_type gid_type; > bool send_only; We shouldn't be able to join anything except a RDMA_PS_UDP to a multicast in the first place: if (id_priv->id.ps == RDMA_PS_UDP) ib.rec.qkey = cpu_to_be32(RDMA_UDP_QKEY); Multicast RC/etc is meaningless. So I guess it should be like this: --- a/drivers/infiniband/core/cma.c +++ b/drivers/infiniband/core/cma.c @@ -4744,7 +4744,7 @@ static int cma_iboe_join_multicast(struct rdma_id_private *id_priv, send_only = mc->join_state == BIT(SENDONLY_FULLMEMBER_JOIN); - if (cma_zero_addr(addr)) + if (cma_zero_addr(addr) || id_priv->id.ps != RDMA_PS_UDP) return -EINVAL; gid_type = id_priv->cma_dev->default_gid_type[id_priv->id.port_num - @@ -4752,8 +4752,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);
On Thu, Jan 06, 2022 at 01:39:41PM -0400, Jason Gunthorpe wrote: > On Thu, Jan 06, 2022 at 03:15:07PM +0200, Leon Romanovsky wrote: > > From: Leon Romanovsky <leonro@nvidia.com> > > > > The ib->rec.qkey field is accessed without being initialized. > > Clear the ib_sa_multicast struct to fix the following syzkaller error/. > > > > ===================================================== > > 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: syzbot+8fcbb77276d43cc8b693@syzkaller.appspotmail.com > > Signed-off-by: Leon Romanovsky <leonro@nvidia.com> > > drivers/infiniband/core/cma.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c > > index 69c9a12dd14e..9c53e1e7de50 100644 > > +++ b/drivers/infiniband/core/cma.c > > @@ -4737,7 +4737,7 @@ static int cma_iboe_join_multicast(struct rdma_id_private *id_priv, > > int err = 0; > > struct sockaddr *addr = (struct sockaddr *)&mc->addr; > > struct net_device *ndev = NULL; > > - struct ib_sa_multicast ib; > > + struct ib_sa_multicast ib = {}; > > enum ib_gid_type gid_type; > > bool send_only; > > We shouldn't be able to join anything except a RDMA_PS_UDP to a > multicast in the first place: > > if (id_priv->id.ps == RDMA_PS_UDP) > ib.rec.qkey = cpu_to_be32(RDMA_UDP_QKEY); > > Multicast RC/etc is meaningless. So I guess it should be like this: Strange that we don't have repro for such deterministic flow. > > --- a/drivers/infiniband/core/cma.c > +++ b/drivers/infiniband/core/cma.c > @@ -4744,7 +4744,7 @@ static int cma_iboe_join_multicast(struct rdma_id_private *id_priv, > > send_only = mc->join_state == BIT(SENDONLY_FULLMEMBER_JOIN); > > - if (cma_zero_addr(addr)) > + if (cma_zero_addr(addr) || id_priv->id.ps != RDMA_PS_UDP) > return -EINVAL; > > gid_type = id_priv->cma_dev->default_gid_type[id_priv->id.port_num - > @@ -4752,8 +4752,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);
On Thu, Jan 06, 2022 at 01:39:41PM -0400, Jason Gunthorpe wrote: > On Thu, Jan 06, 2022 at 03:15:07PM +0200, Leon Romanovsky wrote: > > From: Leon Romanovsky <leonro@nvidia.com> > > > > The ib->rec.qkey field is accessed without being initialized. > > Clear the ib_sa_multicast struct to fix the following syzkaller error/. > > > > ===================================================== > > 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: syzbot+8fcbb77276d43cc8b693@syzkaller.appspotmail.com > > Signed-off-by: Leon Romanovsky <leonro@nvidia.com> > > drivers/infiniband/core/cma.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c > > index 69c9a12dd14e..9c53e1e7de50 100644 > > +++ b/drivers/infiniband/core/cma.c > > @@ -4737,7 +4737,7 @@ static int cma_iboe_join_multicast(struct rdma_id_private *id_priv, > > int err = 0; > > struct sockaddr *addr = (struct sockaddr *)&mc->addr; > > struct net_device *ndev = NULL; > > - struct ib_sa_multicast ib; > > + struct ib_sa_multicast ib = {}; > > enum ib_gid_type gid_type; > > bool send_only; > > We shouldn't be able to join anything except a RDMA_PS_UDP to a > multicast in the first place: > > if (id_priv->id.ps == RDMA_PS_UDP) > ib.rec.qkey = cpu_to_be32(RDMA_UDP_QKEY); > > Multicast RC/etc is meaningless. So I guess it should be like this: I don't know, I used 0 exactly like we have for cma_join_ib_multicast(). Where can I read about this PS limitation? I didn't find anything relevant in the IBTA spec. Thanks
On Sun, Jan 09, 2022 at 02:22:04PM +0200, Leon Romanovsky wrote: > On Thu, Jan 06, 2022 at 01:39:41PM -0400, Jason Gunthorpe wrote: > > On Thu, Jan 06, 2022 at 03:15:07PM +0200, Leon Romanovsky wrote: > > > From: Leon Romanovsky <leonro@nvidia.com> > > > > > > The ib->rec.qkey field is accessed without being initialized. > > > Clear the ib_sa_multicast struct to fix the following syzkaller error/. > > > > > > ===================================================== > > > 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: syzbot+8fcbb77276d43cc8b693@syzkaller.appspotmail.com > > > Signed-off-by: Leon Romanovsky <leonro@nvidia.com> > > > drivers/infiniband/core/cma.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c > > > index 69c9a12dd14e..9c53e1e7de50 100644 > > > +++ b/drivers/infiniband/core/cma.c > > > @@ -4737,7 +4737,7 @@ static int cma_iboe_join_multicast(struct rdma_id_private *id_priv, > > > int err = 0; > > > struct sockaddr *addr = (struct sockaddr *)&mc->addr; > > > struct net_device *ndev = NULL; > > > - struct ib_sa_multicast ib; > > > + struct ib_sa_multicast ib = {}; > > > enum ib_gid_type gid_type; > > > bool send_only; > > > > We shouldn't be able to join anything except a RDMA_PS_UDP to a > > multicast in the first place: > > > > if (id_priv->id.ps == RDMA_PS_UDP) > > ib.rec.qkey = cpu_to_be32(RDMA_UDP_QKEY); > > > > Multicast RC/etc is meaningless. So I guess it should be like this: > > I don't know, I used 0 exactly like we have for cma_join_ib_multicast(). > > Where can I read about this PS limitation? I didn't find anything > relevant in the IBTA spec. It is a Linux thing We should probably check the PS even earlier to prevent the IB side from having the same issue. multicast should never be used in any place that can omit a qkey, IIRC.. Jason
On Mon, Jan 10, 2022 at 11:36:19AM -0400, Jason Gunthorpe wrote: > On Sun, Jan 09, 2022 at 02:22:04PM +0200, Leon Romanovsky wrote: > > On Thu, Jan 06, 2022 at 01:39:41PM -0400, Jason Gunthorpe wrote: > > > On Thu, Jan 06, 2022 at 03:15:07PM +0200, Leon Romanovsky wrote: > > > > From: Leon Romanovsky <leonro@nvidia.com> > > > > > > > > The ib->rec.qkey field is accessed without being initialized. > > > > Clear the ib_sa_multicast struct to fix the following syzkaller error/. > > > > > > > > ===================================================== > > > > 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: syzbot+8fcbb77276d43cc8b693@syzkaller.appspotmail.com > > > > Signed-off-by: Leon Romanovsky <leonro@nvidia.com> > > > > drivers/infiniband/core/cma.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c > > > > index 69c9a12dd14e..9c53e1e7de50 100644 > > > > +++ b/drivers/infiniband/core/cma.c > > > > @@ -4737,7 +4737,7 @@ static int cma_iboe_join_multicast(struct rdma_id_private *id_priv, > > > > int err = 0; > > > > struct sockaddr *addr = (struct sockaddr *)&mc->addr; > > > > struct net_device *ndev = NULL; > > > > - struct ib_sa_multicast ib; > > > > + struct ib_sa_multicast ib = {}; > > > > enum ib_gid_type gid_type; > > > > bool send_only; > > > > > > We shouldn't be able to join anything except a RDMA_PS_UDP to a > > > multicast in the first place: > > > > > > if (id_priv->id.ps == RDMA_PS_UDP) > > > ib.rec.qkey = cpu_to_be32(RDMA_UDP_QKEY); > > > > > > Multicast RC/etc is meaningless. So I guess it should be like this: > > > > I don't know, I used 0 exactly like we have for cma_join_ib_multicast(). > > > > Where can I read about this PS limitation? I didn't find anything > > relevant in the IBTA spec. > > It is a Linux thing > > We should probably check the PS even earlier to prevent the IB side > from having the same issue. What do you think about this? diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c index 835ac54d4a24..0a1f008ca929 100644 --- a/drivers/infiniband/core/cma.c +++ b/drivers/infiniband/core/cma.c @@ -4669,12 +4669,8 @@ static int cma_join_ib_multicast(struct rdma_id_private *id_priv, if (ret) return ret; - ret = cma_set_qkey(id_priv, 0); - if (ret) - return ret; - cma_set_mgid(id_priv, (struct sockaddr *) &mc->addr, &rec.mgid); - rec.qkey = cpu_to_be32(id_priv->qkey); + rec.qkey = cpu_to_be32(RDMA_UDP_QKEY); rdma_addr_get_sgid(dev_addr, &rec.port_gid); rec.pkey = cpu_to_be16(ib_addr_get_pkey(dev_addr)); rec.join_state = mc->join_state; @@ -4748,8 +4744,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); @@ -4796,6 +4791,9 @@ int rdma_join_multicast(struct rdma_cm_id *id, struct sockaddr *addr, if (WARN_ON(id->qp)) return -EINVAL; + if (id->ps != RDMA_PS_UDP) + return -EINVAL; + /* ULP is calling this wrong. */ if (!id->device || (READ_ONCE(id_priv->state) != RDMA_CM_ADDR_BOUND && READ_ONCE(id_priv->state) != RDMA_CM_ADDR_RESOLVED)) > > multicast should never be used in any place that can omit a qkey, > IIRC.. > > Jason
On Mon, Jan 10, 2022 at 08:05:40PM +0200, Leon Romanovsky wrote: > > We should probably check the PS even earlier to prevent the IB side > > from having the same issue. > > What do you think about this? IB is a bit different, it has a bunch of PS's that are UD compatible.. Probably what we really want here is to check/restrict the CM ID to SIDR mode, which does have the qkey and is the only mode that makes sense to be mixed with multicast, and then forget about port space entirely. It may be that port space indirectly restricts the CM ID to SIDR mode, but the language here should be 'is in sidr mode', not some confusing open coded port space check. I'm also not sure of the lifecycle of the qkey, qkeys only exist in SIDR mode so obviously anything that sets/gets a qkey should be restriced to SIDR CM IDs.. > diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c > index 835ac54d4a24..0a1f008ca929 100644 > +++ b/drivers/infiniband/core/cma.c > @@ -4669,12 +4669,8 @@ static int cma_join_ib_multicast(struct rdma_id_private *id_priv, > if (ret) > return ret; > > - ret = cma_set_qkey(id_priv, 0); > - if (ret) > - return ret; > - > cma_set_mgid(id_priv, (struct sockaddr *) &mc->addr, &rec.mgid); > - rec.qkey = cpu_to_be32(id_priv->qkey); > + rec.qkey = cpu_to_be32(RDMA_UDP_QKEY); And I'm not sure this makes sense? The UD qkey should still be negotiated right? Jason
On Mon, Jan 17, 2022 at 12:16:21PM -0400, Jason Gunthorpe wrote: > On Mon, Jan 10, 2022 at 08:05:40PM +0200, Leon Romanovsky wrote: > > > > We should probably check the PS even earlier to prevent the IB side > > > from having the same issue. > > > > What do you think about this? > > IB is a bit different, it has a bunch of PS's that are UD compatible.. > > Probably what we really want here is to check/restrict the CM ID to > SIDR mode, which does have the qkey and is the only mode that makes > sense to be mixed with multicast, and then forget about port space > entirely. > > It may be that port space indirectly restricts the CM ID to SIDR mode, > but the language here should be 'is in sidr mode', not some confusing > open coded port space check. > > I'm also not sure of the lifecycle of the qkey, qkeys only exist in > SIDR mode so obviously anything that sets/gets a qkey should be > restriced to SIDR CM IDs.. > > > diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c > > index 835ac54d4a24..0a1f008ca929 100644 > > +++ b/drivers/infiniband/core/cma.c > > @@ -4669,12 +4669,8 @@ static int cma_join_ib_multicast(struct rdma_id_private *id_priv, > > if (ret) > > return ret; > > > > - ret = cma_set_qkey(id_priv, 0); > > - if (ret) > > - return ret; > > - > > cma_set_mgid(id_priv, (struct sockaddr *) &mc->addr, &rec.mgid); > > - rec.qkey = cpu_to_be32(id_priv->qkey); > > + rec.qkey = cpu_to_be32(RDMA_UDP_QKEY); > > And I'm not sure this makes sense? The UD qkey should still be > negotiated right? Yes, I think so, it will be changed in SIDR phase. The original code has "cma_set_qkey(id_priv, 0)" call, that in IB case will execute this switch anyway: 515 switch (id_priv->id.ps) { 516 case RDMA_PS_UDP: 517 case RDMA_PS_IB: 518 id_priv->qkey = RDMA_UDP_QKEY; The difference is that we won't store RDMA_UDP_QKEY in id_priv->qkey, but I'm unsure that this is right. Thanks > > Jason
On Mon, Jan 17, 2022 at 08:20:40PM +0200, Leon Romanovsky wrote: > On Mon, Jan 17, 2022 at 12:16:21PM -0400, Jason Gunthorpe wrote: > > On Mon, Jan 10, 2022 at 08:05:40PM +0200, Leon Romanovsky wrote: > > > > > > We should probably check the PS even earlier to prevent the IB side > > > > from having the same issue. > > > > > > What do you think about this? > > > > IB is a bit different, it has a bunch of PS's that are UD compatible.. > > > > Probably what we really want here is to check/restrict the CM ID to > > SIDR mode, which does have the qkey and is the only mode that makes > > sense to be mixed with multicast, and then forget about port space > > entirely. > > > > It may be that port space indirectly restricts the CM ID to SIDR mode, > > but the language here should be 'is in sidr mode', not some confusing > > open coded port space check. > > > > I'm also not sure of the lifecycle of the qkey, qkeys only exist in > > SIDR mode so obviously anything that sets/gets a qkey should be > > restriced to SIDR CM IDs.. > > > > > diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c > > > index 835ac54d4a24..0a1f008ca929 100644 > > > +++ b/drivers/infiniband/core/cma.c > > > @@ -4669,12 +4669,8 @@ static int cma_join_ib_multicast(struct rdma_id_private *id_priv, > > > if (ret) > > > return ret; > > > > > > - ret = cma_set_qkey(id_priv, 0); > > > - if (ret) > > > - return ret; > > > - > > > cma_set_mgid(id_priv, (struct sockaddr *) &mc->addr, &rec.mgid); > > > - rec.qkey = cpu_to_be32(id_priv->qkey); > > > + rec.qkey = cpu_to_be32(RDMA_UDP_QKEY); > > > > And I'm not sure this makes sense? The UD qkey should still be > > negotiated right? > > Yes, I think so, it will be changed in SIDR phase. > > The original code has "cma_set_qkey(id_priv, 0)" call, that in IB case will > execute this switch anyway: > 515 switch (id_priv->id.ps) { > 516 case RDMA_PS_UDP: > 517 case RDMA_PS_IB: > 518 id_priv->qkey = RDMA_UDP_QKEY; > > The difference is that we won't store RDMA_UDP_QKEY in id_priv->qkey, > but I'm unsure that this is right. Well the whoele cma_set_qkey() function appears to be complete jumblied nonsense as if qkey is zero then it doesn't do anything if the qkey was already set. When called with 0 it is really some sort of 'make a default qkey if the user hasn't set one already' and in that case defaulting to RDMA_UDP_QKEY does makes some kind of sense. The functions purposes should be split into two functions really. So, we end up with 'make sure the cm id is in SDIR mode' then 'if the qkey is not set, set it to a default', so that the net result is the qkey is always set once the function returns. Though, I'm not sure what the semantics are for qkey during SIDR negotiation, that should be checked in the spec. Jason
On Mon, Jan 17, 2022 at 02:38:32PM -0400, Jason Gunthorpe wrote: > On Mon, Jan 17, 2022 at 08:20:40PM +0200, Leon Romanovsky wrote: > > On Mon, Jan 17, 2022 at 12:16:21PM -0400, Jason Gunthorpe wrote: > > > On Mon, Jan 10, 2022 at 08:05:40PM +0200, Leon Romanovsky wrote: > > > > > > > > We should probably check the PS even earlier to prevent the IB side > > > > > from having the same issue. > > > > > > > > What do you think about this? > > > > > > IB is a bit different, it has a bunch of PS's that are UD compatible.. > > > > > > Probably what we really want here is to check/restrict the CM ID to > > > SIDR mode, which does have the qkey and is the only mode that makes > > > sense to be mixed with multicast, and then forget about port space > > > entirely. > > > > > > It may be that port space indirectly restricts the CM ID to SIDR mode, > > > but the language here should be 'is in sidr mode', not some confusing > > > open coded port space check. > > > > > > I'm also not sure of the lifecycle of the qkey, qkeys only exist in > > > SIDR mode so obviously anything that sets/gets a qkey should be > > > restriced to SIDR CM IDs.. > > > > > > > diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c > > > > index 835ac54d4a24..0a1f008ca929 100644 > > > > +++ b/drivers/infiniband/core/cma.c > > > > @@ -4669,12 +4669,8 @@ static int cma_join_ib_multicast(struct rdma_id_private *id_priv, > > > > if (ret) > > > > return ret; > > > > > > > > - ret = cma_set_qkey(id_priv, 0); > > > > - if (ret) > > > > - return ret; > > > > - > > > > cma_set_mgid(id_priv, (struct sockaddr *) &mc->addr, &rec.mgid); > > > > - rec.qkey = cpu_to_be32(id_priv->qkey); > > > > + rec.qkey = cpu_to_be32(RDMA_UDP_QKEY); > > > > > > And I'm not sure this makes sense? The UD qkey should still be > > > negotiated right? > > > > Yes, I think so, it will be changed in SIDR phase. > > > > The original code has "cma_set_qkey(id_priv, 0)" call, that in IB case will > > execute this switch anyway: > > 515 switch (id_priv->id.ps) { > > 516 case RDMA_PS_UDP: > > 517 case RDMA_PS_IB: > > 518 id_priv->qkey = RDMA_UDP_QKEY; > > > > The difference is that we won't store RDMA_UDP_QKEY in id_priv->qkey, > > but I'm unsure that this is right. > > Well the whoele cma_set_qkey() function appears to be complete > jumblied nonsense as if qkey is zero then it doesn't do anything if > the qkey was already set. > > When called with 0 it is really some sort of 'make a default qkey if > the user hasn't set one already' and in that case defaulting to > RDMA_UDP_QKEY does makes some kind of sense. > > The functions purposes should be split into two functions really. > > So, we end up with 'make sure the cm id is in SDIR mode' then 'if the > qkey is not set, set it to a default', so that the net result is the > qkey is always set once the function returns. > > Though, I'm not sure what the semantics are for qkey during SIDR > negotiation, that should be checked in the spec. There is no negotiation. Device simply sends its qkey to another side and expects to get this qkey in every packet. --------------------------------- Queue Key (Q_Key): Enforces access rights for reliable and unreliable datagram service (RAW datagram service type not included). Administered by the channel adapter. During communication establishment for datagram service, nodes exchange Q_Keys for particular queue pairs and a node uses the value it was passed for a remote QP in all packets it sends to that remote QP. Likewise, the remote node uses the Q_Key it was provided. Receipt of a packet with a different Q_Key than the one the node provided to the remote queue pair means that packet is not valid and thus rejected. ----------------------------------- Thanks > > Jason
diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c index 69c9a12dd14e..9c53e1e7de50 100644 --- a/drivers/infiniband/core/cma.c +++ b/drivers/infiniband/core/cma.c @@ -4737,7 +4737,7 @@ static int cma_iboe_join_multicast(struct rdma_id_private *id_priv, int err = 0; struct sockaddr *addr = (struct sockaddr *)&mc->addr; struct net_device *ndev = NULL; - struct ib_sa_multicast ib; + struct ib_sa_multicast ib = {}; enum ib_gid_type gid_type; bool send_only;