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

[Pavel Shilovsky <piastryyy@gmail.com>]

Hi Ronnie,

Thanks for reviewing the patch, I will add your Reviewed-by.

The mainline version (5.4-rc4) of the patch doesn't apply cleanly to
any active stable kernel. Do you think it still needs the Stable tag?
I was going to prepare a stable version and mention all dependencies
anyway.

--
Best regards,
Pavel Shilovsky

вт, 22 окт. 2019 г. в 14:20, 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!