Hi Doug,
> -----Original Message-----
> From: Doug Ledford <dledford@xxxxxxxxxx>
> Sent: Thursday, September 20, 2018 11:05 AM
> To: Leon Romanovsky <leon@xxxxxxxxxx>; Jason Gunthorpe
> <jgg@xxxxxxxxxxxx>
> Cc: Leon Romanovsky <leonro@xxxxxxxxxxxx>; RDMA mailing list <linux-
> rdma@xxxxxxxxxxxxxxx>; Daniel Jurgens <danielj@xxxxxxxxxxxx>; Parav
> Pandit <parav@xxxxxxxxxxxx>
> Subject: Re: [PATCH rdma-next v1 3/3] RDMA/cma: Introduce and use
> cma_ib_acquire_dev()
>
> On Sat, 2018-09-15 at 12:07 +0300, Leon Romanovsky wrote:
> >
> > + gid_type = listen_id_priv->cma_dev->default_gid_type[req->port - 1];
> > + sgid_attr = cma_validate_port(req->device, req->port,
> > + gid_type, &gid, id_priv);
> > + if (IS_ERR(sgid_attr))
> > + return PTR_ERR(sgid_attr);
> > +
> > + id_priv->id.port_num = req->port;
> > + cma_bind_sgid_attr(id_priv, sgid_attr);
> > + /* Need to acquire lock to protect against reader
> > + * of cma_dev->id_list such as cma_netdev_callback() and
> > + * cma_process_remove().
> > + */
> > + mutex_lock(&lock);
> > + cma_attach_to_dev(id_priv, listen_id_priv->cma_dev);
> > + mutex_unlock(&lock);
>
> Why does cma_ib_acquire_dev perform cma_validate_port and
> cma_bind_sgid_attr outside of the mutex_lock(&lock),
>
cma_bind_sgid_attr doesn't need to be protected because rdma_destroy_id() for listen id should wait until request handler is done with it.
But unfortunately it doesn't because cma_find_listener() doesn't increment the refcnt of listener_id.
So cma_validate_port and code below code line could potentially crash if the cma_process_remove()->cma_remove_id_dev() runs in parallel.
gid_type = listen_id_priv->cma_dev->default_gid_type[req->port - 1];
So with current code structure, cma_ib_acquire_dev() should take lock way early on.
So that cma_deref_id() in cma_process_remove() can create the barrier to not clear cma_dev and port in the listen_id.
Once that is done, cma_attach_to_dev() should return error if the cma_dev already under removal stage.
Above handling is needed for IB & iWarp regardless of taking the lock early on or not, I think.
Because following sequence can happen without these patch series.
CPU-0 CPU-1
-------- ---------
listen_id = cma_ib_id_from_event()
listen_id_without_ref.
cma_acquire_iw_dev() or
cma_acquire_dev()
cma_remove_one()
cma_process_remove()
rdma_destroy_id(listen_id);
mutex_lock();
listen_id (accessed after freed)
so maybe I can spin two unrelated patches before/after this series whose pseudo code is below?
cma_find_listener()
list_spin_lock_other_than_current_mutex_lock();
atomic_inc(&listen_id_priv->refcount);
list_spin_unlock();
cma_ib_req_handler() {
listen_id = ...
work_on_listen_id();
cma_deref_id(listen_id);
}
cma_remove_one() {
mutex_lock();
list_del();
cma_dev->state = REMOVING;
mutex_unlock();
}
int cma_attach_to_dev() {
if (cma_dev->state == REMOVING)
return err;
else {
existing_attach_code();
return 0;
}
cma_acquire_ib/iw_dev() should work on stable listen_id and its cma_dev without depending on mutex lock outside of this patchset.
> >
> > +static int cma_iw_acquire_dev(struct rdma_id_private *id_priv,
> > + const struct rdma_id_private *listen_id_priv)
> > {
> > struct rdma_dev_addr *dev_addr = &id_priv-
> >id.route.addr.dev_addr;
> > const struct ib_gid_attr *sgid_attr;
> > struct cma_device *cma_dev;
> > - union ib_gid gid, iboe_gid, *gidp;
> > enum ib_gid_type gid_type;
> > int ret = -ENODEV;
> > + union ib_gid gid;
> > u8 port;
> >
> > if (dev_addr->dev_type != ARPHRD_INFINIBAND &&
> > id_priv->id.ps == RDMA_PS_IPOIB)
> > return -EINVAL;
> >
> > - mutex_lock(&lock);
> > - rdma_ip2gid((struct sockaddr *)&id_priv->id.route.addr.src_addr,
> > - &iboe_gid);
> > -
> > memcpy(&gid, dev_addr->src_dev_addr +
> > - rdma_addr_gid_offset(dev_addr), sizeof gid);
> > + rdma_addr_gid_offset(dev_addr), sizeof(gid));
> > +
> > + mutex_lock(&lock);
> >
> > cma_dev = listen_id_priv->cma_dev;
> > port = listen_id_priv->id.port_num;
> > - gidp = rdma_protocol_roce(cma_dev->device, port) ? &iboe_gid :
> &gid;
> > gid_type = listen_id_priv->gid_type;
> > sgid_attr = cma_validate_port(cma_dev->device, port,
> > - gid_type, gidp, id_priv);
> > + gid_type, &gid, id_priv);
> > if (!IS_ERR(sgid_attr)) {
> > id_priv->id.port_num = port;
> > cma_bind_sgid_attr(id_priv, sgid_attr);
>
> while cma_iw_acquire_dev does those same two activities under the lock?
>
This reduces the probability with the racing with cma_process_remove(), but doesn't eliminate from what I understand.
> --
> Doug Ledford <dledford@xxxxxxxxxx>
> GPG KeyID: B826A3330E572FDD
> Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD
