[Cluster-devel] [PATCH] gfs2: Avoid alignment hole in struct lm_lockname

[Andreas Gruenbacher]

On Mon, Mar 6, 2017 at 4:15 PM, Steven Whitehouse <swhiteho at redhat.com> wrote:
> On 06/03/17 14:33, Andreas Gruenbacher wrote:
>>
>> 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 <agruenba at redhat.com>
>> CC: <stable at vger.kernel.org> #v4.3+
>> ---
>>   fs/gfs2/incore.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/gfs2/incore.h b/fs/gfs2/incore.h
>> index c45084a..511e1ed 100644
>> --- a/fs/gfs2/incore.h
>> +++ b/fs/gfs2/incore.h
>> @@ -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.

> 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.

> 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