Discussion:
[PATCH] smbd: Fix snapshot query on shares with DFS enabled
Christof Schmitt
2016-08-12 22:59:27 UTC
Permalink
From 3a33435fc3d61605c75ed3f3b87612fbe729b553 Mon Sep 17 00:00:00 2001
From: Christof Schmitt <***@samba.org>
Date: Fri, 12 Aug 2016 14:59:07 -0700
Subject: [PATCH] smbd: Fix snapshot query on shares with DFS enabled

When DFS is enabled (host msdfs = yes and msdfs root = yes), then SMB
clients send create requests in the format \hostname\service\path.
Putting the GMT tag as first element breaks the DFS parsing and results
in OBJECT_NOT_FOUND for snapshotted files. Fix this by appending the
GMT tag to the end of the path.

Signed-off-by: Christof Schmitt <***@samba.org>
---
source3/smbd/smb2_create.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/source3/smbd/smb2_create.c b/source3/smbd/smb2_create.c
index 4c1b81d..75da8a1 100644
--- a/source3/smbd/smb2_create.c
+++ b/source3/smbd/smb2_create.c
@@ -913,14 +913,14 @@ static struct tevent_req *smbd_smb2_create_send(TALLOC_CTX *mem_ctx,

TALLOC_FREE(fname);
fname = talloc_asprintf(state,
- "@GMT-%04u.%02u.%02u-%02u.%02u.%02u\\%s",
+ "%s\\@GMT-%04u.%02u.%02u-%02u.%02u.%02u",
+ in_name,
tm->tm_year + 1900,
tm->tm_mon + 1,
tm->tm_mday,
tm->tm_hour,
tm->tm_min,
- tm->tm_sec,
- in_name);
+ tm->tm_sec);
if (tevent_req_nomem(fname, req)) {
return tevent_req_post(req, ev);
}
--
1.8.3.1
Jeremy Allison
2016-08-12 23:51:35 UTC
Permalink
Post by Christof Schmitt
From 3a33435fc3d61605c75ed3f3b87612fbe729b553 Mon Sep 17 00:00:00 2001
Date: Fri, 12 Aug 2016 14:59:07 -0700
Subject: [PATCH] smbd: Fix snapshot query on shares with DFS enabled
When DFS is enabled (host msdfs = yes and msdfs root = yes), then SMB
clients send create requests in the format \hostname\service\path.
Putting the GMT tag as first element breaks the DFS parsing and results
in OBJECT_NOT_FOUND for snapshotted files. Fix this by appending the
GMT tag to the end of the path.
Great catch ! Pushed with additional comment:

BUG: https://bugzilla.samba.org/show_bug.cgi?id=12150

as we're going to need this backported.
Post by Christof Schmitt
---
source3/smbd/smb2_create.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/source3/smbd/smb2_create.c b/source3/smbd/smb2_create.c
index 4c1b81d..75da8a1 100644
--- a/source3/smbd/smb2_create.c
+++ b/source3/smbd/smb2_create.c
@@ -913,14 +913,14 @@ static struct tevent_req *smbd_smb2_create_send(TALLOC_CTX *mem_ctx,
TALLOC_FREE(fname);
fname = talloc_asprintf(state,
+ in_name,
tm->tm_year + 1900,
tm->tm_mon + 1,
tm->tm_mday,
tm->tm_hour,
tm->tm_min,
- tm->tm_sec,
- in_name);
+ tm->tm_sec);
if (tevent_req_nomem(fname, req)) {
return tevent_req_post(req, ev);
}
--
1.8.3.1
Christof Schmitt
2016-08-15 23:25:10 UTC
Permalink
Post by Jeremy Allison
Post by Christof Schmitt
From 3a33435fc3d61605c75ed3f3b87612fbe729b553 Mon Sep 17 00:00:00 2001
Date: Fri, 12 Aug 2016 14:59:07 -0700
Subject: [PATCH] smbd: Fix snapshot query on shares with DFS enabled
When DFS is enabled (host msdfs = yes and msdfs root = yes), then SMB
clients send create requests in the format \hostname\service\path.
Putting the GMT tag as first element breaks the DFS parsing and results
in OBJECT_NOT_FOUND for snapshotted files. Fix this by appending the
GMT tag to the end of the path.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12150
as we're going to need this backported.
I just came across one more thing: The test is on a system with the
shadowcopy2 module enabled. While the patches fixes the problem for
directories, there seems to be an issue when a SMB client is explicitly
asking for older versions of a file. It looks like shadowcopy leaves a
trailing slash from the GMT tag removal:

(test is a file, not a directory)

[2016/08/15 16:12:36.178799, 10, pid=13125, effective(12000500, 12000513), real(12000500, 0)] ../source3/modules/vfs_shadow_copy2.c:245(shadow_copy2_strip_snapshot)
../source3/modules/vfs_shadow_copy2.c:245: enter path 'test/@GMT-2016.08.12-19.56.11'
[2016/08/15 16:12:36.178814, 10, pid=13125, effective(12000500, 12000513), real(12000500, 0)] ../source3/modules/vfs_shadow_copy2.c:462(shadow_copy2_do_convert)
converting 'test/'
[2016/08/15 16:12:36.178837, 10, pid=13125, effective(12000500, 12000513), real(12000500, 0)] ../source3/modules/vfs_shadow_copy2.c:604(shadow_copy2_do_convert)
Trying[snapdirseverywhere] /fs1/smbexport1/test/.snapshots/@GMT-2016.08.12-19.56.11/: -1 (Not a directory)
[2016/08/15 16:12:36.178860, 10, pid=13125, effective(12000500, 12000513), real(12000500, 0)] ../source3/modules/vfs_shadow_copy2.c:604(shadow_copy2_do_convert)
Trying[snapdirseverywhere] /fs1/smbexport1/.snapshots/@GMT-2016.08.12-19.56.11/test/: -1 (No such file or directory)
[2016/08/15 16:12:36.178886, 10, pid=13125, effective(12000500, 12000513), real(12000500, 0)] ../source3/modules/vfs_shadow_copy2.c:604(shadow_copy2_do_convert)
Trying[snapdirseverywhere] /fs1/.snapshots/@GMT-2016.08.12-19.56.11/smbexport1/test/: -1 (Not a directory)

I will try to take a closer look tomorrow, maybe that can be easily
handled in the shadowcopy2 module. The backport should probably wait
until this is resolved

Christof
Post by Jeremy Allison
Post by Christof Schmitt
---
source3/smbd/smb2_create.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/source3/smbd/smb2_create.c b/source3/smbd/smb2_create.c
index 4c1b81d..75da8a1 100644
--- a/source3/smbd/smb2_create.c
+++ b/source3/smbd/smb2_create.c
@@ -913,14 +913,14 @@ static struct tevent_req *smbd_smb2_create_send(TALLOC_CTX *mem_ctx,
TALLOC_FREE(fname);
fname = talloc_asprintf(state,
+ in_name,
tm->tm_year + 1900,
tm->tm_mon + 1,
tm->tm_mday,
tm->tm_hour,
tm->tm_min,
- tm->tm_sec,
- in_name);
+ tm->tm_sec);
if (tevent_req_nomem(fname, req)) {
return tevent_req_post(req, ev);
}
--
1.8.3.1
Jeremy Allison
2016-08-15 23:44:45 UTC
Permalink
Post by Christof Schmitt
Post by Jeremy Allison
Post by Christof Schmitt
From 3a33435fc3d61605c75ed3f3b87612fbe729b553 Mon Sep 17 00:00:00 2001
Date: Fri, 12 Aug 2016 14:59:07 -0700
Subject: [PATCH] smbd: Fix snapshot query on shares with DFS enabled
When DFS is enabled (host msdfs = yes and msdfs root = yes), then SMB
clients send create requests in the format \hostname\service\path.
Putting the GMT tag as first element breaks the DFS parsing and results
in OBJECT_NOT_FOUND for snapshotted files. Fix this by appending the
GMT tag to the end of the path.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12150
as we're going to need this backported.
I just came across one more thing: The test is on a system with the
shadowcopy2 module enabled. While the patches fixes the problem for
directories, there seems to be an issue when a SMB client is explicitly
asking for older versions of a file. It looks like shadowcopy leaves a
(test is a file, not a directory)
[2016/08/15 16:12:36.178799, 10, pid=13125, effective(12000500, 12000513), real(12000500, 0)] ../source3/modules/vfs_shadow_copy2.c:245(shadow_copy2_strip_snapshot)
[2016/08/15 16:12:36.178814, 10, pid=13125, effective(12000500, 12000513), real(12000500, 0)] ../source3/modules/vfs_shadow_copy2.c:462(shadow_copy2_do_convert)
converting 'test/'
[2016/08/15 16:12:36.178837, 10, pid=13125, effective(12000500, 12000513), real(12000500, 0)] ../source3/modules/vfs_shadow_copy2.c:604(shadow_copy2_do_convert)
[2016/08/15 16:12:36.178860, 10, pid=13125, effective(12000500, 12000513), real(12000500, 0)] ../source3/modules/vfs_shadow_copy2.c:604(shadow_copy2_do_convert)
[2016/08/15 16:12:36.178886, 10, pid=13125, effective(12000500, 12000513), real(12000500, 0)] ../source3/modules/vfs_shadow_copy2.c:604(shadow_copy2_do_convert)
I will try to take a closer look tomorrow, maybe that can be easily
handled in the shadowcopy2 module. The backport should probably wait
until this is resolved
We should probably add a smbclient-based torture test
for both file and directory inside source3/script/tests/test_shadow_copy.sh
once this gets fixed to make sure we don't regress.
Uri Simchoni
2016-08-16 05:05:55 UTC
Permalink
Post by Jeremy Allison
Post by Christof Schmitt
I will try to take a closer look tomorrow, maybe that can be easily
handled in the shadowcopy2 module. The backport should probably wait
until this is resolved
We should probably add a smbclient-based torture test
for both file and directory inside source3/script/tests/test_shadow_copy.sh
once this gets fixed to make sure we don't regress.
Looking again at the shadow_copy2 test, it's a great step forward vs no
test, but it is lacking in some areas.

