gfs2: Avoid alignment hole in struct lm_lockname

Commit 88ffbf3e03 switches to using rhashtables for glocks, hashing over the entire struct lm_lockname instead of its individual fields. On some architectures, struct lm_lockname contains a hole of uninitialized memory due to alignment rules, which now leads to incorrect hash values. Get rid of that hole.

Signed-off-by Andreas Gruenbacher <[email protected]>

CC <[email protected]> #v4.3+

---
  fs/gfs2/incore.h | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

fs/gfs2/incore.h +0 -0

index c45084a..511e1ed 100644

Quoted 13 lines · Andreas Gruenbacher

Commit 88ffbf3e03 switches to using rhashtables for glocks, hashing over the entire struct lm_lockname instead of its individual fields. On some architectures, struct lm_lockname contains a hole of uninitialized memory due to alignment rules, which now leads to incorrect hash values. Get rid of that hole.

Signed-off-by Andreas Gruenbacher <[email protected]>

CC <[email protected]> #v4.3+

---
  fs/gfs2/incore.h | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

fs/gfs2/incore.h struct lm_lockname { · line 212

Quoted 12 lines

fs/gfs2/incore.h +1 -1

index c45084a..511e1ed 100644
@@ -207,7 +207,7 @@ struct lm_lockname {
        struct gfs2_sbd *ln_sbd;
        u64 ln_number;
        unsigned int ln_type;
-};
+} __packed __aligned(sizeof(int));
    #define lm_name_equal(name1, name2) \
          (((name1)->ln_number == (name2)->ln_number) &&        \

I'm still not sure that this is the best solution. Also, either it should be packed or aligned to a certain size, I'm not sure how it can be both?

Changing the alignment may cause access issues on some arches.

I think it
would be better just to initialize the structure to zero when we allocate a new glock, that will ensure that the padding is always zeroed, without changing the alignment,

Quoted 13 lines · Steven Whitehouse

Quoted 12 lines · Andreas Gruenbacher

Commit 88ffbf3e03 switches to using rhashtables for glocks, hashing over the entire struct lm_lockname instead of its individual fields. On some architectures, struct lm_lockname contains a hole of uninitialized memory due to alignment rules, which now leads to incorrect hash values. Get rid of that hole.

Signed-off-by Andreas Gruenbacher <[email protected]>

CC <[email protected]> #v4.3+

---
   fs/gfs2/incore.h | 2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 14 lines

fs/gfs2/incore.h struct lm_lockname { · line 212

Quoted 12 lines

fs/gfs2/incore.h +1 -1

index c45084a..511e1ed 100644
@@ -207,7 +207,7 @@ struct lm_lockname {
         struct gfs2_sbd *ln_sbd;
         u64 ln_number;
         unsigned int ln_type;
-};
+} __packed __aligned(sizeof(int));
     #define lm_name_equal(name1, name2) \
           (((name1)->ln_number == (name2)->ln_number) &&        \

I'm still not sure that this is the best solution. Also, either it should be packed or aligned to a certain size, I'm not sure how it can be both?

Well, the size of struct lm_lockname is 20 in either case. With a __packed struct lm_lockname *p, gcc will generate code that assumes no alignment for p->ln_number and p->ln_type accesses. With a __packed, __aligned(sizeof(int)) struct lm_lockname *p, gcc will generate code that assumes int alignment. That's probably irrelevant on all architectures except SPARC, but it doesn't hurt.

Quoted 1 line

Changing the alignment may cause access issues on some arches.

It could when working with pointers to elements of struct lm_lockname without being careful. That's not happening, and it's unlikely to happen in the future, though.

Quoted 4 lines

I think it
would be better just to initialize the structure to zero when we allocate a new glock, that will ensure that the padding is always zeroed, without changing the alignment,

That's a possibility. We'd always needlessly hash four bytes of zeroes, though.

Thanks,
Andreas

----- Original Message -----

| Commit 88ffbf3e03 switches to using rhashtables for glocks, hashing over
| the entire struct lm_lockname instead of its individual fields.  On some
| architectures, struct lm_lockname contains a hole of uninitialized
| memory due to alignment rules, which now leads to incorrect hash values.
| Get rid of that hole.
|
| Signed-off-by: Andreas Gruenbacher <[email protected]>
| CC: <[email protected]> #v4.3+
| ---

Hi,

Thanks. This is now applied to the for-next branch of the linux-gfs2 tree:
https://git.kernel.org/cgit/linux/kernel/git/gfs2/linux-gfs2.git/commit/?h=for-next&id=415730e5de2361518a2d884c0f64de37c5bfefeb

Will this get sent to Linus in the 4.11 cycle or is it being held for the 4.12 merge window?