[Devel] autofs: fix autofs_v5_packet structure for compat mode

Submitted by Stanislav Kinsburskiy on Aug. 31, 2017, 10:11 a.m.

Details

Message ID 20170831101132.6473.48373.stgit@localhost.localdomain
State New
Series "autofs: fix autofs_v5_packet structure for compat mode"
Headers show

Commit Message

Stanislav Kinsburskiy Aug. 31, 2017, 10:11 a.m.
Due to integer variables alignment size of struct autofs_v5_packet in 300
bytes in 32-bit architectures (instead of 304 bytes in 64-bits architectures).

This may lead to memory corruption (64 bits kernel always send 304 bytes,
while 32-bit userspace application expects for 300).

https://jira.sw.ru/browse/PSBM-71078

Signed-off-by: Stanislav Kinsburskiy <skinsbursky@virtuozzo.com>
---
 include/uapi/linux/auto_fs4.h |    2 ++
 1 file changed, 2 insertions(+)

Patch hide | download patch | download mbox

diff --git a/include/uapi/linux/auto_fs4.h b/include/uapi/linux/auto_fs4.h
index e02982f..8729a47 100644
--- a/include/uapi/linux/auto_fs4.h
+++ b/include/uapi/linux/auto_fs4.h
@@ -137,6 +137,8 @@  struct autofs_v5_packet {
 	__u32 pid;
 	__u32 tgid;
 	__u32 len;
+	__u32 blob;		/* This is needed to align structure up to 8
+				   bytes for ALL archs including 32-bit */
 	char name[NAME_MAX+1];
 };
 

Comments

Konstantin Khorenko Aug. 31, 2017, 10:37 a.m.
Will you send it to mainstream as well?

--
Best regards,

Konstantin Khorenko,
Virtuozzo Linux Kernel Team

On 08/31/2017 01:11 PM, Stanislav Kinsburskiy wrote:
> Due to integer variables alignment size of struct autofs_v5_packet in 300
> bytes in 32-bit architectures (instead of 304 bytes in 64-bits architectures).
>
> This may lead to memory corruption (64 bits kernel always send 304 bytes,
> while 32-bit userspace application expects for 300).
>
> https://jira.sw.ru/browse/PSBM-71078
>
> Signed-off-by: Stanislav Kinsburskiy <skinsbursky@virtuozzo.com>
> ---
>  include/uapi/linux/auto_fs4.h |    2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/include/uapi/linux/auto_fs4.h b/include/uapi/linux/auto_fs4.h
> index e02982f..8729a47 100644
> --- a/include/uapi/linux/auto_fs4.h
> +++ b/include/uapi/linux/auto_fs4.h
> @@ -137,6 +137,8 @@ struct autofs_v5_packet {
>  	__u32 pid;
>  	__u32 tgid;
>  	__u32 len;
> +	__u32 blob;		/* This is needed to align structure up to 8
> +				   bytes for ALL archs including 32-bit */
>  	char name[NAME_MAX+1];
>  };
>
>
> .
>
Stanislav Kinsburskiy Aug. 31, 2017, 10:38 a.m.
Yes

31.08.2017 13:37, Konstantin Khorenko пишет:
> Will you send it to mainstream as well?
> 
> -- 
> Best regards,
> 
> Konstantin Khorenko,
> Virtuozzo Linux Kernel Team
> 
> On 08/31/2017 01:11 PM, Stanislav Kinsburskiy wrote:
>> Due to integer variables alignment size of struct autofs_v5_packet in 300
>> bytes in 32-bit architectures (instead of 304 bytes in 64-bits architectures).
>>
>> This may lead to memory corruption (64 bits kernel always send 304 bytes,
>> while 32-bit userspace application expects for 300).
>>
>> https://jira.sw.ru/browse/PSBM-71078
>>
>> Signed-off-by: Stanislav Kinsburskiy <skinsbursky@virtuozzo.com>
>> ---
>>  include/uapi/linux/auto_fs4.h |    2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/include/uapi/linux/auto_fs4.h b/include/uapi/linux/auto_fs4.h
>> index e02982f..8729a47 100644
>> --- a/include/uapi/linux/auto_fs4.h
>> +++ b/include/uapi/linux/auto_fs4.h
>> @@ -137,6 +137,8 @@ struct autofs_v5_packet {
>>      __u32 pid;
>>      __u32 tgid;
>>      __u32 len;
>> +    __u32 blob;        /* This is needed to align structure up to 8
>> +                   bytes for ALL archs including 32-bit */
>>      char name[NAME_MAX+1];
>>  };
>>
>>
>> .
>>
Dmitry V. Levin Aug. 31, 2017, 10:38 a.m.
On Thu, Aug 31, 2017 at 02:11:34PM +0400, Stanislav Kinsburskiy wrote:
> Due to integer variables alignment size of struct autofs_v5_packet in 300
> bytes in 32-bit architectures (instead of 304 bytes in 64-bits architectures).
> 
> This may lead to memory corruption (64 bits kernel always send 304 bytes,
> while 32-bit userspace application expects for 300).
> 
> https://jira.sw.ru/browse/PSBM-71078
> 
> Signed-off-by: Stanislav Kinsburskiy <skinsbursky@virtuozzo.com>
> ---
>  include/uapi/linux/auto_fs4.h |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/uapi/linux/auto_fs4.h b/include/uapi/linux/auto_fs4.h
> index e02982f..8729a47 100644
> --- a/include/uapi/linux/auto_fs4.h
> +++ b/include/uapi/linux/auto_fs4.h
> @@ -137,6 +137,8 @@ struct autofs_v5_packet {
>  	__u32 pid;
>  	__u32 tgid;
>  	__u32 len;
> +	__u32 blob;		/* This is needed to align structure up to 8
> +				   bytes for ALL archs including 32-bit */
>  	char name[NAME_MAX+1];
>  };

