[v2,1/6] cifs: smbd: Check for iov length on sending the last iov

Message ID 20180417191710.14855-1-longli@linuxonhyperv.com
State New
Headers show
Series
  • [v2,1/6] cifs: smbd: Check for iov length on sending the last iov
Related show

Commit Message

Long Li April 17, 2018, 7:17 p.m.
From: Long Li <longli@microsoft.com>

When sending the last iov that breaks into smaller buffers to fit the
transfer size, it's necessary to check if this is the last iov.

If this is the latest iov, stop and proceed to send pages.

Signed-off-by: Long Li <longli@microsoft.com>
Cc: stable@vger.kernel.org
---
 fs/cifs/smbdirect.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Michael Kelley (EOSG) April 22, 2018, 11:27 p.m. | #1
> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org <linux-kernel-owner@vger.kernel.org> On Behalf
> Of Long Li
> Sent: Tuesday, April 17, 2018 12:17 PM
> To: Steve French <sfrench@samba.org>; linux-cifs@vger.kernel.org; samba-
> technical@lists.samba.org; linux-kernel@vger.kernel.org; linux-rdma@vger.kernel.org
> Cc: Long Li <longli@microsoft.com>; stable@vger.kernel.org
> Subject: [Patch v2 1/6] cifs: smbd: Check for iov length on sending the last iov
> 
> From: Long Li <longli@microsoft.com>
> 
> When sending the last iov that breaks into smaller buffers to fit the
> transfer size, it's necessary to check if this is the last iov.
> 
> If this is the latest iov, stop and proceed to send pages.
> 
> Signed-off-by: Long Li <longli@microsoft.com>
> Cc: stable@vger.kernel.org

Reviewed-by: Michael Kelley <mikelley@microsoft.com>

But a question unrelated to this change arose during my review:  At the
beginning and end of smbd_send(), the field smbd_send_pending is
incremented and decremented, respectively.   The increment/decrement
are not done as atomic operations.  Is this code guaranteed to be single
threaded?  If not, the count could become corrupted, and
smbd_destroy_rdma_work(), which waits for the count to become zero,
could hang.  A similar question applies to smbd_recv_pending in smbd_recv().

> ---
>  fs/cifs/smbdirect.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/cifs/smbdirect.c b/fs/cifs/smbdirect.c
> index 90e673c..b5c6c0d 100644
> --- a/fs/cifs/smbdirect.c
> +++ b/fs/cifs/smbdirect.c
> @@ -2197,6 +2197,8 @@ int smbd_send(struct smbd_connection *info, struct smb_rqst *rqst)
>  						goto done;
>  				}
>  				i++;
> +				if (i == rqst->rq_nvec)
> +					break;
>  			}
>  			start = i;
>  			buflen = 0;
> --
> 2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Long Li April 23, 2018, 7:34 p.m. | #2
> Subject: RE: [Patch v2 1/6] cifs: smbd: Check for iov length on sending the last
> iov
> 
> > -----Original Message-----
> > From: linux-kernel-owner@vger.kernel.org
> > <linux-kernel-owner@vger.kernel.org> On Behalf Of Long Li
> > Sent: Tuesday, April 17, 2018 12:17 PM
> > To: Steve French <sfrench@samba.org>; linux-cifs@vger.kernel.org;
> > samba- technical@lists.samba.org; linux-kernel@vger.kernel.org;
> > linux-rdma@vger.kernel.org
> > Cc: Long Li <longli@microsoft.com>; stable@vger.kernel.org
> > Subject: [Patch v2 1/6] cifs: smbd: Check for iov length on sending
> > the last iov
> >
> > From: Long Li <longli@microsoft.com>
> >
> > When sending the last iov that breaks into smaller buffers to fit the
> > transfer size, it's necessary to check if this is the last iov.
> >
> > If this is the latest iov, stop and proceed to send pages.
> >
> > Signed-off-by: Long Li <longli@microsoft.com>
> > Cc: stable@vger.kernel.org
> 
> Reviewed-by: Michael Kelley <mikelley@microsoft.com>
> 
> But a question unrelated to this change arose during my review:  At the
> beginning and end of smbd_send(), the field smbd_send_pending is
> incremented and decremented, respectively.   The increment/decrement
> are not done as atomic operations.  Is this code guaranteed to be single
> threaded?  If not, the count could become corrupted, and
> smbd_destroy_rdma_work(), which waits for the count to become zero,
> could hang.  A similar question applies to smbd_recv_pending in smbd_recv().

It is safe. The transport sending path is locked by TCP_Server_Info->srv_mutex.

The transport receiving path is single threaded.

> 
> > ---
> >  fs/cifs/smbdirect.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/fs/cifs/smbdirect.c b/fs/cifs/smbdirect.c index
> > 90e673c..b5c6c0d 100644
> > --- a/fs/cifs/smbdirect.c
> > +++ b/fs/cifs/smbdirect.c
> > @@ -2197,6 +2197,8 @@ int smbd_send(struct smbd_connection *info,
> struct smb_rqst *rqst)
> >  						goto done;
> >  				}
> >  				i++;
> > +				if (i == rqst->rq_nvec)
> > +					break;
> >  			}
> >  			start = i;
> >  			buflen = 0;
> > --
> > 2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/fs/cifs/smbdirect.c b/fs/cifs/smbdirect.c
index 90e673c..b5c6c0d 100644
--- a/fs/cifs/smbdirect.c
+++ b/fs/cifs/smbdirect.c
@@ -2197,6 +2197,8 @@  int smbd_send(struct smbd_connection *info, struct smb_rqst *rqst)
 						goto done;
 				}
 				i++;
+				if (i == rqst->rq_nvec)
+					break;
 			}
 			start = i;
 			buflen = 0;