The current test list previous versions of a file, which queries
snapshot file information.
a. Not sure it's complete enough - maybe successfully reading the file
will be safer
b. It does not handle directories at all
c. It only runs on SMB1, and with respect to paths, SMB2 may behave
differently. Strangely, the test seems to pass on SMB3 (adding -m SMB3),
although AFAICT smbclient uses SMB1 calls to list snapshots (so it
should have yielded 0 snapshots). I'll have to look into it.

The SMB3 test must, IMHO, access snapshot files using TWrp create
context (I don't mind doing it once I have time and it shouldn't stop
any bugfixes if it's been reasonably verified that they don't break
things, so it's a note to self).

Uri.
Jeremy Allison
2016-08-16 15:40:15 UTC
Permalink
Post by Uri Simchoni
Post by Jeremy Allison
Post by Christof Schmitt
I will try to take a closer look tomorrow, maybe that can be easily
handled in the shadowcopy2 module. The backport should probably wait
until this is resolved
We should probably add a smbclient-based torture test
for both file and directory inside source3/script/tests/test_shadow_copy.sh
once this gets fixed to make sure we don't regress.
Looking again at the shadow_copy2 test, it's a great step forward vs no
test, but it is lacking in some areas.
The current test list previous versions of a file, which queries
snapshot file information.
a. Not sure it's complete enough - maybe successfully reading the file
will be safer
b. It does not handle directories at all
c. It only runs on SMB1, and with respect to paths, SMB2 may behave
differently. Strangely, the test seems to pass on SMB3 (adding -m SMB3),
although AFAICT smbclient uses SMB1 calls to list snapshots (so it
should have yielded 0 snapshots). I'll have to look into it.
The SMB3 test must, IMHO, access snapshot files using TWrp create
context (I don't mind doing it once I have time and it shouldn't stop
any bugfixes if it's been reasonably verified that they don't break
things, so it's a note to self).
For the current change, it would be enough to test over SMB2 using
path with /@GMT-YYYY... appended to the end, as that's exactly
what the server now does.

Further tests can add access to the Twrp context, but I don't
think they're a blocker for adding tests (one for file, one
for directory) for this specific case.
Jeremy Allison
2016-08-16 22:24:48 UTC
Permalink
Post by Jeremy Allison
Post by Uri Simchoni
Post by Jeremy Allison
Post by Christof Schmitt
I will try to take a closer look tomorrow, maybe that can be easily
handled in the shadowcopy2 module. The backport should probably wait
until this is resolved
We should probably add a smbclient-based torture test
for both file and directory inside source3/script/tests/test_shadow_copy.sh
once this gets fixed to make sure we don't regress.
Looking again at the shadow_copy2 test, it's a great step forward vs no
test, but it is lacking in some areas.
The current test list previous versions of a file, which queries
snapshot file information.
a. Not sure it's complete enough - maybe successfully reading the file
will be safer
b. It does not handle directories at all
c. It only runs on SMB1, and with respect to paths, SMB2 may behave
differently. Strangely, the test seems to pass on SMB3 (adding -m SMB3),
although AFAICT smbclient uses SMB1 calls to list snapshots (so it
should have yielded 0 snapshots). I'll have to look into it.
The SMB3 test must, IMHO, access snapshot files using TWrp create
context (I don't mind doing it once I have time and it shouldn't stop
any bugfixes if it's been reasonably verified that they don't break
things, so it's a note to self).
For the current change, it would be enough to test over SMB2 using
what the server now does.
Further tests can add access to the Twrp context, but I don't
think they're a blocker for adding tests (one for file, one
for directory) for this specific case.
OK, I now have working SMB2 FSCTL_GET_SHADOW_COPY_DATA client
code, but for SMB2 it seems that Twrp contexts are required
to get at the shadow copies on Windows (I can list 'em, but
doing path/@GMT-YYYY etc. fails with OBJECT_PATH_NOT_FOUND).

Shall I add code into the SMB2 client open that converts
a /@GMT-YYYY... trailing path into a Twrp token ?

What do you think ?

Jeremy.
Jeremy Allison
2016-08-16 22:29:36 UTC
Permalink
Post by Jeremy Allison
Post by Jeremy Allison
Post by Uri Simchoni
Post by Jeremy Allison
Post by Christof Schmitt
I will try to take a closer look tomorrow, maybe that can be easily
handled in the shadowcopy2 module. The backport should probably wait
until this is resolved
We should probably add a smbclient-based torture test
for both file and directory inside source3/script/tests/test_shadow_copy.sh
once this gets fixed to make sure we don't regress.
Looking again at the shadow_copy2 test, it's a great step forward vs no
test, but it is lacking in some areas.
The current test list previous versions of a file, which queries
snapshot file information.
a. Not sure it's complete enough - maybe successfully reading the file
will be safer
b. It does not handle directories at all
c. It only runs on SMB1, and with respect to paths, SMB2 may behave
differently. Strangely, the test seems to pass on SMB3 (adding -m SMB3),
although AFAICT smbclient uses SMB1 calls to list snapshots (so it
should have yielded 0 snapshots). I'll have to look into it.
The SMB3 test must, IMHO, access snapshot files using TWrp create
context (I don't mind doing it once I have time and it shouldn't stop
any bugfixes if it's been reasonably verified that they don't break
things, so it's a note to self).
For the current change, it would be enough to test over SMB2 using
what the server now does.
Further tests can add access to the Twrp context, but I don't
think they're a blocker for adding tests (one for file, one
for directory) for this specific case.
OK, I now have working SMB2 FSCTL_GET_SHADOW_COPY_DATA client
code, but for SMB2 it seems that Twrp contexts are required
to get at the shadow copies on Windows (I can list 'em, but
Shall I add code into the SMB2 client open that converts
What do you think ?
FYI. Here is the libsmb client code that plumbs SMB2
FSCTL_GET_SHADOW_COPY_DATA into cli_shadow_copy_data()
if you want to play with it.

Jeremy.
Uri Simchoni
2016-08-17 07:12:45 UTC
Permalink
Post by Jeremy Allison
Post by Jeremy Allison
Post by Jeremy Allison
Post by Uri Simchoni
Post by Jeremy Allison
Post by Christof Schmitt
I will try to take a closer look tomorrow, maybe that can be easily
handled in the shadowcopy2 module. The backport should probably wait
until this is resolved
We should probably add a smbclient-based torture test
for both file and directory inside source3/script/tests/test_shadow_copy.sh
once this gets fixed to make sure we don't regress.
Looking again at the shadow_copy2 test, it's a great step forward vs no
test, but it is lacking in some areas.
The current test list previous versions of a file, which queries
snapshot file information.
a. Not sure it's complete enough - maybe successfully reading the file
will be safer
b. It does not handle directories at all
c. It only runs on SMB1, and with respect to paths, SMB2 may behave
differently. Strangely, the test seems to pass on SMB3 (adding -m SMB3),
although AFAICT smbclient uses SMB1 calls to list snapshots (so it
should have yielded 0 snapshots). I'll have to look into it.
The SMB3 test must, IMHO, access snapshot files using TWrp create
context (I don't mind doing it once I have time and it shouldn't stop
any bugfixes if it's been reasonably verified that they don't break
things, so it's a note to self).
For the current change, it would be enough to test over SMB2 using
what the server now does.
Further tests can add access to the Twrp context, but I don't
think they're a blocker for adding tests (one for file, one
for directory) for this specific case.
OK, I now have working SMB2 FSCTL_GET_SHADOW_COPY_DATA client
code, but for SMB2 it seems that Twrp contexts are required
to get at the shadow copies on Windows (I can list 'em, but
Shall I add code into the SMB2 client open that converts
What do you think ?
FYI. Here is the libsmb client code that plumbs SMB2
FSCTL_GET_SHADOW_COPY_DATA into cli_shadow_copy_data()
if you want to play with it.
Jeremy.
I've played with it some (haven't carefully reviewed it). Adding a patch
that runs shadow-copy2 tests using both SMB3 and NT1, and a test-patch
that puts the @GMT at the end of the path.

The SMB3 variant fails without your snapshot listing code, but passes
with it, no matter where the @GMT is, so I don't understand why you said
that path/@GMT-xxx fails (tested both path\@GMT and path/@GMT).

IMHO smbclient should place a TWrp in its allinfo query - that's what's
going to work against any SMB2 server. Perhaps we can also add smbclient
functionality to access previous versions (somewhat like cd - smbclient
keeps the dir locally and prepends it to paths. In the same way the TWrp
can be added to any open).

For the time being, I think we should apply your shadow_copy_data()
patches and not my patch that puts the @GMT at the end. That would make
"allinfo" properly list previous versions against all (reasonably
recent) versions of samba in SMB2 mode, at the expense of providing a
not-quite-realistic test for vfs_shadow_copy2.

Uri.
Jeremy Allison
2016-08-17 15:52:31 UTC
Permalink
Post by Uri Simchoni
I've played with it some (haven't carefully reviewed it). Adding a patch
that runs shadow-copy2 tests using both SMB3 and NT1, and a test-patch
The SMB3 variant fails without your snapshot listing code, but passes
I was running against Windows, that's why :-). Windows needs TWrp.
Post by Uri Simchoni
IMHO smbclient should place a TWrp in its allinfo query - that's what's
going to work against any SMB2 server. Perhaps we can also add smbclient
functionality to access previous versions (somewhat like cd - smbclient
keeps the dir locally and prepends it to paths. In the same way the TWrp
can be added to any open).
I've been thinking about that. I don't want the client code to arbitrarily
scan SMB2 opens for @GMT-YYYY... paths and convert them into TWrp tokens,
so I was thinking of a modal switch (yeah sucks but I can't think of any
other way).

Like we have cli_set_backup_intent(), I think we need cli_set_previous_version(cli, time_t t).

If that is set, the SMB2 client code then scans for and removes
any @GMT-YYYY... part of the path that contains the time_t t
and sends a TWtp token containing t.

I'm trying to keep compatibility with the SMB1 path-based selection,
as well as the SMB2 TWtp based selection, which is rather hard. The
source3/client/client.c code can set a previous version time using
cli_set_previous_version() as well as append the @GMT-YYYY.. to the
path to cover both cases.

Setting cli_set_previous_version(cli, 0) removes any special casing
in the SMB2 client SMB2create code.


Thoughts ?
Post by Uri Simchoni
For the time being, I think we should apply your shadow_copy_data()
"allinfo" properly list previous versions against all (reasonably
recent) versions of samba in SMB2 mode, at the expense of providing a
not-quite-realistic test for vfs_shadow_copy2.
I need to fix the source3/modules/vfs_snapper.c code first :-).

