| Message ID | 1422349230-17394-1-git-send-email-fan.du@intel.com |
|---|---|
| State | Awaiting Upstream, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Fan Du > structure like xfrm_usersa_info or xfrm_userpolicy_info > has different sizeof when compiled as 32bits and 64bits > due to not appending pack attribute in their definition. Don't 'pack' the structure, just ensure that all the fields are fixed sized and on their natural boundary. Possibly add a compile-time check that the structure is of the expected size. David -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
David Laight <David.Laight@ACULAB.COM> wrote: > From: Fan Du > > structure like xfrm_usersa_info or xfrm_userpolicy_info > > has different sizeof when compiled as 32bits and 64bits > > due to not appending pack attribute in their definition. > > Don't 'pack' the structure, just ensure that all the fields > are fixed sized and on their natural boundary. How do you propose to do this without breaking ABI? > Possibly add a compile-time check that the structure is > of the expected size. Uh, what? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Florian Westphal > David Laight <David.Laight@ACULAB.COM> wrote: > > From: Fan Du > > > structure like xfrm_usersa_info or xfrm_userpolicy_info > > > has different sizeof when compiled as 32bits and 64bits > > > due to not appending pack attribute in their definition. > > > > Don't 'pack' the structure, just ensure that all the fields > > are fixed sized and on their natural boundary. > > How do you propose to do this without breaking ABI? You may already have an ABI fubar. Adding __packed won't make it go away. IIRC your modified structure did have all its 64bit items aligned on 8 byte boundaries - so the __packed wouldn't affect the structure layout. The problem is that you can't add that field when the ABI only requires 4 byte alignment for 64bit items. If you need to access a structure from (eg) an i386 application within an amd64 kernel then you can add __attribute__((aligned(4))) to any 64bit member to force that member to have only 4 byte aligment. On 64bit systems that can't do misaligned transfers (eg sparc64) that will result in two 32bit accesses for that member. (You'll need to do it to all 64bit members if the structure size needs to be an odd multiple of 4.) > > Possibly add a compile-time check that the structure is > > of the expected size. > > Uh, what? eg something based on: typedef foo_check char[sizeof foo == 32 ? 1 : -1]; There will be a standard define for this somewhere (and for the align attribute). David -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: David Laight <David.Laight@ACULAB.COM> Date: Tue, 27 Jan 2015 09:46:21 +0000 > From: Fan Du >> structure like xfrm_usersa_info or xfrm_userpolicy_info >> has different sizeof when compiled as 32bits and 64bits >> due to not appending pack attribute in their definition. > > Don't 'pack' the structure, just ensure that all the fields > are fixed sized and on their natural boundary. This horse went out of the door more than a decade ago, we can't change the layout of any of these structures and must at some point add code to translate instead. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
于 2015年01月27日 17:46, David Laight 写道: > From: Fan Du >> structure like xfrm_usersa_info or xfrm_userpolicy_info >> has different sizeof when compiled as 32bits and 64bits >> due to not appending pack attribute in their definition. > > Don't 'pack' the structure, just ensure that all the fields > are fixed sized and on their natural boundary. Is that simple?? The layout is exactly the same between 32bits and 64bits, only difference is the last cache line padding, because even if last cache line is not fully occupied, but at least use even bytes, not odd bytes. I think this relates to cache HW behaviour, not how the structure is naturally aligned or not. Actually, the structure members is aligned indeed. #### 64bits struct xfrm_usersa_info { struct xfrm_selector sel; /* 0 56 */ struct xfrm_id id; /* 56 24 */ /* XXX last struct has 3 bytes of padding */ /* --- cacheline 1 boundary (64 bytes) was 16 bytes ago --- */ xfrm_address_t saddr; /* 80 16 */ struct xfrm_lifetime_cfg lft; /* 96 64 */ /* --- cacheline 2 boundary (128 bytes) was 32 bytes ago --- */ struct xfrm_lifetime_cur curlft; /* 160 32 */ /* --- cacheline 3 boundary (192 bytes) --- */ struct xfrm_stats stats; /* 192 12 */ __u32 seq; /* 204 4 */ __u32 reqid; /* 208 4 */ __u16 family; /* 212 2 */ __u8 mode; /* 214 1 */ __u8 replay_window; /* 215 1 */ __u8 flags; /* 216 1 */ /* size: 224, cachelines: 4, members: 12 */ /* padding: 7 */ /* paddings: 1, sum paddings: 3 */ /* last cacheline: 32 bytes */ }; #### 32bits struct xfrm_usersa_info { struct xfrm_selector sel; /* 0 56 */ struct xfrm_id id; /* 56 24 */ /* XXX last struct has 3 bytes of padding */ /* --- cacheline 1 boundary (64 bytes) was 16 bytes ago --- */ xfrm_address_t saddr; /* 80 16 */ struct xfrm_lifetime_cfg lft; /* 96 64 */ /* --- cacheline 2 boundary (128 bytes) was 32 bytes ago --- */ struct xfrm_lifetime_cur curlft; /* 160 32 */ /* --- cacheline 3 boundary (192 bytes) --- */ struct xfrm_stats stats; /* 192 12 */ __u32 seq; /* 204 4 */ __u32 reqid; /* 208 4 */ __u16 family; /* 212 2 */ __u8 mode; /* 214 1 */ __u8 replay_window; /* 215 1 */ __u8 flags; /* 216 1 */ /* size: 220, cachelines: 4, members: 12 */ /* padding: 3 */ /* paddings: 1, sum paddings: 3 */ /* last cacheline: 28 bytes */ }; > Possibly add a compile-time check that the structure is > of the expected size. > > David >
From: David Miller > From: David Laight <David.Laight@ACULAB.COM> > Date: Tue, 27 Jan 2015 09:46:21 +0000 > > > From: Fan Du > >> structure like xfrm_usersa_info or xfrm_userpolicy_info > >> has different sizeof when compiled as 32bits and 64bits > >> due to not appending pack attribute in their definition. > > > > Don't 'pack' the structure, just ensure that all the fields > > are fixed sized and on their natural boundary. > > This horse went out of the door more than a decade ago, we can't > change the layout of any of these structures and must at some point > add code to translate instead. From the other mail it might just be tail-padding. So any adaption code is easy. David -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Le 27/01/2015 10:00, Fan Du a écrit : > structure like xfrm_usersa_info or xfrm_userpolicy_info > has different sizeof when compiled as 32bits and 64bits > due to not appending pack attribute in their definition. > This will result in broken SA and SP information when user > trying to configure them through netlink interface. > > Inform user land about this situation instead of keeping > silent, the upper test scripts would behave accordingly. > > Quotes from: http://marc.info/?l=linux-netdev&m=142226348715503&w=2 >> >> Before a clean solution show up, I think it's better to warn user in some way >> like http://patchwork.ozlabs.org/patch/323842/ did. Otherwise, many people >> who stuck there will always spend time and try to fix this issue in whatever way. > > Yes, this is the first thing we should do. I'm willing to accept a patch > > Signed-off-by: Fan Du <fan.du@intel.com> A way to solve this problem was to provide to userland a xfrm compat header file, which match the ABI of the kernel. Something like: #include <linux/xfrm.h> #define xfrm_usersa_info xfrm_usersa_info_64 #define xfrm_usersa_info_compat xfrm_usersa_info struct xfrm_usersa_info_compat { struct xfrm_selector sel; struct xfrm_id id; xfrm_address_t saddr; struct xfrm_lifetime_cfg lft; struct xfrm_lifetime_cur curlft; struct xfrm_stats stats; __u32 seq; __u32 reqid; __u16 family; __u8 mode; __u8 replay_window; __u8 flags; __u8 hole1; __u32 hole2; }; The point I try to make is that patching userland apps allows to use xfrm on a 32bits userland / 64bits kernel. If I understand well your patch, it will not be possible anymore, all messages will be rejected. And this may break existing apps. Regards, Nicolas -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Le 29/01/2015 14:56, David Laight a écrit : > From: Nicolas Dichtel >> Le 27/01/2015 10:00, Fan Du a crit : >>> structure like xfrm_usersa_info or xfrm_userpolicy_info >>> has different sizeof when compiled as 32bits and 64bits >>> due to not appending pack attribute in their definition. >>> This will result in broken SA and SP information when user >>> trying to configure them through netlink interface. >>> >>> Inform user land about this situation instead of keeping >>> silent, the upper test scripts would behave accordingly. >>> >>> Quotes from: http://marc.info/?l=linux-netdev&m=142226348715503&w=2 >>>> >>>> Before a clean solution show up, I think it's better to warn user in some way >>>> like http://patchwork.ozlabs.org/patch/323842/ did. Otherwise, many people >>>> who stuck there will always spend time and try to fix this issue in whatever way. >>> >>> Yes, this is the first thing we should do. I'm willing to accept a patch >>> >>> Signed-off-by: Fan Du <fan.du@intel.com> >> A way to solve this problem was to provide to userland a xfrm compat header >> file, which match the ABI of the kernel. Something like: >> >> #include <linux/xfrm.h> >> >> #define xfrm_usersa_info xfrm_usersa_info_64 >> #define xfrm_usersa_info_compat xfrm_usersa_info >> struct xfrm_usersa_info_compat { >> struct xfrm_selector sel; >> struct xfrm_id id; >> xfrm_address_t saddr; >> struct xfrm_lifetime_cfg lft; >> struct xfrm_lifetime_cur curlft; >> struct xfrm_stats stats; >> __u32 seq; >> __u32 reqid; >> __u16 family; >> __u8 mode; >> __u8 replay_window; >> __u8 flags; >> __u8 hole1; >> __u32 hole2; >> }; >> >> The point I try to make is that patching userland apps allows to use xfrm on a >> 32bits userland / 64bits kernel. >> >> If I understand well your patch, it will not be possible anymore, all messages >> will be rejected. And this may break existing apps. > > Probably OTT in this case. > IIRC the only actual difference if the 'end padding'. > So the wrapper need only ensure the copyin/out isn't too long. It was just an example for one struct. Some other structures need to be patched (eg struct xfrm_userspi_info uses struct xfrm_usersa_info). And some attributes may follow this structure which will be padded to be aligned. The point was more to show that the existing interface can be used (and cannot be used after the patch). Regards, Nicolas -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
于 2015年01月29日 18:29, Nicolas Dichtel 写道: > Le 27/01/2015 10:00, Fan Du a écrit : >> structure like xfrm_usersa_info or xfrm_userpolicy_info >> has different sizeof when compiled as 32bits and 64bits >> due to not appending pack attribute in their definition. >> This will result in broken SA and SP information when user >> trying to configure them through netlink interface. >> >> Inform user land about this situation instead of keeping >> silent, the upper test scripts would behave accordingly. >> >> Quotes from: http://marc.info/?l=linux-netdev&m=142226348715503&w=2 >>> >>> Before a clean solution show up, I think it's better to warn user in some way >>> like http://patchwork.ozlabs.org/patch/323842/ did. Otherwise, many people >>> who stuck there will always spend time and try to fix this issue in whatever way. >> >> Yes, this is the first thing we should do. I'm willing to accept a patch >> >> Signed-off-by: Fan Du <fan.du@intel.com> > A way to solve this problem was to provide to userland a xfrm compat header > file, which match the ABI of the kernel. Something like: > > #include <linux/xfrm.h> > > #define xfrm_usersa_info xfrm_usersa_info_64 > #define xfrm_usersa_info_compat xfrm_usersa_info > struct xfrm_usersa_info_compat { > struct xfrm_selector sel; > struct xfrm_id id; > xfrm_address_t saddr; > struct xfrm_lifetime_cfg lft; > struct xfrm_lifetime_cur curlft; > struct xfrm_stats stats; > __u32 seq; > __u32 reqid; > __u16 family; > __u8 mode; > __u8 replay_window; > __u8 flags; > __u8 hole1; > __u32 hole2; > }; > > The point I try to make is that patching userland apps allows to use xfrm on a > 32bits userland / 64bits kernel. > > If I understand well your patch, it will not be possible anymore, all messages > will be rejected. And this may break existing apps. Add padding in user space does not fix existing 32bits ip binary, moreover not sure all other ARCHes require the same padding. Maillist has various of proposals AFAICT, all is rejected for reasons whatever for a very long time including padding user space structure. In fact those structures are exposed by uapi from kernel to userspace. Speaking of "break", run 32bits ip xfrm {state/policy/monitor} is _already_ broken on 64bits host. This patch is making user not stumble there by seeing invalid xfrm information and wondering what's going on. They can switch to setkey temporally. last, thanks for your comments after looking at the code... > > Regards, > Nicolas -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jan 29, 2015 at 11:29:51AM +0100, Nicolas Dichtel wrote: > A way to solve this problem was to provide to userland a xfrm compat header > file, which match the ABI of the kernel. Something like: > > #include <linux/xfrm.h> > > #define xfrm_usersa_info xfrm_usersa_info_64 > #define xfrm_usersa_info_compat xfrm_usersa_info > struct xfrm_usersa_info_compat { > struct xfrm_selector sel; > struct xfrm_id id; > xfrm_address_t saddr; > struct xfrm_lifetime_cfg lft; > struct xfrm_lifetime_cur curlft; > struct xfrm_stats stats; > __u32 seq; > __u32 reqid; > __u16 family; > __u8 mode; > __u8 replay_window; > __u8 flags; > __u8 hole1; > __u32 hole2; > }; > > The point I try to make is that patching userland apps allows to use xfrm on a > 32bits userland / 64bits kernel. Ugh, I did not know that this is used that way. Which applications do this? So the situation is worse than I thought. What happens to such applications if we add a compat layer in the kernel? I'd guess they will break, right? > > If I understand well your patch, it will not be possible anymore, all messages > will be rejected. And this may break existing apps. This patch would have been a quick solution without the case you mentioned. Now I fear we can't fix all cases, something will remain broken. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Le 02/02/2015 09:44, Steffen Klassert a écrit : > On Thu, Jan 29, 2015 at 11:29:51AM +0100, Nicolas Dichtel wrote: [snip] >> >> The point I try to make is that patching userland apps allows to use xfrm on a >> 32bits userland / 64bits kernel. > > Ugh, I did not know that this is used that way. Which applications do this? > So the situation is worse than I thought. What happens to such applications > if we add a compat layer in the kernel? I'd guess they will break, right? A compat layer will be perfect. I just wanted to highlight the fact that without this patch, it's possible to have a workaround to use netlink-xfrm and after it, it will be impossible. > >> >> If I understand well your patch, it will not be possible anymore, all messages >> will be rejected. And this may break existing apps. > > This patch would have been a quick solution without the case you > mentioned. Now I fear we can't fix all cases, something will remain > broken. > I think you're right, but having a proper solution is probably the best. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Nicolas Dichtel <nicolas.dichtel@6wind.com> Date: Mon, 02 Feb 2015 10:02:50 +0100 > Le 02/02/2015 09:44, Steffen Klassert a écrit : >> On Thu, Jan 29, 2015 at 11:29:51AM +0100, Nicolas Dichtel wrote: > [snip] >>> >>> The point I try to make is that patching userland apps allows to use >>> xfrm on a >>> 32bits userland / 64bits kernel. >> >> Ugh, I did not know that this is used that way. Which applications do >> this? >> So the situation is worse than I thought. What happens to such >> applications >> if we add a compat layer in the kernel? I'd guess they will break, >> right? > > A compat layer will be perfect. I just wanted to highlight the fact > that without this patch, it's possible to have a workaround to use > netlink-xfrm and after it, it will be impossible. Just a little history, there was a case where we tried to work around this in userspace by messing with the structure definitions when building the userland binaries, and that completely exploded. This was with the wireless extensions about 15 years ago. If you work around it in userspace, then you can't fix the kernel to do the right thing without potentially breaking things again for the work around binaries that have been created. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 02, 2015 at 10:02:50AM +0100, Nicolas Dichtel wrote: > Le 02/02/2015 09:44, Steffen Klassert a écrit : > >On Thu, Jan 29, 2015 at 11:29:51AM +0100, Nicolas Dichtel wrote: > [snip] > >> > >>The point I try to make is that patching userland apps allows to use xfrm on a > >>32bits userland / 64bits kernel. > > > >Ugh, I did not know that this is used that way. Which applications do this? > >So the situation is worse than I thought. What happens to such applications > >if we add a compat layer in the kernel? I'd guess they will break, right? > A compat layer will be perfect. I just wanted to highlight the fact that without > this patch, it's possible to have a workaround to use netlink-xfrm and after it, > it will be impossible. You did not answer my question about the applications that do this. If it is just possible, but there are no actual users, we should apply this patch as soon as possible to avoid any abuse of this ABI. I tend to apply this patch unless you can come up with a real world application that will break if we do so. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Le 03/02/2015 13:24, Steffen Klassert a écrit : > On Mon, Feb 02, 2015 at 10:02:50AM +0100, Nicolas Dichtel wrote: > > I tend to apply this patch unless you can come up with a real world > application that will break if we do so. It's a custom patch made for strongswan (not included upstream). My suggestion was to display an error message instead of rejecting the message. But fair enough, let's apply this patch, it will force people to come up with a proper solution. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jan 27, 2015 at 05:00:29PM +0800, Fan Du wrote: > structure like xfrm_usersa_info or xfrm_userpolicy_info > has different sizeof when compiled as 32bits and 64bits > due to not appending pack attribute in their definition. > This will result in broken SA and SP information when user > trying to configure them through netlink interface. > > Inform user land about this situation instead of keeping > silent, the upper test scripts would behave accordingly. > > Quotes from: http://marc.info/?l=linux-netdev&m=142226348715503&w=2 > > > > Before a clean solution show up, I think it's better to warn user in some way > > like http://patchwork.ozlabs.org/patch/323842/ did. Otherwise, many people > > who stuck there will always spend time and try to fix this issue in whatever way. > > Yes, this is the first thing we should do. I'm willing to accept a patch > > Signed-off-by: Fan Du <fan.du@intel.com> Now applied to ipsec-next, thanks a lot Fan! -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c index 8128594..f960bd9 100644 --- a/net/xfrm/xfrm_user.c +++ b/net/xfrm/xfrm_user.c @@ -2419,6 +2419,11 @@ static int xfrm_user_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh) const struct xfrm_link *link; int type, err; +#ifdef CONFIG_COMPAT + if (is_compat_task()) + return -ENOTSUPP; +#endif + type = nlh->nlmsg_type; if (type > XFRM_MSG_MAX) return -EINVAL;