> -----Original Message----- > From: Jason Gunthorpe [mailto:jgg@xxxxxxxx] > Sent: Wednesday, March 14, 2018 6:17 PM > To: Leon Romanovsky <leon@xxxxxxxxxx> > Cc: Doug Ledford <dledford@xxxxxxxxxx>; Leon Romanovsky > <leonro@xxxxxxxxxxxx>; RDMA mailing list <linux-rdma@xxxxxxxxxxxxxxx>; > Daniel Jurgens <danielj@xxxxxxxxxxxx>; Mark Bloch <markb@xxxxxxxxxxxx>; > Parav Pandit <parav@xxxxxxxxxxxx> > Subject: Re: [PATCH rdma-next v1 09/13] IB/cm: Avoid AV ah_attr overwriting > during LAP message handling > > On Tue, Mar 13, 2018 at 04:06:19PM +0200, Leon Romanovsky wrote: > > From: Parav Pandit <parav@xxxxxxxxxxxx> > > > > AH attribute of the cm_id can be overwritten if LAP message is > > received on CM request which is in progress. This bug got introduced > > to avoid sleeping when spin lock is held as part of commit in Fixes tag. > > > > Therefore validate the cm_id state first and continue to perform AV > > ah_attr initialization outside of the non blocking context. > > > > Fixes: 33f93e1ebcf5 ("IB/cm: Fix sleeping while spin lock is held") > > Reviewed-by: Daniel Jurgens <danielj@xxxxxxxxxxxx> > > Signed-off-by: Parav Pandit <parav@xxxxxxxxxxxx> > > Signed-off-by: Leon Romanovsky <leon@xxxxxxxxxx> > > drivers/infiniband/core/cm.c | 24 ++++++++++++++++-------- > > 1 file changed, 16 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/infiniband/core/cm.c > > b/drivers/infiniband/core/cm.c index c5cd1b3ffa54..c9b325040df3 100644 > > +++ b/drivers/infiniband/core/cm.c > > @@ -3168,12 +3168,6 @@ static int cm_lap_handler(struct cm_work *work) > > if (!cm_id_priv) > > return -EINVAL; > > > > - ret = cm_init_av_for_response(work->port, work->mad_recv_wc->wc, > > - work->mad_recv_wc->recv_buf.grh, > > - &cm_id_priv->av); > > - if (ret) > > - goto deref; > > - > > param = &work->cm_event.param.lap_rcvd; > > memset(&work->path[0], 0, sizeof(work->path[1])); > > cm_path_set_rec_type(work->port->cm_dev->ib_device, > > @@ -3218,10 +3212,24 @@ static int cm_lap_handler(struct cm_work *work) > > goto unlock; > > } > > > > - cm_id_priv->id.lap_state = IB_CM_LAP_RCVD; > > - cm_id_priv->tid = lap_msg->hdr.tid; > > + /* It is safe to release spin lock while working on AV > > + * initialization from path record. > > + * While AV initialization in progress, refcount is taken > > + * by cm_acquire_id() at the start, therefore it is safe to > > + * release spin lock and re-acquire before updating state. > > + */ > > + spin_unlock_irq(&cm_id_priv->lock); > > + ret = cm_init_av_for_response(work->port, work->mad_recv_wc->wc, > > + work->mad_recv_wc->recv_buf.grh, > > + &cm_id_priv->av); > > + if (ret) > > + goto deref; > > + > > cm_init_av_by_path(param->alternate_path, &cm_id_priv->alt_av, > > cm_id_priv); > > + spin_lock_irq(&cm_id_priv->lock); > > + cm_id_priv->id.lap_state = IB_CM_LAP_RCVD; > > Oy, this is hard to check. The comment is not good enough.. Firstly this could > only even begin to be safe if because cm_lap_handler is run single threaded > through a work queue. > > Indeed every call to cm_init_av_for_response needs to be run through the work > queue. > > But it just isn't.. > > I'm not sure the history but cm_init_av_by_path looks wrongly locked in other > places too. It looks like it should always be called with the spinlock held and it > isn't.. > > So we can call something like ib_send_cm_req from userspace and race it with > the above code, and corrupt av. Not sure why ibv_send_cm_req doesn't hold the > spinlock while calling cm_init_av_by_path, probably should. AV initialization is blocking call. > > I think all the places that write to the av need to update the av under the spin > lock. Looks like the best way to do it above is to write a temporary av to the > stack and then re-acquire the lock and copy it to the priv ?? All places that write > to av need checking.. This is done for non-LAP message processing path in internal series under review to you, Leon, Mark for a while now. Since you are asking this more high level question, I like to ask what is the state of [1]? [1] is not submitted as RFC, so I presume that is strong need to drop this module. And if we are dropping ucm module out of the kernel than all the APIs which are only called by UCM module should go out too. And LAP message are part of it. So like to know whether to invest more energy in making this code right, which has almost no users? -- 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
