[PATCH net-next 0/2] Pack struct xfrm_usersa_info and struct xfrm_userpolicy_info

[PATCH net-next 0/2] Pack struct xfrm_usersa_info and struct xfrm_userpolicy_info

[Fan Du]

When trying to setup IPsec configuration on a 64bits host with
iproute2(32bits compiled), the intened xfrm policy and sa is
either deficit or wrong when kernel trying to parse user land
information.

Further investigatino shows that:
L: kernel
R: iproute2

          sizeof       userpolicy      usersa
64bits(unpacked)        168/168        224/224
32bits(unpacked)        164/164        220/220
                         ^   ^
                         L   R

To keep kernel and user land see a consistent structure, after
add packing attribute, now it looks like this:

64bits(  packed)        164/164        217/217
32bits(  packed)        164/164        217/217
                         ^   ^
                         L   R

Then different kernel/iproute2 build configuration will not impact IPsec setup.

Fan Du (2):
  include/uapi/linux/xfrm.h: Pack struct xfrm_userpolicy_info
  include/uapi/linux/xfrm.h: Pack struct xfrm_usersa_info

 include/uapi/linux/xfrm.h |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

-- 
1.7.9.5

[PATCH net-next 1/2] include/uapi/linux/xfrm.h: Pack struct xfrm_userpolicy_info

[Fan Du]

Otherwise 64bits kernel has sizeof(struct xfrm_userpolicy_info) 168 bytes,
while 32bits compiled iproute2 see the same structure as 164 bytes, which
leading deficit xfrm policy, in turn broken IPsec connectivity.

Fix this by packing the structure.

Signed-off-by: Fan Du <fan.du@windriver.com>
---
 include/uapi/linux/xfrm.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/uapi/linux/xfrm.h b/include/uapi/linux/xfrm.h
index a8cd6a4..470bfae 100644
--- a/include/uapi/linux/xfrm.h
+++ b/include/uapi/linux/xfrm.h
@@ -405,7 +405,7 @@ struct xfrm_userpolicy_info {
 	/* Automatically expand selector to include matching ICMP payloads. */
 #define XFRM_POLICY_ICMP	2
 	__u8				share;
-};
+} __attribute__((packed));
 
 struct xfrm_userpolicy_id {
 	struct xfrm_selector		sel;
-- 
1.7.9.5

Re: [PATCH net-next 1/2] include/uapi/linux/xfrm.h: Pack struct xfrm_userpolicy_info

[Sergei Shtylyov]

Hello.

On 07-01-2014 10:48, Fan Du wrote:

> Otherwise 64bits kernel has sizeof(struct xfrm_userpolicy_info) 168 bytes,
> while 32bits compiled iproute2 see the same structure as 164 bytes, which
> leading deficit xfrm policy, in turn broken IPsec connectivity.

> Fix this by packing the structure.

    This will force byte-by-byte access to all members on some arches like ARM...

> Signed-off-by: Fan Du <fan.du@windriver.com>
> ---
>   include/uapi/linux/xfrm.h |    2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

> diff --git a/include/uapi/linux/xfrm.h b/include/uapi/linux/xfrm.h
> index a8cd6a4..470bfae 100644
> --- a/include/uapi/linux/xfrm.h
> +++ b/include/uapi/linux/xfrm.h
> @@ -405,7 +405,7 @@ struct xfrm_userpolicy_info {
>   	/* Automatically expand selector to include matching ICMP payloads. */
>   #define XFRM_POLICY_ICMP	2
>   	__u8				share;
> -};
> +} __attribute__((packed));

    Please use the __packed macro instead. I guess you haven't run checkpatch.pl?

WBR, Sergei

Re: [PATCH net-next 1/2] include/uapi/linux/xfrm.h: Pack struct xfrm_userpolicy_info

[Fan Du]

