[PATCH] CIFS: Update CreationTime on every query info request

On Wed, 6 Aug 2014 15:15:43 +0400

Post by Pavel Shilovsky
Samba server can change a creation time of an inode in the default
configuration. The client sets this value during the inode initialization
only and uses it the inode search. In this case harlinked dentries may
link to different inodes. As the results the Cthon basic test #7 fails
on nounix Samba mounts.
Fix this by making the client update the creation time on every
query info request.
---
fs/cifs/inode.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index a174605..2029e7c 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -173,6 +173,9 @@ cifs_fattr_to_inode(struct inode *inode, struct cifs_fattr *fattr)
cifs_i->cifsAttrs = fattr->cf_cifsattrs;
+ /* Samba server changes this value in the default configuration */
+ cifs_i->createtime = fattr->cf_createtime;
+
if (fattr->cf_flags & CIFS_FATTR_NEED_REVAL)
cifs_i->time = 0;
else

NAK

That really sounds more like a bug in Samba. The creation time is
supposed to be immutable, and if it changes then that means that you
have a new inode.

We really *don't* want to go updating it like this.

--
Jeff Layton <jlayton-***@public.gmane.org>

Pavel Shilovsky

2014-08-06 11:59:22 UTC

Post by Jeff Layton
On Wed, 6 Aug 2014 15:15:43 +0400

NAK
That really sounds more like a bug in Samba. The creation time is
supposed to be immutable, and if it changes then that means that you
have a new inode.
We really *don't* want to go updating it like this.

If it is suppose to be immutable and the server does processing right,
the fix will not break things since the cifs_i->createtime value stays
the same.

But if this value was changed and we don't store this latest returned
value from the server and continue to use the old one, does things go
better? We'll end up relying on the value that is wrong...

If Samba server doesn't store the creation time attribute in xattrs
(that is restricted by underlying file system and switched off by
default), it can't return a right value to the client since POSIX file
systems store only access, modify and change timestamps.

So, in the result we have the CIFS client that doesn't work with the
default configuration of Samba. If we can't fix Samba due to POSIX
restrictions), we need to do something with the client, don't we?

--
Best regards,
Pavel Shilovsky.

Jeff Layton

2014-08-06 12:08:33 UTC

On Wed, 6 Aug 2014 15:59:22 +0400

Post by Jeff Layton
On Wed, 6 Aug 2014 15:15:43 +0400

NAK
That really sounds more like a bug in Samba. The creation time is
supposed to be immutable, and if it changes then that means that you
have a new inode.
We really *don't* want to go updating it like this.

If it is suppose to be immutable and the server does processing right,
the fix will not break things since the cifs_i->createtime value stays
the same.

Does anything prevent a server from reusing a uniqueid? If not, then
this is one of the only mechanisms to tell whether this is the same
inode as the previous one or a new one.

Post by Pavel Shilovsky
But if this value was changed and we don't store this latest returned
value from the server and continue to use the old one, does things go
better? We'll end up relying on the value that is wrong...

We rely on the server to send us valid information. If the server
doesn't do that, then it's a server bug.

Post by Pavel Shilovsky
If Samba server doesn't store the creation time attribute in xattrs
(that is restricted by underlying file system and switched off by
default), it can't return a right value to the client since POSIX file
systems store only access, modify and change timestamps.

Yep, faking up valid fields like this is always iffy.

Post by Pavel Shilovsky
So, in the result we have the CIFS client that doesn't work with the
default configuration of Samba. If we can't fix Samba due to POSIX
restrictions), we need to do something with the client, don't we?

Well, no. The default configuration of samba has unix extensions
enabled, and with that the create time is ignored (since it's not
returned at all in that case).

--
Jeff Layton <jlayton-***@public.gmane.org>

Pavel Shilovsky

2014-08-06 12:38:32 UTC

Post by Jeff Layton
On Wed, 6 Aug 2014 15:59:22 +0400

Post by Jeff Layton
On Wed, 6 Aug 2014 15:15:43 +0400

NAK
That really sounds more like a bug in Samba. The creation time is
supposed to be immutable, and if it changes then that means that you
have a new inode.
We really *don't* want to go updating it like this.

If it is suppose to be immutable and the server does processing right,
the fix will not break things since the cifs_i->createtime value stays
the same.

