On Wed, Oct 23, 2019 at 4:40 AM David Wysochanski <dwysocha@xxxxxxxxxx> wrote: > > On Mon, Oct 21, 2019 at 5:55 PM Pavel Shilovsky <piastryyy@xxxxxxxxx> wrote: > > > > сб, 19 окт. 2019 г. в 04:10, David Wysochanski <dwysocha@xxxxxxxxxx>: > > > 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@xxxxxxxxxxxxxxx> to it. I think the patch looks good so whomever sends it to the list, please add a Reviewed-by: Ronnie Sahlberg <lsahlber@xxxxxxxxxx> > 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!
