Bug 11096 - SMB2 leases logic bug.
SMB2 leases logic bug.
Status: NEW
Product: Samba 4.1 and newer
Classification: Unclassified
Component: File services
4.2.0rc4
All All
: P5 normal
: 4.3
Assigned To: Jeremy Allison
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2015-02-13 00:04 UTC by Jeremy Allison
Modified: 2015-03-13 09:37 UTC (History)
2 users (show)

See Also:


Attachments
Steve's wireshark trace showing the problem using Microsoft tests. (60.10 KB, application/x-pcapng)
2015-02-13 00:05 UTC, Jeremy Allison
no flags Details
Raw patch of torture test showing the problem. (4.04 KB, patch)
2015-02-13 00:06 UTC, Jeremy Allison
no flags Details
Raw patch for master. (578 bytes, patch)
2015-02-13 00:07 UTC, Jeremy Allison
no flags Details
Raw patch - Extended torture test smb2.lease.breaking7 (6.61 KB, patch)
2015-02-13 19:46 UTC, Jeremy Allison
no flags Details
wireshark trace of breaking7 against Win2k8r2 (20.54 KB, application/x-pcapng)
2015-02-13 19:47 UTC, Jeremy Allison
no flags Details
wireshark trace of breaking7 against Win2k12r2 (19.55 KB, application/x-pcapng)
2015-02-13 19:47 UTC, Jeremy Allison
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Allison 2015-02-13 00:04:25 UTC
Microsoft leases test run by Steve French and a new smbtorture leases test I wrote clearly show SMB2 servers only check the lease key when deciding to send lease break requests to a client, not both the lease-key and client guid.

More info to follow including wireshark traces, a torture test and a proposed fix for master and 4.2.0.
Comment 1 Jeremy Allison 2015-02-13 00:05:15 UTC
Created attachment 10718 [details]
Steve's wireshark trace showing the problem using Microsoft tests.
Comment 2 Jeremy Allison 2015-02-13 00:06:42 UTC
Created attachment 10719 [details]
Raw patch of torture test showing the problem.
Comment 3 Jeremy Allison 2015-02-13 00:07:14 UTC
Created attachment 10720 [details]
Raw patch for master.
Comment 4 Jeremy Allison 2015-02-13 00:08:08 UTC
Comment on attachment 10720 [details]
Raw patch for master.

With this patch we still pass all the smb2.leases tests.
Comment 5 Jeremy Allison 2015-02-13 00:11:58 UTC
Comment on attachment 10718 [details]
Steve's wireshark trace showing the problem using Microsoft tests.

This trace shows a Windows client not responding to a lease break we send in frame 150 that it doesn't expect. We wait the lease timeout and then reply with SHARING_VIOLATION in frame 270.

Against a Windows server this test immediately responds with SHARING_VIOLATION after the second open with an incompatible share mode.
Comment 6 Jeremy Allison 2015-02-13 00:14:21 UTC
Comment on attachment 10718 [details]
Steve's wireshark trace showing the problem using Microsoft tests.

Note that in both opens the same lease key is used, but they are coming from *different* client guids.

The Windows server doesn't break in this case, as can be seen by running the torture test patch in this bug against a Windows 2K12 server.
Comment 7 Jeremy Allison 2015-02-13 01:18:02 UTC
Marking as blocker.

The only extra thing I need to know is if this only occurs on SHARING_VIOLATION errors or if it's generic across opens on same lease key/different client guid that would also cause break messages.

I think I can extend test7 to investigate this, will do so tomorrow.
Comment 8 Steve French 2015-02-13 06:22:20 UTC
JRA's patch didn't fix the test case in my setup because it doesn't reach the point of comparing the lease keys (the code he changed) - because the lease being sent in from the SMB2 create code is null, because the caller is setting oplocklevel to 0.  Still debugging ....
Comment 9 Stefan Metzmacher 2015-02-13 09:02:26 UTC
Jeremy,

Windows 2008R2 seems to behave like samba.

Windows 2012[R2] doesn't break handle leases on sharing violation.
Isn't that the behavior change we already found and discussed
with dochelp?
Comment 10 Jeremy Allison 2015-02-13 16:50:53 UTC
No I don't think so. That was not breaking leases on byte range locks, this is more generic.

