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

[Ronnie Sahlberg <lsahlber@redhat.com>]

----- Original Message -----
> From: "Pavel Shilovskiy" <pshilov@microsoft.com>
> To: "Ronnie Sahlberg" <lsahlber@redhat.com>
> Cc: "David Wysochanski" <dwysocha@redhat.com>, "linux-cifs" <linux-cifs@vger.kernel.org>, "Frank Sorenson"
> <sorenson@redhat.com>, "Steven French" <Steven.French@microsoft.com>
> Sent: Friday, 18 October, 2019 9:20:51 AM
> Subject: RE: list_del corruption while iterating retry_list in cifs_reconnect still seen on 5.4-rc3
> 
> Agree.
> 
> Probably the change in dequeue_mid() is not needed but won't hurt at least -
> right now dequeue_mid() is being called from the demultiplex thread only, so
> as cifs_reconnect(). I am wondering how your patch behaves with the repro.

Thanks.
Dave, can you test with your reproducer if this makes things better?


> 
> In general, I am starting to think more that we should probably remove a MID
> immediately from the pending list once we parse MessageId from the response
> and find the entry in the list. Especially with the recent parallel
> decryption capability that Steve is working on, we would need to break the
> above assumption and process the mid entry in another thread. There are some
> cases where we don't end up removing the MID but for those cases we may
> simply add the entry back. Anyway, it needs much more thinking and out of
> the scope of the bugfix being discussed.

I agree.

Btw, I think we have had bug sin this code since at least the 3.x kernels
so we need a simple fix that is easy to backport to stable and beyond.


> 
> --
> Best regards,
> Pavel Shilovsky
> 
> -----Original Message-----
> From: Ronnie Sahlberg <lsahlber@redhat.com>
> Sent: Thursday, October 17, 2019 3:54 PM
> To: Pavel Shilovskiy <pshilov@microsoft.com>
> Cc: David Wysochanski <dwysocha@redhat.com>; linux-cifs
> <linux-cifs@vger.kernel.org>; Frank Sorenson <sorenson@redhat.com>
> Subject: Re: list_del corruption while iterating retry_list in cifs_reconnect
> still seen on 5.4-rc3
> 
> That could work. But then we should also use that flag to suppress the other
> places where we do a list_del*, so something like this ?
> 
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h index
> 50dfd9049370..b324fff33e53 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -1702,6 +1702,7 @@ static inline bool is_retryable_error(int error)
>  /* Flags */
>  #define   MID_WAIT_CANCELLED    1 /* Cancelled while waiting for response */
>  #define   MID_DELETED            2 /* Mid has been dequeued/deleted */
> +#define   MID_RECONNECT          4 /* Mid is being used during reconnect */
>  
>  /* Types of response buffer returned from SendReceive2 */
>  #define   CIFS_NO_BUFFER        0    /* Response buffer not returned */
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c index
> bdea4b3e8005..b142bd2a3ef5 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -564,6 +564,8 @@ cifs_reconnect(struct TCP_Server_Info *server)
>         spin_lock(&GlobalMid_Lock);
>         list_for_each_safe(tmp, tmp2, &server->pending_mid_q) {
>                 mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
> +               kref_get(&mid_entry->refcount);
> +               mid_entry->mid_flags |= MID_RECONNECT;
>                 if (mid_entry->mid_state == MID_REQUEST_SUBMITTED)
>                         mid_entry->mid_state = MID_RETRY_NEEDED;
>                 list_move(&mid_entry->qhead, &retry_list); @@ -575,7 +577,9
>                 @@ cifs_reconnect(struct TCP_Server_Info *server)
>         list_for_each_safe(tmp, tmp2, &retry_list) {
>                 mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
>                 list_del_init(&mid_entry->qhead);
> +
>                 mid_entry->callback(mid_entry);
> +               cifs_mid_q_entry_release(mid_entry);
>         }
>  
>         if (cifs_rdma_enabled(server)) { @@ -895,7 +899,7 @@
>         dequeue_mid(struct mid_q_entry *mid, bool malformed)
>         if (mid->mid_flags & MID_DELETED)
>                 printk_once(KERN_WARNING
>                             "trying to dequeue a deleted mid\n");
> -       else
> +       else if (!(mid->mid_flags & MID_RECONNECT))
>                 list_del_init(&mid->qhead);
>         spin_unlock(&GlobalMid_Lock);
>  }
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c index
> 308ad0f495e1..ba4b5ab9cf35 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -173,7 +173,8 @@ void
>  cifs_delete_mid(struct mid_q_entry *mid)  {
>         spin_lock(&GlobalMid_Lock);
> -       list_del_init(&mid->qhead);
> +       if (!(mid->mid_flags & MID_RECONNECT))
> +               list_del_init(&mid->qhead);
>         mid->mid_flags |= MID_DELETED;
>         spin_unlock(&GlobalMid_Lock);
>  
> @@ -872,7 +873,8 @@ cifs_sync_mid_result(struct mid_q_entry *mid, struct
> TCP_Server_Info *server)
>                 rc = -EHOSTDOWN;
>                 break;
>         default:
> -               list_del_init(&mid->qhead);
> +               if (!(mid->mid_flags & MID_RECONNECT))
> +                       list_del_init(&mid->qhead);
>                 cifs_server_dbg(VFS, "%s: invalid mid state mid=%llu
>                 state=%d\n",
>                          __func__, mid->mid, mid->mid_state);
>                 rc = -EIO;
>