| Message ID | 1407323743-12381-1-git-send-email-pshilovsky@samba.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 6 Aug 2014 15:15:43 +0400 Pavel Shilovsky <pshilovsky@samba.org> wrote: > 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. > > Cc: Jeff Layton <jlayton@samba.org> > Signed-off-by: Pavel Shilovsky <pshilovsky@samba.org> > --- > 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.
2014-08-06 15:25 GMT+04:00 Jeff Layton <jlayton@samba.org>: > On Wed, 6 Aug 2014 15:15:43 +0400 > Pavel Shilovsky <pshilovsky@samba.org> wrote: > >> 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. >> >> Cc: Jeff Layton <jlayton@samba.org> >> Signed-off-by: Pavel Shilovsky <pshilovsky@samba.org> >> --- >> 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. 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?
On Wed, 6 Aug 2014 15:59:22 +0400 Pavel Shilovsky <pshilovsky@samba.org> wrote: > 2014-08-06 15:25 GMT+04:00 Jeff Layton <jlayton@samba.org>: > > On Wed, 6 Aug 2014 15:15:43 +0400 > > Pavel Shilovsky <pshilovsky@samba.org> wrote: > > > >> 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. > >> > >> Cc: Jeff Layton <jlayton@samba.org> > >> Signed-off-by: Pavel Shilovsky <pshilovsky@samba.org> > >> --- > >> 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. > > 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. > 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. > 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. > 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).
2014-08-06 16:08 GMT+04:00 Jeff Layton <jlayton@samba.org>: > On Wed, 6 Aug 2014 15:59:22 +0400 > Pavel Shilovsky <pshilovsky@samba.org> wrote: > >> 2014-08-06 15:25 GMT+04:00 Jeff Layton <jlayton@samba.org>: >> > On Wed, 6 Aug 2014 15:15:43 +0400 >> > Pavel Shilovsky <pshilovsky@samba.org> wrote: >> > >> >> 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. >> >> >> >> Cc: Jeff Layton <jlayton@samba.org> >> >> Signed-off-by: Pavel Shilovsky <pshilovsky@samba.org> >> >> --- >> >> 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. >> >> 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. >> 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. > >> 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. > >> 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). 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.
On Wed, 6 Aug 2014 16:38:32 +0400 Pavel Shilovsky <pshilovsky@samba.org> wrote: > 2014-08-06 16:08 GMT+04:00 Jeff Layton <jlayton@samba.org>: > > On Wed, 6 Aug 2014 15:59:22 +0400 > > Pavel Shilovsky <pshilovsky@samba.org> wrote: > > > >> 2014-08-06 15:25 GMT+04:00 Jeff Layton <jlayton@samba.org>: > >> > On Wed, 6 Aug 2014 15:15:43 +0400 > >> > Pavel Shilovsky <pshilovsky@samba.org> wrote: > >> > > >> >> 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. > >> >> > >> >> Cc: Jeff Layton <jlayton@samba.org> > >> >> Signed-off-by: Pavel Shilovsky <pshilovsky@samba.org> > >> >> --- > >> >> 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. > >> > >> 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]---------------- > 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. > 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. > >> 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. > > > >> 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. > > > >> 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). > > 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.
2014-08-06 17:31 GMT+04:00 Jeff Layton <jlayton@samba.org>: > On Wed, 6 Aug 2014 16:38:32 +0400 > Pavel Shilovsky <pshilovsky@samba.org> wrote: >> 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]---------------- > >> 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. >> > > 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. >> 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?
On Wed, Aug 6, 2014 at 10:21 AM, Pavel Shilovsky <pshilovsky@samba.org> wrote: > 2014-08-06 17:31 GMT+04:00 Jeff Layton <jlayton@samba.org>: >> On Wed, 6 Aug 2014 16:38:32 +0400 >> Pavel Shilovsky <pshilovsky@samba.org> wrote: >>> 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]---------------- >> >>> 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. >>> >> >> 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. > >>> 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. Would this problem be solved, if more than nounix, we restrict use/criterion of cratetime in cifs_find_inode() for cases where server does not provide uniqueids? Not sure how can we check that in cifs_find_inode() though. -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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. Cc: Jeff Layton <jlayton@samba.org> Signed-off-by: Pavel Shilovsky <pshilovsky@samba.org> --- fs/cifs/inode.c | 3 +++ 1 file changed, 3 insertions(+)