On Thu, Jun 21, 2018 at 09:37:44AM -0600, Jason Gunthorpe wrote:
> On Thu, Jun 21, 2018 at 03:31:24PM +0300, Leon Romanovsky wrote:
> > From: Parav Pandit <parav@xxxxxxxxxxxx>
> >
> > When sending multicast leave request, consider the net ns in which this
> > cm_id is created.
> >
> > Code was duplicated in cma_leave_mc_groups() and rdma_leave_multicast(),
> > which is now done using a helper function cma_leave_roce_mc_group().
> >
> > Fixes: bee3c3c91865 ("IB/cma: Join and leave multicast groups with IGMP")
> > Reviewed-by: Daniel Jurgens <danielj@xxxxxxxxxxxx>
> > Signed-off-by: Parav Pandit <parav@xxxxxxxxxxxx>
> > Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> > ---
> > drivers/infiniband/core/cma.c | 57 ++++++++++++++++++-------------------------
> > 1 file changed, 24 insertions(+), 33 deletions(-)
>
> There is nothing wrong with this patch.. But looking at this I see
> a bug.. What do you think about the below?
I think that you are right.
>
> From 8e1ec13aeae2105468481fbc5ff01461e91b58b7 Mon Sep 17 00:00:00 2001
> From: Jason Gunthorpe <jgg@xxxxxxxxxxxx>
> Date: Thu, 21 Jun 2018 09:02:30 -0600
> Subject: [PATCH] IB/cm: Remove cma_multicast->igmp_joined
>
> This variable isn't read and written to with proper locking, so it is
> racy. Instead of using an unlocked bool use presence in the mc->list ie
> the caller could race rdma_join_multicast with rdma_leave_multicast which
> would leak a mc join and cause a use after free of mc.
>
> Instead, do not add the mc to the list until it has completed
> initialization, all mcs on the list require leaving.
>
> Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxxxx>
> ---
> drivers/infiniband/core/cma.c | 53 +++++++++++++++--------------------
> 1 file changed, 23 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
> index fca2854749e59a..543254fe23610a 100644
> --- a/drivers/infiniband/core/cma.c
> +++ b/drivers/infiniband/core/cma.c
> @@ -366,7 +366,6 @@ struct cma_multicast {
> void *context;
> struct sockaddr_storage addr;
> struct kref mcref;
> - bool igmp_joined;
> u8 join_state;
> };
>
> @@ -1632,21 +1631,14 @@ static void cma_release_port(struct rdma_id_private *id_priv)
> static void cma_leave_roce_mc_group(struct rdma_id_private *id_priv,
> struct cma_multicast *mc)
> {
> - if (mc->igmp_joined) {
> - struct rdma_dev_addr *dev_addr =
> - &id_priv->id.route.addr.dev_addr;
> - struct net_device *ndev = NULL;
> -
> - if (dev_addr->bound_dev_if)
> - ndev = dev_get_by_index(dev_addr->net,
> - dev_addr->bound_dev_if);
> - if (ndev) {
> - cma_igmp_send(ndev,
> - &mc->multicast.ib->rec.mgid,
> - false);
> - dev_put(ndev);
> - }
> - mc->igmp_joined = false;
> + struct rdma_dev_addr *dev_addr = &id_priv->id.route.addr.dev_addr;
> + struct net_device *ndev = NULL;
> +
> + if (dev_addr->bound_dev_if)
> + ndev = dev_get_by_index(dev_addr->net, dev_addr->bound_dev_if);
> + if (ndev) {
> + cma_igmp_send(ndev, &mc->multicast.ib->rec.mgid, false);
> + dev_put(ndev);
> }
> kref_put(&mc->mcref, release_mc);
> }
> @@ -4175,8 +4167,6 @@ static int cma_iboe_join_multicast(struct rdma_id_private *id_priv,
> if (!send_only) {
> err = cma_igmp_send(ndev, &mc->multicast.ib->rec.mgid,
> true);
> - if (!err)
> - mc->igmp_joined = true;
> }
> }
> } else {
> @@ -4228,26 +4218,29 @@ int rdma_join_multicast(struct rdma_cm_id *id, struct sockaddr *addr,
> memcpy(&mc->addr, addr, rdma_addr_size(addr));
> mc->context = context;
> mc->id_priv = id_priv;
> - mc->igmp_joined = false;
> mc->join_state = join_state;
> - spin_lock(&id_priv->lock);
> - list_add(&mc->list, &id_priv->mc_list);
> - spin_unlock(&id_priv->lock);
>
> if (rdma_protocol_roce(id->device, id->port_num)) {
> kref_init(&mc->mcref);
> ret = cma_iboe_join_multicast(id_priv, mc);
> - } else if (rdma_cap_ib_mcast(id->device, id->port_num))
> + if (ret)
> + goto out_err;
> + } else if (rdma_cap_ib_mcast(id->device, id->port_num)) {
> ret = cma_join_ib_multicast(id_priv, mc);
> - else
> + if (ret)
> + goto out_err;
> + } else {
> ret = -ENOSYS;
> -
> - if (ret) {
> - spin_lock_irq(&id_priv->lock);
> - list_del(&mc->list);
> - spin_unlock_irq(&id_priv->lock);
> - kfree(mc);
> + goto out_err;
> }
> +
> + spin_lock(&id_priv->lock);
> + list_add(&mc->list, &id_priv->mc_list);
> + spin_unlock(&id_priv->lock);
> +
> + return 0;
> +out_err:
> + kfree(mc);
> return ret;
> }
> EXPORT_SYMBOL(rdma_join_multicast);
> --
> 2.17.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Attachment:
signature.asc
Description: PGP signature