On 2014年01月08日 06:52, Sergei Shtylyov wrote:
> Hello.
>
> On 07-01-2014 10:48, Fan Du wrote:
>
>> Otherwise 64bits kernel has sizeof(struct xfrm_userpolicy_info) 168 bytes,
>> while 32bits compiled iproute2 see the same structure as 164 bytes, which
>> leading deficit xfrm policy, in turn broken IPsec connectivity.
>
>> Fix this by packing the structure.
>
>     This will force byte-by-byte access to all members on some arches like ARM...
>
>> Signed-off-by: Fan Du <fan.du@windriver.com>
>> ---
>>   include/uapi/linux/xfrm.h |    2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>
>> diff --git a/include/uapi/linux/xfrm.h b/include/uapi/linux/xfrm.h
>> index a8cd6a4..470bfae 100644
>> --- a/include/uapi/linux/xfrm.h
>> +++ b/include/uapi/linux/xfrm.h
>> @@ -405,7 +405,7 @@ struct xfrm_userpolicy_info {
>>       /* Automatically expand selector to include matching ICMP payloads. */
>>   #define XFRM_POLICY_ICMP    2
>>       __u8                share;
>> -};
>> +} __attribute__((packed));
>
>     Please use the __packed macro instead. I guess you haven't run checkpatch.pl?

Lucky me, I run checkpatch every time before sending patch out.

-- 
浮沉随浪只记今朝笑

--fan

_______________________________________________
Dev mailing list
Dev@lists.strongswan.org
https://lists.strongswan.org/mailman/listinfo/dev

Re: [PATCH net-next 1/2] include/uapi/linux/xfrm.h: Pack struct xfrm_userpolicy_info

[Sergei Shtylyov]

On 01/09/2014 11:39 AM, Fan Du wrote:

>>> Otherwise 64bits kernel has sizeof(struct xfrm_userpolicy_info) 168 bytes,
>>> while 32bits compiled iproute2 see the same structure as 164 bytes, which
>>> leading deficit xfrm policy, in turn broken IPsec connectivity.

>>> Fix this by packing the structure.

>>     This will force byte-by-byte access to all members on some arches like
>> ARM...

>>> Signed-off-by: Fan Du <fan.du@windriver.com>
>>> ---
>>>   include/uapi/linux/xfrm.h |    2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)

>>> diff --git a/include/uapi/linux/xfrm.h b/include/uapi/linux/xfrm.h
>>> index a8cd6a4..470bfae 100644
>>> --- a/include/uapi/linux/xfrm.h
>>> +++ b/include/uapi/linux/xfrm.h
>>> @@ -405,7 +405,7 @@ struct xfrm_userpolicy_info {
>>>       /* Automatically expand selector to include matching ICMP payloads. */
>>>   #define XFRM_POLICY_ICMP    2
>>>       __u8                share;
>>> -};
>>> +} __attribute__((packed));

>>     Please use the __packed macro instead. I guess you haven't run
>> checkpatch.pl?

> Lucky me, I run checkpatch every time before sending patch out.

    Ah, this time it didn't have the *struct* start in the context, so that's 
why there was no complaint (probably). Usually, it suggests using __packed...

WBR, Sergei

Re: [PATCH net-next 1/2] include/uapi/linux/xfrm.h: Pack struct xfrm_userpolicy_info

[Sergei Shtylyov]

On 01/10/2014 01:58 AM, Sergei Shtylyov wrote:

>>>> Otherwise 64bits kernel has sizeof(struct xfrm_userpolicy_info) 168 bytes,
>>>> while 32bits compiled iproute2 see the same structure as 164 bytes, which
>>>> leading deficit xfrm policy, in turn broken IPsec connectivity.

>>>> Fix this by packing the structure.

>>>     This will force byte-by-byte access to all members on some arches like
>>> ARM...

>>>> Signed-off-by: Fan Du <fan.du@windriver.com>
>>>> ---
>>>>   include/uapi/linux/xfrm.h |    2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)

>>>> diff --git a/include/uapi/linux/xfrm.h b/include/uapi/linux/xfrm.h
>>>> index a8cd6a4..470bfae 100644
>>>> --- a/include/uapi/linux/xfrm.h
>>>> +++ b/include/uapi/linux/xfrm.h
>>>> @@ -405,7 +405,7 @@ struct xfrm_userpolicy_info {
>>>>       /* Automatically expand selector to include matching ICMP payloads. */
>>>>   #define XFRM_POLICY_ICMP    2
>>>>       __u8                share;
>>>> -};
>>>> +} __attribute__((packed));

>>>     Please use the __packed macro instead. I guess you haven't run
>>> checkpatch.pl?

>> Lucky me, I run checkpatch every time before sending patch out.

>     Ah, this time it didn't have the *struct* start in the context, so that's
> why there was no complaint (probably). Usually, it suggests using __packed...

    Ah, no. Looking at the script, it just doesn't WARN about this in the 
files under include/uapi/. Probably the macro is undefined in this context.

