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

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

[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 <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) &&	\
-- 
2.7.4

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

[Bob Peterson]

----- 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 <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) &&	\
| --
| 2.7.4
| 
| 
Hi,

Looks good. I've tested it as well.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>

Regards,

Bob Peterson

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

[Steven Whitehouse]

Hi,


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? 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,

Steve.

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

> 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

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

[Steven Whitehouse]

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.

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

[Bob Peterson]

----- 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 <agruenba@redhat.com>
| CC: <stable@vger.kernel.org> #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

Regards,

Bob Peterson
Red Hat File Systems

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

[Andrew Price]

On 06/03/17 18:27, Bob Peterson wrote:
> ----- 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 <agruenba@redhat.com>
> | CC: <stable@vger.kernel.org> #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?

Andy

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

[Andreas Gruenbacher]

On Wed, Mar 15, 2017 at 12:24 PM, Andrew Price <anprice@redhat.com> wrote:
> Will this get sent to Linus in the 4.11 cycle or is it being held for the
> 4.12 merge window?

This causes potential filesystem corruption, so it should go into
4.11. Bob has put it in for-next to give it some exposure but I don't
think he's sent Linus a pull request, yet.

Andreas

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

[Bob Peterson]

----- Original Message -----
| On Wed, Mar 15, 2017 at 12:24 PM, Andrew Price <anprice@redhat.com> wrote:
| > Will this get sent to Linus in the 4.11 cycle or is it being held for the
| > 4.12 merge window?
| 
| This causes potential filesystem corruption, so it should go into
| 4.11. Bob has put it in for-next to give it some exposure but I don't
| think he's sent Linus a pull request, yet.
| 
| Andreas
| 
Hi Guys,

I just sent Linus a pull request for this. Sorry it took so long.

Bob