Bug 15022 - Durable handles won't reconnect if the leased file is written to.
Summary: Durable handles won't reconnect if the leased file is written to.
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: File services (show other bugs)
Version: 4.13.7
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Jule Anger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks: 15035 15038
  Show dependency treegraph
 
Reported: 2022-03-18 00:28 UTC by Jeremy Allison
Modified: 2022-05-02 09:47 UTC (History)
2 users (show)

See Also:


Attachments
Core of the fix for master. (18.27 KB, patch)
2022-03-18 19:32 UTC, Jeremy Allison
no flags Details
git-am fix for 4.16.next. (32.20 KB, patch)
2022-03-24 17:48 UTC, Jeremy Allison
slow: review+
Details
git-am fix for 4.15.next. (31.91 KB, patch)
2022-03-24 17:51 UTC, Jeremy Allison
slow: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Allison 2022-03-18 00:28:07 UTC
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.
Comment 1 Ralph Böhme 2022-03-18 04:53:15 UTC
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?
Comment 2 Jeremy Allison 2022-03-18 16:57:47 UTC
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.
Comment 3 Jeremy Allison 2022-03-18 17:21:52 UTC
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.
Comment 4 Ralph Böhme 2022-03-18 17:42:37 UTC
One innocent call to SMB_VFS_FSTAT() somewhere is enough to clobber it.
Comment 5 Jeremy Allison 2022-03-18 18:23:56 UTC
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.
Comment 6 Jeremy Allison 2022-03-18 19:32:23 UTC
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.
Comment 7 Jeremy Allison 2022-03-18 19:34:33 UTC
Just making sure Ralph gets a heads-up on this :-).
Comment 8 Jeremy Allison 2022-03-18 21:09:38 UTC
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.
Comment 9 Jeremy Allison 2022-03-19 01:24:21 UTC
MR is:

https://gitlab.com/samba-team/samba/-/merge_requests/2448

Ralph, I assigned it to you as you've been following along already :-).
Comment 10 Samba QA Contact 2022-03-24 17:22:08 UTC
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
Comment 11 Jeremy Allison 2022-03-24 17:48:39 UTC
Created attachment 17240 [details]
git-am fix for 4.16.next.

Cherry-picks cleanly from master.
Comment 12 Jeremy Allison 2022-03-24 17:51:03 UTC
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.
Comment 13 Jeremy Allison 2022-03-25 21:48:25 UTC
Apple reports these patches fix the problems they reported.
Comment 14 Ralph Böhme 2022-04-05 17:41:29 UTC
Reassigning to Jule for inclusion in 4.15 and 4.16.
Comment 15 Jule Anger 2022-04-11 07:42:51 UTC
Pushed to autobuild-v4-{16,15}-test.
Comment 16 Samba QA Contact 2022-04-11 08:51:04 UTC
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
Comment 17 Samba QA Contact 2022-04-11 09:22:12 UTC
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
Comment 18 Jule Anger 2022-04-11 11:19:55 UTC
Closing out bug report.

Thanks!
Comment 19 Samba QA Contact 2022-04-26 14:45:30 UTC
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
Comment 20 Samba QA Contact 2022-05-02 09:47:50 UTC
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