Does anything prevent a server from reusing a uniqueid? If not, then
this is one of the only mechanisms to tell whether this is the same
inode as the previous one or a new one.

If server reuses a uniqueid - it is a bug in the server.

The creation time can be changed on the Windows server:
1) we call stat FILE - construct the inode with the recent info;
2) on the server another client calls SetFileTime() and updates
CreationTime of the FILE;
3) we call stat FILE again, get the recent info but don't store the
new CreationTime value and stay with the wrong CreationTime value.

Also, in this case we can't rely on CreationTime to determine if this
is the same inode or not. I think the best thing is to *trust* the
server that uniqueid is unique (at least for SMB 2.0 and higher
versions).

Note, that now we use CreationTIme value for the inode search in
iget(). The above example of changing this value on the server shows
that we can end up with several inodes for each "creationtime" version
of the file.

We rely on the server to send us valid information. If the server
doesn't do that, then it's a server bug.

Yep, faking up valid fields like this is always iffy.

Well, no. The default configuration of samba has unix extensions
enabled, and with that the create time is ignored (since it's not
returned at all in that case).

The problem is that we don't have unix extensions for SMB 2.0 and
higher protocol versions and we need to use nounix either way.

--
Best regards,
Pavel Shilovsky.

Jeff Layton

2014-08-06 13:31:14 UTC

On Wed, 6 Aug 2014 16:38:32 +0400

Post by Jeff Layton
On Wed, 6 Aug 2014 15:59:22 +0400

Post by Jeff Layton
On Wed, 6 Aug 2014 15:15:43 +0400

NAK
That really sounds more like a bug in Samba. The creation time is
supposed to be immutable, and if it changes then that means that you
have a new inode.
We really *don't* want to go updating it like this.

If it is suppose to be immutable and the server does processing right,
the fix will not break things since the cifs_i->createtime value stays
the same.

Does anything prevent a server from reusing a uniqueid? If not, then
this is one of the only mechanisms to tell whether this is the same
inode as the previous one or a new one.

If server reuses a uniqueid - it is a bug in the server.

Then I think that's probably another samba bug. IIRC, it generates
uniqueids using the st_ino and st_dev fields from stat(). There are
many Linux filesystems that will aggressively reuse inode numbers and
there is no API to get at the inode->i_generation field from userland.

--------------[snip]----------------
/********************************************************************
Create a 64 bit FileIndex. If the file is on the same device as
the root of the share, just return the 64-bit inode. If it isn't,
mangle as we used to do.
********************************************************************/

uint64_t get_FileIndex(connection_struct *conn, const SMB_STRUCT_STAT *psbuf)
{
uint64_t file_index;
if (conn->base_share_dev == psbuf->st_ex_dev) {
return (uint64_t)psbuf->st_ex_ino;
}
file_index = ((psbuf->st_ex_ino) & UINT32_MAX); /* FileIndexLow */
file_index |= ((uint64_t)((psbuf->st_ex_dev) & UINT32_MAX)) << 32; /* FileIndexHigh */
return file_index;
}
--------------[snip]----------------

Post by Pavel Shilovsky
1) we call stat FILE - construct the inode with the recent info;
2) on the server another client calls SetFileTime() and updates
CreationTime of the FILE;
3) we call stat FILE again, get the recent info but don't store the
new CreationTime value and stay with the wrong CreationTime value.
Also, in this case we can't rely on CreationTime to determine if this
is the same inode or not. I think the best thing is to *trust* the
server that uniqueid is unique (at least for SMB 2.0 and higher
versions).
Note, that now we use CreationTIme value for the inode search in
iget(). The above example of changing this value on the server shows
that we can end up with several inodes for each "creationtime" version
of the file.

Sure, it's settable and you can abuse it, but who actually does that?

It's very rare for someone to mess with the create time like that. I
imagine it's generally only done by backup utilities when restoring
and maybe rootkits. Those are both sort of outside the scope of
normal usage.

We rely on the server to send us valid information. If the server
doesn't do that, then it's a server bug.

Yep, faking up valid fields like this is always iffy.

Well, no. The default configuration of samba has unix extensions
enabled, and with that the create time is ignored (since it's not
returned at all in that case).

The problem is that we don't have unix extensions for SMB 2.0 and
higher protocol versions and we need to use nounix either way.