Also, the problem with this one is that when we do send the break the client (at least the MS-test client) isn't expecting it and times out the break request.

I'm going to flesh out test7 a little more today to understand whether we need this behavior (ignoring client guid) only on SHARING_VIOLATION errors, or more generically.

I'll be testing against W2K12.
Comment 11 Steve French 2015-02-13 17:19:57 UTC
I don't expect Windows 2012R2 to behave any differently in this test case than Windows 8.1.

The good news is that JRA's latest patch modification (followon to patch he posted last night) does get past the premature lease break (ie we shouldn't send a lease break when we are going to return a sharing violation anyway on the 2nd opener).

But it still fails (at least test 446) now later on.  Near the end of the test - Looks like the SetInfo FILE_RENAME_INFO does not trigger a lease break and the test case expects it too.
Comment 12 Jeremy Allison 2015-02-13 17:55:23 UTC
(In reply to Steve French from comment #11)

> I don't expect Windows 2012R2 to behave any differently in this test case than 
> Windows 8.1.

So I just tested Win2k8r2 with breaking7, and it *does* issue a lease break for
this case (open with same lease key, but different client guid).

So we need to know if the Win2k12 behavior is a regression. We have to ask dochelp I think.

This also means (as I'm assuming Microsoft tested Win8+ clients against Win2k8r2) that the behavior required by the Microsoft test cases is merely an identity against W2k12, and is only testing the current state of the implementation, bugs notwithstanding (as we've already discovered the Microsoft tests are sending lease requests without setting requested_oplock_level = SMB2_OPLOCK_LEVEL_LEASE, but setting it to SMB2_OPLOCK_LEVEL_NONE - so a Win2k12 server isn't checking that field).
Comment 13 Jeremy Allison 2015-02-13 19:20:26 UTC
Ok, reducing the severity of this bug, and removing it as a blocker for 4.2.0 release.

We're behaving exactly as W2K8R2 does here, in that we look at both the client guid+lease key when determining if we need to issue an oplock break on sharing violation.

W2K12R2 seems to only look at lease key, not client guid.

However, this only seems to be triggered by the Microsoft tests that Stevef is running, which also have their own problems (sending lease blobs whist not setting oplock level lease in open requests for example), so I'm inclined to think this bug is around getting us to pass the MS tests, rather than a real logic error in our lease implementation.

I'll raise this on dochelp.
Comment 14 Jeremy Allison 2015-02-13 19:46:21 UTC
Created attachment 10725 [details]
Raw patch - Extended torture test smb2.lease.breaking7
Comment 15 Jeremy Allison 2015-02-13 19:47:00 UTC
Created attachment 10726 [details]
wireshark trace of breaking7 against Win2k8r2
Comment 16 Jeremy Allison 2015-02-13 19:47:22 UTC
Created attachment 10727 [details]
wireshark trace of breaking7 against Win2k12r2
Comment 17 Jeremy Allison 2015-02-13 19:47:45 UTC
Data sent to dochelp.
Comment 18 Jeremy Allison 2015-02-13 19:47:57 UTC
Hi all,

Whist Steve French was running the Microsoft leases
tests against Samba, we discovered a change in how
Windows handles lease breaks.

In Windows 2008r2, when a client opens 2 connections
with different client guids, and then opens the same
file on each connection with the same lease key with
incompatible share modes - the server issues a lease
break to the client before returning with a sharing
violation error.

In Windows 2012r2 in the same situation, the server
*does not* issue a lease break to the client before
returning with a sharing violation error - even though
the opens are issues on different connections with
different client guids.

Both 2008r2 and 2012r2 issue lease breaks when
an open is issued with an incompatible share mode
but the lease key is different, as expected (this
is also tested in the wireshark trace).

This seems to me to be a regression in server behavior,
in that leases should be a combination of both
client guid+lease key, and the server should always
be looking at both when deciding what actions to
take on a lease.

Currently Samba behaves as Windows 2008r2
in this matter, but the Microsft SMB2 tests
seem to be expecting the Windows 2012r2 behavior,
which I think is a problem for implementors.

Attached are 2 wireshark traces showing the
problem.

As SMB traffic isn't allowed on the LAN where
I am these were taken locally between VM's
on ports 4444 and 4442, so use wireshark's
'Decode As' -> NBSS to see as SMB2 traffic.

Cheers,

Jeremy.