Bug 13130 - smbd on disk file corruption bug under heavy threaded load.
smbd on disk file corruption bug under heavy threaded load.
Status: RESOLVED FIXED
Product: Samba 4.1 and newer
Classification: Unclassified
Component: File services
unspecified
All All
: P1 critical
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2017-11-10 22:07 UTC by Jeremy Allison
Modified: 2017-11-16 17:14 UTC (History)
5 users (show)

See Also:


Attachments
Instrumentation of tevent that allowed Volker to find the bug. (1.14 KB, patch)
2017-11-10 22:10 UTC, Jeremy Allison
no flags Details
git-am fix submitted to master. (1.89 KB, patch)
2017-11-10 22:22 UTC, Jeremy Allison
no flags Details
git-am fix for 4.7.next. (2.06 KB, patch)
2017-11-11 03:44 UTC, Jeremy Allison
vl: review+
Details
git-am fix for 4.6.next (2.06 KB, patch)
2017-11-11 03:45 UTC, Jeremy Allison
vl: review+
Details
tevent-0.9.34 for v4-7-test (20.53 KB, patch)
2017-11-13 22:14 UTC, Stefan Metzmacher
jra: review+
metze: review? (vl)
Details
tevent-0.9.34 for v4-6-test (60.68 KB, patch)
2017-11-13 22:32 UTC, Stefan Metzmacher
jra: review+
metze: review? (vl)
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Allison 2017-11-10 22:07:33 UTC
A race condition in the tevent threaded code can cause tevent_common_wakeup_fd(tctx->wakeup_fd) to be called where wakeup_fd is being read out of reused memory unprotected by mutex lock (tctx may no longer exist).

If the reused memory happens to point to a wakeup_fd value that matches a file descriptor in use by smbd to write to a on-disk file, file corruption will result.

Found and fixed by Volker Lendecke <vl@samba.org>.
Comment 1 Jeremy Allison 2017-11-10 22:10:31 UTC
Created attachment 13766 [details]
Instrumentation of tevent that allowed Volker to find the bug.

This patch isn't part of the fix, I'm including it here mainly to help other who may have to debug similar issues in the future.
Comment 2 Jeremy Allison 2017-11-10 22:22:23 UTC
Created attachment 13767 [details]
git-am fix submitted to master.

NB. This already has been reviewed by <jra@samba.org>
Comment 3 Jeremy Allison 2017-11-11 03:44:31 UTC
Created attachment 13768 [details]
git-am fix for 4.7.next.

Cherry-pick from master.
Comment 4 Jeremy Allison 2017-11-11 03:45:05 UTC
Created attachment 13769 [details]
git-am fix for 4.6.next

Back-port of cherry-pick from master.
Comment 5 Volker Lendecke 2017-11-11 14:34:02 UTC
Hi, Karolin!

Unfortunately I believe this makes an emergency interim release necessary. Sorry for that, but data corruption is really, really bad.

Volker
Comment 6 Jeremy Allison 2017-11-11 17:28:37 UTC
Yes, I'm afraid I concur with Volker. This can cause file corruption, and the *single* thing file servers must do is not corrupt files. So this is more important than a security release IMHO. Sorry :-(.
Comment 7 Karolin Seeger 2017-11-13 07:32:47 UTC
(In reply to Volker Lendecke from comment #5)
Ok, thanks.
Should we add all fixes available up to now or only this one?
Comment 8 Volker Lendecke 2017-11-13 08:03:29 UTC
If you ask me, I'd vote for "just this patch" to make adoption easier for people.
Comment 9 Volker Lendecke 2017-11-13 08:04:39 UTC
On the other hand, this might be considerable effort in the release branches, right? So I don't want to put additional burden on you.
Comment 10 Karolin Seeger 2017-11-13 08:28:14 UTC
Pushed to autobuild-v4-{6,7}-test.
Comment 11 Stefan Metzmacher 2017-11-13 09:59:08 UTC
We need to backport full tevent releases, otherwise someone could still build against a broken system libtevent.

I'm preparing tevent 0.9.34 and push it to autobuild shortly.
Comment 12 Ralph Böhme 2017-11-13 10:04:25 UTC
(In reply to Stefan Metzmacher from comment #11)
Thanks metze and sorry for not spotting this!
Comment 13 Karolin Seeger 2017-11-13 10:43:57 UTC
(In reply to Volker Lendecke from comment #9)
That's not the problem.
I checked the (very few) commits since the last bugfix releases and think it makes sense to ship the other fixes also. 
Waiting for the tevent release and the adapted patch for Samba before preparing the releases.
Comment 14 Jeremy Allison 2017-11-13 18:21:55 UTC
Yes, I realized we'll need a tevent new release as well just after posting the patches.
Comment 15 Stefan Metzmacher 2017-11-13 22:14:24 UTC
Created attachment 13777 [details]
tevent-0.9.34 for v4-7-test
Comment 16 Stefan Metzmacher 2017-11-13 22:32:09 UTC
Created attachment 13778 [details]
tevent-0.9.34 for v4-6-test
Comment 17 Ralph Wuerthner 2017-11-14 06:41:46 UTC
The bug description above states

   Product: Samba 4.1 and newer

I was under the impression this only applies to Samba 4.6 and newer?
Comment 18 Volker Lendecke 2017-11-14 08:25:52 UTC
(In reply to Ralph Wuerthner from comment #17)
> The bug description above states
> 
>    Product: Samba 4.1 and newer
> 
> I was under the impression this only applies to Samba 4.6 and newer?

We don't have products for individual Samba versions anymore. To the best of my knowledge, this bug does not apply to any other version of Samba than 4.6 and 4.7. Do you have a reproducer of the issue in any other version? Please send me instructions how to reproduce it with 4.5 and older.

Thanks!
Comment 19 Karolin Seeger 2017-11-14 09:38:47 UTC
Pushed tevent patches to autobuild-v4-{7,6}-test.
Comment 20 Karolin Seeger 2017-11-15 11:34:59 UTC
(In reply to Karolin Seeger from comment #19)
Pushed to both branches.
Closing out bug report.

Thanks!