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

[Steven Whitehouse <swhiteho@redhat.com>]

Hi,


On 06/03/17 16:55, Andreas Gruenbacher wrote:
> On Mon, Mar 6, 2017 at 4:15 PM, Steven Whitehouse <swhiteho@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@redhat.com>
>>> CC: <stable@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.
Yes, we should not need pointers to ln_type, since it would be at least 
as large as the info itself, so it would be a bit strange, I agree. It 
does look at bit odd like that 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
Yes, we don't really want to take that hit either.... perhaps we'll have 
to live with the alignment thing...

Steve.