yashsingh1/xdaforums huggingface dataset:
[General Omni Discussion] Lanchon:
it seems like the same bug.

its an interesting start. i was so sold on the kernel bug idea, the bug being there before kitkat, and sdcard.c being ok in other SoCs, that i didn't even look for bugs in sdcard.c.

however i now did, and i don't see a bug.

the hypothesis:

the declaration "const struct fuse_in_header* hdr" lets the compiler assume *hdr is constant, and thus can cache its value or delay reading its value any way it sees fit.

the compiler optimizes:

__u64 unique = hdr->unique;

pread64(...)

fuse_reply(...unique...)

...to:

pread64(...)

fuse_reply(...hdr->unique...)

...across the pread64() call because *hdr is constant.

problems:

1) this would be a compiler bug, not an sdcard.c bug. the compiler cannot assume *hdr is constant just because hdr is declared const T*. there could be non-const aliases, other ways to access *hdr (by name, references or pointers) that permit its modification. see here:

http://www.compileroptimizations.com/category/alias_optimization_const_qualifier.htm

pread64() cannot be inlined, so it must be assumed that it can potentially modify *hdr.

even if handle_read and its callers are inlined, the compiler cannot prove that *hdr is constant simply because it is not: hdr is not initialized to point to a const object.

2) more importantly, handle_read compiled code would not be 'designed' to function differently if there is an interrupt. the code would always read unique from *hdr, always getting a clobbered value. and that is not happening.

well... that is, unless most of the reads are too short to clobber. but this is not the case as we see here:

http://xdaforums.com/showthread.php?p=51237856

so the hypothesis is wrong. something else is going on that i dont understand. your fix could be working just by chance.

for starters, lets see if simply resizing the struct kills the bug.

change:

/* To save memory, we never use the contents of the request buffer and the read

* buffer at the same time. This allows us to share the underlying storage. */

union {

__u8 request_buffer[MAX_REQUEST_SIZE];

__u8 read_buffer[MAX_READ];

};

to:

/* To save memory, we never use the contents of the request buffer and the read

* buffer at the same time. This allows us to share the underlying storage. */

union {

__u8 request_buffer[MAX_REQUEST_SIZE];

__u8 read_buffer[MAX_READ];

};

__u8 dummy_read_buffer[MAX_READ];

and see if the bug is still there.

(sorry i dont have a device to test.)

---------- Post added at 08:32 AM ---------- Previous post was at 08:27 AM ----------

i don't have the tools.

could someone please disassemble handle_read so we can see where unique is kept across pread64()?
[General Omni Discussion] zeitferne:
if someone could read ARM assembler

Now it would be really helpful if someone could read ARM assembler… Or at least show a way to produce a decently readable listing.

