* e2fsprogs alignment issues @ 2012-07-11 13:20 Gordan Bobic 2012-07-11 15:04 ` Eric Sandeen 0 siblings, 1 reply; 8+ messages in thread From: Gordan Bobic @ 2012-07-11 13:20 UTC (permalink / raw) To: linux-ext4 I would like to bring the following bug report to people's attention: https://bugzilla.redhat.com/show_bug.cgi?id=680090 The issue is that the code used in e2fsprogs WRT allocating unaligned buffers and then casting them into structs is non-portable and dangerous - eyewateringly dangerous in something like e2fsprogs where it can lead to thorough trashing of the file system. Can something be done to improve the portability of the userspace code? The issue has been discussed on the Fedora-ARM mailing list: http://lists.fedoraproject.org/pipermail/arm/2012-July/003637.html but the argument there was that the issue needs to be fixed upstream, hence why I am posting it here. This is an issue on all ARMs up to ARMv5, possibly ARMv6, and likely other CPU architectures that don't have transparent alignment fixup in hardware (Itanium, SPARC, probably others). Apart from being dangerous, using unaligned arrays also affects performance because it leads to data needlessly straddling cache lines in the CPU. But that doesn't really matter in this case. Many thanks. Gordan ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: e2fsprogs alignment issues 2012-07-11 13:20 e2fsprogs alignment issues Gordan Bobic @ 2012-07-11 15:04 ` Eric Sandeen 2012-07-11 20:05 ` Theodore Ts'o 2012-07-11 21:24 ` Gordan Bobic 0 siblings, 2 replies; 8+ messages in thread From: Eric Sandeen @ 2012-07-11 15:04 UTC (permalink / raw) To: Gordan Bobic; +Cc: linux-ext4 On 7/11/12 8:20 AM, Gordan Bobic wrote: > I would like to bring the following bug report to people's > attention: > > https://bugzilla.redhat.com/show_bug.cgi?id=680090 > > The issue is that the code used in e2fsprogs WRT allocating unaligned > buffers and then casting them into structs is non-portable and > dangerous - eyewateringly dangerous in something like e2fsprogs where > it can lead to thorough trashing of the file system. > > Can something be done to improve the portability of the userspace > code? > > The issue has been discussed on the Fedora-ARM mailing list: > http://lists.fedoraproject.org/pipermail/arm/2012-July/003637.html > but the argument there was that the issue needs to be fixed upstream, > hence why I am posting it here. > > This is an issue on all ARMs up to ARMv5, possibly ARMv6, and likely > other CPU architectures that don't have transparent alignment fixup > in hardware (Itanium, SPARC, probably others). > > Apart from being dangerous, using unaligned arrays also affects > performance because it leads to data needlessly straddling cache > lines in the CPU. But that doesn't really matter in this case. For what it's worth, I've never seen reports from anywhere that indicate that it didn't get fixed up. But I agree that if we can eliminate the need for the fixups it'd be best. Many of these are the result of things like: char buf[4096] = ""; struct fiemap *fiemap = (struct fiemap *)buf; Ted, is this construct just an attempt to avoid malloc/free for simplicity of the code? I think Gordan suggested (if I understand it right) that doing an array of ints might also solve the problem, since ints should be on natural alignment. Or maybe in some cases malloc/free would be more obvious, if handling errors isn't too tricky. Gordan, until I get handy access to a real arm box, if you could do a "make check" in the e2fsprogs tree, it might flush out a few more of these alignment warnings, and adding them all to the bug for tracking purposes might be helpful. (IIRC "make gcc-wall" will also emit warnings for casts which change natural alignment, among other things) Thanks, -Eric > Many thanks. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: e2fsprogs alignment issues 2012-07-11 15:04 ` Eric Sandeen @ 2012-07-11 20:05 ` Theodore Ts'o 2012-07-11 21:22 ` Gordan Bobic ` (2 more replies) 2012-07-11 21:24 ` Gordan Bobic 1 sibling, 3 replies; 8+ messages in thread From: Theodore Ts'o @ 2012-07-11 20:05 UTC (permalink / raw) To: Eric Sandeen; +Cc: Gordan Bobic, linux-ext4 On Wed, Jul 11, 2012 at 10:04:51AM -0500, Eric Sandeen wrote: > On 7/11/12 8:20 AM, Gordan Bobic wrote: > > I would like to bring the following bug report to people's > > attention: > > > > https://bugzilla.redhat.com/show_bug.cgi?id=680090 > > > > The issue is that the code used in e2fsprogs WRT allocating unaligned > > buffers and then casting them into structs is non-portable and > > dangerous - eyewateringly dangerous in something like e2fsprogs where > > it can lead to thorough trashing of the file system. I think you're being a bit over-dramatic here; can you point out a place where this is actually going to lead to "trashing of the file system"? I will observe that being able to derefrence unaligned pointers is something that Linus has said any architecture which does *not* provide fixups is probably doomed to fail. There's an awful lot of userspace code out there which assumes this works. Doing a quick grep, I'm seeing code in the blkid library (which has now been moved into util-linux) which does this. I'm also seeing one case of it in the tdb library (which we imported from Samba). That being said, doing fixups is slow since it requires a kernel fault, and I have no problem with trying to avoid this practice. > > Can something be done to improve the portability of the userspace > > code? Yes, someone can send me patches. :-) Ranting and raving about how file systems are being trashed will be met with the observation that no one has actually reported this. And this won't be the only place where the Linux kernel code makes assumptions which are not guaranteed by the C standard, but the official position that Linus has taken is any architecture (or C compiler) is insane, and it's up to the architecture to provide workarounds. > Many of these are the result of things like: > > char buf[4096] = ""; > struct fiemap *fiemap = (struct fiemap *)buf; > > > Ted, is this construct just an attempt to avoid malloc/free for > simplicity of the code? Probably; it's the way Kalpak Shaw wrote the code when he submitted it. And while I do have higher standards of portability that the Linux kernel, this is not one of the things that I test for. Please do note that malloc() is guaranteed to return a block of memory which satisifies the worse case alignment requirements of the architecture, so there are plenty of places where we cast a buffer pointer to an int * or a struct * which are completely safe. It's only when someone is allocating space on the stack where this is potentially problematic. > I think Gordan suggested (if I understand it > right) that doing an array of ints might also solve the problem, since > ints should be on natural alignment. Or maybe in some cases malloc/free > would be more obvious, if handling errors isn't too tricky. In the specific case which Gordon has pointed out, the obvious thing to do is to just to set errno to ENOMEM, and return -1. since we already reflect an error code up to the caller if the FIEMAP ioctl() fails. If someone sends me the patch, I will happily apply it. > (IIRC "make gcc-wall" will also emit warnings for casts which change > natural alignment, among other things) I'd have to check to be sure, but I don't think so, since it would have way too many false positives. We *do* have code where we take char *'s and and then cast them to some other pointer type, and then dereference them. And we do currently assume that it is safe to do this for on-disk data structures which are 4 byte aligned, in the directory entry code, for example. I will *not* accept a patch which uses memcpy to copy each field in the on-disk superblock, or directory entry, into an int, just in case there is some insane architecture which requires that 4 byte integers be 32-byte aligned, or something else insane like that. What we are currently doing may not be 100% portable, but we're not going to penalize sane archiectures like x86 just because there might be some future insane architecture that requires 32-byte aligned ints! All the ranting in the world about how this could hypothetically cause file system corruption on said insane architecture will just cause me to laugh at you. But that being said, changing places where we allocate a char [] buffer on the stack and assuming that it is 4-byte aligned does seem reasonable. But I see this as an optimization issue more than I do a performance issue.... Regards, - Ted ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: e2fsprogs alignment issues 2012-07-11 20:05 ` Theodore Ts'o @ 2012-07-11 21:22 ` Gordan Bobic 2012-07-12 0:05 ` Theodore Ts'o 2012-07-11 21:26 ` Eric Sandeen 2012-07-13 22:25 ` Dave Chinner 2 siblings, 1 reply; 8+ messages in thread From: Gordan Bobic @ 2012-07-11 21:22 UTC (permalink / raw) To: Theodore Ts'o; +Cc: Eric Sandeen, linux-ext4 On 11/07/2012 21:05, Theodore Ts'o wrote: > On Wed, Jul 11, 2012 at 10:04:51AM -0500, Eric Sandeen wrote: >> On 7/11/12 8:20 AM, Gordan Bobic wrote: >>> I would like to bring the following bug report to people's >>> attention: >>> >>> https://bugzilla.redhat.com/show_bug.cgi?id=680090 >>> >>> The issue is that the code used in e2fsprogs WRT allocating unaligned >>> buffers and then casting them into structs is non-portable and >>> dangerous - eyewateringly dangerous in something like e2fsprogs where >>> it can lead to thorough trashing of the file system. > > I think you're being a bit over-dramatic here; can you point out a > place where this is actually going to lead to "trashing of the file > system"? I haven't gone through the code with a fine tooth comb, but the fact that this sort of unportable operations happens implies that it wasn't written with safety and portability considerations in mind. > I will observe that being able to derefrence unaligned pointers is > something that Linus has said any architecture which does *not* > provide fixups is probably doomed to fail. That sounds like a resigned recognition that a lot of developers will use unsafe practices anyway, not a model for how things should be done. > There's an awful lot of > userspace code out there which assumes this works. Doing a quick > grep, I'm seeing code in the blkid library (which has now been moved > into util-linux) which does this. I'm also seeing one case of it in > the tdb library (which we imported from Samba). I make a point with running with fixup+warn on all my ARMv5 devices, and I file bugs against packages that cause these to crop up. It is relatively rare, though, from what I have been able to observe. > That being said, doing fixups is slow since it requires a kernel > fault, and I have no problem with trying to avoid this practice. Splendid, we agree, then. :) >>> Can something be done to improve the portability of the userspace >>> code? > > Yes, someone can send me patches. :-) Sure, I can sed __attribute__((aligned(4))) on every char[] definition, as long as no art rather than engineering oriented individuals aren;t going to complain that it "looks ugly". :) > Ranting and raving about how file systems are being trashed will be > met with the observation that no one has actually reported this. And > this won't be the only place where the Linux kernel code makes > assumptions which are not guaranteed by the C standard, but the > official position that Linus has taken is any architecture (or C > compiler) is insane, and it's up to the architecture to provide > workarounds. Extending that view to it's logical conclusion, let's drop support for ARM < v7 completely, then? I just don't think cutting corners at the expense of correctness should be deemed acceptable. >> Many of these are the result of things like: >> >> char buf[4096] = ""; >> struct fiemap *fiemap = (struct fiemap *)buf; >> >> >> Ted, is this construct just an attempt to avoid malloc/free for >> simplicity of the code? > > Probably; it's the way Kalpak Shaw wrote the code when he submitted > it. And while I do have higher standards of portability that the > Linux kernel, this is not one of the things that I test for. That's OK - somebody will find it. :) > Please do note that malloc() is guaranteed to return a block of memory > which satisifies the worse case alignment requirements of the > architecture, so there are plenty of places where we cast a buffer > pointer to an int * or a struct * which are completely safe. It's > only when someone is allocating space on the stack where this is > potentially problematic. Indeed, and only when allocating it using arrays of type that is smaller than word, which most of the time means char. Everything else will be word aligned anyway. >> I think Gordan suggested (if I understand it >> right) that doing an array of ints might also solve the problem, since >> ints should be on natural alignment. Or maybe in some cases malloc/free >> would be more obvious, if handling errors isn't too tricky. > > In the specific case which Gordon has pointed out, the obvious thing > to do is to just to set errno to ENOMEM, and return -1. since we > already reflect an error code up to the caller if the FIEMAP ioctl() > fails. > > If someone sends me the patch, I will happily apply it. As opposed to aligning the char arrays explicitly? >> (IIRC "make gcc-wall" will also emit warnings for casts which change >> natural alignment, among other things) > > I'd have to check to be sure, but I don't think so, since it would > have way too many false positives. We *do* have code where we take > char *'s and and then cast them to some other pointer type, and then > dereference them. And we do currently assume that it is safe to do > this for on-disk data structures which are 4 byte aligned, in the > directory entry code, for example. > > I will *not* accept a patch which uses memcpy to copy each field in > the on-disk superblock, or directory entry, into an int, just in case > there is some insane architecture which requires that 4 byte integers > be 32-byte aligned, or something else insane like that. > > What we are currently doing may not be 100% portable, but we're not > going to penalize sane archiectures like x86 just because there might > be some future insane architecture that requires 32-byte aligned ints! > All the ranting in the world about how this could hypothetically cause > file system corruption on said insane architecture will just cause me > to laugh at you. By that definition, the majority of architectures supported by Linux are insane. Word alignment requirements apply to ARM, and IIRC to Itanium and SPARC (not sure about the most recent SPARCs) too. If you think that Linux should become x86-only, that's fair enough, I'm sure the ARM kernel maintainers are getting sufficiently frustrated with the current situation that they might think this is a positive development. But meanwhile, back in the real world, we do need to deal with the issue. > But that being said, changing places where we allocate a char [] > buffer on the stack and assuming that it is 4-byte aligned does seem > reasonable. But I see this as an optimization issue more than I do a > performance issue.... I'm not sure exactly what you mean here. But either way, what's wrong with any of the following: 1) Checking sizeof(int) and definining the buffer in suitably sized array of int instead of char or 2) making sure that arrays of char are explicitly aligned to 4 bytes (or even 8 bytes, just to be on the safe side for 64-bit architectures - that should be safe for quite a foreseeable future Gordan ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: e2fsprogs alignment issues 2012-07-11 21:22 ` Gordan Bobic @ 2012-07-12 0:05 ` Theodore Ts'o 0 siblings, 0 replies; 8+ messages in thread From: Theodore Ts'o @ 2012-07-12 0:05 UTC (permalink / raw) To: Gordan Bobic; +Cc: Eric Sandeen, linux-ext4 On Wed, Jul 11, 2012 at 10:22:45PM +0100, Gordan Bobic wrote: > >I will observe that being able to derefrence unaligned pointers is > >something that Linus has said any architecture which does *not* > >provide fixups is probably doomed to fail. > > That sounds like a resigned recognition that a lot of developers > will use unsafe practices anyway, not a model for how things should > be done. The reality is there is a lot of userspace code which does things a certain way. Ignoring that reality will very often doom you to irrelvance. (For example, Plan9 and Mach assumed that the existing huge amount of Posix-compatible userspace code wasn't important, and people would implement new applications to their new OS interfaces from scratch.) > >Yes, someone can send me patches. :-) > > Sure, I can sed __attribute__((aligned(4))) on every char[] > definition, as long as no art rather than engineering oriented > individuals aren;t going to complain that it "looks ugly". :) Um, no. NAK. #1, it's not going to be necessary in all cases, and #2, we try very hard in e2fsprogs to make sure we are not dependent on GCC extensions. If you must use a GCC extension, it must be under an #ifdef __GNUC__ and there must be a reasonable alternative if it's compiled using (for example) the Solaris icc.... Weren't you the one ranting on and on about how portability was so important? > >> I think Gordan suggested (if I understand it > >>right) that doing an array of ints might also solve the problem, since > >>ints should be on natural alignment. Or maybe in some cases malloc/free > >>would be more obvious, if handling errors isn't too tricky. > > > >In the specific case which Gordon has pointed out, the obvious thing > >to do is to just to set errno to ENOMEM, and return -1. since we > >already reflect an error code up to the caller if the FIEMAP ioctl() > >fails. > > > >If someone sends me the patch, I will happily apply it. > > As opposed to aligning the char arrays explicitly? The right thing in the case of the filefrag example which you gave is to allocate the structure dynamically using malloc(). In other cases there may be other solutions which may be a better way to do things. Please do *not* bother to do a global sed style patch. I will reject that summarily. Thought and care are required, not a blunt instrument. > >What we are currently doing may not be 100% portable, but we're not > >going to penalize sane archiectures like x86 just because there might > >be some future insane architecture that requires 32-byte aligned ints! > >All the ranting in the world about how this could hypothetically cause > >file system corruption on said insane architecture will just cause me > >to laugh at you. > > By that definition, the majority of architectures supported by Linux > are insane. Word alignment requirements apply to ARM, and IIRC to > Itanium and SPARC (not sure about the most recent SPARCs) too. If > you think that Linux should become x86-only, that's fair enough, I'm > sure the ARM kernel maintainers are getting sufficiently frustrated > with the current situation that they might think this is a positive > development. But meanwhile, back in the real world, we do need to > deal with the issue. You're not reading what I wrote. There are plenty of places where we read in an on-disk structure into a malloc'ed buffer, where all of the fields of that structure which have 4-byte alignment for 32-bit fields, and 2-byte alignment for 16-bit fields. We dereference those fields directly, without doing a manual copy operation. Strictly speaking, this may not be portable, since there may be architectures that do something insane with greater than 8 byte alignment requirements for 4-byte types. We (both e2fsprogs and the Linux kernel) also assume that the bit pattern field for NULL is 0x00. Strictly speaking the C standard does not require this; it only requires that if you cast an integer value of 0, the result must be the NULL pointer. For example, on the Honeywell GE-645, a NULL pointer was (segment -2, offset 0). Architectures which have odd alignments or wierd bit patterns for NULL pointers are what I am calling "insane". And I don't care about complete generic portability to weird architectures. So please don't try to make arguments about portability as a platonic ideal that we must adhere to at all costs. If we did that, we would have to manually copy each field from the on-disk structure to an in-memory structure, and that would be insanely slow. Can you imagine doing that in the TCP stack?!? I am willing to support the ARM architecture, but we need to do this in a careful way; not via some quick and dirty sed script. But please note that other architectures, including the Alpha, have always supported doing unaligned pointer fixups (even if it is slow), because if they didn't vast amounts of userspace *would* break. Regards, - Ted ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: e2fsprogs alignment issues 2012-07-11 20:05 ` Theodore Ts'o 2012-07-11 21:22 ` Gordan Bobic @ 2012-07-11 21:26 ` Eric Sandeen 2012-07-13 22:25 ` Dave Chinner 2 siblings, 0 replies; 8+ messages in thread From: Eric Sandeen @ 2012-07-11 21:26 UTC (permalink / raw) To: Theodore Ts'o; +Cc: Gordan Bobic, linux-ext4 On 7/11/12 3:05 PM, Theodore Ts'o wrote: > On Wed, Jul 11, 2012 at 10:04:51AM -0500, Eric Sandeen wrote: ... >> I think Gordan suggested (if I understand it >> right) that doing an array of ints might also solve the problem, since >> ints should be on natural alignment. Or maybe in some cases malloc/free >> would be more obvious, if handling errors isn't too tricky. > > In the specific case which Gordon has pointed out, the obvious thing > to do is to just to set errno to ENOMEM, and return -1. since we > already reflect an error code up to the caller if the FIEMAP ioctl() > fails. > > If someone sends me the patch, I will happily apply it. Sure, I was planning on it :) >> (IIRC "make gcc-wall" will also emit warnings for casts which change >> natural alignment, among other things) > > I'd have to check to be sure, but I don't think so, since it would > have way too many false positives. We *do* have code where we take > char *'s and and then cast them to some other pointer type, and then > dereference them. And we do currently assume that it is safe to do > this for on-disk data structures which are 4 byte aligned, in the > directory entry code, for example. > > I will *not* accept a patch which uses memcpy to copy each field in > the on-disk superblock, or directory entry, into an int, just in case > there is some insane architecture which requires that 4 byte integers > be 32-byte aligned, or something else insane like that. Well, let's just see where we're at, first, and see what it'll take, case by case. -Eric ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: e2fsprogs alignment issues 2012-07-11 20:05 ` Theodore Ts'o 2012-07-11 21:22 ` Gordan Bobic 2012-07-11 21:26 ` Eric Sandeen @ 2012-07-13 22:25 ` Dave Chinner 2 siblings, 0 replies; 8+ messages in thread From: Dave Chinner @ 2012-07-13 22:25 UTC (permalink / raw) To: Theodore Ts'o; +Cc: Eric Sandeen, Gordan Bobic, linux-ext4 On Wed, Jul 11, 2012 at 04:05:57PM -0400, Theodore Ts'o wrote: > I will *not* accept a patch which uses memcpy to copy each field in > the on-disk superblock, or directory entry, into an int, just in case > there is some insane architecture which requires that 4 byte integers > be 32-byte aligned, or something else insane like that. This is a problem we've had to deal with in the XFS userspace for years because of the way extent records are packed into inodes. Hence we have "get/set_unaligned_be32" and "get/set_unaligned_be64" functions to read and write unaligned variables directly out of buffers without having platform specific alignment issues. It's exactly the same generic code as the kernel uses for this purpose. See libxfs/xfs.h.... Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: e2fsprogs alignment issues 2012-07-11 15:04 ` Eric Sandeen 2012-07-11 20:05 ` Theodore Ts'o @ 2012-07-11 21:24 ` Gordan Bobic 1 sibling, 0 replies; 8+ messages in thread From: Gordan Bobic @ 2012-07-11 21:24 UTC (permalink / raw) To: Eric Sandeen; +Cc: linux-ext4 On 11/07/2012 16:04, Eric Sandeen wrote: > Gordan, until I get handy access to a real arm box, if you could do a > "make check" in the e2fsprogs tree, it might flush out a few more > of these alignment warnings, and adding them all to the bug for tracking > purposes might be helpful. Sure, will do. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2012-07-13 22:25 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-07-11 13:20 e2fsprogs alignment issues Gordan Bobic 2012-07-11 15:04 ` Eric Sandeen 2012-07-11 20:05 ` Theodore Ts'o 2012-07-11 21:22 ` Gordan Bobic 2012-07-12 0:05 ` Theodore Ts'o 2012-07-11 21:26 ` Eric Sandeen 2012-07-13 22:25 ` Dave Chinner 2012-07-11 21:24 ` Gordan Bobic
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.