[CIFS] still incorrect cifs_reconnect fix?

[CIFS] still incorrect cifs_reconnect fix?

[Petr Tesarik]

Hi,

while merging commits f01d5e14e764b14b6bf5512678523d009254b209 and
638b250766272fcaaa0f7ed2776f58f4ac701914 into SLES10, I've noticed that
there's apparently a bug. The code currently looks like this:

		pdu_length = 4; /* enough to get RFC1001 header */
incomplete_rcv:
		length =
		    kernel_recvmsg(csocket, &smb_msg,
				&iov, 1, pdu_length, 0 /* BB other flags? */);

/* ... some irrelevant code left out ... */

		} else if (length < 4) {	/* <----- HERE IS THE PROBLEM
			cFYI(1, ("less than four bytes received (%d bytes)",
			      length));
			pdu_length -= length;
			msleep(1);
			goto incomplete_rcv;
		}

I think we should be checking for length < pdu_length, not for length <
4, because if we read 2 bytes in the first run and 2 bytes in the second
un, CIFS will still treat the second run as incomplete (and possibly run
in an infinite loop). Am I missing something obvious?

Kind regards,
Petr Tesarik

Re: [linux-cifs-client] [CIFS] still incorrect cifs_reconnect fix?

[Jeff Layton]

On Mon, 19 Nov 2007 10:30:45 +0100
Petr Tesarik <ptesarik@suse.cz> wrote:

> Hi,
> 
> while merging commits f01d5e14e764b14b6bf5512678523d009254b209 and
> 638b250766272fcaaa0f7ed2776f58f4ac701914 into SLES10, I've noticed
> that there's apparently a bug. The code currently looks like this:
> 
> 		pdu_length = 4; /* enough to get RFC1001 header */
> incomplete_rcv:
> 		length =
> 		    kernel_recvmsg(csocket, &smb_msg,
> 				&iov, 1, pdu_length, 0 /* BB other
> flags? */);
> 
> /* ... some irrelevant code left out ... */
> 
> 		} else if (length < 4) {	/* <----- HERE IS THE
> PROBLEM cFYI(1, ("less than four bytes received (%d bytes)",
> 			      length));
> 			pdu_length -= length;
> 			msleep(1);
> 			goto incomplete_rcv;
> 		}
> 
> I think we should be checking for length < pdu_length, not for length
> < 4, because if we read 2 bytes in the first run and 2 bytes in the
> second un, CIFS will still treat the second run as incomplete (and
> possibly run in an infinite loop). Am I missing something obvious?
> 

I think you're right that the logic there is wrong, but I don't see an
infinite loop happening. It looks like in this situation, that the next
call to kernel_recvmsg will be called a size of 0, which should
eventually return 0. It'll then fall into the previous condition -- do
a reconnect and then go around the big loop again.

A patch to clean that up would probably be a good thing. We likely
don't need to do a reconnect here.

--
Jeff Layton <jlayton@redhat.com>

Re: [linux-cifs-client] [PATCH] get rid of spurious session reconnects

[Petr Tesarik]

On Mon, 2007-11-19 at 06:42 -0500, Jeff Layton wrote:
> On Mon, 19 Nov 2007 10:30:45 +0100
> Petr Tesarik <ptesarik@suse.cz> wrote:
> 
> > Hi,
> > 
> > while merging commits f01d5e14e764b14b6bf5512678523d009254b209 and
> > 638b250766272fcaaa0f7ed2776f58f4ac701914 into SLES10, I've noticed
> > that there's apparently a bug. The code currently looks like this:
> > 
> > 		pdu_length = 4; /* enough to get RFC1001 header */
> > incomplete_rcv:
> > 		length =
> > 		    kernel_recvmsg(csocket, &smb_msg,
> > 				&iov, 1, pdu_length, 0 /* BB other
> > flags? */);
> > 
> > /* ... some irrelevant code left out ... */
> > 
> > 		} else if (length < 4) {	/* <----- HERE IS THE
> > PROBLEM cFYI(1, ("less than four bytes received (%d bytes)",
> > 			      length));
> > 			pdu_length -= length;
> > 			msleep(1);
> > 			goto incomplete_rcv;
> > 		}
> > 
> > I think we should be checking for length < pdu_length, not for length
> > < 4, because if we read 2 bytes in the first run and 2 bytes in the
> > second un, CIFS will still treat the second run as incomplete (and
> > possibly run in an infinite loop). Am I missing something obvious?
> > 
> 
> I think you're right that the logic there is wrong, but I don't see an
> infinite loop happening. It looks like in this situation, that the next
> call to kernel_recvmsg will be called a size of 0, which should
> eventually return 0. It'll then fall into the previous condition -- do
> a reconnect and then go around the big loop again.
> 
> A patch to clean that up would probably be a good thing. We likely
> don't need to do a reconnect here.

OK, here we go:

When retrying kernel_recvmsg() because of a short read, check returned
length against the remaining length, not against total length. This
avoids unneeded session reconnects which would otherwise occur when
kernel_recvmsg() finally returns zero when asked to read zero bytes.

Signed-off-by: Petr Tesarik <ptesarik@suse.cz>

 connect.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index c52a76f..f972ec9 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -438,7 +438,7 @@ incomplete_rcv:
 			csocket = server->ssocket;
 			wake_up(&server->response_q);
 			continue;
-		} else if (length < 4) {
+		} else if (length < pdu_length) {
 			cFYI(1, ("less than four bytes received (%d bytes)",
 			      length));
 			pdu_length -= length;

Re: [linux-cifs-client] [PATCH] get rid of spurious session reconnects

[Jeff Layton]

On Mon, 19 Nov 2007 14:23:13 +0100
Petr Tesarik <ptesarik@suse.cz> wrote:

> On Mon, 2007-11-19 at 06:42 -0500, Jeff Layton wrote:
> > On Mon, 19 Nov 2007 10:30:45 +0100
> > Petr Tesarik <ptesarik@suse.cz> wrote:
> > 
> > > Hi,
> > > 
> > > while merging commits f01d5e14e764b14b6bf5512678523d009254b209 and
> > > 638b250766272fcaaa0f7ed2776f58f4ac701914 into SLES10, I've noticed
> > > that there's apparently a bug. The code currently looks like this:
> > > 
> > > 		pdu_length = 4; /* enough to get RFC1001 header */
> > > incomplete_rcv:
> > > 		length =
> > > 		    kernel_recvmsg(csocket, &smb_msg,
> > > 				&iov, 1, pdu_length, 0 /* BB other
> > > flags? */);
> > > 
> > > /* ... some irrelevant code left out ... */
> > > 
> > > 		} else if (length < 4) {	/* <----- HERE IS
> > > THE PROBLEM cFYI(1, ("less than four bytes received (%d bytes)",
> > > 			      length));
> > > 			pdu_length -= length;
> > > 			msleep(1);
> > > 			goto incomplete_rcv;
> > > 		}
> > > 
> > > I think we should be checking for length < pdu_length, not for
> > > length < 4, because if we read 2 bytes in the first run and 2
> > > bytes in the second un, CIFS will still treat the second run as
> > > incomplete (and possibly run in an infinite loop). Am I missing
> > > something obvious?
> > > 
> > 
> > I think you're right that the logic there is wrong, but I don't see
> > an infinite loop happening. It looks like in this situation, that
> > the next call to kernel_recvmsg will be called a size of 0, which
> > should eventually return 0. It'll then fall into the previous
> > condition -- do a reconnect and then go around the big loop again.
> > 
> > A patch to clean that up would probably be a good thing. We likely
> > don't need to do a reconnect here.
> 
> OK, here we go:
> 
> When retrying kernel_recvmsg() because of a short read, check returned
> length against the remaining length, not against total length. This
> avoids unneeded session reconnects which would otherwise occur when
> kernel_recvmsg() finally returns zero when asked to read zero bytes.
> 
> Signed-off-by: Petr Tesarik <ptesarik@suse.cz>
> 

Looks good (nice catch on this problem, btw). How about we fix up the
cFYI at the same time? Note that this should probably be tested, even
though it seems logical.

Signed-off-by: Jeff Layton <jlayton@redhat.com>

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index c4b32b7..b6ed933 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -438,9 +438,9 @@ incomplete_rcv:
 			csocket = server->ssocket;
 			wake_up(&server->response_q);
 			continue;
-		} else if (length < 4) {
-			cFYI(1, ("less than four bytes received (%d bytes)",
-			      length));
+		} else if (length < pdu_length) {
+			cFYI(1, ("less than %d bytes received (%d bytes)",
+			      pdu_length, length));
 			pdu_length -= length;
 			msleep(1);
 			goto incomplete_rcv;

Re: [linux-cifs-client] [PATCH] get rid of spurious session reconnects

[Steve French]

Merged into cifs-2.6.git

I still want to review/test more on the one other bug fix (quota or
out of disk space, cifs sync issue which Jeff proposed a fix for
earlier) and the ASCII vs. Unicode plain-text password check bug

On Nov 19, 2007 8:47 AM, Jeff Layton <jlayton@redhat.com> wrote:
> On Mon, 19 Nov 2007 14:23:13 +0100
> Petr Tesarik <ptesarik@suse.cz> wrote:
>
> > On Mon, 2007-11-19 at 06:42 -0500, Jeff Layton wrote:
> > > On Mon, 19 Nov 2007 10:30:45 +0100
> > > Petr Tesarik <ptesarik@suse.cz> wrote:
> > >
> > > > Hi,
> > > >
> > > > while merging commits f01d5e14e764b14b6bf5512678523d009254b209 and
> > > > 638b250766272fcaaa0f7ed2776f58f4ac701914 into SLES10, I've noticed
> > > > that there's apparently a bug. The code currently looks like this:
> > > >
> > > >           pdu_length = 4; /* enough to get RFC1001 header */
> > > > incomplete_rcv:
> > > >           length =
> > > >               kernel_recvmsg(csocket, &smb_msg,
> > > >                           &iov, 1, pdu_length, 0 /* BB other
> > > > flags? */);
> > > >
> > > > /* ... some irrelevant code left out ... */
> > > >
> > > >           } else if (length < 4) {        /* <----- HERE IS
> > > > THE PROBLEM cFYI(1, ("less than four bytes received (%d bytes)",
> > > >                         length));
> > > >                   pdu_length -= length;
> > > >                   msleep(1);
> > > >                   goto incomplete_rcv;
> > > >           }
> > > >
> > > > I think we should be checking for length < pdu_length, not for
> > > > length < 4, because if we read 2 bytes in the first run and 2
> > > > bytes in the second un, CIFS will still treat the second run as
> > > > incomplete (and possibly run in an infinite loop). Am I missing
> > > > something obvious?
> > > >
> > >
> > > I think you're right that the logic there is wrong, but I don't see
> > > an infinite loop happening. It looks like in this situation, that
> > > the next call to kernel_recvmsg will be called a size of 0, which
> > > should eventually return 0. It'll then fall into the previous
> > > condition -- do a reconnect and then go around the big loop again.
> > >
> > > A patch to clean that up would probably be a good thing. We likely
> > > don't need to do a reconnect here.
> >
> > OK, here we go:
> >
> > When retrying kernel_recvmsg() because of a short read, check returned
> > length against the remaining length, not against total length. This
> > avoids unneeded session reconnects which would otherwise occur when
> > kernel_recvmsg() finally returns zero when asked to read zero bytes.
> >
> > Signed-off-by: Petr Tesarik <ptesarik@suse.cz>
> >
>
> Looks good (nice catch on this problem, btw). How about we fix up the
> cFYI at the same time? Note that this should probably be tested, even
> though it seems logical.
>
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
>
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index c4b32b7..b6ed933 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -438,9 +438,9 @@ incomplete_rcv:
>                         csocket = server->ssocket;
>                         wake_up(&server->response_q);
>                         continue;
> -               } else if (length < 4) {
> -                       cFYI(1, ("less than four bytes received (%d bytes)",
> -                             length));
> +               } else if (length < pdu_length) {
> +                       cFYI(1, ("less than %d bytes received (%d bytes)",
> +                             pdu_length, length));
>                         pdu_length -= length;
>                         msleep(1);
>                         goto incomplete_rcv;
>



-- 
Thanks,

Steve