Ceph broken on big-endian systems
Hello, we've been trying to get Ceph to work on IBM Z, a big-endian system, and have been running into various serious issues relating to endian conversion code. The main issue we've been seeing is that while the old-style decode/encode machinery in include/encoding.h automatically byte-swaps all integers on big-endian systems (to ensure the serialized format is always little endian), the *new-style* machinery in include/denc.h does not. This seemed confusing at first since there is quite a bit of code there that appears intended to perform exactly that function, e.g. template<typename T> struct ExtType<T, std::enable_if_t<std::is_same_v<T, int32_t> || std::is_same_v<T, uint32_t>>> { using type = __le32; }; However, it turns out that at this point __le32 is actually just an alias for __u32, so this whole machinery doesn't really do anything at all. Looking at the old code in encoding.h, I notice that it works similarly, but uses ceph_le32 instead of __le32. The former is a C++ class that actually does perform byte-swap on access. Even more confusing, there is this code in include/types.h: // temporarily remap __le* to ceph_le* for benefit of shared kernel/userland headers #define __le16 ceph_le16 #define __le32 ceph_le32 #define __le64 ceph_le64 #include "ceph_fs.h" #include "ceph_frag.h" #include "rbd_types.h" #undef __le16 #undef __le32 #undef __le64 which --sometimes-- redefines __le32 as ceph_le32, but those redefines are not active at the point denc.h is included. So it would appear that the usage of __le32 in denc.h is incorrect, and this code should be using ceph_le32 instead. Is this right? But even so, grepping for __le32 throughout the code base shows quite a bit of additional places where it is used, most of which also appear to make the assumption that byte-swaps automatically happen. In addition, there appear to be some places where e.g. ceph_fs.h is included directly, without going via types.h -- and in those places, we suddenly no longer get the byte-swaps ... Now I was wondering whether the best way forward might be to just have __le32 always be defined as ceph_le32 when compiling user space code. But then I noticed that it used to be that way, but that was deliberated changed by this commit back in 2010: commit 737b5043576153817a6b4195b292672585df10d3 Author: Sage Weil <sage@newdream.net> Date: Fri May 7 13:45:00 2010 -0700 endian: simplify __le* type hackery Instead of preventing linux/types.h from being included, instead name our types ceph_le*, and remap using #define _only_ when including the shared kernel/userspace headers. So I'm a bit at a loss to understand how all this is supposed to be working. Any suggestions would be welcome -- we'd be willing to implement whatever's needed, but would like some guidance as to how the solution should look like ... Bye, Ulrich
Hi Ulrich, On Thu, 18 Jul 2019, Ulrich Weigand wrote:
Hello,
we've been trying to get Ceph to work on IBM Z, a big-endian system, and have been running into various serious issues relating to endian conversion code.
The main issue we've been seeing is that while the old-style decode/encode machinery in include/encoding.h automatically byte-swaps all integers on big-endian systems (to ensure the serialized format is always little endian), the *new-style* machinery in include/denc.h does not.
This seemed confusing at first since there is quite a bit of code there that appears intended to perform exactly that function, e.g.
template<typename T> struct ExtType<T, std::enable_if_t<std::is_same_v<T, int32_t> || std::is_same_v<T, uint32_t>>> { using type = __le32; };
However, it turns out that at this point __le32 is actually just an alias for __u32, so this whole machinery doesn't really do anything at all.
Looking at the old code in encoding.h, I notice that it works similarly, but uses ceph_le32 instead of __le32. The former is a C++ class that actually does perform byte-swap on access.
Even more confusing, there is this code in include/types.h:
// temporarily remap __le* to ceph_le* for benefit of shared kernel/userland headers #define __le16 ceph_le16 #define __le32 ceph_le32 #define __le64 ceph_le64 #include "ceph_fs.h" #include "ceph_frag.h" #include "rbd_types.h" #undef __le16 #undef __le32 #undef __le64
which --sometimes-- redefines __le32 as ceph_le32, but those redefines are not active at the point denc.h is included.
So it would appear that the usage of __le32 in denc.h is incorrect, and this code should be using ceph_le32 instead. Is this right?
But even so, grepping for __le32 throughout the code base shows quite a bit of additional places where it is used, most of which also appear to make the assumption that byte-swaps automatically happen. In addition, there appear to be some places where e.g. ceph_fs.h is included directly, without going via types.h -- and in those places, we suddenly no longer get the byte-swaps ...
Now I was wondering whether the best way forward might be to just have __le32 always be defined as ceph_le32 when compiling user space code. But then I noticed that it used to be that way, but that was deliberated changed by this commit back in 2010:
commit 737b5043576153817a6b4195b292672585df10d3 Author: Sage Weil <sage@newdream.net> Date: Fri May 7 13:45:00 2010 -0700
endian: simplify __le* type hackery
Instead of preventing linux/types.h from being included, instead name our types ceph_le*, and remap using #define _only_ when including the shared kernel/userspace headers.
So I'm a bit at a loss to understand how all this is supposed to be working. Any suggestions would be welcome -- we'd be willing to implement whatever's needed, but would like some guidance as to how the solution should look like ...
I think the cleanest thing is to s/__le/ceph_le/ everywhere *except* msgr.h, rados.h, and ceph_fs.h. It seems like what happened was we mostly forgot to always use the ceph_ variant after 2010. That should resolve the issue, right? sage
Hi Sage,
I think the cleanest thing is to s/__le/ceph_le/ everywhere *except* msgr.h, rados.h, and ceph_fs.h. It seems like what happened was we mostly forgot to always use the ceph_ variant after 2010.
That should resolve the issue, right?
That's what we've tried to do, and it does indeed make work Ceph a lot better on IBM Z (still some issues, but probably unrelated). Once we've gotten a bit farther with testing, we can certainly send patches to that effect. However, where I'm still unsure is that there appear to be a number of files that e.g. just #include "ceph_fs.h" directly, without going through the re-define in types.h ... Now maybe this doesn't matter because none of those places actually use any of the structs that use __le32 from those headers, but that seems hard to verify (and a bit fragile ...). Would the correct fix for this be to replace the direct includes with includes of types.h? Or should those headers be changed to be more self-contained and always define __le32 themselves when built in user space (but I'm not quite sure how to do that without conflicting with the __le32 definition in linux/types.h ...)? In any case, it would be really good to find a way to have those rules (e.g. never use __le32, never include ceph_fs.h directly) automatically enforced at build time, to avoid any reoccurance of the problem with future code changes. Thanks for your quick reply! Bye, Ulrich
On Thu, 18 Jul 2019, Ulrich Weigand wrote:
Hi Sage,
I think the cleanest thing is to s/__le/ceph_le/ everywhere *except* msgr.h, rados.h, and ceph_fs.h. It seems like what happened was we mostly forgot to always use the ceph_ variant after 2010.
That should resolve the issue, right?
That's what we've tried to do, and it does indeed make work Ceph a lot better on IBM Z (still some issues, but probably unrelated). Once we've gotten a bit farther with testing, we can certainly send patches to that effect.
Great, thanks!
However, where I'm still unsure is that there appear to be a number of files that e.g. just #include "ceph_fs.h" directly, without going through the re-define in types.h ...
Now maybe this doesn't matter because none of those places actually use any of the structs that use __le32 from those headers, but that seems hard to verify (and a bit fragile ...).
Would the correct fix for this be to replace the direct includes with includes of types.h? Or should those headers be changed to be more self-contained and always define __le32 themselves when built in user space (but I'm not quite sure how to do that without conflicting with the __le32 definition in linux/types.h ...)?
In any case, it would be really good to find a way to have those rules (e.g. never use __le32, never include ceph_fs.h directly) automatically enforced at build time, to avoid any reoccurance of the problem with future code changes.
Yeah, agree. I think the best option would be to make those headers have a bit at the top that ensure thigns are correctly defined for userspace. We don't want that block to leak into the kernel tree, but it's easy enough to ignore a single block of code; much harder to keep them in sync if we were to, say, s/__le32/ceph_le32/ in the headers. sage
Sage Weil <sage@newdream.net> wrote on 18.07.2019 17:00:18:
On Thu, 18 Jul 2019, Ulrich Weigand wrote:
I think the cleanest thing is to s/__le/ceph_le/ everywhere *except* msgr.h, rados.h, and ceph_fs.h. It seems like what happened was we mostly forgot to always use the ceph_ variant after 2010.
That should resolve the issue, right?
That's what we've tried to do, and it does indeed make work Ceph a lot better on IBM Z (still some issues, but probably unrelated). Once we've gotten a bit farther with testing, we can certainly send patches to that effect.
Great, thanks!
There's a few places where this approach causes problems because __le32 etc. are used for objects that are being operated on. The ceph_le32 etc. types really only allow assigning from and to objects of the type, not any other operations. (Not even initialization, which is a bit weird in some places -- should there be a constructor defined?) In most cases I was able to resolve those issues in a straightforward way (e.g. for a local variable we should just use __u32 instead of __le32). However, one file makes widespread use of this: os/Transaction.h. I'm not really sure how to fix this -- are the structures in this file intended to be local in-memory objects (then why are they using __leXX?), or are they intended to be on-disk/network serialized forms (then why are there lots of arithmetic and other operations performed directly on those types?).
However, where I'm still unsure is that there appear to be a number of files that e.g. just #include "ceph_fs.h" directly, without going through the re-define in types.h ...
Now maybe this doesn't matter because none of those places actually use any of the structs that use __le32 from those headers, but that seems hard to verify (and a bit fragile ...).
Would the correct fix for this be to replace the direct includes with includes of types.h? Or should those headers be changed to be more self-contained and always define __le32 themselves when built in user space (but I'm not quite sure how to do that without conflicting with the __le32 definition in linux/types.h ...)?
In any case, it would be really good to find a way to have those rules (e.g. never use __le32, never include ceph_fs.h directly) automatically enforced at build time, to avoid any reoccurance of the problem with future code changes.
Yeah, agree.
I think the best option would be to make those headers have a bit at the top that ensure thigns are correctly defined for userspace. We don't want that block to leak into the kernel tree, but it's easy enough to ignore a
single block of code; much harder to keep them in sync if we were to, say, s/__le32/ceph_le32/ in the headers.
It does indeed look like by adding something like #ifndef __KERNEL__ #include "byteorder.h" #define __le16 ceph_le16 #define __le32 ceph_le32 #define __le64 ceph_le64 #endif at the beginning and the corresponding #ifndef __KERNEL__ #undef __le16 #undef __le32 #undef __le64 #endif at the end of those files. Bye, Ulrich
Sage Weil <sage@newdream.net> wrote on 18.07.2019 17:00:18:
On Thu, 18 Jul 2019, Ulrich Weigand wrote:
That's what we've tried to do, and it does indeed make work Ceph a lot better on IBM Z (still some issues, but probably unrelated). Once we've gotten a bit farther with testing, we can certainly send patches to that effect.
Great, thanks!
I've now made significant progress. In fact, with my current patch set I can build Ceph on Z and run the "ctest" suite with zero fails: 100% tests passed, 0 tests failed out of 169 In addition to the endian-related changes, which I'll describe in detail below, I needed three more patches fixing minor testing issues, as well as a critical fix for crashes in the Boost "lockfree" library on Z. I'll be preparing patches for submission via issues and pull requests, but I've never contributed to Ceph before, so I may have to first get more familar with how the project operates ... I'd appreciate any comments or tips, e.g. on how to (possibly) split up the patch etc. (See attached file: ceph-endian.diff) This patch fixes many problems with Ceph on big-endian systems due to missed endian conversions in various places. The primary change is: - Remove all instances of __le16/__le32/__le64 and replace them with ceph_le16/ceph_le32/ceph_le64 instead; the latter will perform automatic byte swapping on access if required. (Note that in select places I believe using a byte-swapping type is unnecessary, e.g. for local variables or parameters; in those cases I've changed existing __le type uses to plain unsigned integer types instead.) - The exception is three header files (ceph_fs.h, msgr.h, rados.h), which are shared with the Linux kernel. These files continue to use the __le types, but redefine them to ceph_le types when compiled for user-space. This is now done in those files themselves instead of in types.h, so that also direct includers are covered. In addition, I noticed inconsistencies with how __le and/or ceph_le types were initialized. Given that ceph_le currently has a public member, it can be initialized as an aggregate, which means the responsibility for byte swapping is on each user. Some places did indeed perform byte swapping (either via init_le16/32/64 or directly via mswab), but not all. To ensure consistency, I'm suggesting to change ceph_le to make its member private, which will ensure any value must be set by the assignment operator (which always byte-swaps). Ideally, we'd also have a constructor; but that collides with the way the __le types are used in the shared headers as part of anonymous structs/unions. GCC does not allow any type with constructor in such places. For convenience, I've therefore changed the init_le functions to return ceph_le types, so that you can at least initialize ceph_le variables to the return values of init_le. All places where a ceph_le (or formerly __le) variable needs to be initialized are changed to use this new method; all direct uses of mswab were eliminated. Also, in a very small number of places existing code actually tried to manually byte-swap accesses to __le types using the leXX_to_cpu routines (but that was a no-op anyway ...). With the ceph_le types, that now works correctly automatically, so I've removed that code. In addition, the patch contains a number of changes to work around follow-on problems from the decisions made above. Specifically: - Now that ceph_fs.h always uses C++ types (in user space), it cannot be included in C files any more. There was exactly one of those, src/mds/locks.c, and it only used a few CEPH_CAP_... constants from the header. To fix this, I've simply duplicated those definitions; as those values appear to be unchangeable ABI constants, I'd not expect that duplication to cause any problems later on. (Of course, any alternative suggestions are appreciated!) - Even in the absence of constructors, the new ceph_le types are now no longer aggregate types; this causes a few issues when interacting with compiler extensions, in particular anonymous structs/unions. GCC no longer allows (without warning) the case where a packed struct contains *non-packed* structs that contain ceph_le. In practice, all those structs and substructs were always intended to be completely packed (and actually are, those substructs where the attribute was missing happen to be naturally packed), so I've simply added the missing attributes. - Similarly, GCC no longer allows a std::array to be an element of a packed struct that also contains a non-aggregate type (like the new ceph_le). This happens in one file, msg/async/frames_v2.h. I've replaced the std::array with a plain C array with the same layout. - Also, the following code now gives warnings due to overwriting a private member via memcpy: copy_from_legacy_head(struct ceph_mds_request_head *head, struct ceph_mds_request_head_legacy *legacy) { memcpy(&(head->oldest_client_tid), legacy, sizeof(*legacy)); I'm now simply using a struct assignment instead. - Several places in os/Transaction.h use operators like += or ++ on (what is now) ceph_le members, but the class doesn't implement those. I guess it would be possible to add them at the class level, but since it's just the one file, and in general it seems preferable to not perform too many operations on byte-swapped types anyway, I've simply expanded those operations in place. - Because ceph_le has no constructor, it is impossible to construct a constexpr variable of any type containing ceph_le. This only occurs in one test case, where I simply removed the constexpr. Finally, I've run into three places where endian handling looked simply wrong, completely independently of the __le vs. ceph_le issue: - librbd/operation/ResizeRequest.cc:ResizeRequest<I>::send_update_header has the comment: // NOTE: format 1 image headers are not stored in fixed endian format This seems just wrong, both from looking at test failures, and from looking at code handling those format 1 image headers in the Linux kernel driver (which uses le_to_cpu for those fields). - struct PGTempMap in osd/OSDMap.h tracks a number of int32_t pointers pointing into a buffer list. But that list was generated via encode, which means int members are bytes-swapped. Fixed by using ceph_le32 pointers instead. - Similarly, struct HeaderHelper in tools/immutable_object_cache/Types.h is used to overlay buffer list data, so it should use ceph_le32 instead of uint32_t. Bye, Ulrich
Sage Weil <sage@newdream.net> wrote on 18.07.2019 17:00:18:
On Thu, 18 Jul 2019, Ulrich Weigand wrote:
Hi Sage,
I think the cleanest thing is to s/__le/ceph_le/ everywhere *except* msgr.h, rados.h, and ceph_fs.h. It seems like what happened was we mostly forgot to always use the ceph_ variant after 2010.
That should resolve the issue, right?
That's what we've tried to do, and it does indeed make work Ceph a lot better on IBM Z (still some issues, but probably unrelated). Once we've gotten a bit farther with testing, we can certainly send patches to that effect.
Great, thanks!
I've now opened issue https://tracker.ceph.com/issues/41605 to track this, and posted a first set of fixes as pull request: https://github.com/ceph/ceph/pull/30079 Bye, Ulrich
participants (2)
-
Sage Weil
-
Ulrich Weigand