Another report: http://samba.2283325.n4.nabble.com/andoid-access-samba4-share-td4644855.html

Please upload a debug level 10 log of smbd for both cases

Created attachment 8627 [details]
Level 10 logs from smbd, nmbd and jcifs

This looks very unusual. The Read&X calls get INVALID_PARAMETER, but there is nothing in advance to that. Need to take a closer look.

Try disabling "large readwrite" in your smb.conf to see if this makes the problem go away.

Or disable "unix extensions" in the config to see if this makes it go away.

The problem is the client is sending the upper_size of the read as 0xFFFF.

  smb_vwv[ 5]= 8192 (0x2000)
  smb_vwv[ 6]= 8192 (0x2000)
  smb_vwv[ 7]=65535 (0xFFFF)
  smb_vwv[ 8]=65535 (0xFFFF)

It almost certainly shouldn't do this and it only wants an 8k read. We decide if we should pay attention to the upper_size (smb_vwv[ 7]) using the following code.

        /* Samba client ? No problem. */
        if (get_remote_arch() == RA_SAMBA) {
                return true;
        }
        /* Need UNIX extensions. */
        if (!lp_unix_extensions()) {
                return false;
        }
        return true;

so Christian is correct that setting "unix extensions = false" should fix this.

Jeremy.

However, the client isn't setting CAP_LARGE_READX in the sessionsetup. A MacOSX client does do this.

So I think the check for supporting large reads should actually look something like this to cope with all the supported clients.

        /* Samba client ? No problem. */
        if (get_remote_arch() == RA_SAMBA) {
                return true;
        }
        /* Need UNIX extensions. */
        if (!lp_unix_extensions()) {
                return false;
        }
        /* Need CAP_LARGE_READXCAP_LARGE_READX */
        return (global_client_caps & CAP_LARGE_READX);

Also, here is a really interesting tech note from MS-SMB.pdf

in the SMB_READ_AND_X section:

MaxCountHigh (2 bytes): This field is a 16-bit integer. If the read being requested is larger than 0xFFFF bytes in size, then the client MUST use the MaxCountHigh field to hold the two most significant bytes of the requested size, which allows for 32-bit read lengths to be requested when combined with MaxCountOfBytesToReturn. If the read is not larger than 0xFFFF bytes, then the client MUST set the MaxCountHigh to zero.<26>

<26> Section 2.2.4.2.1: Windows-based servers support MaxCountHigh, but ignore it if set to 0xFFFF.

So the follow-up code should explicitly ignore MaxCountHigh = 0xFFFF to match Windows.

Patch to follow.

Jeremy.

Created attachment 8631 [details]
git-am fix for master and 4.0.next

Created attachment 8632 [details]
Better git-am fix for master and 4.0.x

I think this one is more correct in detecting and allowing large reads.

Jeremy.

Not talking to Android is a major issue :-). Re-classifying as such.
Jeremy.

Thinking about it the only thing I can think of that might make the patch cleaner is removal of the check of lp_unix_extensions(), and always just look at the upper bytes if the client sent us CAP_LARGE_READX.

This is the only thing we looked at in 3.6.x and we never had these problems with that release :-).

Other Comments/reviews very welcome.

Jeremy.

