[PATCH] Initialise mid_q_entry before putting it on the pending queue

[PATCH] Initialise mid_q_entry before putting it on the pending queue

[Sachin Prabhu]

A user reported a crash in cifs_demultiplex_thread() caused by an
incorrectly set mid_q_entry->callback() function. It appears that the
callback assignment made in cifs_call_async() was not flushed back to
memory suggesting that a memory barrier was required here. Changing the
code to make sure that the mid_q_entry structure was completely
initialised before it was added to the pending queue fixes the problem.

Signed-off-by: Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/transport.c |   20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index 3097ee5..c5faf02 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -365,14 +365,10 @@ cifs_setup_async_request(struct TCP_Server_Info *server, struct kvec *iov,
 	if (mid == NULL)
 		return -ENOMEM;
 
-	/* put it on the pending_mid_q */
-	spin_lock(&GlobalMid_Lock);
-	list_add_tail(&mid->qhead, &server->pending_mid_q);
-	spin_unlock(&GlobalMid_Lock);
-
 	rc = cifs_sign_smb2(iov, nvec, server, &mid->sequence_number);
 	if (rc)
-		delete_mid(mid);
+		DeleteMidQEntry(mid);
+
 	*ret_mid = mid;
 	return rc;
 }
@@ -407,17 +403,21 @@ cifs_call_async(struct TCP_Server_Info *server, struct kvec *iov,
 	mid->callback_data = cbdata;
 	mid->mid_state = MID_REQUEST_SUBMITTED;
 
+	/* put it on the pending_mid_q */
+	spin_lock(&GlobalMid_Lock);
+	list_add_tail(&mid->qhead, &server->pending_mid_q);
+	spin_unlock(&GlobalMid_Lock);
+
+
 	cifs_in_send_inc(server);
 	rc = smb_sendv(server, iov, nvec);
 	cifs_in_send_dec(server);
 	cifs_save_when_sent(mid);
 	mutex_unlock(&server->srv_mutex);
 
-	if (rc)
-		goto out_err;
+	if (rc == 0)
+		return 0;
 
-	return rc;
-out_err:
 	delete_mid(mid);
 	add_credits(server, 1);
 	wake_up(&server->request_q);
-- 
1.7.10.4

Re: [PATCH] Initialise mid_q_entry before putting it on the pending queue

[Jeff Layton]

On Tue, 10 Jul 2012 19:58:40 +0100
Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:

> A user reported a crash in cifs_demultiplex_thread() caused by an
> incorrectly set mid_q_entry->callback() function. It appears that the
> callback assignment made in cifs_call_async() was not flushed back to
> memory suggesting that a memory barrier was required here. Changing the
> code to make sure that the mid_q_entry structure was completely
> initialised before it was added to the pending queue fixes the problem.
> 
> Signed-off-by: Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/transport.c |   20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index 3097ee5..c5faf02 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -365,14 +365,10 @@ cifs_setup_async_request(struct TCP_Server_Info *server, struct kvec *iov,
>  	if (mid == NULL)
>  		return -ENOMEM;
>  
> -	/* put it on the pending_mid_q */
> -	spin_lock(&GlobalMid_Lock);
> -	list_add_tail(&mid->qhead, &server->pending_mid_q);
> -	spin_unlock(&GlobalMid_Lock);
> -
>  	rc = cifs_sign_smb2(iov, nvec, server, &mid->sequence_number);
>  	if (rc)
> -		delete_mid(mid);
> +		DeleteMidQEntry(mid);
> +
>  	*ret_mid = mid;
>  	return rc;
>  }
> @@ -407,17 +403,21 @@ cifs_call_async(struct TCP_Server_Info *server, struct kvec *iov,
>  	mid->callback_data = cbdata;
>  	mid->mid_state = MID_REQUEST_SUBMITTED;
>  
> +	/* put it on the pending_mid_q */
> +	spin_lock(&GlobalMid_Lock);
> +	list_add_tail(&mid->qhead, &server->pending_mid_q);
> +	spin_unlock(&GlobalMid_Lock);
> +
> +
>  	cifs_in_send_inc(server);
>  	rc = smb_sendv(server, iov, nvec);
>  	cifs_in_send_dec(server);
>  	cifs_save_when_sent(mid);
>  	mutex_unlock(&server->srv_mutex);
>  
> -	if (rc)
> -		goto out_err;
> +	if (rc == 0)
> +		return 0;
>  
> -	return rc;
> -out_err:
>  	delete_mid(mid);
>  	add_credits(server, 1);
>  	wake_up(&server->request_q);

Looks correct.

Reviewed-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH] Initialise mid_q_entry before putting it on the pending queue

[Shirish Pargaonkar]

On Tue, Jul 10, 2012 at 1:58 PM, Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> A user reported a crash in cifs_demultiplex_thread() caused by an
> incorrectly set mid_q_entry->callback() function. It appears that the
> callback assignment made in cifs_call_async() was not flushed back to
> memory suggesting that a memory barrier was required here. Changing the
> code to make sure that the mid_q_entry structure was completely
> initialised before it was added to the pending queue fixes the problem.
>
> Signed-off-by: Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/transport.c |   20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index 3097ee5..c5faf02 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -365,14 +365,10 @@ cifs_setup_async_request(struct TCP_Server_Info *server, struct kvec *iov,
>         if (mid == NULL)
>                 return -ENOMEM;
>
> -       /* put it on the pending_mid_q */
> -       spin_lock(&GlobalMid_Lock);
> -       list_add_tail(&mid->qhead, &server->pending_mid_q);
> -       spin_unlock(&GlobalMid_Lock);
> -
>         rc = cifs_sign_smb2(iov, nvec, server, &mid->sequence_number);
>         if (rc)
> -               delete_mid(mid);
> +               DeleteMidQEntry(mid);
> +
>         *ret_mid = mid;

Looks correct.  But just a thought, why bother assigning ret_mid
a Deleted mid if rc is not null?

>         return rc;
>  }
> @@ -407,17 +403,21 @@ cifs_call_async(struct TCP_Server_Info *server, struct kvec *iov,
>         mid->callback_data = cbdata;
>         mid->mid_state = MID_REQUEST_SUBMITTED;
>
> +       /* put it on the pending_mid_q */
> +       spin_lock(&GlobalMid_Lock);
> +       list_add_tail(&mid->qhead, &server->pending_mid_q);
> +       spin_unlock(&GlobalMid_Lock);
> +
> +
>         cifs_in_send_inc(server);
>         rc = smb_sendv(server, iov, nvec);
>         cifs_in_send_dec(server);
>         cifs_save_when_sent(mid);
>         mutex_unlock(&server->srv_mutex);
>
> -       if (rc)
> -               goto out_err;
> +       if (rc == 0)
> +               return 0;
>
> -       return rc;
> -out_err:
>         delete_mid(mid);
>         add_credits(server, 1);
>         wake_up(&server->request_q);
> --
> 1.7.10.4
>

Re: [PATCH] Initialise mid_q_entry before putting it on the pending queue

[Sachin Prabhu]

On Tue, 2012-07-10 at 14:26 -0500, Shirish Pargaonkar wrote:
> On Tue, Jul 10, 2012 at 1:58 PM, Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > A user reported a crash in cifs_demultiplex_thread() caused by an
> > incorrectly set mid_q_entry->callback() function. It appears that the
> > callback assignment made in cifs_call_async() was not flushed back to
> > memory suggesting that a memory barrier was required here. Changing the
> > code to make sure that the mid_q_entry structure was completely
> > initialised before it was added to the pending queue fixes the problem.
> >
> > Signed-off-by: Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >  fs/cifs/transport.c |   20 ++++++++++----------
> >  1 file changed, 10 insertions(+), 10 deletions(-)
> >
> > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> > index 3097ee5..c5faf02 100644
> > --- a/fs/cifs/transport.c
> > +++ b/fs/cifs/transport.c
> > @@ -365,14 +365,10 @@ cifs_setup_async_request(struct TCP_Server_Info *server, struct kvec *iov,
> >         if (mid == NULL)
> >                 return -ENOMEM;
> >
> > -       /* put it on the pending_mid_q */
> > -       spin_lock(&GlobalMid_Lock);
> > -       list_add_tail(&mid->qhead, &server->pending_mid_q);
> > -       spin_unlock(&GlobalMid_Lock);
> > -
> >         rc = cifs_sign_smb2(iov, nvec, server, &mid->sequence_number);
> >         if (rc)
> > -               delete_mid(mid);
> > +               DeleteMidQEntry(mid);
> > +
> >         *ret_mid = mid;
> 
> Looks correct.  But just a thought, why bother assigning ret_mid
> a Deleted mid if rc is not null?

I did notice this line which was already set to do this when I wrote the
patch. Since it did not result in any error, I left it alone. I will
create another patch which cleans this bit of the code.

Sachin Prabhu
> 
> >         return rc;
> >  }
> > @@ -407,17 +403,21 @@ cifs_call_async(struct TCP_Server_Info *server, struct kvec *iov,
> >         mid->callback_data = cbdata;
> >         mid->mid_state = MID_REQUEST_SUBMITTED;
> >
> > +       /* put it on the pending_mid_q */
> > +       spin_lock(&GlobalMid_Lock);
> > +       list_add_tail(&mid->qhead, &server->pending_mid_q);
> > +       spin_unlock(&GlobalMid_Lock);
> > +
> > +
> >         cifs_in_send_inc(server);
> >         rc = smb_sendv(server, iov, nvec);
> >         cifs_in_send_dec(server);
> >         cifs_save_when_sent(mid);
> >         mutex_unlock(&server->srv_mutex);
> >
> > -       if (rc)
> > -               goto out_err;
> > +       if (rc == 0)
> > +               return 0;
> >
> > -       return rc;
> > -out_err:
> >         delete_mid(mid);
> >         add_credits(server, 1);
> >         wake_up(&server->request_q);
> > --
> > 1.7.10.4
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html