Re: list_del corruption while iterating retry_list in cifs_reconnect still seen on 5.4-rc3

[David Wysochanski <dwysocha@redhat.com>]

On Fri, Oct 18, 2019 at 6:45 PM Pavel Shilovskiy <pshilov@microsoft.com> wrote:
>
> On Fri, Oct 18, 2019 at 5:21 PM David Wysochanski <dwysocha@redhat.com> wrote:
> >
> > On Fri, Oct 18, 2019 at 4:59 PM Pavel Shilovskiy <pshilov@microsoft.com> wrote:
> > >
> > > Thanks for the good news that the patch is stable in your workload!
> > >
> > The attached patch I ran on top of 5.4-rc3 for over 5 hrs today on the
> > reboot test - before it would crash after a few minutes tops.
>
> This is great! Thanks for verifying the fix.
>
> > > The extra flag may not be necessary and we may rely on a MID state but we would need to handle two states actually: MID_RETRY_NEEDED and MID_SHUTDOWN - see clean_demultiplex_info() which is doing the same things with mid as cifs_reconnect(). Please add ref counting to both functions since they both can race with system call threads.
> > >
>
> > I agree that loop has the same problem.  I can add that you're ok with the mid_state approach.  I think the only other option is probably a flag like Ronnie
> > suggested.
> > I will have to review the state machine more when I am more alert if you are concerned about possible subtle regressions.
>
> I am ok with both approaches as long as the stable patch is minimal. Thinking about this conditional assignment of the mid retry state: I don't think there is any case in the current code base where the WARN_ON you proposed would fire but I can't be sure about all possible stable kernel that the stable patch is going to be applied.
>

Right but look at it this way.  If we conditionally set the state,
then what is preventing a duplicate list_del_init call?  Let's say we
get into the special case that you're not sure it could happen
(mid_entry->mid_state == MID_REQUEST_SUBMITTED is false), and so the
mid_state does not get set to MID_RETRY_NEEDED inside cifs_reconnect
but yet the mid gets added to retry_list.  In that case both the
cifs_reconnect code path will call list_del_init as well as the other
code paths which we're adding the conditional tests and that will
cause a blowup again because cifs_reconnect retry_list loop will end
up in a singleton list and exhaust the refcount, leading to the same
crash.  This is exactly why the refcount only patch crashed again -
it's erroneous to think it's ok to modify mid_entry->qhead without a)
taking globalMid_Lock and b) checking mid_state is what you think it
should be.  But if you're really concerned about that 'if' condition
and want to leave it, and you want a stable patch, then the extra flag
seems like the way to go.  But that has the downside that it's only
being done for stable, so a later patch will likely remove it
(presumably).  I am not sure what such policy is or if that is even
acceptable or allowed.

> Another more general thought: we have cifs_delete_mid -> DeleteMidQEntry -> _cifs_mid_q_entry_release chain of calls and every function frees its own part of the mid entry. I think we should merge the last two at least. It would allow us to guarantee that holding a reference to the mid means:
>
> 1) the mid itself is valid;
> 2) the mid response buffer is valid;
> 3) the mid is in a list if it is REQUEST_SUBMITTED, RETRY_NEEDED or SHUTDOWN and is not in a list if it is ALLOCATED, RESPONSE_RECEIVED, RESPONSE_MALFORMED or FREE; the release function should remove the mid from the list or warn appropriately depending on a state of the mid.
>
> The mid state and list location are changed only when the GlobalMid_Lock is held. In this case cifs_delete_mid is not needed too because all what it does will be done in the release function. I think this would allow to avoid all the problems discussed in this thread but looks too risky for stable.
>
> --
> Best regards,
> Pavel Shilovsky