This change breaks ABI because it changes offsetof(struct autofs_v5_packet, name).
If you need to fix the alignment, use  __attribute__((aligned(8))).

An alignment change would also be an ABI breakage on 32-bit architectures,
though.
Stanislav Kinsburskiy Aug. 31, 2017, 10:48 a.m.
31.08.2017 13:38, Dmitry V. Levin пишет:
> On Thu, Aug 31, 2017 at 02:11:34PM +0400, Stanislav Kinsburskiy wrote:
>> Due to integer variables alignment size of struct autofs_v5_packet in 300
>> bytes in 32-bit architectures (instead of 304 bytes in 64-bits architectures).
>>
>> This may lead to memory corruption (64 bits kernel always send 304 bytes,
>> while 32-bit userspace application expects for 300).
>>
>> https://jira.sw.ru/browse/PSBM-71078
>>
>> Signed-off-by: Stanislav Kinsburskiy <skinsbursky@virtuozzo.com>
>> ---
>>  include/uapi/linux/auto_fs4.h |    2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/include/uapi/linux/auto_fs4.h b/include/uapi/linux/auto_fs4.h
>> index e02982f..8729a47 100644
>> --- a/include/uapi/linux/auto_fs4.h
>> +++ b/include/uapi/linux/auto_fs4.h
>> @@ -137,6 +137,8 @@ struct autofs_v5_packet {
>>  	__u32 pid;
>>  	__u32 tgid;
>>  	__u32 len;
>> +	__u32 blob;		/* This is needed to align structure up to 8
>> +				   bytes for ALL archs including 32-bit */
>>  	char name[NAME_MAX+1];
>>  };
> 
> This change breaks ABI because it changes offsetof(struct autofs_v5_packet, name).
> If you need to fix the alignment, use  __attribute__((aligned(8))).
> 

Nice to know you're watching.
Yes, attribute is better.
But how ABI is broken? On x86_64 this alignment is implied, so nothing is changed.

> An alignment change would also be an ABI breakage on 32-bit architectures,
> though.
> 

True.
But from my POW better have it working on 64bit archs for 32bit apps.
But anyway, upstream guys will device, whether they want 32-bit autofs applications properly work on 64 or 32 bits.
Dmitry V. Levin Aug. 31, 2017, 11:40 a.m.
On Thu, Aug 31, 2017 at 01:48:27PM +0300, Stanislav Kinsburskiy wrote:
> 
> 
> 31.08.2017 13:38, Dmitry V. Levin пишет:
> > On Thu, Aug 31, 2017 at 02:11:34PM +0400, Stanislav Kinsburskiy wrote:
> >> Due to integer variables alignment size of struct autofs_v5_packet in 300
> >> bytes in 32-bit architectures (instead of 304 bytes in 64-bits architectures).
> >>
> >> This may lead to memory corruption (64 bits kernel always send 304 bytes,
> >> while 32-bit userspace application expects for 300).
> >>
> >> https://jira.sw.ru/browse/PSBM-71078
> >>
> >> Signed-off-by: Stanislav Kinsburskiy <skinsbursky@virtuozzo.com>
> >> ---
> >>  include/uapi/linux/auto_fs4.h |    2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/include/uapi/linux/auto_fs4.h b/include/uapi/linux/auto_fs4.h
> >> index e02982f..8729a47 100644
> >> --- a/include/uapi/linux/auto_fs4.h
> >> +++ b/include/uapi/linux/auto_fs4.h
> >> @@ -137,6 +137,8 @@ struct autofs_v5_packet {
> >>  	__u32 pid;
> >>  	__u32 tgid;
> >>  	__u32 len;
> >> +	__u32 blob;		/* This is needed to align structure up to 8
> >> +				   bytes for ALL archs including 32-bit */
> >>  	char name[NAME_MAX+1];
> >>  };
> > 
> > This change breaks ABI because it changes offsetof(struct autofs_v5_packet, name).
> > If you need to fix the alignment, use  __attribute__((aligned(8))).
> > 
> 
> Nice to know you're watching.
> Yes, attribute is better.
> But how ABI is broken? On x86_64 this alignment is implied, so nothing is changed.

