Re: [Devel] [PATCH] autofs: fix autofs_v5_packet structure for compat mode

[Stanislav Kinsburskiy]

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 
> ---
>  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?
___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH] autofs: fix autofs_v5_packet structure for compat mode

[Dmitry V. Levin]

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


-- 
ldv


signature.asc
Description: PGP signature
___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH] autofs: fix autofs_v5_packet structure for compat mode

[Stanislav Kinsburskiy]

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 
>> ---
>>  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.
___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH] autofs: fix autofs_v5_packet structure for compat mode

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


-- 
ldv


signature.asc
Description: PGP signature
___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH] autofs: fix autofs_v5_packet structure for compat mode

[Stanislav Kinsburskiy]

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 
>> ---
>>  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];
>>  };
>>
>>
>> .
>>
___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH] autofs: fix autofs_v5_packet structure for compat mode

[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 
---
 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];
 };


.


___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH] autofs: fix autofs_v5_packet structure for compat mode

[Stanislav Kinsburskiy]

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 
---
 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];
 };
 

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel