Re: [PATCH] RDMA/rxe: Fix a race condition related to the QP error state

[Bart Van Assche <Bart.VanAssche-Sjgp3cTcYWE@public.gmane.org>]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2137 bytes --]

On Fri, 2018-01-12 at 08:23 +0200, Leon Romanovsky wrote:
> On Thu, Jan 11, 2018 at 07:07:06PM +0000, Bart Van Assche wrote:
> > On Thu, 2018-01-11 at 21:00 +0200, Leon Romanovsky wrote:
> > > On Thu, Jan 11, 2018 at 04:02:33PM +0000, Bart Van Assche wrote:
> > > > On Thu, 2018-01-11 at 08:22 +0200, Leon Romanovsky wrote:
> > > > > The proposed patch definitely decreases the chance of races, but it is not fixing them.
> > > > > There is a chance to have change in qp state immediately after your "if ..." check.
> > > > 
> > > > Hello Leon,
> > > > 
> > > > Please have a look at rxe_qp_error() and you will see that the patch I posted
> > > > is a proper fix. In the scenario you described rxe_qp_error() will trigger a
> > > > run of rxe_completer().
> > > 
> > > Bart,
> > > 
> > > What am I missing?
> > > 
> > > CPU1							CPU2
> > > 							if (unlikely....
> > > 						<---
> > > /* move the qp to the error state */
> > > void rxe_qp_error(struct rxe_qp *qp)
> > > {
> > >         qp->req.state = QP_STATE_ERROR;
> > >         qp->resp.state = QP_STATE_ERROR;
> > >         qp->attr.qp_state = IB_QPS_ERR;
> > > 						--->
> > > 							rxe_run_task(&qp->req.task, must_sched);
> > > 
> > > 
> > > 
> > > It is more or less the same as without "if (unlikely..."
> > 
> > Hello Leon,
> > 
> > In the above the part of rxe_qp_error() that I was referring to in my e-mail
> > is missing:
> > 
> > 	if (qp_type(qp) == IB_QPT_RC)
> > 		rxe_run_task(&qp->comp.task, 1);
> 
> 
> But it is exactly where race exists, as long QP isn't protected, it can
> switch CPUs and create race.

Hello Leon,

Can you clarify which race you are referring to? rxe_run_task() uses the
tasklet mechanism and tasklets are guaranteed to run on at most one CPU at a
time. See also the "Top and Bottom Halves" chapter in Linux Device Drivers,
3rd edition. See also the tasklet_schedule() implementation in
<linux/interrupt.h> and in kernel/softirq.c.

Thanks,

Bart.N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·¥Š{±­ÙšŠ{ayº\x1dʇڙë,j\a­¢f£¢·hš‹»öì\x17/oSc¾™Ú³9˜uÀ¦æå‰È&jw¨®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿïêäz¹Þ–Šàþf£¢·hšˆ§~ˆmš