Your change increases offsetof(struct autofs_v5_packet, name) by 4 on all
architectures.  On architectures where the structure is 32-bit aligned
this also leads to increase of its size by 4.

> > An alignment change would also be an ABI breakage on 32-bit architectures,
> > though.
> > 
> 
> True.
> But from my POW better have it working on 64bit archs for 32bit apps.
> But anyway, upstream guys will device, whether they want 32-bit autofs applications properly work on 64 or 32 bits.

Let's fix old bugs without introducing new bugs.
The right fix here seems to be a compat structure, that is, both 64-bit
and 32-bit kernels should send the same 32-bit aligned structure, and
it has to be the same structure sent by traditional 32-bit kernels.
Dmitry V. Levin Aug. 31, 2017, 12:05 p.m.
On Thu, Aug 31, 2017 at 02:40:23PM +0300, Dmitry V. Levin wrote:
> On Thu, Aug 31, 2017 at 01:48:27PM +0300, Stanislav Kinsburskiy wrote:
> > 
> > 
> > 31.08.2017 13:38, Dmitry V. Levin пишет:
> > > On Thu, Aug 31, 2017 at 02:11:34PM +0400, Stanislav Kinsburskiy wrote:
> > >> Due to integer variables alignment size of struct autofs_v5_packet in 300
> > >> bytes in 32-bit architectures (instead of 304 bytes in 64-bits architectures).
> > >>
> > >> This may lead to memory corruption (64 bits kernel always send 304 bytes,
> > >> while 32-bit userspace application expects for 300).
> > >>
> > >> https://jira.sw.ru/browse/PSBM-71078
> > >>
> > >> Signed-off-by: Stanislav Kinsburskiy <skinsbursky@virtuozzo.com>
> > >> ---
> > >>  include/uapi/linux/auto_fs4.h |    2 ++
> > >>  1 file changed, 2 insertions(+)
> > >>
> > >> diff --git a/include/uapi/linux/auto_fs4.h b/include/uapi/linux/auto_fs4.h
> > >> index e02982f..8729a47 100644
> > >> --- a/include/uapi/linux/auto_fs4.h
> > >> +++ b/include/uapi/linux/auto_fs4.h
> > >> @@ -137,6 +137,8 @@ struct autofs_v5_packet {
> > >>  	__u32 pid;
> > >>  	__u32 tgid;
> > >>  	__u32 len;
> > >> +	__u32 blob;		/* This is needed to align structure up to 8
> > >> +				   bytes for ALL archs including 32-bit */
> > >>  	char name[NAME_MAX+1];
> > >>  };
> > > 
> > > This change breaks ABI because it changes offsetof(struct autofs_v5_packet, name).
> > > If you need to fix the alignment, use  __attribute__((aligned(8))).
> > > 
> > 
> > Nice to know you're watching.
> > Yes, attribute is better.
> > But how ABI is broken? On x86_64 this alignment is implied, so nothing is changed.
> 
> Your change increases offsetof(struct autofs_v5_packet, name) by 4 on all
> architectures.  On architectures where the structure is 32-bit aligned
> this also leads to increase of its size by 4.
> 
> > > An alignment change would also be an ABI breakage on 32-bit architectures,
> > > though.
> > > 
> > 
> > True.
> > But from my POW better have it working on 64bit archs for 32bit apps.
> > But anyway, upstream guys will device, whether they want 32-bit autofs applications properly work on 64 or 32 bits.
> 
> Let's fix old bugs without introducing new bugs.
> The right fix here seems to be a compat structure, that is, both 64-bit
> and 32-bit kernels should send the same 32-bit aligned structure, and
> it has to be the same structure sent by traditional 32-bit kernels.

