Message ID | 20180726023144.31066-1-dima@arista.com |
---|---|
Headers | show |
Series | xfrm: Add compat layer | expand |
Dmitry Safonov <dima@arista.com> wrote: > So, here I add a compatible layer to xfrm. > As xfrm uses netlink notifications, kernel should send them in ABI > format that an application will parse. The proposed solution is > to save the ABI of bind() syscall. The realization detail is > to create kernel-hidden, non visible to userspace netlink groups > for compat applications. Why not use exisiting netlink support? Just add the 32bit skb to skb64->frag_list and let netlink find if tasks needs 64 or 32 one. It only needs this small fix to properly signal the end of a dump: https://marc.info/?l=linux-netdev&m=126625240303351&w=2 I had started a second attempt to make xfrm compat work, but its still in early stage. One link that might still have some value: https://git.breakpoint.cc/cgit/fw/net-next.git/commit/?h=xfrm_config_compat_07&id=f64430e6d9e297f3990f485a4832e273751b9869 (compat structure definitions with BUILD_BUG_ON checking) My plan was to make xfrm compat work strictly as shrinker (64->32) and expander (32->64), i.e. no/little changes to exisiting code and pass all "expanded" skbs through existing xfrm rcv functions. Example to illustrate idea: https://git.breakpoint.cc/cgit/fw/net-next.git/commit/?h=xfrm_config_compat_07&id=c622f067849b02170127b69471cb3481e4bc9e49 ... its supposed to take 64bit skb and create a 32bit one from it. Just for reference; I currently don't plan to work on this again.
On Thu, Jul 26, 2018 at 10:49:59AM +0200, Florian Westphal wrote: > Dmitry Safonov <dima@arista.com> wrote: > > So, here I add a compatible layer to xfrm. > > As xfrm uses netlink notifications, kernel should send them in ABI > > format that an application will parse. The proposed solution is > > to save the ABI of bind() syscall. The realization detail is > > to create kernel-hidden, non visible to userspace netlink groups > > for compat applications. > > Why not use exisiting netlink support? > Just add the 32bit skb to skb64->frag_list and let > netlink find if tasks needs 64 or 32 one. > > It only needs this small fix to properly signal the end of a dump: > https://marc.info/?l=linux-netdev&m=126625240303351&w=2 > > I had started a second attempt to make xfrm compat work, > but its still in early stage. > > One link that might still have some value: > https://git.breakpoint.cc/cgit/fw/net-next.git/commit/?h=xfrm_config_compat_07&id=f64430e6d9e297f3990f485a4832e273751b9869 > (compat structure definitions with BUILD_BUG_ON checking) > > My plan was to make xfrm compat work strictly as shrinker (64->32) > and expander (32->64), i.e. no/little changes to exisiting code and > pass all "expanded" skbs through existing xfrm rcv functions. I agree here with Florian. The code behind this ABI is already complicated. Please stay away from generic code a much as possible. Generic and compat code should be clearly separated.
On Fri, 2018-07-27 at 09:37 +0200, Steffen Klassert wrote: > On Thu, Jul 26, 2018 at 10:49:59AM +0200, Florian Westphal wrote: > > Dmitry Safonov <dima@arista.com> wrote: > > > So, here I add a compatible layer to xfrm. > > > As xfrm uses netlink notifications, kernel should send them in > > > ABI > > > format that an application will parse. The proposed solution is > > > to save the ABI of bind() syscall. The realization detail is > > > to create kernel-hidden, non visible to userspace netlink groups > > > for compat applications. > > > > Why not use exisiting netlink support? > > Just add the 32bit skb to skb64->frag_list and let > > netlink find if tasks needs 64 or 32 one. > > > > It only needs this small fix to properly signal the end of a dump: > > https://marc.info/?l=linux-netdev&m=126625240303351&w=2 > > > > I had started a second attempt to make xfrm compat work, > > but its still in early stage. > > > > One link that might still have some value: > > https://git.breakpoint.cc/cgit/fw/net-next.git/commit/?h=xfrm_confi > > g_compat_07&id=f64430e6d9e297f3990f485a4832e273751b9869 > > (compat structure definitions with BUILD_BUG_ON checking) > > > > My plan was to make xfrm compat work strictly as shrinker (64->32) > > and expander (32->64), i.e. no/little changes to exisiting code and > > pass all "expanded" skbs through existing xfrm rcv functions. > > I agree here with Florian. The code behind this ABI > is already complicated. Please stay away from generic > code a much as possible. Generic and compat code should > be clearly separated. Yeah, I tend to agree that it would be better to separate it. But: 1. It will double copy netlink messages, making it O(n) instead of O(1), where n - is number of bind()s.. Probably we don't care much. 2. The patches not-yet-done on the link have +500 added lines - as much as my working patches set, so probably it'll add more code. Probably, we don't care that much about amount of code added and additional copies than about separating compat layer from the main code. Will look into that.
Dmitry Safonov <dima@arista.com> wrote: > 1. It will double copy netlink messages, making it O(n) instead of > O(1), where n - is number of bind()s.. Probably we don't care much. About those bind() patches, I don't understand why they are needed. Why can't you just add the compat skb to the native skb when doing the multicast call? skb_shinfo(skb)->frag_list = compat_skb; xfrm_nlmsg_multicast(net, skb, 0, ...
On Fri, 2018-07-27 at 16:19 +0200, Florian Westphal wrote: > Dmitry Safonov <dima@arista.com> wrote: > > 1. It will double copy netlink messages, making it O(n) instead of > > O(1), where n - is number of bind()s.. Probably we don't care much. > > About those bind() patches, I don't understand why they are needed. > > Why can't you just add the compat skb to the native skb when doing > the multicast call? > > skb_shinfo(skb)->frag_list = compat_skb; > xfrm_nlmsg_multicast(net, skb, 0, ... Oh yeah, sorry, I think I misread the patch - will try to add compat skb in the multicast call.
*We (Android) are very interested in removing the restriction for 32-bit userspace processes accessing xfrm netlink on 64-bit kernels. IPsec support is required to pass Android conformance tests, and any manufacturer wishing to ship 32-bit userspace with a recent kernel needs out-of-tree changes (removing the compat_task check) to do so.That said, it’s not difficult to work around alignment issues directly in userspace, so maybe we could just remove the check and make this the caller's responsibility? Here’s an example of the workaround currently in the Android tree:https://android.googlesource.com/platform/system/netd/+/refs/heads/master/server/XfrmController.h#257 <https://android.googlesource.com/platform/system/netd/+/refs/heads/master/server/XfrmController.h#257>We could also employ a (relatively simple) solution such as the one above in the uapi XFRM header itself, though it would require a caller to declare the target kernel ABI at compile time. Maybe that’s not unthinkable for an uncommon case?-Nathan* On Fri, Jul 27, 2018 at 7:51 AM, Dmitry Safonov <dima@arista.com> wrote: > On Fri, 2018-07-27 at 16:19 +0200, Florian Westphal wrote: > > Dmitry Safonov <dima@arista.com> wrote: > > > 1. It will double copy netlink messages, making it O(n) instead of > > > O(1), where n - is number of bind()s.. Probably we don't care much. > > > > About those bind() patches, I don't understand why they are needed. > > > > Why can't you just add the compat skb to the native skb when doing > > the multicast call? > > > > skb_shinfo(skb)->frag_list = compat_skb; > > xfrm_nlmsg_multicast(net, skb, 0, ... > > Oh yeah, sorry, I think I misread the patch - will try to add compat > skb in the multicast call. > > -- > Thanks, > Dmitry > <div dir="ltr"><b style="font-weight:normal" id="gmail-docs-internal-guid-e4b05990-dca1-4f17-94c5-d2141c339ad6"><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="font-size:11pt;font-family:Arial;color:rgb(0,0,0);background-color:transparent;font-weight:400;font-style:normal;font-variant:normal;text-decoration:none;vertical-align:baseline;white-space:pre-wrap">We (Android) are very interested in removing the restriction for 32-bit userspace processes accessing xfrm netlink on 64-bit kernels. IPsec support is required to pass Android conformance tests, and any manufacturer wishing to ship 32-bit userspace with a recent kernel needs out-of-tree changes (removing the compat_task check) to do so.</span></p><br><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="font-size:11pt;font-family:Arial;color:rgb(0,0,0);background-color:transparent;font-weight:400;font-style:normal;font-variant:normal;text-decoration:none;vertical-align:baseline;white-space:pre-wrap">That said, it’s not difficult to work around alignment issues directly in userspace, so maybe we could just remove the check and make this the caller's responsibility? Here’s an example of the workaround currently in the Android tree:</span></p><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><a href="https://android.googlesource.com/platform/system/netd/+/refs/heads/master/server/XfrmController.h#257" style="text-decoration:none"><span style="font-size:11pt;font-family:Arial;color:rgb(17,85,204);background-color:transparent;font-weight:400;font-style:normal;font-variant:normal;text-decoration:underline;vertical-align:baseline;white-space:pre-wrap">https://android.googlesource.com/platform/system/netd/+/refs/heads/master/server/XfrmController.h#257</span></a></p><br><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="font-size:11pt;font-family:Arial;color:rgb(0,0,0);background-color:transparent;font-weight:400;font-style:normal;font-variant:normal;text-decoration:none;vertical-align:baseline;white-space:pre-wrap">We could also employ a (relatively simple) solution such as the one above in the uapi XFRM header itself, though it would require a caller to declare the target kernel ABI at compile time. Maybe that’s not unthinkable for an uncommon case?</span></p><br><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="font-size:11pt;font-family:Arial;color:rgb(0,0,0);background-color:transparent;font-weight:400;font-style:normal;font-variant:normal;text-decoration:none;vertical-align:baseline;white-space:pre-wrap">-Nathan</span></p></b><br></div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jul 27, 2018 at 7:51 AM, Dmitry Safonov <span dir="ltr"><<a href="mailto:dima@arista.com" target="_blank">dima@arista.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Fri, 2018-07-27 at 16:19 +0200, Florian Westphal wrote:<br> > Dmitry Safonov <<a href="mailto:dima@arista.com">dima@arista.com</a>> wrote:<br> > > 1. It will double copy netlink messages, making it O(n) instead of<br> > > O(1), where n - is number of bind()s.. Probably we don't care much.<br> > <br> > About those bind() patches, I don't understand why they are needed.<br> > <br> > Why can't you just add the compat skb to the native skb when doing<br> > the multicast call?<br> > <br> > skb_shinfo(skb)->frag_list = compat_skb;<br> > xfrm_nlmsg_multicast(net, skb, 0, ...<br> <br> </div></div>Oh yeah, sorry, I think I misread the patch - will try to add compat<br> skb in the multicast call.<br> <span class="HOEnZb"><font color="#888888"><br> -- <br> Thanks,<br> Dmitry<br> </font></span></blockquote></div><br></div>
> On Jul 27, 2018, at 9:48 AM, Nathan Harold <nharold@google.com> wrote: > > We (Android) are very interested in removing the restriction for 32-bit userspace processes accessing xfrm netlink on 64-bit kernels. IPsec support is required to pass Android conformance tests, and any manufacturer wishing to ship 32-bit userspace with a recent kernel needs out-of-tree changes (removing the compat_task check) to do so. > > That said, it’s not difficult to work around alignment issues directly in userspace, so maybe we could just remove the check and make this the caller's responsibility? Here’s an example of the workaround currently in the Android tree: > https://android.googlesource.com/platform/system/netd/+/refs/heads/master/server/XfrmController.h#257 > > We could also employ a (relatively simple) solution such as the one above in the uapi XFRM header itself, though it would require a caller to declare the target kernel ABI at compile time. Maybe that’s not unthinkable for an uncommon case? > Could there just be an XFRM2 that is entirely identical to XFRM for 64-bit userspace but makes the 32-bit structures match? If there are a grand total of two or so userspace implementations, that should cover most use cases. L > -Nathan > > >> On Fri, Jul 27, 2018 at 7:51 AM, Dmitry Safonov <dima@arista.com> wrote: >> On Fri, 2018-07-27 at 16:19 +0200, Florian Westphal wrote: >> > Dmitry Safonov <dima@arista.com> wrote: >> > > 1. It will double copy netlink messages, making it O(n) instead of >> > > O(1), where n - is number of bind()s.. Probably we don't care much. >> > >> > About those bind() patches, I don't understand why they are needed. >> > >> > Why can't you just add the compat skb to the native skb when doing >> > the multicast call? >> > >> > skb_shinfo(skb)->frag_list = compat_skb; >> > xfrm_nlmsg_multicast(net, skb, 0, ... >> >> Oh yeah, sorry, I think I misread the patch - will try to add compat >> skb in the multicast call. >> >> -- >> Thanks, >> Dmitry > <html><head><meta http-equiv="content-type" content="text/html; charset=utf-8"></head><body dir="auto"><br><div><br>On Jul 27, 2018, at 9:48 AM, Nathan Harold <<a href="mailto:nharold@google.com">nharold@google.com</a>> wrote:<br><br></div><blockquote type="cite"><div><div dir="ltr"><b style="font-weight:normal" id="gmail-docs-internal-guid-e4b05990-dca1-4f17-94c5-d2141c339ad6"><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="font-size:11pt;font-family:Arial;color:rgb(0,0,0);background-color:transparent;font-weight:400;font-style:normal;font-variant:normal;text-decoration:none;vertical-align:baseline;white-space:pre-wrap">We (Android) are very interested in removing the restriction for 32-bit userspace processes accessing xfrm netlink on 64-bit kernels. IPsec support is required to pass Android conformance tests, and any manufacturer wishing to ship 32-bit userspace with a recent kernel needs out-of-tree changes (removing the compat_task check) to do so.</span></p><br><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="font-size:11pt;font-family:Arial;color:rgb(0,0,0);background-color:transparent;font-weight:400;font-style:normal;font-variant:normal;text-decoration:none;vertical-align:baseline;white-space:pre-wrap">That said, it’s not difficult to work around alignment issues directly in userspace, so maybe we could just remove the check and make this the caller's responsibility? Here’s an example of the workaround currently in the Android tree:</span></p><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><a href="https://android.googlesource.com/platform/system/netd/+/refs/heads/master/server/XfrmController.h#257" style="text-decoration:none"><span style="font-size:11pt;font-family:Arial;color:rgb(17,85,204);background-color:transparent;font-weight:400;font-style:normal;font-variant:normal;text-decoration:underline;vertical-align:baseline;white-space:pre-wrap">https://android.googlesource.com/platform/system/netd/+/refs/heads/master/server/XfrmController.h#257</span></a></p><br><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="font-size:11pt;font-family:Arial;color:rgb(0,0,0);background-color:transparent;font-weight:400;font-style:normal;font-variant:normal;text-decoration:none;vertical-align:baseline;white-space:pre-wrap">We could also employ a (relatively simple) solution such as the one above in the uapi XFRM header itself, though it would require a caller to declare the target kernel ABI at compile time. Maybe that’s not unthinkable for an uncommon case?</span></p><br></b></div></div></blockquote><div><br></div><div>Could there just be an XFRM2 that is entirely identical to XFRM for 64-bit userspace but makes the 32-bit structures match? If there are a grand total of two or so userspace implementations, that should cover most use cases. L</div><br><blockquote type="cite"><div><div dir="ltr"><b style="font-weight:normal" id="gmail-docs-internal-guid-e4b05990-dca1-4f17-94c5-d2141c339ad6"><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="font-size:11pt;font-family:Arial;color:rgb(0,0,0);background-color:transparent;font-weight:400;font-style:normal;font-variant:normal;text-decoration:none;vertical-align:baseline;white-space:pre-wrap">-Nathan</span></p></b><br></div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jul 27, 2018 at 7:51 AM, Dmitry Safonov <span dir="ltr"><<a href="mailto:dima@arista.com" target="_blank">dima@arista.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Fri, 2018-07-27 at 16:19 +0200, Florian Westphal wrote:<br> > Dmitry Safonov <<a href="mailto:dima@arista.com">dima@arista.com</a>> wrote:<br> > > 1. It will double copy netlink messages, making it O(n) instead of<br> > > O(1), where n - is number of bind()s.. Probably we don't care much.<br> > <br> > About those bind() patches, I don't understand why they are needed.<br> > <br> > Why can't you just add the compat skb to the native skb when doing<br> > the multicast call?<br> > <br> > skb_shinfo(skb)->frag_list = compat_skb;<br> > xfrm_nlmsg_multicast(net, skb, 0, ...<br> <br> </div></div>Oh yeah, sorry, I think I misread the patch - will try to add compat<br> skb in the multicast call.<br> <span class="HOEnZb"><font color="#888888"><br> -- <br> Thanks,<br> Dmitry<br> </font></span></blockquote></div><br></div> </div></blockquote></body></html>
On Fri, 2018-07-27 at 09:48 -0700, Nathan Harold wrote: > We (Android) are very interested in removing the restriction for 32- > bit userspace processes accessing xfrm netlink on 64-bit kernels. > IPsec support is required to pass Android conformance tests, and any > manufacturer wishing to ship 32-bit userspace with a recent kernel > needs out-of-tree changes (removing the compat_task check) to do so. Glad to hear - that justify my attempts more :) > That said, it’s not difficult to work around alignment issues > directly in userspace, so maybe we could just remove the check and > make this the caller's responsibility? Here’s an example of the > workaround currently in the Android tree: > https://android.googlesource.com/platform/system/netd/+/refs/heads/ma > ster/server/XfrmController.h#257 We've kinda same workarounds in our userspace.. But I don't think reverting the check makes much sense - it'll make broken compat ABI in stone. If you're fine with disgraceful hacks and just want to get rid of additional non-mainstream patch - you can make 64-bit syscalls from 32- bit task (hint: examples in x86 selftests). > We could also employ a (relatively simple) solution such as the one > above in the uapi XFRM header itself, though it would require a > caller to declare the target kernel ABI at compile time. Maybe that’s > not unthinkable for an uncommon case? Well, I think, I'll rework my patches set according to critics and separate compat xfrm layer. I've already a selftest to check that 32/64 bit xfrm works - so the most time-taking part is done. So, if you'll wait a week or two - you may help me to justify acception of mainstreaming those patches. > On Fri, Jul 27, 2018 at 7:51 AM, Dmitry Safonov <dima@arista.com> > wrote: > > On Fri, 2018-07-27 at 16:19 +0200, Florian Westphal wrote: > > > Dmitry Safonov <dima@arista.com> wrote: > > > > 1. It will double copy netlink messages, making it O(n) instead > > of > > > > O(1), where n - is number of bind()s.. Probably we don't care > > much. > > > > > > About those bind() patches, I don't understand why they are > > needed. > > > > > > Why can't you just add the compat skb to the native skb when > > doing > > > the multicast call? > > > > > > skb_shinfo(skb)->frag_list = compat_skb; > > > xfrm_nlmsg_multicast(net, skb, 0, ... > > > > Oh yeah, sorry, I think I misread the patch - will try to add > > compat > > skb in the multicast call. > >
From: Dmitry Safonov <dima@arista.com> Date: Sat, 28 Jul 2018 17:26:55 +0100 > Well, I think, I'll rework my patches set according to critics and > separate compat xfrm layer. I've already a selftest to check that 32/64 > bit xfrm works - so the most time-taking part is done. The way you've done the compat structures using __packed is only going to work on x86, just FYI. The "32-bit alignment for 64-bit objects" thing x86 has is very much not universal amongst ABIs having 32-bit and 64-bit variants.
On Sat, 2018-07-28 at 14:18 -0700, David Miller wrote: > From: Dmitry Safonov <dima@arista.com> > Date: Sat, 28 Jul 2018 17:26:55 +0100 > > > Well, I think, I'll rework my patches set according to critics and > > separate compat xfrm layer. I've already a selftest to check that > 32/64 > > bit xfrm works - so the most time-taking part is done. > > The way you've done the compat structures using __packed is only > going > to work on x86, just FYI. Thanks for pointing, so I'll probably cover it under something like HAS_COMPAT_XFRM. (if there isn't any better idea). > The "32-bit alignment for 64-bit objects" thing x86 has is very much > not universal amongst ABIs having 32-bit and 64-bit variants.
Dmitry Safonov <dima@arista.com> wrote: > On Sat, 2018-07-28 at 14:18 -0700, David Miller wrote: > > From: Dmitry Safonov <dima@arista.com> > > Date: Sat, 28 Jul 2018 17:26:55 +0100 > > > > > Well, I think, I'll rework my patches set according to critics and > > > separate compat xfrm layer. I've already a selftest to check that > > 32/64 > > > bit xfrm works - so the most time-taking part is done. > > > > The way you've done the compat structures using __packed is only > > going > > to work on x86, just FYI. > > Thanks for pointing, so I'll probably cover it under something like > HAS_COMPAT_XFRM. > (if there isn't any better idea). You can do that, I suspect you can use CONFIG_COMPAT_FOR_U64_ALIGNMENT as AFAICR the only reason for the compat problem is different alignment requirements of 64bit integer types in the structs, not e.g. due to "long" size differences. Instead of __packed, you can use the "compat" data types, e.g. compat_u64 instead of u64: struct compat_xfrm_lifetime_cur { compat_u64 bytes, packets, add_time, use_time; }; /* same size on i386, but only 4 byte alignment required even on x86_64*/ You might be able to reuse https://git.breakpoint.cc/cgit/fw/net-next.git/commit/?h=xfrm_config_compat_07&id=f64430e6d9e297f3990f485a4832e273751b9869 in your patch set. I can try to submit the first few patches (which are not related to compat, they just add const qualifiers) for inclusion later this week.