(Whilst still leaving in place the check to ignore the upper value if it's 0xffff of course).

Jeremy.

Thinking about it, not working with Android clients is a blocker for 4.0.next.

Jeremy.

Created attachment 8637 [details]
git-am fix for 4.0.x and master

Ok, here's the patchset I think is correct.

It includes a new test suite, LARGE_READX which tests around the boundaries of large readX calls with read requests of:

0xFFFF0001 (which should return 1 byte).
0x10000 (which should always return 0x10000)
0x1FFFF (which returns 0x1FFFF on Samba, and 0x10000 on Windows)
0x100000 (which returns 0x100000 on Samba, and 0x10000 on Windows).

Small fix for 3.6.x to follow.

Jeremy.

Created attachment 8638 [details]
git-am fix for 3.6.next.

Minimal version for 3.6.next.

Comment on attachment 8638 [details]
git-am fix for 3.6.next.

This is a more complex problem than we should fix for 3.6.next (IMHO). Leave sleeping dogs alone as it works in 3.6.x and fix correctly in 4.0.next.

Created attachment 8651 [details]
git-am fix for 4.0.x

Fix for master fix posted to mailing list. This is the 4.0.x version.

Implements the short read restrictions correctly I think.

Please review.

Jeremy.

Comment on attachment 8651 [details]
git-am fix for 4.0.x

I'll comment on the mailing list, why I think we need more work on this one.

I'm all ears... But I'm getting a bit tired of being pushed back on this. If you're going to make objections I'd like to see a trace where the current patch fails. Apologies if you have this.

Jeremy.

Actually if you're going to NAK the patch you need to comment here, so we don't lose the history on this.

So please add any comments to the bug about the patch review *before* doing a NAK next time please.

Jeremy.

Keeping the discussion within the bug so we can track it.

> From a490a85376da04f73f50e036a6d1d3a969ac0367 Mon Sep 17 00:00:00 2001
> From: Jeremy Allison <jra@samba.org>
> Date: Wed, 13 Mar 2013 14:56:06 -0700
> Subject: [PATCH 01/10] Ensure we send CAP_LARGE_READX/CAP_LARGE_WRITEX to the
>  server on SMB1 sessionsetupX.
>
> A Windows client doesn't do this, but that's because they never do large
> reads/writes over SMB1. It's in the spec that this should occur, and
> Samba 3.6.x uses it. Also, MacOSX, CIFSFS and Android (jCIFS) honour
> this setting and send it. We need to as well in order to give servers
> a better idea of our capabilities.

All tests have shown that it should make no difference if the client
send CAP_LARGE_READX or CAP_LARGE_WRITEX. Even if the client doesn't
send it, it's possible to do large reads and write.

If the client doesn't support large reads or write it would just doesn't
trigger them, there's really nothing more a client need to tell the server.

That's why we should should just skip this patch.

> From e40984781f28ff25a7f7aec0d00fbf6fdb7e5cbb Mon Sep 17 00:00:00 2001
> From: Jeremy Allison <jra@samba.org>
> Date: Wed, 13 Mar 2013 15:23:52 -0700
> Subject: [PATCH 02/10] smb1cli_inbuf_parse_chain() and
> smb1cli_conn_dispatch_incoming() should use smb_len_tcp.

> They have to cope with large READX call replies that have
> a length greater than smb_len_nbt() can handle.

This one is fine.

> From 8796fa108100942089b5d29a3a856e69e2993248 Mon Sep 17 00:00:00 2001
> From: Jeremy Allison <jra@samba.org>
> Date: Fri, 15 Mar 2013 11:53:04 -0700
> Subject: [PATCH 03/10] Remove server_will_accept_large_read() and erroneous
> comment.
>
> We're going to replace this with a function that calculates
> the max PDU to return on a read and supports short reads.

This also looks good.


> Subject: [PATCH 07/10] Add set_XX() functions for fields we need to modify for
>  testing large READX.

Instead of fixing the connection values, we should just remove the silly
restriction
in cli_read_andx_create() that check for cli_read_max_bufsize(),
the server should just return a short read if wanted.

> From 094b76bc108da4cd001022d16cf2558fab208ea8 Mon Sep 17 00:00:00 2001
> From: Jeremy Allison <jra@samba.org>
> Date: Fri, 15 Mar 2013 11:57:48 -0700
> Subject: [PATCH 04/10] Add functions calc_max_read_pdu()/calc_read_size() to
>  work out the length we should return.
>
> LARGE_READX test shows it's always safe to return a short read.
> Windows does so. Do the calculations to return what will fit
> in a read depending on what the client negotiated.
>
> Conservative, in that if no flags are negotiated then the
> max read is set to default Win2K12 max_pdu size of 0x10000 + headers.
>
> Signed-off-by: Jeremy Allison <jra@samba.org>

(smb_size -4 + 12*2) seems to be wrong, which my modified version
I trigger the 0xffffff smb_panic in create_outbuf().

calc_max_read_pdu() should only calculate the max_pdu size and not the
0x10000 windows limitation, and it should not depend on
smb1.unix_info.client_cap_low nor on global_client_caps.

For the encrypted case we should use the max_send size from the client
instead.
The same applies to chained requests, which means we should not return a
short read
instead of an error for chained requests.

> From 69c288d13e76c7196c8c58dd375b2e1d6345f5fc Mon Sep 17 00:00:00 2001
> From: Jeremy Allison <jra@samba.org>
> Date: Wed, 13 Mar 2013 15:43:21 -0700
> Subject: [PATCH 08/10] Add new LARGE_READX test to investigate large SMBreadX
> behavior.

calc_expected_return() should check for CIFS_UNIX_LARGE_READ_CAP instead of
any value in cli->server_posix_capabilities, we also need to
fill call cli_unix_extensions_version() if the server returns CAP_UNIX.

I've attached my current patches, but they're not complete
and fail make test. I'll try to fix this tomorrow.

Thanks for your patience!

metze

On Mon, Mar 18, 2013 at 08:38:01PM +0100, Stefan (metze) Metzmacher wrote:
>
> All tests have shown that it should make no difference if the client
> send CAP_LARGE_READX or CAP_LARGE_WRITEX. Even if the client doesn't
> send it, it's possible to do large reads and write.
>
> If the client doesn't support large reads or write it would just doesn't
> trigger them, there's really nothing more a client need to tell the server.
>
> That's why we should should just skip this patch.

That's true for Samba clients to Windows servers. It's not
true for Samba or MacOS clients to Samba servers 3.6.x or
lower. In the v3-6-test code reply_read_and_X() we have:

       if (global_client_caps & CAP_LARGE_READX) {
                size_t upper_size = SVAL(req->vwv+7, 0);

- that's the only place we look at the upper bytes in
a SMBReadX call. So yes, it does matter.

Both older Samba client libraries, MacOSX client, CIFSFS
clients and jCIFS clients all correctly send (or not) the
CAP_LARGE_READX bit on the sessionsetupX request from
the client.

You changed that in the initial 4.0.x client code release.
I think that was a mistake, and we need to put it back. It
doesn't affect how Windows servers behave (or after we fix
this code how Samba servers will behave), but it's the correct
thing to do to inform servers (what they do with that knowledge
is up to them).

I'll keep one email reply per issue, so it's easier
not to have to wade through enourmous email threads :-).

