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

[ronnie sahlberg <ronniesahlberg@gmail.com>]

On Wed, Oct 23, 2019 at 4:40 AM David Wysochanski <dwysocha@redhat.com> wrote:
>
> On Mon, Oct 21, 2019 at 5:55 PM Pavel Shilovsky <piastryyy@gmail.com> wrote:
> >
> > сб, 19 окт. 2019 г. в 04:10, David Wysochanski <dwysocha@redhat.com>:
> > > 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.
> >
> > This is acceptable and it is a good practice to fix the existing issue
> > with the smallest possible patch and then enhance the code/fix for the
> > current master branch if needed. This simplify backporting a lot.
> >
> > Actually looking at the code:
> >
> > cifsglob.h:
> >
> > 1692 #define   MID_DELETED            2 /* Mid has been dequeued/deleted */
> >
> >                     ^^^
> > Isn't "deqeueued" what we need? It seems so because it serves the same
> > purpose: to indicate that a request has been deleted from the pending
> > queue. So, I think we need to just make use of this existing flag and
> > mark the mid with MID_DELETED every time we remove the mid from the
> > pending list. Also assume moving mids from the pending lists to the
> > local lists in cleanup_demultiplex_info and cifs_reconnect as a
> > deletion too because those lists are not exposed globally and mids are
> > removed from those lists before the functions exit.
> >
> > I made a patch which is using MID_DELETED logic and merging
> > DeleteMidQEntry and cifs_mid_q_entry_release into one function to
> > avoid possible use-after free of mid->resp_buf.
> >
> > David, could you please test the attached patch in your environment? I
> > only did sanity testing of it.
> >
> I ran 5.4-rc4 plus this patch with the reproducer, and it ran fine for
> over 6 hours.

That is great news and sounds like it is time to get this submitted to for-next
and stable.

Can you send this as a proper patch to the list so we can get it into
steves for-next branch.
Please add a CC: Stable <stable@vger.kernel.org> to it.


I think the patch looks good so whomever sends it to the list, please add a
Reviewed-by: Ronnie Sahlberg <lsahlber@redhat.com>


> I verified 5.4-rc4 would still crash too - at first I wasn't sure
> since it took about 30 mins to crash, but it definitely crashes too
> (not surprising).
>
> Your patch seems reasonable to me and is in the spirit of the existing
> code and the flag idea that Ronnie had.
>
> To be honest when I look at the other flag (unrelated to this problem)
> I am also not sure if it should be a state or a flag, but you probably
> know the history on mid_state vs flag better than me.  For purposes of
> this bug, I think your patch is fine and if you're wanting a stable
> patch and this looks better, FWIW this is fine with me.  I think
> probably as your comments earlier there is probably more refactoring
> or work that can be done in this area, but is beyond the scope of a
> stable patch.
>
> Thanks!