I'll send a patch to review soon !
Christof Schmitt
2016-08-17 17:18:21 UTC
Permalink
Post by Jeremy Allison
Post by Uri Simchoni
I've played with it some (haven't carefully reviewed it). Adding a patch
that runs shadow-copy2 tests using both SMB3 and NT1, and a test-patch
The SMB3 variant fails without your snapshot listing code, but passes
I was running against Windows, that's why :-). Windows needs TWrp.
Post by Uri Simchoni
IMHO smbclient should place a TWrp in its allinfo query - that's what's
going to work against any SMB2 server. Perhaps we can also add smbclient
functionality to access previous versions (somewhat like cd - smbclient
keeps the dir locally and prepends it to paths. In the same way the TWrp
can be added to any open).
I've been thinking about that. I don't want the client code to arbitrarily
so I was thinking of a modal switch (yeah sucks but I can't think of any
other way).
Like we have cli_set_backup_intent(), I think we need cli_set_previous_version(cli, time_t t).
If that is set, the SMB2 client code then scans for and removes
and sends a TWtp token containing t.
I'm trying to keep compatibility with the SMB1 path-based selection,
as well as the SMB2 TWtp based selection, which is rather hard. The
source3/client/client.c code can set a previous version time using
path to cover both cases.
Setting cli_set_previous_version(cli, 0) removes any special casing
in the SMB2 client SMB2create code.
Thoughts ?
If cli_set_previous_version(cli, time_t t) already registers the
timestamp that should be used for opens, then the cli code could create
the twrp context from that for SMB2, or in the SMB1 case insert the @GMT
token in the path. That way the protocol differences between SMB1 and
SMB2 would be hidden from the application using the cli.
Post by Jeremy Allison
Post by Uri Simchoni
For the time being, I think we should apply your shadow_copy_data()
"allinfo" properly list previous versions against all (reasonably
recent) versions of samba in SMB2 mode, at the expense of providing a
not-quite-realistic test for vfs_shadow_copy2.
I need to fix the source3/modules/vfs_snapper.c code first :-).
I'll send a patch to review soon !
Should we push the fixes you have for shadowcopy2, or do you want to wait
until everything is complete, including snapper and tests?

Christof
Jeremy Allison
2016-08-17 17:32:38 UTC
Permalink
Post by Christof Schmitt
If cli_set_previous_version(cli, time_t t) already registers the
timestamp that should be used for opens, then the cli code could create
token in the path. That way the protocol differences between SMB1 and
SMB2 would be hidden from the application using the cli.
It's the 'insert the @GMT token in the path' for SMB1 part that is
hard. It would touch a *LOT* of client code :-(.
Post by Christof Schmitt
Post by Jeremy Allison
Post by Uri Simchoni
For the time being, I think we should apply your shadow_copy_data()
"allinfo" properly list previous versions against all (reasonably
recent) versions of samba in SMB2 mode, at the expense of providing a
not-quite-realistic test for vfs_shadow_copy2.
I need to fix the source3/modules/vfs_snapper.c code first :-).
I'll send a patch to review soon !
Should we push the fixes you have for shadowcopy2, or do you want to wait
until everything is complete, including snapper and tests?
Already pushed the shadowcopy2 fixes with your reviewed-by.