Without being able to do so I can only speculate: A compiler bug is not totally unlikely, especially since flags such as -mtune=cortex-a9 are used which might lead to optimizations being made that are not that well tested. Still, interrupt handling should be the same for FAT32 and ext4 (maybe not for exFAT which supposedly uses another FUSE layer) so why would using ext4 make this bug go away? (That being said, I've only tested exFAT myself, not ext4.)

If it's a compiler bug, the most elegant solution would of course be to get the GCC devs to fix this. ;)

Or when you're more concerned with efficiency than with elegance, since both buffers have the same type, simply use only one name for the buffer instead of two, removing the union (warning, this is totally just from the top of my head), i.e. #define MAX_HANDLER_BUF_SIZE (maximum of the old MAX_REQUEST_SIZE and MAX_READ) and replace request_buffer and read_buffer with a simply generically named buffer and use that everywhere in place of the old names. Should be semantically equivalent.
[General Omni Discussion] AndDiSa:
There is one interesting thing to note: comparing sdcard.c from omnirom/cm with sdcard.c from AOSP, there is is the following difference:

...
static bool int_equals(void *keyA, void *keyB) {
@@ -232,7 +232,7 @@ struct fuse_handler {
* buffer at the same time. This allows us to share the underlying storage. */
union {
__u8 request_buffer[MAX_REQUEST_SIZE];
- __u8 read_buffer[MAX_READ];
+ __u8 read_buffer[MAX_READ + PAGESIZE];
};
};
...

probably the buffersize is just too small.
[General Omni Discussion] zeitferne:
Corresponding commit (from 2014-02-14) also does some more (partly) obscure changes: sdcard: direct I/O file access fix:

It seems that there have been more changes to sdcard last month, as the log indicates.

"will not work" is not the most detailed description, but with some fantasy… I'm wondering, however, if O_DIRECT is used in the problematic cases. If so, this could very well be related. The next commit's message seems to indicate that it is a well behaved "will not work", namely writes returning EINVAL.
[General Omni Discussion] AndDiSa:
It's not only since last month. I replaced sdcard.c from cm/omni by sdcard.c from AOSP on my builds already in March.
[General Omni Discussion] Lanchon:
there is nothing obscure. linux man pages state that read and write buffers, file offset, etc need to be aligned under O_DIRECT. reads are always aligned because it's basically free (MAX_REQUEST_SIZE is much larger than MAX_READ + PAGESIZE). writes need a memcpy and alignment is conditional. i see nothing strange.

---------- Post added at 03:44 AM ---------- Previous post was at 03:41 AM ----------

why are omni and cm not pulling these changes? i went through all and there are important bugfixes. is it normal that they dont pull bugfixes?
[General Omni Discussion] GidiK:
I tried that code, the problem still exists. Ran it twice to be sure... So it's not the buffer size.

But it's a good thing you reminded me of those earlier posts in this thread, where a certain commit in AOSP's sdcard.c (this) was mentioned that [MENTION=591147]Entropy512[/MENTION] suspected could have fixed this bug. I retested that change (that makes sure the read buffer starts at page boundaries) and the problem is fixed. I ran this ten times...

I tried many other variations, with putting in a dummy buffer before the read buffer to prevent the overlap. But the conclusions of that were that even a dummy buffer much larger than the request buffers didn't stop the problem, just made it less likely; only when the dummy buffer was PAGESIZE, the problem disappeared. It turns out that for us it is not needed to really start on page boundaries, but the real trick is that we need the read buffer to start on the next page.

And this AOSP change does that. So I propose to cherry pick that change. With that I'm quite sure at least the problem I'm measuring is gone.
[General Omni Discussion] Lanchon:
IIRC that commit was tested NOT to solve the issue. it's back on this thread.

so you say the buffer address sent to pread64 determines whether the bug is detected or not... first we need to verify that this is true. for example, with code like this:

static volatile int test_offset = 0; // or PAGESIZE

static int handle_read(struct fuse* fuse.... {
.....

res = pread64(h->fd, handler->read_buffer + test_offset, size, offset);
if (res < 0) {
return -errno;
}
fuse_reply(fuse, unique, handler->read_buffer + test_offset, res);

max read is much smaller than the buffer size so you can safely play with test_offset within reason.

all else being equal, if different values of test_offset cause different behavior, we are on our way to proving your address hypothesis. but we need a way to initialize the variable from a config file or property to be sure that nothing else changes. (different literal values are not enough.) i'll leave this to you cause i don't really know android and the simplest way to do it.

maybe we are dealing with a mem transfer race condition. im imagining a race condition in DMA setup, in which some bytes are written elsewhere in ram instead of to their proper destination. they are scattered around in a way that depends on the intended destination... if DMA is used at all here.

btw this makes you think that just copying files might corrupt data now and then. in fact just reading files repeatedly would trigger the bug. (calculating MD5s of big files repeatedly is a simple test to run on stock devices.)

---------- Post added at 05:07 AM ---------- Previous post was at 04:33 AM ----------

i disassembled CM11-M10 sdcard.c, haven't looked at it yet, it's here:

https://gist.github.com/Lanchon/f80460e416aac37e5569
[General Omni Discussion] zeitferne:
I only meant that since nine weeks ago, quite a few commits were made, although that was not last month either but two months ago (three now, since it's October :) ).

I was referring to this line, doing bit-operations on pointers:

__u8 *read_buffer = (__u8 *) ((uintptr_t)(handler->read_buffer + PAGESIZE) & ~((uintptr_t)PAGESIZE-1));

Although I'm sure that when you read the corresponding documentation it will become clear.

I would also prefer to just merge all changes: It's really all bugfixes (except maybe for "Make sdcard log to the log rather than stderr. " which is useful for debugging).

At the very least, when picking that "sdcard: direct I/O file access fix" commit, the next one ("sdcard: direct I/O file access write buffer alignment") is a fix for that and should also be included.

As a side note, I wonder why CyanogenMod's sdcard: Respect check_caller_access_to_name on readdir() calls or an equivalent fix seems not to be included in AOSP.

And since the struct's size seems to matter, could the AOSP commit Use the correct fuse_init_out structure size. be related?
[General Omni Discussion] Lanchon:
nothing strange. adds a page size to the address, then zero out all address bits that are offsets within a page and leaves the higher bits (ie page number) intact. in effect selects the start address of the next page.

it'd be more "correct" to do (X + (PS-1)) & ~(PS-1)

this would return X (instead of the current X+PS) if X is already at the start of a page

but for this case, both results are equally valid

---------- Post added at 06:13 AM ---------- Previous post was at 06:09 AM ----------

maybe they didn't upstream?

---------- Post added at 06:14 AM ---------- Previous post was at 06:13 AM ----------

no. that's an important commit but the struct is only used in handle_init.

---------- Post added at 06:47 AM ---------- Previous post was at 06:14 AM ----------

so i'm looking at the disassembly code. its a bit of a mess due to heavy inlining and optimizations. plus not knowing the instructions, addressing modes nor calling conventions also slows me down.

i'm seeing something so far compatible with a compiler bug, but i need to see more code to confirm or disprove. unfortunately i'll have to leave it for later because this is not what i should be doing right now.

the problem i'm having is that your observations contradict this bug, so i want to make sure i understood you correctly. you said "I tried many other variations, with putting in a dummy buffer before the read buffer to prevent the overlap. But the conclusions of that were that even a dummy buffer much larger than the request buffers didn't stop the problem". how exactly did you do this? did you do something like this:

union {
__u8 request_buffer[MAX_REQUEST_SIZE];
struct {
__u8 dummy[...];
__u8 read_buffer[MAX_READ];
};
};

or like this:

union {
__u8 request_buffer[MAX_REQUEST_SIZE];
__u8 dummy[...];
__u8 read_buffer[MAX_READ];
};

of course the second option would have no effect whatsoever. sorry to ask this question, sometimes people just make simple mistakes, i need to rule this out.
[General Omni Discussion] GidiK:
I'm quite prone to simple mistakes, but in this case I used the first option, with struct inside the union.

As the size for the dummy I took the size of the two request buffers added together (this failed somewhere in the second run if I remember correctly), 8 times these request buffer sizes (this failed at the end of the sixth run, from my memory), the buffer sizes + PAGESIZE (this didn't fail). One run is reading two nandroids at full speed. The read speed is around 16MB/s. I also did a variation where I used just the AOSP commit, but added "13" to the calculated read buffer address to force a misalignment (but still in the next page): this also didn't fail.

Without any modifications the test usually fails somewhere in the first run, after having read some GB already. I observe nothing in common happening about when it fails, though it is on a live phone; in fact, with the same build it tends to fail around the same moment if you reboot and try again.

The missing "-1" in the AOSP commit actually quite helps us...
[General Omni Discussion] Lanchon:
yes, it forces the "next page" thing you are so fond of :)

thanks for the clarifications
[General Omni Discussion] Lanchon:
CM11-M10 sdcard.c (hammerhead)

source:

https://github.com/CyanogenMod/andr...d078e477baba9583544d9b26b00f9/sdcard/sdcard.c

gcc command:

prebuilts/gcc/linux-x86/arm/arm-linux-androideabi-4.7/bin/arm-linux-androideabi-gcc -I system/core/sdcard -I /home/XXX/android/system/out/target/product/hammerhead/obj/STATIC_LIBRARIES/libsdcard_intermediates -I libnativehelper/include/nativehelper -isystem system/core/include -isystem hardware/libhardware/include -isystem hardware/libhardware_legacy/include -isystem hardware/ril/include -isystem libnativehelper/include -isystem frameworks/native/include -isystem frameworks/native/opengl/include -isystem frameworks/av/include -isystem frameworks/base/include -isystem external/skia/include -isystem /home/XXX/android/system/out/target/product/hammerhead/obj/include -isystem device/lge/hammerhead/kernel-headers -isystem hardware/qcom/msm8x74/kernel-headers -isystem bionic/libc/arch-arm/include -isystem bionic/libc/include -isystem bionic/libstdc++/include -isystem bionic/libc/kernel/common -isystem bionic/libc/kernel/arch-arm -isystem bionic/libm/include -isystem bionic/libm/include/arm -isystem bionic/libthread_db/include -c -fno-exceptions -Wno-multichar -msoft-float -fpic -fPIE -ffunction-sections -fdata-sections -funwind-tables -fstack-protector -Wa,--noexecstack -Werror=format-security -D_FORTIFY_SOURCE=2 -fno-short-enums -mcpu=cortex-a15 -mfloat-abi=softfp -mfpu=neon -include build/core/combo/include/arch/linux-arm/AndroidConfig.h -I build/core/combo/include/arch/linux-arm/ -Wno-unused-but-set-variable -fno-builtin-sin -fno-strict-volatile-bitfields -Wno-psabi -mthumb-interwork -DANDROID -fmessage-length=0 -W -Wall -Wno-unused -Winit-self -Wpointer-arith -DNO_SECURE_DISCARD -Werror=return-type -Werror=non-virtual-dtor -Werror=address -Werror=sequence-point -DNDEBUG -g -Wstrict-aliasing=2 -fgcse-after-reload -frerun-cse-after-loop -frename-registers -DNDEBUG -UDEBUG -mthumb -Os -fomit-frame-pointer -fno-strict-aliasing -Wall -Wno-unused-parameter -MD -MF /home/XXX/android/system/out/target/product/hammerhead/obj/STATIC_LIBRARIES/libsdcard_intermediates/sdcard.d -o /home/XXX/android/system/out/target/product/hammerhead/obj/STATIC_LIBRARIES/libsdcard_intermediates/sdcard.o system/core/sdcard/sdcard.c​

listing:

https://gist.github.com/Lanchon/9bbd717f96346c7e15b0

objdump:

https://gist.github.com/Lanchon/8bb37d20e493c75e10b4
[General Omni Discussion] Lanchon:
bug (sort of) found!

fuse_reply receives unique in r2,r3 and stores it.

inlined handle_read, after calling pread64, restores unique into r2,r3 from a double-precision floating-point register before invoking fuse_reply.

because of two levels of inlining, unique is actually saved in handle_fuse_requests and the intermediate redundant save of handle_read is optimized away.

the interrupts are clobbering the VFP registers, not the stack!
[General Omni Discussion] patrickph:
Great news, dose this mean the bug is real fixed? No more work around?
[General Omni Discussion] Lanchon:
well its a start!

i did the analysis on the code generated for my phone, hammerhead. i need to repeat it for the i9100 when i have time.

we can now build sdcard or the whole system so that it wont be affected by the corruption, but still 3rd party apps will. so we need to find what interrupt is clobbering the FP regs and fix it.

meanwhile, i'll share an interesting tidbit:
[General Omni Discussion] Lanchon:
CM11-M10 sdcard.c (i9100)

source:

https://github.com/CyanogenMod/andr...d078e477baba9583544d9b26b00f9/sdcard/sdcard.c

gcc command:

prebuilts/gcc/linux-x86/arm/arm-linux-androideabi-4.7/bin/arm-linux-androideabi-gcc -I device/samsung/galaxys2-common/include -I system/core/sdcard -I /home/XXX/android/system/out/target/product/i9100/obj/STATIC_LIBRARIES/libsdcard_intermediates -I libnativehelper/include/nativehelper -isystem system/core/include -isystem hardware/libhardware/include -isystem hardware/libhardware_legacy/include -isystem hardware/ril/include -isystem libnativehelper/include -isystem frameworks/native/include -isystem frameworks/native/opengl/include -isystem frameworks/av/include -isystem frameworks/base/include -isystem external/skia/include -isystem /home/XXX/android/system/out/target/product/i9100/obj/include -isystem bionic/libc/arch-arm/include -isystem bionic/libc/include -isystem bionic/libstdc++/include -isystem bionic/libc/kernel/common -isystem bionic/libc/kernel/arch-arm -isystem bionic/libm/include -isystem bionic/libm/include/arm -isystem bionic/libthread_db/include -c -fno-exceptions -Wno-multichar -msoft-float -fpic -fPIE -ffunction-sections -fdata-sections -funwind-tables -fstack-protector -Wa,--noexecstack -Werror=format-security -D_FORTIFY_SOURCE=2 -fno-short-enums -mcpu=cortex-a9 -mfloat-abi=softfp -mfpu=neon -include build/core/combo/include/arch/linux-arm/AndroidConfig.h -I build/core/combo/include/arch/linux-arm/ -Wno-unused-but-set-variable -fno-builtin-sin -fno-strict-volatile-bitfields -Wno-psabi -mthumb-interwork -DANDROID -fmessage-length=0 -W -Wall -Wno-unused -Winit-self -Wpointer-arith -DEXYNOS4_ENHANCEMENTS -DEXYNOS4210_ENHANCEMENTS -DDISABLE_HW_ID_MATCH_CHECK -DFORCE_SCREENSHOT_CPU_PATH -DWORKAROUND_BUG_10194508 -DHAVE_ISO -DSAMSUNG_CAMERA_HARDWARE -Werror=return-type -Werror=non-virtual-dtor -Werror=address -Werror=sequence-point -DNDEBUG -g -Wstrict-aliasing=2 -fgcse-after-reload -frerun-cse-after-loop -frename-registers -DNDEBUG -UDEBUG -mthumb -Os -fomit-frame-pointer -fno-strict-aliasing -Wall -Wno-unused-parameter -MD -MF /home/XXX/android/system/out/target/product/i9100/obj/STATIC_LIBRARIES/libsdcard_intermediates/sdcard.d -o /home/XXX/android/system/out/target/product/i9100/obj/STATIC_LIBRARIES/libsdcard_intermediates/sdcard.o system/core/sdcard/sdcard.c​

listing:

https://gist.github.com/Lanchon/1996581ebd30c3a2c877

objdump:

https://gist.github.com/Lanchon/c8599c7a4535e8420803

---------- Post added at 02:21 AM ---------- Previous post was at 02:08 AM ----------

bug find confirmed!

the build for i9100 saves and restores unique in the same way as the previously commented hammerhead build.

bug confirmed to be clobbering the FP registers.
[General Omni Discussion] Lanchon:
CM11-M10 Music Bug Fix

this is just sdcard.c compiled with "-mfloat-abi=soft" instead of "-mfloat-abi=softfp". disassembly confirms that regular instead of FP registers are used to temporarily save unique.

sdcard.c is usually left alone in roms. so although this fix is for CM11-M10, it probably works fine on all kitkat roms.

[MENTION=5054150]GidiK[/MENTION], could you please test? just flash from recovery.

thank you!
[General Omni Discussion] Lanchon:
[MENTION=591147]Entropy512[/MENTION], now that we know that something (interrupts? context switches?) is clobbering the FP registers, you have any idea of where to go?

my problem is that i dont know the first thing about arm arch, linux kernel, or android.

compiling all of android with "-mfloat-abi=soft" is out of the question. plus it wouldnt help the asm parts or 3rd party apps.

i thought of a tool to instrument arm ISRs: prolog to save FP regs on entry, epilog to verify that FP regs were undisturbed or properly restored by the ISR. i googled it but instead i found that 1) ISRs dont use FP, and 2) the kernel doesnt use FP. so there's no such tool.

code can inadvertently use FP regs by using lib functions such as memcpy(), memcmp() or memset(), but those should not be used from ISRs or the kernel anyway.

maybe some driver or part of the kernel is being erroneously compiled with "-mfloat-abi=softfp"? can you think of any reason why FP regs would get clobbered in 4210 but not 4412? different drivers? different libs? different compile options?

it would be trivial to make a clobber detector in userland. the detector would trigger immediately, as soon as it's scheduled again. is there a way we can find out which was the last ISR run? (of course there are races, but this is informative.) is there a way to log ISR invocation times and then match them to clobbering times?

are there closed-source ISRs in our kernel? what about drivers?

is there a way to instrument ISRs or otherwise install ISR wrappers in linux? we could detect clobbering, or save/restore FP regs in the wrapper even for closed-source ISRs.

how about instrumenting driver entry points with similar motives?

there must be some kind of tool that can help.

thanks!
[General Omni Discussion] GidiK:
All great, great work, thank you! I can't test until tomorrow evening; maybe someone else can.
[General Omni Discussion] jnetode:
I did!

Should it really work so easilly?

I flashed on recovery and restarted. Than I made copies from internal to external SDCard, which always gets stuck.

I copied about 4GB and no hangs.

I mounted the SDCard on PC and also copied about 1GB, it would also get stuck.

It worked flawlessly too.
[General Omni Discussion] Dasmikko:
Going to try it too later!

UPDATE #1:

Flashed it, everything boots as usual.

Will listen to music for the next 3 hours and see if it hangs.
[General Omni Discussion] j1gga84:
Will it also work on AOSP based ROMs ? Or will I get problems when I try this patch?

Regards
[General Omni Discussion] Lanchon:
i imagine it will work. take a nandroid and try. (suffices to take a nandroid of just /system.)
[General Omni Discussion] nex-54:
How can i apply this patch?

Just copy sdcard to \system\bin or i need something else?
[General Omni Discussion] Zile995:
Read...
[General Omni Discussion] Lanchon:
there is a widespread problem in the platform. this only shields from it the fuse process that usually hangs, so only that particular process will be ok.

in fact not even that: none of the libraries that the process uses are shielded, so the process is not really safe. but in practice, only userland code (kernel should not use FP regs, mostly) and only floating point calculations, or some uses of long ints, or functions with spilling (too many) registers should be affected. the fuse process probably uses none of these things in the libraries, and will probably be in the clear even though the libs are not shielded.

this is stopgap. we need to fix the platform.

---------- Post added at 12:17 PM ---------- Previous post was at 12:16 PM ----------

yes, you can just copy sdcard. flashing the zip does the same thing.