Note: "permalinks" may not be as permanent as we would like,
direct links of old sources may well be a few messages off.
> -----Original Message-----
> From: drbd-user-bounces at lists.linbit.com [mailto:drbd-user-
> bounces at lists.linbit.com] On Behalf Of Lars Ellenberg
> Sent: Saturday, October 14, 2017 1:05 PM
> To: drbd-user at lists.linbit.com
> Subject: Re: [DRBD-user] Warning: Data Corruption Issue Discovered in DRBD
> 8.4 and 9.0
>
> On Thu, Oct 12, 2017 at 11:14:55AM +0200, Robert Altnoeder wrote:
> > On 10/11/2017 11:30 PM, Eric Robinson wrote:
> > > The TrimTester program consists of three parts. The main executable
> > > (TrimTester) just writes loads of data to the drive and tests for
> > > file corruption. My C++ consultant says, "It writes sequential
> > > numbers wrapped at 256, spanning multiple files. It checks
> > > previously written files, and if the file data is all zeroes, it is
> > > considered to be corrupted."
> > Are you referring to this program?
> > https://github.com/algolia/trimtester/blob/master/trimtester.cpp
> >
> > One thing that I can tell you right away is that this program does not
> > appear to be very trustworthy, because it may malfunction due to the
> > use of incorrect datatypes for the purpose - apparently, it is
> > attempting to memory-map quite large files (~ 70 GiB) and check using
> > a byte-indexed offset declared as type 'unsigned', which is commonly
> > only 32 bits wide, and therefore inadequate for the byte-wise indexing
> > of anything that is larger than 4 GiB.
>
> The test program has other issues as well, like off-by-one (and thus stack
> corruption) when initializing the "buffer" in its "writeAtomically", unlinking
> known non-existent files, and other things.
> Probably harmless.
>
> But this 32bit index vs >= 4 GiByte file content is the real bug here, thank you
> Robert for pointing that out.
>
> Why it does not trigger if DRBD is not in the stack I cannot tell, maybe the
> timing is just strangely scewed, and somehow your disk fills up and
> everything terminates before the "DetectCorruption" thread tries to check a
> >= 4GiB file for the first time.
>
> Anyways: what happens is:
>
> void _checkFile(const std::string &path, const char *file, std::string
> &filename) {
> filename.resize(0);
> filename.append(path);
> filename.push_back('/');
> filename.append(file);
> MMapedFile mmap(filename.c_str());
> if (mmap.loaded()) {
> bool corrupted = false;
> // Detect all 512-bytes page inside the file filled by 0 -> can be caused
> by a buggy Trim
> for (unsigned i = 0; !corrupted && i < mmap.len(); i += 512) {
>
> // after some number of iterrations,
> // i = 4294966784, 2 ** 32 - 512;
> // mmap.len however is *larger*.
> // in the "i << mmpa.len()", the 32bit integer i is "upscaled", // size-
> extended, before the comparison, so that remains true.
>
> if (mmap.len() - i > 4) { // only check page > 4-bytes to avoid false
> positive
>
> // again, size-extension to 64bit, condition is true
>
> bool pagecorrupted = true;
>
> // *assume* that the "page" was corrupted,
>
> for (unsigned j = i; j < mmap.len() && j < (i + 512); ++j) {
>
> // j = i, which is j = 4294966784, (i << mmap.len) is again true because // of
> the size-extension of i to 64bit in that term, // but for the (j < i+ 512) term,
> neither j nor i is size-extended, // i + 512 wraps to 0, j < 0 is false, // loop will
> not execute even once, // which means no single byte is checked
>
> if (mmap.content()[j] != 0)
> pagecorrupted = false;
> }
> if (pagecorrupted)
> corrupted = true;
>
> // any we "won" a "corrupted" flag by simply "assuming"
> // no bytes are bad bytes.
> // "So sad." ;-)
>
> }
> }
> if (corrupted) {
> std::cerr << "Corrupted file found: " << filename << std::endl;
> exit(1);
> }
>
> }
> }
>
>
> Just change "unsigned" to "uint64_t" there, and be happy.
>
>
> Don't believe it?
> Create any file of 4 GiB or larger,
> make sure it does not contain 512 (aligned) consecutive zeros, and "check" it
> for "corruption" with that logic of trimtester.
> It will report that file as corrupted each time.
>
>
> rm -rf trimtester-is-broken/
> mkdir trimtester-is-broken
> o=trimtester-is-broken/x1
> echo X > $o
> l=$o
> for i in `seq 2 32`; do
> o=trimtester-is-broken/x$i;
> cat $l $l > $o ;
> rm -f $l;
> l=$o;
> done
> ./TrimTester trimtester-is-broken
>
> Wahwahwa Corrupted file found: trimtester-is-broken/x32 mimimimi
>
>
> Thanks,
> that was a nice excercise in proofreading cpp code.
>
> --
> : Lars Ellenberg
> : LINBIT | Keeping the Digital World Running
> : DRBD -- Heartbeat -- Corosync -- Pacemaker
>
Well, damn. As the program was supposedly reviewed by Samsung engineers as part of their efforts to diagnose the root cause of TRIM errors, it never occurred to me that it was that buggy. I can't thank you enough for finding that! The rollout of some new DRBD clusters has been on hold for 2+ months while we've been trying to track this down.
--Eric