Created attachment 7524 [details]
Network capture, OK

Created attachment 7525 [details]
Syslog, failure

Created attachment 7526 [details]
Syslog, OK

Attached files:

Packet captures and syslogs of two cases:

1. NetApp server with problems
2. NetApp server working fine


Packet captures are created withou session setup/closing, "filter tcp.port == 445 and (smb or smb2)"

Syslogs are created with "echo 7 > /proc/fs/cifs/cifsFYI"

This is unfortunately, not the first report of these sorts of problems from Netapp filers, and the story is almost always the same -- the filer is sending malformed responses to requests.

One of the first things that cifs.ko does when it gets a response is to check its validity by ensuring that the word and byte count fields inside the response are consistent with the length of the entire packet. The smbCalcSize function does:

        return (sizeof(struct smb_hdr) + (2 * ptr->WordCount) +
                2 /* size of the bcc field */ + get_bcc(ptr));

Looking at your "failure" capture. In frame 22, the server sends a response to a QUERY_PATH_INFO request. That frame has a length on the entire SMB message of 156.

Now, let's total up the values inside the packet:

The SMB header is 32 bytes.
The Word count is 1 byte long.
The value of the word count is 10 -- we double that to get a length of 20 bytes.
The byte count is 2 bytes long
The value of the byte count is 105.

So:

    32 + 1 + 20 + 2 + 105 = 160

That's 4 bytes too big and the client throws out the frame. In point of fact, it looks like the byte count is where the bug is. It should be 101 instead of 105.  It also looks like the trans2 data count is also wrong inside
the frame -- 100 instead of 96, so we can't even reasonably ignore the byte count and attempt to use that instead.

If we were to trust that those lengths are correct (and some of the call response handlers will), then we'd potentially end up reading past the end of the response. Best case, that's uninitialized garbage -- worst case, it could crash the machine with a general protection fault.

Could we handle this more gracefully somehow? Perhaps, but the root cause of this issue is malformed responses from the server. If you want to open a bug with netapp and ask them to fix this it certainly would be good. I'd be happy to discuss the problem with them if it would help.

One would think that they'd want to, but to my knowledge they haven't cared much in the past about interoperating with non-windows clients. Still hope springs eternal...

I'm going to go ahead and close this with a resolution of INVALID. Please reopen it though if you wish to discuss further...

Many thanks for this explanation. :-)

But I've a few questions left... maybe more than before:

1. In packet 22 the QUERY_PATH_INFO data states to have a length of 28 - but there are only 24 bytes left. (4 byte too much)

2. As you said, the trans2 data states to contain 105 bytes, but there are only 101 bytes left (again, 4 bytes too much).

3. The whole trans2 response is 128 bytes long, adding 32 smb header bytes we have 160 (again... +4...)

4. The whole frame has a real length of 160 bytes - but the length field states 156.

So, whats' wrong?

1. The length field (156)? 4 bytes too short?
2. The File name length(28) 4 bytes too long?


We have a 156 bytes of data, but this path length field states 4 bytes too much.