WBR, Sergei

[PATCH net-next 2/2] include/uapi/linux/xfrm.h: Pack struct xfrm_usersa_info

[Fan Du]

Otherwise 64bits kernel has sizeof(struct xfrm_usersa_info) 224 bytes,
while 32bits compiled iproute2 see the same structure as 220 bytes, which
leading deficit xfrm sa, in turn broken IPsec connectivity.

Fix this by packing the structure.

Signed-off-by: Fan Du <fan.du@windriver.com>
---
 include/uapi/linux/xfrm.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/uapi/linux/xfrm.h b/include/uapi/linux/xfrm.h
index 470bfae..61460c4 100644
--- a/include/uapi/linux/xfrm.h
+++ b/include/uapi/linux/xfrm.h
@@ -366,7 +366,7 @@ struct xfrm_usersa_info {
 #define XFRM_STATE_AF_UNSPEC	32
 #define XFRM_STATE_ALIGN4	64
 #define XFRM_STATE_ESN		128
-};
+} __attribute__((packed));
 
 #define XFRM_SA_XFLAG_DONT_ENCAP_DSCP	1
 
-- 
1.7.9.5

Re: [PATCH net-next 2/2] include/uapi/linux/xfrm.h: Pack struct xfrm_usersa_info

[Ben Hutchings]

On Tue, 2014-01-07 at 14:48 +0800, Fan Du wrote:
> Otherwise 64bits kernel has sizeof(struct xfrm_usersa_info) 224 bytes,
> while 32bits compiled iproute2 see the same structure as 220 bytes, which
> leading deficit xfrm sa, in turn broken IPsec connectivity.
>
> Fix this by packing the structure.
> 
> Signed-off-by: Fan Du <fan.du@windriver.com>
> ---
>  include/uapi/linux/xfrm.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/xfrm.h b/include/uapi/linux/xfrm.h
> index 470bfae..61460c4 100644
> --- a/include/uapi/linux/xfrm.h
> +++ b/include/uapi/linux/xfrm.h
> @@ -366,7 +366,7 @@ struct xfrm_usersa_info {
>  #define XFRM_STATE_AF_UNSPEC	32
>  #define XFRM_STATE_ALIGN4	64
>  #define XFRM_STATE_ESN		128
> -};
> +} __attribute__((packed));
>  
>  #define XFRM_SA_XFLAG_DONT_ENCAP_DSCP	1
>  

That change will make access to the structure very slow on some
architectures, and I suspect it will cause other compatibility problems.

I think the right thing to do is to reduce the minimum length of the
structure in the netlink policy so that padding at the end is not
required.  (It looks like all field offsets will be the same on all
32/64-bit architecture pairs and there is only a differing amount of
padding at the end of the structure for 32/64-bit alignment.)

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH net-next 2/2] include/uapi/linux/xfrm.h: Pack struct xfrm_usersa_info

[Fan Du]

On 2014年01月09日 04:33, Ben Hutchings wrote:
> On Tue, 2014-01-07 at 14:48 +0800, Fan Du wrote:
>> Otherwise 64bits kernel has sizeof(struct xfrm_usersa_info) 224 bytes,
>> while 32bits compiled iproute2 see the same structure as 220 bytes, which
>> leading deficit xfrm sa, in turn broken IPsec connectivity.
>>
>> Fix this by packing the structure.
>>
>> Signed-off-by: Fan Du<fan.du@windriver.com>
>> ---
>>   include/uapi/linux/xfrm.h |    2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/uapi/linux/xfrm.h b/include/uapi/linux/xfrm.h
>> index 470bfae..61460c4 100644
>> --- a/include/uapi/linux/xfrm.h
>> +++ b/include/uapi/linux/xfrm.h
>> @@ -366,7 +366,7 @@ struct xfrm_usersa_info {
>>   #define XFRM_STATE_AF_UNSPEC	32
>>   #define XFRM_STATE_ALIGN4	64
>>   #define XFRM_STATE_ESN		128
>> -};
>> +} __attribute__((packed));
>>
>>   #define XFRM_SA_XFLAG_DONT_ENCAP_DSCP	1
>>
>
> That change will make access to the structure very slow on some
> architectures, and I suspect it will cause other compatibility problems.
>
> I think the right thing to do is to reduce the minimum length of the
> structure in the netlink policy so that padding at the end is not
> required.

Could you please be more specific about this? Thanks.

