This one is subtle (and thanks to Brad @ Apple for helping find this). When we create a file on disk we store the "birth" time (btime hereafter) in the "user.DosAttrib" extended attribute. When we get a disconnection event that causes us to write out a durable handle record on disk inside vfs_default_durable_disconnect() we fstat() the file to ensure we are storing the latest up-to-date timestamps of atime, ctime and mtime in the "cookie" we check to allow durable handle reconnection. However, we were forgetting to update the btime from the stored "user.DosAttrib" extended attribute, meaning if we use a calculated btime of the minimum of (atime|ctime|mtime) we stored btime in the cookie as the calculated btime, not the btime stored in the "user.DosAttrib" extended attribute. When the client tries to reconnect, we are correctly reading the btime from the stored "user.DosAttrib" extended attribute before comparing it with the btime stored in the cookie, and so it will never match if the timestamps on disk have changed (i.e. the file was written to since after it had been opened). I have a regression test and fix.
Hm, but vfs_default_durable_disconnect() uses vfs_stat_fsp() which should preserve the btime. So the problem doesn't seem to be that vfs_default_durable_disconnect() clobbers the btime, but that the caller passes in an already clobbered one?
Yes, I'm planning to investigate that today. But adding the fdos_mode(fsp) call fixes my regression test, so it's definitely a clobbered btime.
Hmmm. When it gets to vfs_default_durable_disconnect() we have: st_ex_iflags = 0x7 So: ST_EX_IFLAG_CALCULATED_BTIME ST_EX_IFLAG_CALCULATED_ITIME ST_EX_IFLAG_CALCULATED_FILE_ID are all set.
One innocent call to SMB_VFS_FSTAT() somewhere is enough to clobber it.
OK, I've found the underlying issue. There are a couple of problems: 1). When we create a file on Linux, as we use the stat/fstat/lstat calls that don't get btime, we end up with a created btime set as the minimum of [ctime|mtime|atime] and we set the ST_EX_IFLAG_CALCULATED_BTIME (0x1) flag in st_ex_iflags, as we should. We then call file_set_dosmode() to write the user.DosAttrib EA onto the newly created file. This stores the btime in the EA - but if this is successful we don't then clear the ST_EX_IFLAG_CALCULATED_BTIME flag after storing the btime in the EA. Afterwards, if we call fdos_mode(fsp) to read the user.DosAttrib EA, as part of that call we then set btime from the EA we just read, and then clear ST_EX_IFLAG_CALCULATED_BTIME as we've just gotten it from the EA. We actually do that as part of the SMB2 create return, in smbd_smb2_create_after_exec() when returning the dos attributes to the caller, but this leaves a lot of code after the create before the return where ST_EX_IFLAG_CALCULATED_BTIME is set even after storing in the EA. So fix number 1 is to call update_stat_ex_create_time() from set_ea_dos_attribute() if we get a success return from SMB_VFS_FSETXATTR(). This doesn't fix the underlying bug though -). 2). The *REAL* bug is in smb2_setinfo (and also in smb2_getinfo and other places). In smb2_setinfo we validate the fsp passed in as a handle, and then we call SMB_VFS_FSTAT(fsp, &fsp->fsp_name->st). THIS CLOBBERS THE st_ex_iflags and btime on fsp->fsp_name->st. We've now lost the fact that this is a EA stored btime, not a calculated one (and indeed fsp->fsp_name->st.st_ex_iflags is now 0x7. We should be calling vfs_stat_fsp(fsp) here, and indeed in every place we are using: SMB_VFS_FSTAT(fsp, &fsp->fsp_name->st) i.e. where the passed in stat struct is the stat struct associated with the fsp. There are a few other places in the code where we do this, so I'm going to fix them all in this patchset. Long term, the difference between SMB_VFS_FSTAT() and vfs_stat_fsp() is too subtle to easily understand, and is a bad API and source of future errors. We should probably discuss how to fix this in future.
Created attachment 17228 [details] Core of the fix for master. Just so I don't lose it, here's the core of the fixes (without regression test). I have a regression test, I'll add that to the final patchset I run through ci.
Just making sure Ralph gets a heads-up on this :-).
Yay. The raw: https://bugzilla.samba.org/attachment.cgi?id=17228 passes ci, so I didn't break anything :-). Now I need to add in the regression test and create an official MR.
MR is: https://gitlab.com/samba-team/samba/-/merge_requests/2448 Ralph, I assigned it to you as you've been following along already :-).
This bug was referenced in samba master: 0036617a5c76e6003e3c9a5039c325d77d897709 9f62a149f12e899dbc2a7bac9458e7a375bf6608 d460118be3ad2a3100bb3458f6f0223df12f7c3f 2b246dbf687cbb6ef1e31e22fd64a95bdca8f4e9 ec2fb9d22842332992b8f667a8218f7aaa1be7c4 a604dd02ffb468ba472ac4dd64f06d30ecdcc810 18694c81cc3c3d94dc99e3b11878021052279eb6 cfadecca802600fa9a01fa9781e4df9a0b71fa3c 064c5770deb4307272c60f0b08f6bc137df4b227 7f5c484804c87f37493e1f79d5c7e1738a4e0113 b53a69f4ffcaa5ec9b8660152802f5a7b2effc1f 8d3812daa5b0c9eb534515d37498d0d6e1a40ec8 6a25b6997ff9f99fde309db1e163d16cd70ca5f5 c4193f11d1871051d4cce4521b1f444a083c9189 fbc6cdfbeda4086851dfb0f91fd23b4fd541e174 23d5c909286d438534f1a7defb2faacd1877fea1 7fb2038faca256c03c2bd7d982f39f5b1a57f784 c4f9c372405bea8a7d9c6b39e04cebefa3322a19
Created attachment 17240 [details] git-am fix for 4.16.next. Cherry-picks cleanly from master.
Created attachment 17241 [details] git-am fix for 4.15.next. Mostly cherry-pick cleanly from master, with one (minor) change to back port: s3: cmd_vfs: cmd_set_nt_acl(). All calls to SMB_VFS_FSTAT(fsp, &fsp->fsp_name->st) clobber fsp->fsp_name->st.st_ex_iflags. (original commit removed "int ret;" 4.15.next code still uses 'int ret' for the return value of the SMB_VFS_CLOSE() call.
Apple reports these patches fix the problems they reported.
Reassigning to Jule for inclusion in 4.15 and 4.16.
Pushed to autobuild-v4-{16,15}-test.
This bug was referenced in samba v4-16-test: 96bf06efad9412fd81d39d5fb320c3e0d03d8011 fff4845206ec7157e2fc81f3afa9fcb2305c0790 008999b0cab8098119393486f9774406ad70214e 061c2f52f3027646b36fe82b2ab91288951d536e f1030ba8db34b6e5026d5e78bb99a5cddf26220b f46dad0a2b9c11315c587bbf07b749f41631603e 09bc8b2bb8209c63550aa04ee8cf49b46631b68e c2d6b29cf3aac177bcb4d82cfbbd9b60efbc6220 2d7568cd4151232fed9e5932b339d3a31fd16dba 640b6a01bd8f6467ae134b59c9188344548e5354 386325da318f642dd54f1b41a69e61630ea1a4e1 c48414de71f5bae3bf05324a8ac134b763446b89 6d66132ed267b0b4eb92369d8ec2883f734946c6 bbf4e324f73f1a197eedc69fd584c8ca6b77c980 b88c1f1bc2f8687b0c200afb5d943ba60f1e17e0 c9763e71bc70f2d75aa45c86b8b196c4ceabee88 c8b6ddb08c389567c7afa8de1aeeea314084668a 0f0c12b64fd56f32d19aaa54e20e5b0307e0a30d
This bug was referenced in samba v4-15-test: 8b8c80d15079b0ccda70bc42c9c5e464663f6167 b47077ee7702104514ebc032fefc8b7839b59218 0b3fdccf2c603225ac3f272c74ee677b105d3edc b3eb7cfdce1bdbb1265c310a207c3bc55608a897 59ef773603a6e25ca441d3a4a19f9f05ea44ee07 1e32786854d4bc6f06f67fb1d27ebf46a19163bd 7600f2f0da41fc74805587426c078f17b04cd6b0 ae9e3609129e0854c0a4ec744e67d56904dd4969 e191e40de485915afee71040f51a002b90216e2f e51a120f620bb9f7d6c0f372312f1a3deacc529e 462ccb63241a8043e8e2b128b63acb1f8d3960ad 3905cfe754c9a73fc40644e305732b53b13f0694 df48c0050136e58eb8badd9ca9cdc7f09202fb92 8880efcc4a152b53b630d093332444f37d8113bd 687e9cc84581e930060ecd79236e4801eed476f4 0671b340fbbc00e07412b5f8f8d3bed4f9c53cfb b93079ca2a7a13345f9d9f89ba94df4dddf0aa47 f475832bd2e491dad11d1a7cbd6bc669c12ef7df
Closing out bug report. Thanks!
This bug was referenced in samba v4-15-stable (Release samba-4.15.7): 8b8c80d15079b0ccda70bc42c9c5e464663f6167 b47077ee7702104514ebc032fefc8b7839b59218 0b3fdccf2c603225ac3f272c74ee677b105d3edc b3eb7cfdce1bdbb1265c310a207c3bc55608a897 59ef773603a6e25ca441d3a4a19f9f05ea44ee07 1e32786854d4bc6f06f67fb1d27ebf46a19163bd 7600f2f0da41fc74805587426c078f17b04cd6b0 ae9e3609129e0854c0a4ec744e67d56904dd4969 e191e40de485915afee71040f51a002b90216e2f e51a120f620bb9f7d6c0f372312f1a3deacc529e 462ccb63241a8043e8e2b128b63acb1f8d3960ad 3905cfe754c9a73fc40644e305732b53b13f0694 df48c0050136e58eb8badd9ca9cdc7f09202fb92 8880efcc4a152b53b630d093332444f37d8113bd 687e9cc84581e930060ecd79236e4801eed476f4 0671b340fbbc00e07412b5f8f8d3bed4f9c53cfb b93079ca2a7a13345f9d9f89ba94df4dddf0aa47 f475832bd2e491dad11d1a7cbd6bc669c12ef7df
This bug was referenced in samba v4-16-stable (Release samba-4.16.1): 96bf06efad9412fd81d39d5fb320c3e0d03d8011 fff4845206ec7157e2fc81f3afa9fcb2305c0790 008999b0cab8098119393486f9774406ad70214e 061c2f52f3027646b36fe82b2ab91288951d536e f1030ba8db34b6e5026d5e78bb99a5cddf26220b f46dad0a2b9c11315c587bbf07b749f41631603e 09bc8b2bb8209c63550aa04ee8cf49b46631b68e c2d6b29cf3aac177bcb4d82cfbbd9b60efbc6220 2d7568cd4151232fed9e5932b339d3a31fd16dba 640b6a01bd8f6467ae134b59c9188344548e5354 386325da318f642dd54f1b41a69e61630ea1a4e1 c48414de71f5bae3bf05324a8ac134b763446b89 6d66132ed267b0b4eb92369d8ec2883f734946c6 bbf4e324f73f1a197eedc69fd584c8ca6b77c980 b88c1f1bc2f8687b0c200afb5d943ba60f1e17e0 c9763e71bc70f2d75aa45c86b8b196c4ceabee88 c8b6ddb08c389567c7afa8de1aeeea314084668a 0f0c12b64fd56f32d19aaa54e20e5b0307e0a30d