Question: Are there bytes missing in path (I dont's think so because the name is complete) OR is just the length value wrong?

If(!) this length value is the root of all evil it should be possible to correct it's value.

And:
There are 2 0x00 appended to the path name. Why? Are they correct? In other successful PATH_INFO responses they are absent. 


I'm will to try out some of my ideas but I've never coded in kernel modules before.

Is there a way to try this in user space?

(In reply to comment #6)
> Many thanks for this explanation. :-)
> 
> But I've a few questions left... maybe more than before:
> 
> 1. In packet 22 the QUERY_PATH_INFO data states to have a length of 28 - but
> there are only 24 bytes left. (4 byte too much)
> 
> 2. As you said, the trans2 data states to contain 105 bytes, but there are only
> 101 bytes left (again, 4 bytes too much).
> 
> 3. The whole trans2 response is 128 bytes long, adding 32 smb header bytes we
> have 160 (again... +4...)
> 
> 4. The whole frame has a real length of 160 bytes - but the length field states
> 156.
> 

The SMB frame has a length of 156 and the NetBIOS length says 156. The NetBIOS frame has a length of 160 since it includes the 4 byte NetBIOS header + the 156 byte SMB frame. It's possible their error in calculation extends from some mixup between the two, but we can't really do much more with that than speculate.


> So, whats' wrong?
> 
> 1. The length field (156)? 4 bytes too short?

The length field is correct. There are only 156 bytes left in the frame.

> 2. The File name length(28) 4 bytes too long?
> 

Not just the "File Name Len", but many of the other embedded lengths within
the response:

The byte count (BCC)
The Trans2 Data Count
The Total Data Count
...etc...

> 
> We have a 156 bytes of data, but this path length field states 4 bytes too
> much.
> 
> Question: Are there bytes missing in path (I dont's think so because the name
> is complete) OR is just the length value wrong?
> 

I think it's just the lengths that are wrong. They have an "off by four" somewhere in their length calculations.

> If(!) this length value is the root of all evil it should be possible to
> correct it's value.
> 


The actual length of the SMB frame in the netbios header is correct. The real problem is that they calculate the other lengths that are embedded within the response incorrectly.

> And:
> There are 2 0x00 appended to the path name. Why? Are they correct? In other
> successful PATH_INFO responses they are absent. 
> 

No, I see them in almost all of the qpathinfo responses...

I'm fairly sure that's NULL termination, but I don't recall offhand whether
the spec mandates that or not. In truth, it matters little -- we don't actually use that name for anything.

> 
> I'm will to try out some of my ideas but I've never coded in kernel modules
> before.
> 
> Is there a way to try this in user space?

Possibly with libsmbclient -- I'm not sure.

There are a number of problems here...

This packet gets thrown out because the BCC is wrong. Presumably you could fix that by altering the response from the server (just overwrite it with the smaller, correct value). But then you have another problem -- the signature check on a signed connection.

If you have signing enabled, then your now-altered packet will end up causing the signature check to fail. So now, you need to check the signature on receive *before* you do this alteration. That's doable but you'll need to rearrange the code a bit and you'll also want to do that in such a way that you don't end up single threading the signature checks...

Now, you have more problems though. You've fixed the BCC, but what about the other fields in this frame (the data count, file name len, etc)? You need to either fix those as well. Or, more likely fix the callers not to trust those lengths implicitly and to instead limit them to the size of the actual frame. That's a lot of code you'll need to patch, but it's doable...

But, it's quite frankly, silly. This server is just plain broken and sending bad frames. Why should we go to such extraordinary lengths to work around that brokenness?

Still, if you want to do the legwork on it, knock yourself out!

> Not just the "File Name Len", but many of the other embedded lengths within
> the response:
> 
> The byte count (BCC)
> The Trans2 Data Count
> The Total Data Count
> ...etc...

Yes, and they all are sums of some (child) data structures - but the path length is a "real" value without any child structures.

So the algorithm I'd like to test looks like this.

1. Read the packet.
2. Test if the packet is valid.
3. If not, calculate difference k and correct the path length (and all depending length values) by k.
4. Go on...

I think this could work if (an only if) NetApp failed to calculated the path length.

Hmm, k seems to be 4 in all invalid packages...


> Still, if you want to do the legwork on it, knock yourself out!

Good point... 
Probably I'm finished with the fix when NetApp releases a bugfixed piece of software for their filers... ;-)

Created attachment 9336 [details]
patch for cifs 2.0

NetApp sends the invalid frames in response to trans2_query_path_info with info level smb_file_all_info. The patch implements a fallback to info levels smb_file_basic_info and smb_file_standard_info which yields the same information see bug #9335

(In reply to comment #9)
> Created attachment 9336 [details]
> patch for cifs 2.0
> 
> NetApp sends the invalid frames in response to trans2_query_path_info with info
> level smb_file_all_info. The patch implements a fallback to info levels
> smb_file_basic_info and smb_file_standard_info which yields the same
> information see bug #9335

ups, see bug #10230

Created attachment 9337 [details]
patch for cifs 2.0

and now the real patch

(In reply to comment #11)
> Created attachment 9337 [details]
> patch for cifs 2.0
> 
> and now the real patch

I'd like to test this, but how?
Can I apply the patch to kernel-CIFS?

(In reply to comment #12)
> (In reply to comment #11)
> > Created attachment 9337 [details] [details]
> > patch for cifs 2.0
> > 
> > and now the real patch
> 
> I'd like to test this, but how?
> Can I apply the patch to kernel-CIFS?

yes, you kan make a copy of fs/cifs from kernel 3.8, apply the patches and build it from the cifs directory with:

$ make -C /lib/modules/$(uname -r)/build M=$(pwd)

If needed I can provide patches for cifs 1.76 from the 3.2 kernel.

Hm I need kernel 3.10 of kernel 3.11 on all my machines, this is required by the installed udev version.
Patch to 3.9.9 works, but i can't boot it.

Patch to 3.11.6 partially fails:

patching file cifspdu.h
Hunk #1 succeeded at 2233 (offset 12 lines).
patching file cifssmb.c
Hunk #1 succeeded at 3958 (offset 2 lines).
Hunk #2 FAILED at 3970.
Hunk #3 succeeded at 4011 (offset 2 lines).
Hunk #4 succeeded at 4025 (offset 2 lines).
Hunk #5 succeeded at 4042 (offset 2 lines).
1 out of 5 hunks FAILED -- saving rejects to file cifssmb.c.rej
patching file cifsproto.h
Hunk #1 succeeded at 240 (offset 4 lines).
patching file cifssmb.c
Hunk #1 succeeded at 4065 (offset 2 lines).
patching file cifsproto.h
Hunk #1 succeeded at 243 (offset 4 lines).
patching file cifssmb.c
Hunk #1 succeeded at 4076 (offset 2 lines).
patching file smb1ops.c
Hunk #1 succeeded at 513 (offset 4 lines).
patching file smb1ops.c
Hunk #1 succeeded at 540 (offset 4 lines).
Hunk #2 succeeded at 564 (offset 4 lines).
patching file cifssmb.c
Hunk #1 FAILED at 3933.
Hunk #2 FAILED at 4128.
Hunk #3 FAILED at 4214.
3 out of 3 hunks FAILED -- saving rejects to file cifssmb.c.rej
patching file smb1ops.c
Hunk #1 succeeded at 522 (offset 4lines). Hunk #2 succeeded at 548 (offset 4 lines).

I tested the patched 3.9.9 cifs.ko with 3.10.12 - same problem, same error messages:

CIFS VFS: RFC1001 size 156 smaller than SMB for mid=84
Bad SMB: : dump of 48 bytes of data at 0xffff8801bf518e00
CIFS VFS: RFC1001 size 154 smaller than SMB for mid=87
Bad SMB: : dump of 48 bytes of data at 0xffff8801bf518c40
.
.
.

(In reply to comment #15)
> I tested the patched 3.9.9 cifs.ko with 3.10.12 - same problem, same error
> messages:
> 
> CIFS VFS: RFC1001 size 156 smaller than SMB for mid=84
> Bad SMB: : dump of 48 bytes of data at 0xffff8801bf518e00
> CIFS VFS: RFC1001 size 154 smaller than SMB for mid=87
> Bad SMB: : dump of 48 bytes of data at 0xffff8801bf518c40
> .
> .
> .

yes, netapp is still sending bad packets, but in that case we retry the operation with a different info level. grep for "FALLBACK" in the cFYI log

Ui, nice, at a first glance this seems to work :-)

Still the same error messages (see err.log) but directory browsing seems OK.

Created attachment 9339 [details]
cifsFYI with patch

Still very nice after some directory browsing and file searching.

But how to prevent the error messages - or better - the fault itself?

I think the cifs client should keep an internal state like "I'm connected to a buggy NetApp box" an never try the same info level again.

Created attachment 9345 [details]
patch for cifs 2.01 from kernel 3.11

* ported changes to kernel 3.11
* cache we have a brocken qpathinfo per tcon

(In reply to comment #20)
> * ported changes to kernel 3.11
> * cache we have a brocken qpathinfo per tcon

Works fine, only one error message per connection:
CIFS VFS: RFC1001 size 150 smaller than SMB for mid=23
Bad SMB: : dump of 48 bytes of data at 0xffff8800876bb140
[hex dump]


Maybe this message should be clarified (Broken NetApp server, pathinfo problem, non-RFC, falling back to other path info...)

If this is the final solution to this problem the hexdump should not appear in normal log files, hexdumps in syslogs always cause increased blood pressure. ;-)

I'd probably suggest not doing any sort of automatic fallback, but rather add in some sort of switch to make the kernel use these calls in preference to the normal ones. The cifs code is already a mess of fallbacks and special cases, and adding more is going to make things worse...

With the current code in mainline, it might be reasonable to add a new smb_version for this and simply overload the appropriate operations with the new ones. Then you could mount with your vers=<foo> option and get the behavior that will work to some degree with this broken server.

(In reply to comment #22)
> With the current code in mainline, it might be reasonable to add a new
> smb_version for this and simply overload the appropriate operations with the
> new ones. Then you could mount with your vers=<foo> option and get the behavior
> that will work to some degree with this broken server.

I guess the big majority of Linux users will not find out about that trick. They will just see that it is broken and bite into the table.

From a programmers point of view I fully agree to Jeffs comment, a lot of exceptions, fallbacks and special cases are a pain in the ass.

From a pure users point of view every error messages and every incomprehensible failure are even worse...

The patch works still fine, currently with kernel. This really makes kernel CIFS usable without thinking about the DFS structure and asking myself "on which machine is this located". Really nice! :-)

Michael, don't close bugs, please. There is a patch here in bugzilla which works for you but the discussion how to fix it upstream finaly going on. The developers will close the bug when it is fixed upstream.

Oups, sorry, reopening is better of course...

Gregor, could you please port your patch to kernel 3.12.3?
With 3.12.2 it was fine, but in 3.12.3 the "symbolic links fix" in smb1ops.c cause some trouble...

Created attachment 9546 [details]
patch for NetApp-CIFS problem

Modified Gregors patch to work with kernel 3.12.6

So someone sent me a capture between a netapp filer and cifs.ko. One thing I notice in there is this in the NEGOTIATE response:

.... .... .... .... ..0. .... .... .... = Infolevel Passthru: NT information level request passthrough is not supported

...given that that isn't set, cifs.ko ought not be sending passthru infolevel requests to the server. That may be the best mechanism to key off your new code. If that bit isn't set, you could force the client to use the new codepaths you're adding.

I'd still prefer to see that done as a new set of operations vectors and have a new vers= value that forces that behavior though.

Oops, I stand corrected...

The client isn't using passthru infolevels in most of these cases, so trying to key this off of something like flag that wouldn't be appropriate. I think you're stuck with some sort of mount-time switch if you want to get this merged.

(In reply to comment #31)
> Oops, I stand corrected...
> 
> The client isn't using passthru infolevels in most of these cases, so trying to
> key this off of something like flag that wouldn't be appropriate. I think
> you're stuck with some sort of mount-time switch if you want to get this
> merged.

Would a mount-time flag work in a DFS environment?

(In reply to comment #32)
> (In reply to comment #31)
> > Oops, I stand corrected...
> > 
> > The client isn't using passthru infolevels in most of these cases, so trying to
> > key this off of something like flag that wouldn't be appropriate. I think
> > you're stuck with some sort of mount-time switch if you want to get this
> > merged.
> 
> Would a mount-time flag work in a DFS environment?

...I don't think so, you have to add the mount-time option to the DFS mount...

Created attachment 9898 [details]
patch for NetApp-CIFS problem, Kernel 3.14.2

Comment on attachment 9898 [details]
patch for NetApp-CIFS problem, Kernel 3.14.2

Modified Gregors patch to work with kernel 3.14.2

The patch fixed all my problems. Is there any chance that it will be merged into mainline? Thanks

Created attachment 10036 [details]
SMB2_FILE_ALL_INFO tarce

We setup a new NetApp filer (FAS2240 with Data Ontap 8.1.2P4) and I can't access it from my Fedora 20 or RHEL 7 RC when forcing SMB2. SMB1 is OK.
I'm wondering if it's the same problem or not. I attached a wireshark trace.

Could you tell me if this is the same problem than this bug ? If no, is that a NetApp bug that I can report to them ?

Level 12 Query Info ("File All Info") not supported by NetApp over SMB2.  Surprising, since this should be common.  Maybe it is possible to fall back to a more primitive level which they do support?

For completeness, the result of my mount.cifs command is :
mount error(95): Operation not supported
Refer to the mount.cifs(8) manual page (e.g. man mount.cifs)

(In reply to comment #39)
> Maybe it is possible to fall back to a more primitive level which they do support?

Like SMB1, you mean ?
Yes, SMB1 works.

(In reply to comment #40)
> For completeness, the result of my mount.cifs command is :
> mount error(95): Operation not supported
> Refer to the mount.cifs(8) manual page (e.g. man mount.cifs)
> 
> (In reply to comment #39)
> > Maybe it is possible to fall back to a more primitive level which they do support?
> 
> Like SMB1, you mean ?
> Yes, SMB1 works.

No, even with SMB3 we could work around this server problem - I mean it might be practical (albeit a bit slower) to (have the Linux client) 'fall back' to other information levels, other than the common FILE_ALL_INFO - but requesting the information piece by piece (multiple calls with different info levels to do the equivalent).

This is a NetApp server problem though - so would be good to have them fix it.   I (and others that I have talked with) think supporting this is an SMB2/SMB3 server requirement.  See bottom of page 120 (or search for FileAllInformation) of MS-SMB2.pdf, the official smb2/smb3 protocol specification, http://msdn.microsoft.com/en-us/library/cc246482.aspx

(In reply to comment #41)
> No, even with SMB3 we could work around this server problem - I mean it might
> be practical (albeit a bit slower) to (have the Linux client) 'fall back' to
> other information levels, other than the common FILE_ALL_INFO - but requesting
> the information piece by piece (multiple calls with different info levels to do
> the equivalent).
> 
> This is a NetApp server problem though - so would be good to have them fix it. 
>  I (and others that I have talked with) think supporting this is an SMB2/SMB3
> server requirement.  See bottom of page 120 (or search for FileAllInformation)
> of MS-SMB2.pdf, the official smb2/smb3 protocol specification,
> http://msdn.microsoft.com/en-us/library/cc246482.aspx

Thanks.
I opened a case on our NetApp support, but no response yet.

Do you want I open a new bug for that, if you plan to workaround this NetApp problem anyway ?

(In reply to comment #42)
> I opened a case on our NetApp support, but no response yet.

They replied me that my configuration is not listed in their interoperability matrix. They only support RHEL up to 6.5.

Patch also works with kernel  3.15.6, the line offset warnings can be ignored.

What is the current status of this with NetApp and SMB3 mounts (vers=3.0)? We really don't want to be using CIFS to NetApp given all of the server bugs we have found and their previous lack of interest in fixing bugs from cifs.ko in cifs protocol.

Does this work to NetApp with vers=3.0?

I would not expect any problems to NetApp mounting from cifs.ko with vers=3.0, especially from 3.17 kernel and on in which many minor SMB3 cleanup fixes were done (other than we don't support cifsacl yet to get permission bits).

I can't test vers=3.0 against the defective filer, its OnTap 7.3.4 which only speaks 1.0.

Created attachment 10556 [details]
NatApp patch fpr kernel 3.18.1

I still maintain that this is a server bug and that netapp should fix it. Michael if you're still interested in fixing this, then you should probably send an updated patch to linux-cifs@vger.kernel.org.

I think this should be closed.
The server bug was "resolved" by changing from NetApp to Hitachi - so I can't test this anymore.

*** Bug 9032 has been marked as a duplicate of this bug. ***