I'm working on the snapper fix right now which I'll propose
for review today, and then the tests.
Christof Schmitt
2016-08-17 17:54:15 UTC
Permalink
Post by Jeremy Allison
Post by Christof Schmitt
If cli_set_previous_version(cli, time_t t) already registers the
timestamp that should be used for opens, then the cli code could create
token in the path. That way the protocol differences between SMB1 and
SMB2 would be hidden from the application using the cli.
hard. It would touch a *LOT* of client code :-(.
Ok, that is the issue of SMB1 doing everything based on the path. If we
want to avoid large changes, then i am not sure how we could have a
clean API.
Post by Jeremy Allison
Post by Christof Schmitt
Post by Jeremy Allison
Post by Uri Simchoni
For the time being, I think we should apply your shadow_copy_data()
"allinfo" properly list previous versions against all (reasonably
recent) versions of samba in SMB2 mode, at the expense of providing a
not-quite-realistic test for vfs_shadow_copy2.
I need to fix the source3/modules/vfs_snapper.c code first :-).
I'll send a patch to review soon !
Should we push the fixes you have for shadowcopy2, or do you want to wait
until everything is complete, including snapper and tests?
Already pushed the shadowcopy2 fixes with your reviewed-by.
Found them now. Thanks for moving those along.
Post by Jeremy Allison
I'm working on the snapper fix right now which I'll propose
for review today, and then the tests.
Christof
Jeremy Allison
2016-08-17 19:15:40 UTC
Permalink
Post by Christof Schmitt
Post by Jeremy Allison
Post by Christof Schmitt
If cli_set_previous_version(cli, time_t t) already registers the
timestamp that should be used for opens, then the cli code could create
token in the path. That way the protocol differences between SMB1 and
SMB2 would be hidden from the application using the cli.
hard. It would touch a *LOT* of client code :-(.
Ok, that is the issue of SMB1 doing everything based on the path. If we
want to avoid large changes, then i am not sure how we could have a
clean API.
So right now, the only use of the shadow_copy client code is
in smbclient where in the 'allinfo' command we get previous
versions and then try and list the meta-data on them, by
constructing an SMB1 path (prepending the @GMT-token).

I'm willing to do something "dirty" such as the dual
cli_set_previous_version(cli, time_t t)/@GMT-YYYY.. path
append inside smbclient for now - for test purposes until
we figure out what the real API should be for users such
as the GNOME VFS etc.
Post by Christof Schmitt
Post by Jeremy Allison
I'm working on the snapper fix right now which I'll propose
for review today, and then the tests.
Christof
Here is the fix for the vfs_snapper module.

I think this is the last VFS module fix to all the /@GMT-YYYY-token
append inside SMB2 to work against all supported modules.

I'll work on the test case code next.

Please review !

Jeremy.
Christof Schmitt
2016-08-17 20:48:38 UTC
Permalink
Post by Jeremy Allison
Post by Christof Schmitt
Post by Jeremy Allison
Post by Christof Schmitt
If cli_set_previous_version(cli, time_t t) already registers the
timestamp that should be used for opens, then the cli code could create
token in the path. That way the protocol differences between SMB1 and
SMB2 would be hidden from the application using the cli.
hard. It would touch a *LOT* of client code :-(.
Ok, that is the issue of SMB1 doing everything based on the path. If we
want to avoid large changes, then i am not sure how we could have a
clean API.
So right now, the only use of the shadow_copy client code is
in smbclient where in the 'allinfo' command we get previous
versions and then try and list the meta-data on them, by
I'm willing to do something "dirty" such as the dual
append inside smbclient for now - for test purposes until
we figure out what the real API should be for users such
as the GNOME VFS etc.
Post by Christof Schmitt
Post by Jeremy Allison
I'm working on the snapper fix right now which I'll propose
for review today, and then the tests.
Christof
Here is the fix for the vfs_snapper module.
append inside SMB2 to work against all supported modules.
I'll work on the test case code next.
Please review !
Jeremy.
From 5982e72013a1a2e29a99f3d45c8a5ab04c011259 Mon Sep 17 00:00:00 2001
Date: Wed, 17 Aug 2016 10:49:50 -0700
Subject: [PATCH 1/3] s3: vfs: shadow_copy2: Replace all uses of (p-name) with
len_before_gmt.
p and name don't change, and we've already calculated this length.
Part of the effort to make the code inside vfs_snapper.c that does
the same thing more similar (we can't make these functions identical
due to the 'snapdir' use case).
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12150
---
source3/modules/vfs_shadow_copy2.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/source3/modules/vfs_shadow_copy2.c b/source3/modules/vfs_shadow_copy2.c
index 4ac16d3..2a72740 100644
--- a/source3/modules/vfs_shadow_copy2.c
+++ b/source3/modules/vfs_shadow_copy2.c
@@ -514,7 +514,7 @@ static bool shadow_copy2_strip_snapshot(TALLOC_CTX *mem_ctx,
q += 1;
rest_len = strlen(q);
- dst_len = (p-name) + rest_len;
+ dst_len = len_before_gmt + rest_len;
if (priv->config->snapdirseverywhere) {
char *insert;
@@ -580,10 +580,10 @@ static bool shadow_copy2_strip_snapshot(TALLOC_CTX *mem_ctx,
return false;
}
if (p > name) {
- memcpy(stripped, name, p-name);
+ memcpy(stripped, name, len_before_gmt);
}
if (rest_len > 0) {
- memcpy(stripped + (p-name), q, rest_len);
+ memcpy(stripped + len_before_gmt, q, rest_len);
}
stripped[dst_len] = '\0';
*pstripped = stripped;
--
2.8.0.rc3.226.g39d4020
From 4a20434b0eb591839ea944464ee71d93dbb90507 Mon Sep 17 00:00:00 2001
Date: Wed, 17 Aug 2016 10:53:08 -0700
Subject: [PATCH 2/3] s3: vfs: snapper: Add and use len_before_gmt, calculated
as (p-name).
Make the code closer to the same functionality in shadow_copy2.c:shadow_copy2_strip_snapshot().
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12150
---
source3/modules/vfs_snapper.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/source3/modules/vfs_snapper.c b/source3/modules/vfs_snapper.c
index 8f3c647..f5ccc15 100644
--- a/source3/modules/vfs_snapper.c
+++ b/source3/modules/vfs_snapper.c
@@ -1729,6 +1729,7 @@ static bool snapper_gmt_strip_snapshot(TALLOC_CTX *mem_ctx,
char *q;
char *stripped;
size_t rest_len, dst_len;
+ ptrdiff_t len_before_gmt;
if (p == NULL) {
@@ -1737,6 +1738,7 @@ static bool snapper_gmt_strip_snapshot(TALLOC_CTX *mem_ctx,
if ((p > name) && (p[-1] != '/')) {
goto no_snapshot;
}
+ len_before_gmt = p - name;
q = strptime(p, GMT_FORMAT, &tm);
if (q == NULL) {
goto no_snapshot;
@@ -1763,7 +1765,7 @@ static bool snapper_gmt_strip_snapshot(TALLOC_CTX *mem_ctx,
q += 1;
rest_len = strlen(q);
- dst_len = (p-name) + rest_len;
+ dst_len = len_before_gmt + rest_len;
if (pstripped != NULL) {
stripped = talloc_array(mem_ctx, char, dst_len+1);
@@ -1772,10 +1774,10 @@ static bool snapper_gmt_strip_snapshot(TALLOC_CTX *mem_ctx,
return false;
}
if (p > name) {
- memcpy(stripped, name, p-name);
+ memcpy(stripped, name, len_before_gmt);
}
if (rest_len > 0) {
- memcpy(stripped + (p-name), q, rest_len);
+ memcpy(stripped + len_before_gmt, q, rest_len);
}
stripped[dst_len] = '\0';
*pstripped = stripped;
--
2.8.0.rc3.226.g39d4020
From 5161e85884f39840ca5b6275c864cb736d7d9628 Mon Sep 17 00:00:00 2001
Date: Wed, 17 Aug 2016 10:57:10 -0700
Subject: [PATCH 3/3] s3: vfs: snapper: Fix snapper_gmt_strip_snapshot()
shadow_copy2.c:shadow_copy2_strip_snapshot()
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12150
---
source3/modules/vfs_snapper.c | 22 ++++++++++++++++++++--
1 file changed, 20 insertions(+), 2 deletions(-)
diff --git a/source3/modules/vfs_snapper.c b/source3/modules/vfs_snapper.c
index f5ccc15..5c1821d 100644
--- a/source3/modules/vfs_snapper.c
+++ b/source3/modules/vfs_snapper.c
@@ -1748,9 +1748,23 @@ static bool snapper_gmt_strip_snapshot(TALLOC_CTX *mem_ctx,
if (timestamp == (time_t)-1) {
goto no_snapshot;
}
- if ((p == name) && (q[0] == '\0')) {
+ if (q[0] == '\0') {
+ /*
+ * The name consists of only the GMT token or the GMT
+ * token is at the end of the path. XP seems to send
+ * with a path prefix.
+ */
if (pstripped != NULL) {
- stripped = talloc_strdup(mem_ctx, "");
+ if (len_before_gmt > 0) {
+ /*
+ * There is a slash before
+ */
+ len_before_gmt -= 1;
+ }
+ stripped = talloc_strndup(mem_ctx, name,
+ len_before_gmt);
if (stripped == NULL) {
return false;
}
@@ -1760,6 +1774,10 @@ static bool snapper_gmt_strip_snapshot(TALLOC_CTX *mem_ctx,
return true;
}
if (q[0] != '/') {
+ /*
+ * It is not a complete path component, i.e. the path
+ * component continues after the gmt-token.
+ */
goto no_snapshot;
}
q += 1;
--
2.8.0.rc3.226.g39d4020
David Disseldorp
2016-08-17 22:32:52 UTC
Permalink
Post by Jeremy Allison
Here is the fix for the vfs_snapper module.
append inside SMB2 to work against all supported modules.
I'll work on the test case code next.
Please review !
Looks good to me too - pushed to autobuild.

Cheers, David
Jeremy Allison
2016-08-18 00:00:48 UTC
Permalink
Post by David Disseldorp
Post by Jeremy Allison
Here is the fix for the vfs_snapper module.
append inside SMB2 to work against all supported modules.
I'll work on the test case code next.
Please review !
Looks good to me too - pushed to autobuild.
David - do you have a working Windows box exposing shadow
copies ?

I'm trying to get master smbclient using the allinfo command
to list the snapshots available on a Windows server over
SMB1 using the client cli_shadow_copy_data() function
called from the allinfo command.

For the life of me I can't get Windows to reply that
there are any shadow copies available, even though
I know there are (and when I query over SMB2 it's
correctly reporting them).

If anyone can send me a wireshark trace of this
working smbclient -> Windows over SMB1 I'd be
eternally grateful.

I'm trying to get the test cases sorted out.

Cheers,

Jeremy.
David Disseldorp
2016-08-18 12:09:55 UTC
Permalink
Post by Jeremy Allison
David - do you have a working Windows box exposing shadow
copies ?
I'm trying to get master smbclient using the allinfo command
to list the snapshots available on a Windows server over
SMB1 using the client cli_shadow_copy_data() function
called from the allinfo command.
For the life of me I can't get Windows to reply that
there are any shadow copies available, even though
I know there are (and when I query over SMB2 it's
correctly reporting them).
I see the same with master and earlier smbclient releases. Was thinking
it was due to a malformed request, after the Wireshark dissector shows
the NTtrans Setup section misaligned by one byte (it interprets the
first as padding). However, looking at MS-SMB 2.2.7.2.1 the request
appears to be okay. Probably worth checking against Windows SMB1 client
behaviour...

Cheers, David
Jeremy Allison
2016-08-18 18:32:35 UTC
Permalink
Post by David Disseldorp
Post by Jeremy Allison
David - do you have a working Windows box exposing shadow
copies ?
I'm trying to get master smbclient using the allinfo command
to list the snapshots available on a Windows server over
SMB1 using the client cli_shadow_copy_data() function
called from the allinfo command.
For the life of me I can't get Windows to reply that
there are any shadow copies available, even though
I know there are (and when I query over SMB2 it's
correctly reporting them).
I see the same with master and earlier smbclient releases. Was thinking
it was due to a malformed request, after the Wireshark dissector shows
the NTtrans Setup section misaligned by one byte (it interprets the
first as padding). However, looking at MS-SMB 2.2.7.2.1 the request
appears to be okay. Probably worth checking against Windows SMB1 client
behaviour...
Found the problem. For SMB1 we need to call cli_shadow_copy_data()
*TWICE* in order for a Windows server to return results. Once with
bool get_names = false, in order to send the max_data_return as
16, which somehow primes the server to expect the second call,
with max_data_return of CLI_BUFFER_SIZE, which then correctly
fetches the data.

This is a Windows SMB1 bug - I'm guessing they never tested the
SMB1 server with a client that just tried a large enough value
to start with to prevent a round trip and their client always
does the '16' followed by whatever that returned.

The Windows SMB2 server works correctly though with one call with a
large enough value.

We still have the problem that when we create the paths previous
versions path to query the SMB1 server returns NT_STATUS_OBJECT_PATH_NOT_FOUND,
due to the fact we're not setting the flags2 FLAGS2_REPARSE_PATH
bit in the request when we're doing the path query. We don't
currently have capabilities to do that, so I'm going to create
another bug and add create fixes for all these things.

The client and server shadow copy/snapper code is a bit of
a mess. What I indend to do for master / 4.6.0 (too late
for 4.5.x) is to add a flag to struct smb_filename to
note that the path came in via a call with FLAGS2_REPARSE_PATH
set. If we don't have that we don't need to do any path
manipulation in the vfs_shadow_copy2 or vfs_snapper
layers - that should speed things up a bit.

Alternatively, now we know in the pathname parser
that we're dealing with an @GMT- path (in SMB1)
or faked-up @GMT- path (in SMB2 via the TWrp token)
then we could remove the @GMT- entry from the path
inside the filename parser unix_convert(), and instead
of a flag in struct smb_filename we could add a time_t
entry which represents the 'previous version timestamp'
this path refers to.

That might be cleaner in the long run as there are
other areas in the smbd server code above the VFS
where we do pathname manipulation (parent_dirname()
etc.) which doesn't currently know about @GMT- pathnames.

Thoughts ?
Jeremy Allison
2016-08-18 21:51:50 UTC
Permalink
Post by Jeremy Allison
We still have the problem that when we create the paths previous
versions path to query the SMB1 server returns NT_STATUS_OBJECT_PATH_NOT_FOUND,
due to the fact we're not setting the flags2 FLAGS2_REPARSE_PATH
bit in the request when we're doing the path query. We don't
currently have capabilities to do that, so I'm going to create
another bug and add create fixes for all these things.
OK, here is a patchset that fixes enumerating and accessing
shadow copy paths from smbclient and libsmbclient.

It looks a bigger patchset than it is :-).

Overview:

01 - Add missing FLAGS2_REPARSE_PATH flag definitions.
02 - 04 - Add additional_flags2 arg to internal cli_XXX() functions
that need to pass paths that might contain @GMT- tokens.
05 - Add clistr_is_previous_version_path() function that checks
for a valid @GMT- path.
06 - 21 - For all cli_XX() functions that take a pathname, call
clistr_is_previous_version_path() and add in the
FLAGS2_REPARSE_PATH to the additional_flags2 parameter
if it maches.

Only function not fixed is cli_set_ea_path(), which
uses the sync versions of cli_trans() which I didn't
change to add additional_flags2 arg. As this is a 'set'
function I don't think this works with a @GMT- path.
Patch 20 is a reminder to change cli_set_ea_path()
to use the async cli_trans_send()/cli_trans_recv()
if we ever need to fix this.

22 - Fix wrong size in cli_shadow_copy_data_send() params.
23 - Harden cli_shadow_copy_data_recv() parsing in case
of bad server. Only crash bug here, nothing worse.
24 - Fix smbclient to use cli_shadow_copy_data() correctly
over SMB1.

Using this I can do an 'allinfo' against a Windows server
with shadow copies, and see them available to smbclient.

Please review and push if happy ! Passes local samba3.blackbox.shadow_copy2
tests.

Once this is in I'll do the same fix for accessing shadow
copies over SMB2.

Cheers,

Jeremy.
Uri Simchoni
2016-08-19 08:22:54 UTC
Permalink
Post by Jeremy Allison
Post by Jeremy Allison
We still have the problem that when we create the paths previous
versions path to query the SMB1 server returns NT_STATUS_OBJECT_PATH_NOT_FOUND,
due to the fact we're not setting the flags2 FLAGS2_REPARSE_PATH
bit in the request when we're doing the path query. We don't
currently have capabilities to do that, so I'm going to create
another bug and add create fixes for all these things.
OK, here is a patchset that fixes enumerating and accessing
shadow copy paths from smbclient and libsmbclient.
It looks a bigger patchset than it is :-).
01 - Add missing FLAGS2_REPARSE_PATH flag definitions.
02 - 04 - Add additional_flags2 arg to internal cli_XXX() functions
05 - Add clistr_is_previous_version_path() function that checks
06 - 21 - For all cli_XX() functions that take a pathname, call
clistr_is_previous_version_path() and add in the
FLAGS2_REPARSE_PATH to the additional_flags2 parameter
if it maches.
Only function not fixed is cli_set_ea_path(), which
uses the sync versions of cli_trans() which I didn't
change to add additional_flags2 arg. As this is a 'set'
Patch 20 is a reminder to change cli_set_ea_path()
to use the async cli_trans_send()/cli_trans_recv()
if we ever need to fix this.
22 - Fix wrong size in cli_shadow_copy_data_send() params.
23 - Harden cli_shadow_copy_data_recv() parsing in case
of bad server. Only crash bug here, nothing worse.
24 - Fix smbclient to use cli_shadow_copy_data() correctly
over SMB1.
Using this I can do an 'allinfo' against a Windows server
with shadow copies, and see them available to smbclient.
Please review and push if happy ! Passes local samba3.blackbox.shadow_copy2
tests.
Once this is in I'll do the same fix for accessing shadow
copies over SMB2.
Cheers,
Jeremy.
Pushed with a small space-before-tab correction in "libsmb: Add uint16_t
addtional_flags2 to cli_trans_send()"

Thanks,
Uri.
Uri Simchoni
2016-08-19 16:18:17 UTC
Permalink
Post by Uri Simchoni
Post by Jeremy Allison
Post by Jeremy Allison
We still have the problem that when we create the paths previous
versions path to query the SMB1 server returns NT_STATUS_OBJECT_PATH_NOT_FOUND,
due to the fact we're not setting the flags2 FLAGS2_REPARSE_PATH
bit in the request when we're doing the path query. We don't
currently have capabilities to do that, so I'm going to create
another bug and add create fixes for all these things.
OK, here is a patchset that fixes enumerating and accessing
shadow copy paths from smbclient and libsmbclient.
It looks a bigger patchset than it is :-).
01 - Add missing FLAGS2_REPARSE_PATH flag definitions.
02 - 04 - Add additional_flags2 arg to internal cli_XXX() functions
05 - Add clistr_is_previous_version_path() function that checks
06 - 21 - For all cli_XX() functions that take a pathname, call
clistr_is_previous_version_path() and add in the
FLAGS2_REPARSE_PATH to the additional_flags2 parameter
if it maches.
Only function not fixed is cli_set_ea_path(), which
uses the sync versions of cli_trans() which I didn't
change to add additional_flags2 arg. As this is a 'set'
Patch 20 is a reminder to change cli_set_ea_path()
to use the async cli_trans_send()/cli_trans_recv()
if we ever need to fix this.
22 - Fix wrong size in cli_shadow_copy_data_send() params.
23 - Harden cli_shadow_copy_data_recv() parsing in case
of bad server. Only crash bug here, nothing worse.
24 - Fix smbclient to use cli_shadow_copy_data() correctly
over SMB1.
Using this I can do an 'allinfo' against a Windows server
with shadow copies, and see them available to smbclient.
Please review and push if happy ! Passes local samba3.blackbox.shadow_copy2
tests.
Once this is in I'll do the same fix for accessing shadow
copies over SMB2.
Cheers,
Jeremy.
Pushed with a small space-before-tab correction in "libsmb: Add uint16_t
addtional_flags2 to cli_trans_send()"
Thanks,
Uri.
Something strange has happened - it seems to have completed the tests in
autobuild - moved to "master passed", but instead of being committed,
there's another queued job from me without me queuing it. I'll see what
happens with that job.

Uri.
Jeremy Allison
2016-08-20 03:37:16 UTC
Permalink
Post by Uri Simchoni
Post by Jeremy Allison
Once this is in I'll do the same fix for accessing shadow
copies over SMB2.
Cheers,
Jeremy.
Pushed with a small space-before-tab correction in "libsmb: Add uint16_t
addtional_flags2 to cli_trans_send()"
Thanks for the fix.

Here is the follow-up patchset to fix getting shadow
copies using the 'allinfo' command over SMB2+.

Passes local make test.

Patch #1 is a leftover I forgot from the previous
patchset.

Patch #2 took FAR TOO LONG to figure out. 6 hours
of staring at smbclient SMB2create calls returning
NT_STATUS_INVALID_PARAMETER until I finally started
looking at padding :-).

Please review and push !

Cheers,

Jeremy.
Uri Simchoni
2016-08-22 09:50:45 UTC
Permalink
Post by Jeremy Allison
Post by Uri Simchoni
Post by Jeremy Allison
Once this is in I'll do the same fix for accessing shadow
copies over SMB2.
Cheers,
Jeremy.
Pushed with a small space-before-tab correction in "libsmb: Add uint16_t
addtional_flags2 to cli_trans_send()"
Thanks for the fix.
Here is the follow-up patchset to fix getting shadow
copies using the 'allinfo' command over SMB2+.
Passes local make test.
Patch #1 is a leftover I forgot from the previous
patchset.
Patch #2 took FAR TOO LONG to figure out. 6 hours
of staring at smbclient SMB2create calls returning
NT_STATUS_INVALID_PARAMETER until I finally started
looking at padding :-).
Please review and push !
Cheers,
Jeremy.
I didn't understand why the following check is made in
Post by Jeremy Allison
+ if (state->out_output_buffer.length +
+ (2 * sizeof(SHADOW_COPY_LABEL)) <
+ state->out_output_buffer.length) {
+ return NT_STATUS_INVALID_NETWORK_RESPONSE;
+ }
+
(besides not understanding its purpose, we should also keep in mind that
smb2cli_ioctl_recv() has made certain that the output buffer is not
larger than what we've asked for. The move to pointer arithmetic does
not prevent overflow in 32-bit platforms, but the check that the
response is < 64K does)

Also if you happen to touch the code, perhaps it's worth commenting that
dlength + 12 > state->out_output_buffer.length *is* a valid response (it
means there are more snapshots than our buffer allows), but we currently
don't support that.

Otherwise RB+ me.

Thanks,
Uri
Jeremy Allison
2016-08-22 16:50:28 UTC
Permalink
Post by Uri Simchoni
I didn't understand why the following check is made in
Post by Jeremy Allison
+ if (state->out_output_buffer.length +
+ (2 * sizeof(SHADOW_COPY_LABEL)) <
+ state->out_output_buffer.length) {
+ return NT_STATUS_INVALID_NETWORK_RESPONSE;
+ }
+
(besides not understanding its purpose, we should also keep in mind that
smb2cli_ioctl_recv() has made certain that the output buffer is not
larger than what we've asked for. The move to pointer arithmetic does
not prevent overflow in 32-bit platforms, but the check that the
response is < 64K does)
Yes, smb2cli_ioctl_recv() does make that guarantee, but as a matter
of security coding I prefer that each layer of code that directly
reads and manipulates remote returned offsets and pointers has it's own overflow
and wrap protection, on top of what called or calling functions do for it.

That way if the code gets cut-and-pasted elsewhere (as does happen :-)
the offset and wrap checks usually get pasted with it, retaining the
protection if the data comes from a source that *isn't* making such
a guarentee.

So the above makes sense as integer wrap protection if state->out_output_buffer.length
comes from a 'raw' remote source.
Post by Uri Simchoni
Also if you happen to touch the code, perhaps it's worth commenting that
dlength + 12 > state->out_output_buffer.length *is* a valid response (it
means there are more snapshots than our buffer allows), but we currently
don't support that.
OK, I'll add a comment here.
Post by Uri Simchoni
Otherwise RB+ me.
Thanks !
Uri Simchoni
2016-08-23 11:45:21 UTC
Permalink
Post by Jeremy Allison
Post by Uri Simchoni
Otherwise RB+ me.
Thanks !
Jeremy,

Now that we have full client shadow-copy functionality in place, I am
extending our test suite to:
- Test both SMB1 and SMB3 (was just SMB1 up to now)
- Test ability to read snapshot files and list directories (was mostly
testing ability to stat snapshot files)

However, as things stand, the directory listing test fails for SMB1
(passes SMB3), so this cannot be pushed yet. I'll look at it later, but
am attaching this in case you want to have a look yourself (and also to
notify that I'm implementing the tests :))

Those test are pretty much complete, but not quite. Gaps are:
- In the case of regex snapshots, they don't test that we get the right
content (ran out of cleverness...)
- Directory tests also just check success/fail, and not that we got the
listing from the right snapshot.

Thanks,
Uri.
Jeremy Allison
2016-08-23 22:32:29 UTC
Permalink
Post by Uri Simchoni
Post by Jeremy Allison
Post by Uri Simchoni
Otherwise RB+ me.
Thanks !
Jeremy,
Now that we have full client shadow-copy functionality in place, I am
- Test both SMB1 and SMB3 (was just SMB1 up to now)
- Test ability to read snapshot files and list directories (was mostly
testing ability to stat snapshot files)
However, as things stand, the directory listing test fails for SMB1
(passes SMB3), so this cannot be pushed yet. I'll look at it later, but
am attaching this in case you want to have a look yourself (and also to
notify that I'm implementing the tests :))
Thanks, I'm out of the office today, so away from my test env,
but I'll take a look tomorrow.
Uri Simchoni
2016-08-24 04:14:22 UTC
Permalink
Post by Jeremy Allison
Post by Uri Simchoni
Post by Jeremy Allison
Post by Uri Simchoni
Otherwise RB+ me.
Thanks !
Jeremy,
Now that we have full client shadow-copy functionality in place, I am
- Test both SMB1 and SMB3 (was just SMB1 up to now)
- Test ability to read snapshot files and list directories (was mostly
testing ability to stat snapshot files)
However, as things stand, the directory listing test fails for SMB1
(passes SMB3), so this cannot be pushed yet. I'll look at it later, but
am attaching this in case you want to have a look yourself (and also to
notify that I'm implementing the tests :))
Thanks, I'm out of the office today, so away from my test env,
but I'll take a look tomorrow.
This is actually a bug (opened
https://bugzilla.samba.org/show_bug.cgi?id=12172), probably a regression
introduced some months ago by (my) fix to bug 11580. Only occurs if the
client uses SMB1. I think I have a simple fix in vfs_shadow_copy2, but I
have to think it over. I may get back to it only in a couple of days though.

Thanks,
Uri.

David Disseldorp
2016-08-18 22:46:23 UTC
Permalink
Post by Jeremy Allison
Found the problem. For SMB1 we need to call cli_shadow_copy_data()
*TWICE* in order for a Windows server to return results. Once with
bool get_names = false, in order to send the max_data_return as
16, which somehow primes the server to expect the second call,
with max_data_return of CLI_BUFFER_SIZE, which then correctly
fetches the data.
This is a Windows SMB1 bug - I'm guessing they never tested the
SMB1 server with a client that just tried a large enough value
to start with to prevent a round trip and their client always
does the '16' followed by whatever that returned.
Nice find Jeremy!
Post by Jeremy Allison
The Windows SMB2 server works correctly though with one call with a
large enough value.
We still have the problem that when we create the paths previous
versions path to query the SMB1 server returns NT_STATUS_OBJECT_PATH_NOT_FOUND,
due to the fact we're not setting the flags2 FLAGS2_REPARSE_PATH
bit in the request when we're doing the path query. We don't
currently have capabilities to do that, so I'm going to create
another bug and add create fixes for all these things.
The client and server shadow copy/snapper code is a bit of
a mess. What I indend to do for master / 4.6.0 (too late
for 4.5.x) is to add a flag to struct smb_filename to
note that the path came in via a call with FLAGS2_REPARSE_PATH
set. If we don't have that we don't need to do any path
manipulation in the vfs_shadow_copy2 or vfs_snapper
layers - that should speed things up a bit.
Alternatively, now we know in the pathname parser
inside the filename parser unix_convert(), and instead
of a flag in struct smb_filename we could add a time_t
entry which represents the 'previous version timestamp'
this path refers to.
That might be cleaner in the long run as there are
other areas in the smbd server code above the VFS
where we do pathname manipulation (parent_dirname()
Thoughts ?
I'm very much in favour of your second approach here:
- It should move a large amount of unnecessary complexity out of the
VFS modules.
- The SMB2+ (timewarp) case would be streamline

Regarding you client allinfo/client patchset - I won't be able to get to
it this week, but will take a look early next week, unless someone else
gets there first.

Cheers, David
Volker Lendecke
2016-08-19 05:31:56 UTC
Permalink
Post by David Disseldorp
Post by Jeremy Allison
Alternatively, now we know in the pathname parser
inside the filename parser unix_convert(), and instead
of a flag in struct smb_filename we could add a time_t
entry which represents the 'previous version timestamp'
this path refers to.
That might be cleaner in the long run as there are
other areas in the smbd server code above the VFS
where we do pathname manipulation (parent_dirname()
Thoughts ?
- It should move a large amount of unnecessary complexity out of the
VFS modules.
- The SMB2+ (timewarp) case would be streamline
This will give us more than one bad day with the shadow_copy2 module,
but when those are over, it will be a LOT cleaner, so go for time_t in
smb_filename!

Volker
Uri Simchoni
2016-08-17 18:10:59 UTC
Permalink
Post by Jeremy Allison
Post by Christof Schmitt
If cli_set_previous_version(cli, time_t t) already registers the
timestamp that should be used for opens, then the cli code could create
token in the path. That way the protocol differences between SMB1 and
SMB2 would be hidden from the application using the cli.
hard. It would touch a *LOT* of client code :-(.
Post by Christof Schmitt
Post by Jeremy Allison
Post by Uri Simchoni
For the time being, I think we should apply your shadow_copy_data()
"allinfo" properly list previous versions against all (reasonably
recent) versions of samba in SMB2 mode, at the expense of providing a
not-quite-realistic test for vfs_shadow_copy2.
I need to fix the source3/modules/vfs_snapper.c code first :-).
I'll send a patch to review soon !
Should we push the fixes you have for shadowcopy2, or do you want to wait
until everything is complete, including snapper and tests?
Already pushed the shadowcopy2 fixes with your reviewed-by.
I'm working on the snapper fix right now which I'll propose
for review today, and then the tests.
IMHO, the @GMT parsing (if at all) should be outside the client libs. It
is an application issue (smbclient), how you represent a previous
version of a file. The client lib needs to expose an API that allows its
user to define create contexts in an easy to use way.

To implement that, I was thinking of "stealing" code from source4 -
moving it into libcli, and thus make it available for source3/libsmb to
use. That would affect only the SMB2 code, and indeed touch a lot of
files due the extra parameter (the added pointer to create contexts on open.

The attached patch is a not-tested-but-compiling first step in that
direction.

Once we have that, and we add the capability to add create contexts to
source3/libsmb, then we can start by just having "allinfo" use that, and
continue with either a modal variable in smbclient, or path parsing, or
both.

How does that sound?

Thanks,
Uri.
Jeremy Allison
2016-08-17 19:24:33 UTC
Permalink
Post by Uri Simchoni
Post by Jeremy Allison
Post by Christof Schmitt
If cli_set_previous_version(cli, time_t t) already registers the
timestamp that should be used for opens, then the cli code could create
token in the path. That way the protocol differences between SMB1 and
SMB2 would be hidden from the application using the cli.
hard. It would touch a *LOT* of client code :-(.
Post by Christof Schmitt
Post by Jeremy Allison
Post by Uri Simchoni
For the time being, I think we should apply your shadow_copy_data()
"allinfo" properly list previous versions against all (reasonably
recent) versions of samba in SMB2 mode, at the expense of providing a
not-quite-realistic test for vfs_shadow_copy2.
I need to fix the source3/modules/vfs_snapper.c code first :-).
I'll send a patch to review soon !
Should we push the fixes you have for shadowcopy2, or do you want to wait
until everything is complete, including snapper and tests?
Already pushed the shadowcopy2 fixes with your reviewed-by.
I'm working on the snapper fix right now which I'll propose
for review today, and then the tests.
is an application issue (smbclient), how you represent a previous
version of a file. The client lib needs to expose an API that allows its
user to define create contexts in an easy to use way.
To implement that, I was thinking of "stealing" code from source4 -
moving it into libcli, and thus make it available for source3/libsmb to
use. That would affect only the SMB2 code, and indeed touch a lot of
files due the extra parameter (the added pointer to create contexts on open.
The attached patch is a not-tested-but-compiling first step in that
direction.
Once we have that, and we add the capability to add create contexts to
source3/libsmb, then we can start by just having "allinfo" use that, and
continue with either a modal variable in smbclient, or path parsing, or
both.
How does that sound?
The lower-level smb2cli_create_send() does accept create contexts.

smb2cli_create_send() is called by cli_smb2_create_fnum_send() which
is called by cli_ntcreate_send()

So if we have a cli_set_previous_version(cli, time_t t) implicitly
add a TWrp create context inside cli_smb2_create_fnum_send(),
then we have a way for the upper-level code to get at this
for SMB2 at least.

As we're designing this change to the API going forward we
can mandate this is SMB2+ functionality, and not expose it
for SMB1 ?

Haven' had time to look at your patch yet, will do so shortly.
Uri Simchoni
2016-08-17 20:05:23 UTC
Permalink
Post by Jeremy Allison
The lower-level smb2cli_create_send() does accept create contexts.
smb2cli_create_send() is called by cli_smb2_create_fnum_send() which
is called by cli_ntcreate_send()
Yes, I meant making create contexts available to
cli_smb2_create_fnum_send(), AND to make them easy to use a-la source4
client.
Post by Jeremy Allison
So if we have a cli_set_previous_version(cli, time_t t) implicitly
add a TWrp create context inside cli_smb2_create_fnum_send(),
then we have a way for the upper-level code to get at this
for SMB2 at least.
That'll work but, to use your words in this thread, it sucks. If one
client opens multiple files it shouldn't "change modes" in between.
Supporting create contexts at some level is better, and it will serve us
in other areas too, I'm sure.
Post by Jeremy Allison
As we're designing this change to the API going forward we
can mandate this is SMB2+ functionality, and not expose it
for SMB1 ?
There are two different requirements here:
1. invent an API that would allow clients to specify files in snapshots
- here the @GMT-xxx/path/to/file is the natural one because that's how
you do it in Windows (there's no timewarp parameter to CreateFile()) and
it's automatically supported by SMB1. So for SMB2 we'll need to parse
the path and convert to TWrp create context (yeah I was wrong saying the
client lib shouldn't mess with it - someone has to :)) - somewhere in
the transition from protocol-agnostic layer to smb2-specific layer.

2. for smbclient utility only, support "chdir'ing" into a snapshot (as
you can do with Windows explorer) - that can be done using a modal
variable in smbclient, that prepends the @GMT stuff to the current
working directory.

With that it can be supported by both protocols.
Post by Jeremy Allison
Haven' had time to look at your patch yet, will do so shortly.
Jeremy Allison
2016-08-17 20:19:11 UTC
Permalink
Post by Uri Simchoni
That'll work but, to use your words in this thread, it sucks. If one
client opens multiple files it shouldn't "change modes" in between.
Supporting create contexts at some level is better, and it will serve us
in other areas too, I'm sure.
It's a fair cop 'gov. A modal change does indeed suck :-).
Post by Uri Simchoni
Post by Jeremy Allison
As we're designing this change to the API going forward we
can mandate this is SMB2+ functionality, and not expose it
for SMB1 ?
1. invent an API that would allow clients to specify files in snapshots
you do it in Windows (there's no timewarp parameter to CreateFile()) and
it's automatically supported by SMB1. So for SMB2 we'll need to parse
the path and convert to TWrp create context (yeah I was wrong saying the
client lib shouldn't mess with it - someone has to :)) - somewhere in
the transition from protocol-agnostic layer to smb2-specific layer.
Ah ok - that's the missing piece ! I'd prefer the /server/share/path/to/file/@GMT-xxx
approach as that prevents us having to mess understand when we have
a DFS vs. regular path.

I'm assuming the above also works with Windows ? I'm having
trouble on my W2K12R2 box getting @GMT paths to work with SMB1.

They're visible with SMB2, not SMB1. Don't know why yet...

cli_smb2_create_fnum_send() is the perfect place to mess
with the path and create the TWrp blob, as it already strips
/ characters from the start and end of the path.

I'll work on that for the 'allinfo' tests.
Post by Uri Simchoni
2. for smbclient utility only, support "chdir'ing" into a snapshot (as
you can do with Windows explorer) - that can be done using a modal
working directory.
With that it can be supported by both protocols.
Let me think about that for smbclient..
Jeremy Allison
2016-08-17 20:23:41 UTC
Permalink
Post by Jeremy Allison
approach as that prevents us having to mess understand when we have
a DFS vs. regular path.
I'm assuming the above also works with Windows ? I'm having
Ah ! Yes it does. Just found in MS-SMB:

-----------------------------------------------------------------
2.2.1.1.1 Pathname Extensions

Pathnames are allowed to contain a previous version token (or @GMT token), as a directory
element in a path. A previous version token indicates that the pathname is a request to access the
previous version (or shadow copy) of the file or directory at a particular point in time. This feature
is available on any path-based operation (for example, SMB_COM_NT_CREATE_ANDX). A pathname
MUST NOT contain more than one previous version token.
For example, requesting a previous version of the file \\server\mydocs\reviews\feb01.doc at
2:44:00 P.M. on March30, 2001 UTC is specified in the following format:
\\server\mydocs\reviews\@GMT-2001.03.30-14.44.00\feb01.doc
The same technique can be used to build a path that represents a previous version of a directory as
opposed to a file.
For example, requesting a previous version of the directory \\server\mydocs\reviews at 2:44:00 PM
on 3/30/01 UTC can be specified in either of the following formats:
A token appearing as an intermediate path component:

\\server\mydocs\@GMT-2001.03.30-14.44.00\reviews

A token appearing as a final path component:

\\server\mydocs\reviews\@GMT-2001.03.30-14.44.00
-----------------------------------------------------------------
Uri Simchoni
2016-08-17 20:33:21 UTC
Permalink
Post by Jeremy Allison
Post by Jeremy Allison
approach as that prevents us having to mess understand when we have
a DFS vs. regular path.
I'm assuming the above also works with Windows ? I'm having
-----------------------------------------------------------------
2.2.1.1.1 Pathname Extensions
element in a path. A previous version token indicates that the pathname is a request to access the
previous version (or shadow copy) of the file or directory at a particular point in time. This feature
is available on any path-based operation (for example, SMB_COM_NT_CREATE_ANDX). A pathname
MUST NOT contain more than one previous version token.
For example, requesting a previous version of the file \\server\mydocs\reviews\feb01.doc at
The same technique can be used to build a path that represents a previous version of a directory as
opposed to a file.
For example, requesting a previous version of the directory \\server\mydocs\reviews at 2:44:00 PM
-----------------------------------------------------------------
*as a directory element* ...
Jeremy Allison
2016-08-17 20:55:04 UTC
Permalink
Post by Uri Simchoni
Post by Jeremy Allison
Post by Jeremy Allison
approach as that prevents us having to mess understand when we have
a DFS vs. regular path.
I'm assuming the above also works with Windows ? I'm having
-----------------------------------------------------------------
2.2.1.1.1 Pathname Extensions
element in a path. A previous version token indicates that the pathname is a request to access the
previous version (or shadow copy) of the file or directory at a particular point in time. This feature
is available on any path-based operation (for example, SMB_COM_NT_CREATE_ANDX). A pathname
MUST NOT contain more than one previous version token.
For example, requesting a previous version of the file \\server\mydocs\reviews\feb01.doc at
The same technique can be used to build a path that represents a previous version of a directory as
opposed to a file.
For example, requesting a previous version of the directory \\server\mydocs\reviews at 2:44:00 PM
-----------------------------------------------------------------
*as a directory element* ...
The last component *Is* a directory element :-). The long
and the short of it is, does this work against Windows ?

Right now I can't get SMB1 @GMT- components to work against
a Win2K12 server. An SMB1 cli_shadow_copy_data() call returns an empty
list, but an SMB2 cli_shadow_copy_data() (with my patch)
can see them.

Any idea what I'm missing ?
Jeremy Allison
2016-08-17 21:03:28 UTC
Permalink
Post by Jeremy Allison
Post by Uri Simchoni
Post by Jeremy Allison
Post by Jeremy Allison
approach as that prevents us having to mess understand when we have
a DFS vs. regular path.
I'm assuming the above also works with Windows ? I'm having
-----------------------------------------------------------------
2.2.1.1.1 Pathname Extensions
element in a path. A previous version token indicates that the pathname is a request to access the
previous version (or shadow copy) of the file or directory at a particular point in time. This feature
is available on any path-based operation (for example, SMB_COM_NT_CREATE_ANDX). A pathname
MUST NOT contain more than one previous version token.
For example, requesting a previous version of the file \\server\mydocs\reviews\feb01.doc at
The same technique can be used to build a path that represents a previous version of a directory as
opposed to a file.
For example, requesting a previous version of the directory \\server\mydocs\reviews at 2:44:00 PM
-----------------------------------------------------------------
*as a directory element* ...
The last component *Is* a directory element :-). The long
and the short of it is, does this work against Windows ?
a Win2K12 server. An SMB1 cli_shadow_copy_data() call returns an empty
list, but an SMB2 cli_shadow_copy_data() (with my patch)
can see them.
Any idea what I'm missing ?
Ah, this might be a clue...

http://www.snia.org/sites/default/orig/sdc_archives/2008_presentations/thursday/DavidKruse_BeyondCIFS.pdf

SMB_FLAGS2_REPARSE_PATH hints that @GMT is present
Jeremy Allison
2016-08-17 23:15:16 UTC
Permalink
Post by Jeremy Allison
Ah, this might be a clue...
http://www.snia.org/sites/default/orig/sdc_archives/2008_presentations/thursday/DavidKruse_BeyondCIFS.pdf
Been doing some digging, and it seems for SMB1 we
can ignore any @GMT-token in the path if:

FLAGS2_REPARSE_PATH 0x0400

is *NOT* set. At least in master we have
the flags field inside struct smb_filename,
which should allow us to flag any path
containing a @GMT-token to make the
shadow2 and snapper modules much more
efficient (they won't have to strip
paths on every VFS call).

This could help a *lot*, if we plumb
this in via unix_convert() pathname
processing for both SMB1 and SMB2..
Jeremy Allison
2016-08-16 16:59:35 UTC
Permalink
Post by Christof Schmitt
Post by Jeremy Allison
Post by Christof Schmitt
From 3a33435fc3d61605c75ed3f3b87612fbe729b553 Mon Sep 17 00:00:00 2001
Date: Fri, 12 Aug 2016 14:59:07 -0700
Subject: [PATCH] smbd: Fix snapshot query on shares with DFS enabled
When DFS is enabled (host msdfs = yes and msdfs root = yes), then SMB
clients send create requests in the format \hostname\service\path.
Putting the GMT tag as first element breaks the DFS parsing and results
in OBJECT_NOT_FOUND for snapshotted files. Fix this by appending the
GMT tag to the end of the path.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12150
as we're going to need this backported.
I just came across one more thing: The test is on a system with the
shadowcopy2 module enabled. While the patches fixes the problem for
directories, there seems to be an issue when a SMB client is explicitly
asking for older versions of a file. It looks like shadowcopy leaves a
(test is a file, not a directory)
[2016/08/15 16:12:36.178799, 10, pid=13125, effective(12000500, 12000513), real(12000500, 0)] ../source3/modules/vfs_shadow_copy2.c:245(shadow_copy2_strip_snapshot)
[2016/08/15 16:12:36.178814, 10, pid=13125, effective(12000500, 12000513), real(12000500, 0)] ../source3/modules/vfs_shadow_copy2.c:462(shadow_copy2_do_convert)
converting 'test/'
[2016/08/15 16:12:36.178837, 10, pid=13125, effective(12000500, 12000513), real(12000500, 0)] ../source3/modules/vfs_shadow_copy2.c:604(shadow_copy2_do_convert)
[2016/08/15 16:12:36.178860, 10, pid=13125, effective(12000500, 12000513), real(12000500, 0)] ../source3/modules/vfs_shadow_copy2.c:604(shadow_copy2_do_convert)
[2016/08/15 16:12:36.178886, 10, pid=13125, effective(12000500, 12000513), real(12000500, 0)] ../source3/modules/vfs_shadow_copy2.c:604(shadow_copy2_do_convert)
I will try to take a closer look tomorrow, maybe that can be easily
handled in the shadowcopy2 module. The backport should probably wait
until this is resolved
I think this should fix it - can you test ?

I'll work on a regression test now.

Thanks,

Jeremy.
Christof Schmitt
2016-08-16 22:39:44 UTC
Permalink
Post by Jeremy Allison
Post by Christof Schmitt
Post by Jeremy Allison
Post by Christof Schmitt
From 3a33435fc3d61605c75ed3f3b87612fbe729b553 Mon Sep 17 00:00:00 2001
Date: Fri, 12 Aug 2016 14:59:07 -0700
Subject: [PATCH] smbd: Fix snapshot query on shares with DFS enabled
When DFS is enabled (host msdfs = yes and msdfs root = yes), then SMB
clients send create requests in the format \hostname\service\path.
Putting the GMT tag as first element breaks the DFS parsing and results
in OBJECT_NOT_FOUND for snapshotted files. Fix this by appending the
GMT tag to the end of the path.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12150
as we're going to need this backported.
I just came across one more thing: The test is on a system with the
shadowcopy2 module enabled. While the patches fixes the problem for
directories, there seems to be an issue when a SMB client is explicitly
asking for older versions of a file. It looks like shadowcopy leaves a
(test is a file, not a directory)
[2016/08/15 16:12:36.178799, 10, pid=13125, effective(12000500, 12000513), real(12000500, 0)] ../source3/modules/vfs_shadow_copy2.c:245(shadow_copy2_strip_snapshot)
[2016/08/15 16:12:36.178814, 10, pid=13125, effective(12000500, 12000513), real(12000500, 0)] ../source3/modules/vfs_shadow_copy2.c:462(shadow_copy2_do_convert)
converting 'test/'
[2016/08/15 16:12:36.178837, 10, pid=13125, effective(12000500, 12000513), real(12000500, 0)] ../source3/modules/vfs_shadow_copy2.c:604(shadow_copy2_do_convert)
[2016/08/15 16:12:36.178860, 10, pid=13125, effective(12000500, 12000513), real(12000500, 0)] ../source3/modules/vfs_shadow_copy2.c:604(shadow_copy2_do_convert)
[2016/08/15 16:12:36.178886, 10, pid=13125, effective(12000500, 12000513), real(12000500, 0)] ../source3/modules/vfs_shadow_copy2.c:604(shadow_copy2_do_convert)
I will try to take a closer look tomorrow, maybe that can be easily
handled in the shadowcopy2 module. The backport should probably wait
until this is resolved
I think this should fix it - can you test ?
Yes, this works.
Post by Jeremy Allison
I'll work on a regression test now.
Thanks,
Jeremy.
From 3b54af0e18172e4b3aae294df0edfe97e572a480 Mon Sep 17 00:00:00 2001
Date: Tue, 16 Aug 2016 09:41:43 -0700
Subject: [PATCH 1/2] s3: vfs: shadow_copy2: Re-use an existing variable
already set to the right value (p - name).
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12150
---
source3/modules/vfs_shadow_copy2.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/source3/modules/vfs_shadow_copy2.c b/source3/modules/vfs_shadow_copy2.c
index bda934e..b1aae1e 100644
--- a/source3/modules/vfs_shadow_copy2.c
+++ b/source3/modules/vfs_shadow_copy2.c
@@ -486,7 +486,8 @@ static bool shadow_copy2_strip_snapshot(TALLOC_CTX *mem_ctx,
* with a path prefix.
*/
if (pstripped != NULL) {
- stripped = talloc_strndup(mem_ctx, name, p - name);
+ stripped = talloc_strndup(mem_ctx, name,
+ len_before_gmt);
if (stripped == NULL) {
return false;
}
--
2.8.0.rc3.226.g39d4020
From 3b14aa977d069479ceadd912ca4c109887849f20 Mon Sep 17 00:00:00 2001
Date: Tue, 16 Aug 2016 09:43:37 -0700
Subject: [PATCH 2/2] s3: vfs: shadow_copy2. Remove any trailing slash when
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12150
---
source3/modules/vfs_shadow_copy2.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/source3/modules/vfs_shadow_copy2.c b/source3/modules/vfs_shadow_copy2.c
index b1aae1e..4ac16d3 100644
--- a/source3/modules/vfs_shadow_copy2.c
+++ b/source3/modules/vfs_shadow_copy2.c
@@ -486,6 +486,13 @@ static bool shadow_copy2_strip_snapshot(TALLOC_CTX *mem_ctx,
* with a path prefix.
*/
if (pstripped != NULL) {
+ if (len_before_gmt > 0) {
+ /*
+ * There is a slash before
+ */
+ len_before_gmt -= 1;
+ }
stripped = talloc_strndup(mem_ctx, name,
len_before_gmt);
if (stripped == NULL) {
--
2.8.0.rc3.226.g39d4020
Jeremy Allison
2016-08-16 23:51:29 UTC
Permalink
Post by Jeremy Allison
Post by Christof Schmitt
Post by Jeremy Allison
Post by Christof Schmitt
From 3a33435fc3d61605c75ed3f3b87612fbe729b553 Mon Sep 17 00:00:00 2001
Date: Fri, 12 Aug 2016 14:59:07 -0700
Subject: [PATCH] smbd: Fix snapshot query on shares with DFS enabled
When DFS is enabled (host msdfs = yes and msdfs root = yes), then SMB
clients send create requests in the format \hostname\service\path.
Putting the GMT tag as first element breaks the DFS parsing and results
in OBJECT_NOT_FOUND for snapshotted files. Fix this by appending the
GMT tag to the end of the path.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12150
as we're going to need this backported.
I just came across one more thing: The test is on a system with the
shadowcopy2 module enabled. While the patches fixes the problem for
directories, there seems to be an issue when a SMB client is explicitly
asking for older versions of a file. It looks like shadowcopy leaves a
(test is a file, not a directory)
[2016/08/15 16:12:36.178799, 10, pid=13125, effective(12000500, 12000513), real(12000500, 0)] ../source3/modules/vfs_shadow_copy2.c:245(shadow_copy2_strip_snapshot)
[2016/08/15 16:12:36.178814, 10, pid=13125, effective(12000500, 12000513), real(12000500, 0)] ../source3/modules/vfs_shadow_copy2.c:462(shadow_copy2_do_convert)
converting 'test/'
[2016/08/15 16:12:36.178837, 10, pid=13125, effective(12000500, 12000513), real(12000500, 0)] ../source3/modules/vfs_shadow_copy2.c:604(shadow_copy2_do_convert)
[2016/08/15 16:12:36.178860, 10, pid=13125, effective(12000500, 12000513), real(12000500, 0)] ../source3/modules/vfs_shadow_copy2.c:604(shadow_copy2_do_convert)
[2016/08/15 16:12:36.178886, 10, pid=13125, effective(12000500, 12000513), real(12000500, 0)] ../source3/modules/vfs_shadow_copy2.c:604(shadow_copy2_do_convert)
I will try to take a closer look tomorrow, maybe that can be easily
handled in the shadowcopy2 module. The backport should probably wait
until this is resolved
I think this should fix it - can you test ?
I'll work on a regression test now.
David, I think the source3/modules/vfs_snapper.c code needs
these same changes to work correctly with appended /@GMT-YYYY..
snapshot paths.

Can you confirm please ?

Thanks,

Jeremy.
Post by Jeremy Allison
From 3b54af0e18172e4b3aae294df0edfe97e572a480 Mon Sep 17 00:00:00 2001
Date: Tue, 16 Aug 2016 09:41:43 -0700
Subject: [PATCH 1/2] s3: vfs: shadow_copy2: Re-use an existing variable
already set to the right value (p - name).
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12150
---
source3/modules/vfs_shadow_copy2.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/source3/modules/vfs_shadow_copy2.c b/source3/modules/vfs_shadow_copy2.c
index bda934e..b1aae1e 100644
--- a/source3/modules/vfs_shadow_copy2.c
+++ b/source3/modules/vfs_shadow_copy2.c
@@ -486,7 +486,8 @@ static bool shadow_copy2_strip_snapshot(TALLOC_CTX *mem_ctx,
* with a path prefix.
*/
if (pstripped != NULL) {
- stripped = talloc_strndup(mem_ctx, name, p - name);
+ stripped = talloc_strndup(mem_ctx, name,
+ len_before_gmt);
if (stripped == NULL) {
return false;
}
--
2.8.0.rc3.226.g39d4020
From 3b14aa977d069479ceadd912ca4c109887849f20 Mon Sep 17 00:00:00 2001
Date: Tue, 16 Aug 2016 09:43:37 -0700
Subject: [PATCH 2/2] s3: vfs: shadow_copy2. Remove any trailing slash when
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12150
---
source3/modules/vfs_shadow_copy2.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/source3/modules/vfs_shadow_copy2.c b/source3/modules/vfs_shadow_copy2.c
index b1aae1e..4ac16d3 100644
--- a/source3/modules/vfs_shadow_copy2.c
+++ b/source3/modules/vfs_shadow_copy2.c
@@ -486,6 +486,13 @@ static bool shadow_copy2_strip_snapshot(TALLOC_CTX *mem_ctx,
* with a path prefix.
*/
if (pstripped != NULL) {
+ if (len_before_gmt > 0) {
+ /*
+ * There is a slash before
+ */
+ len_before_gmt -= 1;
+ }
stripped = talloc_strndup(mem_ctx, name,
len_before_gmt);
if (stripped == NULL) {
--
2.8.0.rc3.226.g39d4020
David Disseldorp
2016-08-17 23:15:31 UTC
Permalink
Post by Jeremy Allison
David, I think the source3/modules/vfs_snapper.c code needs
snapshot paths.
Can you confirm please ?
Yes. Thanks for the heads-up and fix here :-)

Cheers, David
Loading...