Alternatively, a much more simple fix would be to change 64-bit kernels
not to send the trailing 4 padding bytes of 64-bit aligned
struct autofs_v5_packet.  That is, just send
offsetofend(struct autofs_v5_packet, name) bytes instead of
sizeof(struct autofs_v5_packet) regardless of architecture.
Stanislav Kinsburskiy Aug. 31, 2017, 1:12 p.m.
31.08.2017 15:05, Dmitry V. Levin пишет:
> On Thu, Aug 31, 2017 at 02:40:23PM +0300, Dmitry V. Levin wrote:
>> On Thu, Aug 31, 2017 at 01:48:27PM +0300, Stanislav Kinsburskiy wrote:
>>>
>>>
>>> 31.08.2017 13:38, Dmitry V. Levin пишет:
>>>> On Thu, Aug 31, 2017 at 02:11:34PM +0400, Stanislav Kinsburskiy wrote:
>>>>> Due to integer variables alignment size of struct autofs_v5_packet in 300
>>>>> bytes in 32-bit architectures (instead of 304 bytes in 64-bits architectures).
>>>>>
>>>>> This may lead to memory corruption (64 bits kernel always send 304 bytes,
>>>>> while 32-bit userspace application expects for 300).
>>>>>
>>>>> https://jira.sw.ru/browse/PSBM-71078
>>>>>
>>>>> Signed-off-by: Stanislav Kinsburskiy <skinsbursky@virtuozzo.com>
>>>>> ---
>>>>>  include/uapi/linux/auto_fs4.h |    2 ++
>>>>>  1 file changed, 2 insertions(+)
>>>>>
>>>>> diff --git a/include/uapi/linux/auto_fs4.h b/include/uapi/linux/auto_fs4.h
>>>>> index e02982f..8729a47 100644
>>>>> --- a/include/uapi/linux/auto_fs4.h
>>>>> +++ b/include/uapi/linux/auto_fs4.h
>>>>> @@ -137,6 +137,8 @@ struct autofs_v5_packet {
>>>>>  	__u32 pid;
>>>>>  	__u32 tgid;
>>>>>  	__u32 len;
>>>>> +	__u32 blob;		/* This is needed to align structure up to 8
>>>>> +				   bytes for ALL archs including 32-bit */
>>>>>  	char name[NAME_MAX+1];
>>>>>  };
>>>>
>>>> This change breaks ABI because it changes offsetof(struct autofs_v5_packet, name).
>>>> If you need to fix the alignment, use  __attribute__((aligned(8))).
>>>>
>>>
>>> Nice to know you're watching.
>>> Yes, attribute is better.
>>> But how ABI is broken? On x86_64 this alignment is implied, so nothing is changed.
>>
>> Your change increases offsetof(struct autofs_v5_packet, name) by 4 on all
>> architectures.  On architectures where the structure is 32-bit aligned
>> this also leads to increase of its size by 4.
>>
>>>> An alignment change would also be an ABI breakage on 32-bit architectures,
>>>> though.
>>>>
>>>
>>> True.
>>> But from my POW better have it working on 64bit archs for 32bit apps.
>>> But anyway, upstream guys will device, whether they want 32-bit autofs applications properly work on 64 or 32 bits.
>>
>> Let's fix old bugs without introducing new bugs.
>> The right fix here seems to be a compat structure, that is, both 64-bit
>> and 32-bit kernels should send the same 32-bit aligned structure, and
>> it has to be the same structure sent by traditional 32-bit kernels.
> 
> Alternatively, a much more simple fix would be to change 64-bit kernels
> not to send the trailing 4 padding bytes of 64-bit aligned
> struct autofs_v5_packet.  That is, just send
> offsetofend(struct autofs_v5_packet, name) bytes instead of
> sizeof(struct autofs_v5_packet) regardless of architecture.
> 

Fair enough, thanks!
But this approach won't work, because autofs pipe has O_DIRECT flag.
Compat structure looks more promising, but not yet clear to me, how to define one properly.
Probably by replacing "len" with char array like this:

/* autofs v5 common packet struct */
struct autofs_v5_packet {
        struct autofs_packet_hdr hdr;
        autofs_wqt_t wait_queue_token;
        __u32 dev;
        __u64 ino;
        __u32 uid;
        __u32 gid;
        __u32 pid;
        __u32 tgid;
        __u8  len[4];
        char name[NAME_MAX+1];
};

What do you think?