On Tue, Aug 28, 2018 at 02:29:21PM -0400, Dennis Dalessandro wrote:
> On 8/28/2018 2:14 PM, Leon Romanovsky wrote:
> > On Tue, Aug 28, 2018 at 02:02:56PM -0400, Dennis Dalessandro wrote:
> > > On 8/28/2018 1:50 PM, Leon Romanovsky wrote:
> > > > On Tue, Aug 28, 2018 at 01:08:55PM -0400, Dennis Dalessandro wrote:
> > > > > On 8/28/2018 7:45 AM, Leon Romanovsky wrote:
> > > > > > From: Parav Pandit <parav@xxxxxxxxxxxx>
> > > > > >
> > > > > > When AF_IB addresses are used during rdma_resolve_addr(), currently lock
> > > > > > is not held. A cma device can get removed while such list traversal is
> > > > > > in progress which may lead to crash.
> > > > > >
> > > > > > Therefore, hold a lock while traversing the list which avoids such
> > > > > > situation.
> > > > > > This patch is not tested. It was discovered during code review.
> > > > > >
> > > > > > Cc: <stable@xxxxxxxxxxxxxxx> # 3.10
> > > > > > Fixes: f17df3b0d ("RDMA/cma: Add support for AF_IB to rdma_resolve_addr()")
> > > > > > Signed-off-by: Parav Pandit <parav@xxxxxxxxxxxx>
> > > > > > Reviewed-by: Daniel Jurgens <danielj@xxxxxxxxxxxx>
> > > > > > Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> > > > >
> > > > > Really? You are sending something to stable that hasn't been tested?
> > > > >
> > > >
> > > > Why are you saying that?
> > > >
> > > > Thanks,
> > > >
> > >
> > > I know I personally wouldn't let any patches from our drivers hit the list
> > > without being tested, and certainly patches that touch common code and may
> > > impact other drivers would definitely be tested.
> > >
> > > And I would guess this probably was tested or at least run through your QA,
> > > in which case, just strike that sentence from the commit message. We are
> > > trying to establish/maintain a good reputation here for our subsystem and
> > > when the casual reader of the git log comes along and sees this it tells
> > > them, hey RDMA folks take anything! Not the message we want to send I don't
> > > think.
> >
> > I agree with you, the more proper commit message should be:
> > "This patch is not tested. ..." -> "This patch doesn't come as an outcome of
> > real crash, but from code review. It was tested as part of our regular
> > QA submission process without simulation of pre vs. post specific flows.
> > No new bugs were found by the QA after applying this patch."
> >
> > Is it better?
>
> Much better. I'd even be fine just dropping the sentence, but this makes it
> clear why there isn't a stack trace or anything.
Well, it is a race, as a general rule, I would prefer to see a CPU
ladder diagram explaining how the race occures so the assertion there
is a race can actually be checked by someone..
Reproducing races is hard, so it is not surprising that a test case
wasn't created.
Jason