Jeremy.

On Mon, Mar 18, 2013 at 08:38:01PM +0100, Stefan (metze) Metzmacher wrote:
> 
> (smb_size -4 + 12*2) seems to be wrong, which my modified version
> I trigger the 0xffffff smb_panic in create_outbuf().

Hmmm. Is that smb_panic check in create_outbuf() correct ?

The 0xFFFFFF restriction is on the bytes following the
4 byte 'type+length' header. We have type = 0, 3 bytes
of big endian length of packet following. So the max
length of the following packet, NOT INCLUDING THE 4
BYTE type+length field is 0xFFFFFF.

smb_size is the 35 byte SMB headers field + the 4 byte 'type+length' header == 39,
so when calculating max read length I think it really should be
max_read_length = 0xFFFFFF - (smb_size - 4 + num_words*2),
Substituting numerical values:

0xFFFFFF - (39 - 4 + 24) which is 16777156 == 0xFFFFC4 bytes.

Looks to me like the code in create_outbuf() is off by
the 4 byte type+length field and really should be:

        /*
         * Protect against integer wrap
         */
        if ((num_bytes > 0xffffff)
            || ((num_bytes + smb_size -4 + num_words*2) > 0xffffff)) {

I'll try that with your patches and do some more tests.

> calc_max_read_pdu() should only calculate the max_pdu size and not the
> 0x10000 windows limitation, and it should not depend on
> smb1.unix_info.client_cap_low nor on global_client_caps.

Ok, that's fair enough. We don't really have to look at
CAP_LARGE_READX on the server if we're using max_pdu.

> For the encrypted case we should use the max_send size from the client
> instead.
> The same applies to chained requests, which means we should not return a
> short read
> instead of an error for chained requests.

Good point - +1 on both of these changes.

> > From 69c288d13e76c7196c8c58dd375b2e1d6345f5fc Mon Sep 17 00:00:00 2001
> > From: Jeremy Allison <jra@samba.org>
> > Date: Wed, 13 Mar 2013 15:43:21 -0700
> > Subject: [PATCH 08/10] Add new LARGE_READX test to investigate large SMBreadX
> > behavior.
> 
> calc_expected_return() should check for CIFS_UNIX_LARGE_READ_CAP instead of
> any value in cli->server_posix_capabilities, we also need to
> fill call cli_unix_extensions_version() if the server returns CAP_UNIX.

That code is only checking for 'any value in cli->server_posix_capabilities'
due to the capabilities manipulation which removes the CIFS_UNIX_LARGE_READ_CAP
in server_posix_capabilitiesI added earlier to get around the
restrictions in cli_read_max_bufsize() that are triggered by the
cli->server_posix_capabilities & CIFS_UNIX_LARGE_READ_CAP check inside
that function.

Now you're proposed removing cli_read_max_bufsize() then I can
correctly check for cli->server_posix_capabilities & CIFS_UNIX_LARGE_READ_CAP
as I originally intended :-).