I'm afraid we can only rearrange structure member order to reduce size
on 64bits, alas that's not feasible here :(

  (It looks like all field offsets will be the same on all
> 32/64-bit architecture pairs and there is only a differing amount of
> padding at the end of the structure for 32/64-bit alignment.)
>
> Ben.
>

-- 
浮沉随浪只记今朝笑

--fan

_______________________________________________
Dev mailing list
Dev@lists.strongswan.org
https://lists.strongswan.org/mailman/listinfo/dev

Re: [PATCH net-next 2/2] include/uapi/linux/xfrm.h: Pack struct xfrm_usersa_info

[Ben Hutchings]

On Thu, 2014-01-09 at 16:24 +0800, Fan Du wrote:
> 
> On 2014年01月09日 04:33, Ben Hutchings wrote:
> > On Tue, 2014-01-07 at 14:48 +0800, Fan Du wrote:
> >> Otherwise 64bits kernel has sizeof(struct xfrm_usersa_info) 224 bytes,
> >> while 32bits compiled iproute2 see the same structure as 220 bytes, which
> >> leading deficit xfrm sa, in turn broken IPsec connectivity.
> >>
> >> Fix this by packing the structure.
> >>
> >> Signed-off-by: Fan Du<fan.du@windriver.com>
> >> ---
> >>   include/uapi/linux/xfrm.h |    2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/include/uapi/linux/xfrm.h b/include/uapi/linux/xfrm.h
> >> index 470bfae..61460c4 100644
> >> --- a/include/uapi/linux/xfrm.h
> >> +++ b/include/uapi/linux/xfrm.h
> >> @@ -366,7 +366,7 @@ struct xfrm_usersa_info {
> >>   #define XFRM_STATE_AF_UNSPEC	32
> >>   #define XFRM_STATE_ALIGN4	64
> >>   #define XFRM_STATE_ESN		128
> >> -};
> >> +} __attribute__((packed));
> >>
> >>   #define XFRM_SA_XFLAG_DONT_ENCAP_DSCP	1
> >>
> >
> > That change will make access to the structure very slow on some
> > architectures, and I suspect it will cause other compatibility problems.
> >
> > I think the right thing to do is to reduce the minimum length of the
> > structure in the netlink policy so that padding at the end is not
> > required.
> 
> Could you please be more specific about this? Thanks.
[...]

Each netlink message and attribute has a specified minimum size, and
messages are rejected if the size provided by the sender is too small.
The minimum size is normally the same as the structure size, but doesn't
have to be.

Something like this might work:

--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -2266,8 +2266,15 @@ static int xfrm_send_migrate(const struct xfrm_selector *sel, u8 dir, u8 type,
 
 #define XMSGSIZE(type) sizeof(struct type)
 
+/* Padding at the end of struct xfrm_usersa_info differs between
+ * architectures so for 32/64-bit compat we don't require padding
+ */
+#define XMSGSIZE_XFRM_USERSA_INFO				\
+	(offsetof(struct xfrm_usersa_info, flags) +		\
+	 sizeof(((struct xfrm_usersa_info *)NULL)->flags))
+
 static const int xfrm_msg_min[XFRM_NR_MSGTYPES] = {
-	[XFRM_MSG_NEWSA       - XFRM_MSG_BASE] = XMSGSIZE(xfrm_usersa_info),
+	[XFRM_MSG_NEWSA       - XFRM_MSG_BASE] = XMSGSIZE_XFRM_USERSA_INFO,
 	[XFRM_MSG_DELSA       - XFRM_MSG_BASE] = XMSGSIZE(xfrm_usersa_id),
 	[XFRM_MSG_GETSA       - XFRM_MSG_BASE] = XMSGSIZE(xfrm_usersa_id),
 	[XFRM_MSG_NEWPOLICY   - XFRM_MSG_BASE] = XMSGSIZE(xfrm_userpolicy_info),
@@ -2277,7 +2284,7 @@ static const int xfrm_msg_min[XFRM_NR_MSGTYPES] = {
 	[XFRM_MSG_ACQUIRE     - XFRM_MSG_BASE] = XMSGSIZE(xfrm_user_acquire),
 	[XFRM_MSG_EXPIRE      - XFRM_MSG_BASE] = XMSGSIZE(xfrm_user_expire),
 	[XFRM_MSG_UPDPOLICY   - XFRM_MSG_BASE] = XMSGSIZE(xfrm_userpolicy_info),
-	[XFRM_MSG_UPDSA       - XFRM_MSG_BASE] = XMSGSIZE(xfrm_usersa_info),
+	[XFRM_MSG_UPDSA       - XFRM_MSG_BASE] = XMSGSIZE_XFRM_USERSA_INFO,
 	[XFRM_MSG_POLEXPIRE   - XFRM_MSG_BASE] = XMSGSIZE(xfrm_user_polexpire),
 	[XFRM_MSG_FLUSHSA     - XFRM_MSG_BASE] = XMSGSIZE(xfrm_usersa_flush),
 	[XFRM_MSG_FLUSHPOLICY - XFRM_MSG_BASE] = 0,
@@ -2292,7 +2299,7 @@ static const int xfrm_msg_min[XFRM_NR_MSGTYPES] = {
 #undef XMSGSIZE
 
 static const struct nla_policy xfrma_policy[XFRMA_MAX+1] = {
-	[XFRMA_SA]		= { .len = sizeof(struct xfrm_usersa_info)},
+	[XFRMA_SA]		= { .len = XMSGSIZE_XFRM_USERSA_INFO },
 	[XFRMA_POLICY]		= { .len = sizeof(struct xfrm_userpolicy_info)},
 	[XFRMA_LASTUSED]	= { .type = NLA_U64},
 	[XFRMA_ALG_AUTH_TRUNC]	= { .len = sizeof(struct xfrm_algo_auth)},
---

But beware that if the structure is copied in and then copied back out
to userland then the padding will need to be explicitly cleared.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH net-next 0/2] Pack struct xfrm_usersa_info and struct xfrm_userpolicy_info

[Fan Du]

Oh, I don't know why '[strongSwan-dev]' is tagged in front of subject for only part of whole patch set.
In fact, I didn't mean to add '[strongSwan-dev]' into subject line. Sorry for the inconvenience.

On 2014年01月07日 14:48, Fan Du wrote:
> When trying to setup IPsec configuration on a 64bits host with
> iproute2(32bits compiled), the intened xfrm policy and sa is
> either deficit or wrong when kernel trying to parse user land
> information.
>
> Further investigatino shows that:
> L: kernel
> R: iproute2
>
>            sizeof       userpolicy      usersa
> 64bits(unpacked)        168/168        224/224
> 32bits(unpacked)        164/164        220/220
>                           ^   ^
>                           L   R
>
> To keep kernel and user land see a consistent structure, after
> add packing attribute, now it looks like this:
>
> 64bits(  packed)        164/164        217/217
> 32bits(  packed)        164/164        217/217
>                           ^   ^
>                           L   R
>
> Then different kernel/iproute2 build configuration will not impact IPsec setup.
>
> Fan Du (2):
>    include/uapi/linux/xfrm.h: Pack struct xfrm_userpolicy_info
>    include/uapi/linux/xfrm.h: Pack struct xfrm_usersa_info
>
>   include/uapi/linux/xfrm.h |    4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>

-- 
浮沉随浪只记今朝笑

--fan

_______________________________________________
Dev mailing list
Dev@lists.strongswan.org
https://lists.strongswan.org/mailman/listinfo/dev

Re: [PATCH net-next 0/2] Pack struct xfrm_usersa_info and struct xfrm_userpolicy_info

[Steffen Klassert]

On Tue, Jan 07, 2014 at 02:48:57PM +0800, Fan Du wrote:
> When trying to setup IPsec configuration on a 64bits host with
> iproute2(32bits compiled), the intened xfrm policy and sa is
> either deficit or wrong when kernel trying to parse user land
> information.
> 
> Further investigatino shows that:
> L: kernel
> R: iproute2
> 
>           sizeof       userpolicy      usersa
> 64bits(unpacked)        168/168        224/224
> 32bits(unpacked)        164/164        220/220
>                          ^   ^
>                          L   R
> 
> To keep kernel and user land see a consistent structure, after
> add packing attribute, now it looks like this:
> 
> 64bits(  packed)        164/164        217/217
> 32bits(  packed)        164/164        217/217
>                          ^   ^
>                          L   R
> 

We don't change userspace exported structures. This breaks
existing userspace tools.

Re: [PATCH net-next 0/2] Pack struct xfrm_usersa_info and struct xfrm_userpolicy_info

[Fan Du]

On 2014年01月07日 15:47, Steffen Klassert wrote:
> On Tue, Jan 07, 2014 at 02:48:57PM +0800, Fan Du wrote:
>> When trying to setup IPsec configuration on a 64bits host with
>> iproute2(32bits compiled), the intened xfrm policy and sa is
>> either deficit or wrong when kernel trying to parse user land
>> information.
>>
>> Further investigatino shows that:
>> L: kernel
>> R: iproute2
>>
>>            sizeof       userpolicy      usersa
>> 64bits(unpacked)        168/168        224/224
>> 32bits(unpacked)        164/164        220/220
>>                           ^   ^
>>                           L   R
>>
>> To keep kernel and user land see a consistent structure, after
>> add packing attribute, now it looks like this:
>>
>> 64bits(  packed)        164/164        217/217
>> 32bits(  packed)        164/164        217/217
>>                           ^   ^
>>                           L   R
>>
>
> We don't change userspace exported structures. This breaks
> existing userspace tools.
>

Then user with 32bits iproute2 or StrongSwan has to rebuild as 64bits?

-- 
浮沉随浪只记今朝笑

--fan

_______________________________________________
Dev mailing list
Dev@lists.strongswan.org
https://lists.strongswan.org/mailman/listinfo/dev

RE: [PATCH net-next 0/2] Pack struct xfrm_usersa_info and struct xfrm_userpolicy_info

[David Laight]

> From: Fan Du
> Sent: 07 January 2014 08:00
> On 2014?01?07? 15:47, Steffen Klassert wrote:
> > On Tue, Jan 07, 2014 at 02:48:57PM +0800, Fan Du wrote:
> >> When trying to setup IPsec configuration on a 64bits host with
> >> iproute2(32bits compiled), the intened xfrm policy and sa is
> >> either deficit or wrong when kernel trying to parse user land
> >> information.
> >>
> >> Further investigatino shows that:
> >> L: kernel
> >> R: iproute2
> >>
> >>            sizeof       userpolicy      usersa
> >> 64bits(unpacked)        168/168        224/224
> >> 32bits(unpacked)        164/164        220/220
> >>                           ^   ^
> >>                           L   R
> >>
> >> To keep kernel and user land see a consistent structure, after
> >> add packing attribute, now it looks like this:
> >>
> >> 64bits(  packed)        164/164        217/217
> >> 32bits(  packed)        164/164        217/217
> >>                           ^   ^
> >>                           L   R
> >>
> >
> > We don't change userspace exported structures. This breaks
> > existing userspace tools.
> >
> 
> Then user with 32bits iproute2 or StrongSwan has to rebuild as 64bits?

The kernel has support (in various places) for different structure
layouts for 32bit and 64bit processes.
Looks like it needs one here as well (I'm not volunteering though).

At a guess the structures contain a 64bit field that is on an 8n+4
byte boundary. On i386 64bit fields are only 4 byte aligned, on
amd64 they are 8 byte aligned.

Packing the structures is definitely wrong. Some 32bit systems (IIRC sparc)
align 64bit items on 8 byte boundaries. Not to mention the expensive byte
by byte accesses this forces on some systems.

The structure could have been defined with the 64bit field marked
__attribute__((aligned(4))) - so using the same layout on 32bit
and 64 bit systems , but it is too late to do that now.

	David

Re: [PATCH net-next 0/2] Pack struct xfrm_usersa_info and struct xfrm_userpolicy_info

[Fan Du]

On 2014年01月07日 18:00, David Laight wrote:
>> From: Fan Du
>> >  Sent: 07 January 2014 08:00
>> >  On 2014?01?07? 15:47, Steffen Klassert wrote:
>>> >  >  On Tue, Jan 07, 2014 at 02:48:57PM +0800, Fan Du wrote:
>>>> >  >>  When trying to setup IPsec configuration on a 64bits host with
>>>> >  >>  iproute2(32bits compiled), the intened xfrm policy and sa is
>>>> >  >>  either deficit or wrong when kernel trying to parse user land
>>>> >  >>  information.
>>>> >  >>
>>>> >  >>  Further investigatino shows that:
>>>> >  >>  L: kernel
>>>> >  >>  R: iproute2
>>>> >  >>
>>>> >  >>              sizeof       userpolicy      usersa
>>>> >  >>  64bits(unpacked)        168/168        224/224
>>>> >  >>  32bits(unpacked)        164/164        220/220
>>>> >  >>                             ^   ^
>>>> >  >>                             L   R
>>>> >  >>
>>>> >  >>  To keep kernel and user land see a consistent structure, after
>>>> >  >>  add packing attribute, now it looks like this:
>>>> >  >>
>>>> >  >>  64bits(  packed)        164/164        217/217
>>>> >  >>  32bits(  packed)        164/164        217/217
>>>> >  >>                             ^   ^
>>>> >  >>                             L   R
>>>> >  >>
>>> >  >
>>> >  >  We don't change userspace exported structures. This breaks
>>> >  >  existing userspace tools.
>>> >  >
>> >
>> >  Then user with 32bits iproute2 or StrongSwan has to rebuild as 64bits?
> The kernel has support (in various places) for different structure
> layouts for 32bit and 64bit processes.
> Looks like it needs one here as well (I'm not volunteering though).
>
> At a guess the structures contain a 64bit field that is on an 8n+4
> byte boundary. On i386 64bit fields are only 4 byte aligned, on
> amd64 they are 8 byte aligned.
>
> Packing the structures is definitely wrong. Some 32bit systems (IIRC sparc)
> align 64bit items on 8 byte boundaries. Not to mention the expensive byte
> by byte accesses this forces on some systems.
I don't know much about sparc, if I read your message right, you mean 32bit sparc
system also has padding even if pack attribute is supplied.

And in this scenario, the code path here is only for configuration, i.e., not hot
path, so performance issue is not so crucial here.

>
> The structure could have been defined with the 64bit field marked
> __attribute__((aligned(4))) - so using the same layout on 32bit
> and 64 bit systems , but it is too late to do that now.

Thanks for this useful information.

-- 
浮沉随浪只记今朝笑

--fan

_______________________________________________
Dev mailing list
Dev@lists.strongswan.org
https://lists.strongswan.org/mailman/listinfo/dev

RE: [PATCH net-next 0/2] Pack struct xfrm_usersa_info and struct xfrm_userpolicy_info

[David Laight]

> From: Fan Du 
> > Packing the structures is definitely wrong. Some 32bit systems (IIRC sparc)
> > align 64bit items on 8 byte boundaries. Not to mention the expensive byte
> > by byte accesses this forces on some systems.
>
> I don't know much about sparc, if I read your message right, you mean 32bit sparc
> system also has padding even if pack attribute is supplied.

Without packing:
On 32bit x86 64bit integers are aligned on 4byte boundaries.
On 32bit sparc 64bit integers are aligned on 8byte boundaries.

If the structure is packed then, for sparc (etc), the compiler has
to generate code to read everything using byte accesses.

	David

Re: [PATCH net-next 0/2] Pack struct xfrm_usersa_info and struct xfrm_userpolicy_info

[David Miller]

From: Fan Du <fan.du@windriver.com>
Date: Tue, 7 Jan 2014 14:48:57 +0800

> When trying to setup IPsec configuration on a 64bits host with
> iproute2(32bits compiled), the intened xfrm policy and sa is
> either deficit or wrong when kernel trying to parse user land
> information.

You can't make this change without breaking userspace.

We'll have to translate the data structures somehow with a compat
layer like we have for 32/64 bit compatability for system calls.

Re: [PATCH net-next 0/2] Pack struct xfrm_usersa_info and struct xfrm_userpolicy_info

[Fan Du]

On 2014年01月08日 02:07, David Miller wrote:
> From: Fan Du<fan.du@windriver.com>
> Date: Tue, 7 Jan 2014 14:48:57 +0800
>
>> >  When trying to setup IPsec configuration on a 64bits host with
>> >  iproute2(32bits compiled), the intened xfrm policy and sa is
>> >  either deficit or wrong when kernel trying to parse user land
>> >  information.
> You can't make this change without breaking userspace.
>
> We'll have to translate the data structures somehow with a compat
> layer like we have for 32/64 bit compatability for system calls.

Do you mean below code snippet?

compat_sys_sendmsg
   -> __sys_sendmsg
     -> ___sys_sendmsg
       -> get_compat_msghdr <- translate the structure here

It's a bit of late in the code flow to do the translation in
xfrm_user code, or I missed what you actually mean.

-- 
浮沉随浪只记今朝笑

--fan

_______________________________________________
Dev mailing list
Dev@lists.strongswan.org
https://lists.strongswan.org/mailman/listinfo/dev