Then that's a problem that will need to eventually be addressed anyway.
I don't see how this is anything but a way to paper over that fact in
the meantime.

--
Jeff Layton <jlayton-***@public.gmane.org>

Pavel Shilovsky

2014-08-06 15:21:55 UTC

Post by Jeff Layton
On Wed, 6 Aug 2014 16:38:32 +0400

Post by Pavel Shilovsky
If server reuses a uniqueid - it is a bug in the server.

Then I think that's probably another samba bug. IIRC, it generates
uniqueids using the st_ino and st_dev fields from stat(). There are
many Linux filesystems that will aggressively reuse inode numbers and
there is no API to get at the inode->i_generation field from userland.
--------------[snip]----------------
/********************************************************************
Create a 64 bit FileIndex. If the file is on the same device as
the root of the share, just return the 64-bit inode. If it isn't,
mangle as we used to do.
********************************************************************/
uint64_t get_FileIndex(connection_struct *conn, const SMB_STRUCT_STAT *psbuf)
{
uint64_t file_index;
if (conn->base_share_dev == psbuf->st_ex_dev) {
return (uint64_t)psbuf->st_ex_ino;
}
file_index = ((psbuf->st_ex_ino) & UINT32_MAX); /* FileIndexLow */
file_index |= ((uint64_t)((psbuf->st_ex_dev) & UINT32_MAX)) << 32; /* FileIndexHigh */
return file_index;
}
--------------[snip]----------------

Sure, it's settable and you can abuse it, but who actually does that?
It's very rare for someone to mess with the create time like that. I
imagine it's generally only done by backup utilities when restoring
and maybe rootkits. Those are both sort of outside the scope of
normal usage.

Microsoft Office Excel is doing this -- see
http://www.techrepublic.com/blog/the-enterprise-cloud/why-writing-a-windows-compatible-file-server-is-still-hard/
story.
So, it is not a rootkit or backup utility, it's a normal way Windows
is working with files.

Post by Pavel Shilovsky
The problem is that we don't have unix extensions for SMB 2.0 and
higher protocol versions and we need to use nounix either way.

Then that's a problem that will need to eventually be addressed anyway.
I don't see how this is anything but a way to paper over that fact in
the meantime.

I really started to think that we can't use CreationTime in
find_inode() since this is not constant on both Windows and Samba (by
default). Even more, when negotiating Unix Extensions we always set
this field to 0. I suggest that we need to do the same thing for
nounix mounts too. At least we should do it for SMB 2/3 since we don't
have Unix Extensions for them now.

So, we have two choices from my point of view:
1) to update CreationTime value any time we are querying info from the
server (what exactly this patch does);
2) to remove CreationTime checks in find_inode() for nounix at least
for SMB 2/3 (probably for CIFS too).

Thoughts?

--
Best regards,
Pavel Shilovsky.

Shirish Pargaonkar

2014-08-11 15:02:01 UTC

Post by Jeff Layton
On Wed, 6 Aug 2014 16:38:32 +0400

Post by Pavel Shilovsky
If server reuses a uniqueid - it is a bug in the server.

Then I think that's probably another samba bug. IIRC, it generates
uniqueids using the st_ino and st_dev fields from stat(). There are
many Linux filesystems that will aggressively reuse inode numbers and
there is no API to get at the inode->i_generation field from userland.
--------------[snip]----------------
/********************************************************************
Create a 64 bit FileIndex. If the file is on the same device as
the root of the share, just return the 64-bit inode. If it isn't,
mangle as we used to do.
********************************************************************/
uint64_t get_FileIndex(connection_struct *conn, const SMB_STRUCT_STAT *psbuf)
{
uint64_t file_index;
if (conn->base_share_dev == psbuf->st_ex_dev) {
return (uint64_t)psbuf->st_ex_ino;
}
file_index = ((psbuf->st_ex_ino) & UINT32_MAX); /* FileIndexLow */
file_index |= ((uint64_t)((psbuf->st_ex_dev) & UINT32_MAX)) << 32; /* FileIndexHigh */
return file_index;
}
--------------[snip]----------------

Sure, it's settable and you can abuse it, but who actually does that?
It's very rare for someone to mess with the create time like that. I
imagine it's generally only done by backup utilities when restoring
and maybe rootkits. Those are both sort of outside the scope of
normal usage.