> I've attached my current patches, but they're not complete
> and fail make test. I'll try to fix this tomorrow.

I'll also update your patches and re-run tests.

Jeremy.

Created attachment 8659 [details]
git-am fix for master.

Updated, based on Metze's modifications. Adding here so we don't lose intermediate steps.

Jeremy.

Created attachment 8663 [details]
git-am fix for master.

Metze's latest patchset, with appropriate 'Reviewed-by' tags added.

Passes make test for me.

Jeremy.

(In reply to comment #26)
> Created attachment 8663 [details]
> git-am fix for master.
> 
> Metze's latest patchset, with appropriate 'Reviewed-by' tags added.
> 
> Passes make test for me.

Maybe the raw.read fix was enough, I didn't had time to run a test with it...

Well I'm not getting any problem now, so I'm happy to push if you give this a + on the review.

I'll upload the same version modified for 4.0.x so we can get this into 4.0.next once you're ok with the master version.

Jeremy.

Created attachment 8664 [details]
Patches for master

This passes autobuild for me, but I'd like to propose some additional patches on top later.

Created attachment 8665 [details]
Additional patches for master (need discussion)

Comment on attachment 8664 [details]
Patches for master

Sigh. You lost the additional "Reviwed-by" tags I added which I'll have to go back and re-add before pushing. That's a bit of a waste of my time, so I wish you'd just worked on top of the last version I posted.

Secondly. without me having to go through an re-diff all the different versions, can you tell me what is the difference between this version and the last version I uploaded please ?

Jeremy.

Comment on attachment 8664 [details]
Patches for master

Never mind. I just did the diff. Essentially the only differences are the changes in the source4/torture/raw/read.c code to add the samba3/samba4 checks. All other parts of the patch are identical (except for the Reviewed-by's). I'll re-merge the source4/torture/raw/read.c changes, add the additional Reviewed-by's and push.

Jeremy.

Created attachment 8666 [details]
Pathes for mater, preserving all the reviewed-by lines.

Planning to push to autobuild once it passes make test here.
Jeremy.

Comment on attachment 8665 [details]
Additional patches for master (need discussion)

Ok, I'm not fond of these changes. I don't think we should restrict ourselves to Windows-size replies.

There are existing clients out there that use our large read ability to reduce latency on slow links. It's an advantage of our implementation I don't think we should attach to the unix extensions.

Jeremy

Created attachment 8667 [details]
git-am fix for 4.0.x.

Version of the patch I've pushed to autobuild for master, back-ported to 4.0.next (only change is samba_tevent_context_init() -> tevent_context_init) in the LARGE_READX change.

Jeremy.

Comment on attachment 8667 [details]
git-am fix for 4.0.x.

Looks good

(In reply to comment #34)
> Comment on attachment 8665 [details]
> Additional patches for master (need discussion)
> 
> Ok, I'm not fond of these changes. I don't think we should restrict ourselves
> to Windows-size replies.
> 
> There are existing clients out there that use our large read ability to reduce
> latency on slow links. It's an advantage of our implementation I don't think we
> should attach to the unix extensions.

As far as I know all this existing clients are unix clients.

I mean unix extensions are on by default and if someone wants to
disable them, we should really behave like a windows server.
That makes it much easier to compare captures, if there're problems...

metze

Karolin, please pick the fix for 4.0.x

Pushed to autobuild-v4-0-test.

Pushed to v4-0-test.
Closing out bug report.

Thanks!