From: Paul Moore <paul@paul-moore.com> Date: 2017-07-21 22:19:08
Hello,
I've been seeing a SELinux regression with IP_PASSSEC on the v4.13-rcX
kernels and finally tracked the problem down to the
skb_release_head_state() call in __udp_queue_rcv_skb(). Looking at
the code and the git log it would appear that the likely culprit is
0a463c78d25b ("udp: avoid a cache miss on dequeue
"); it looks similar to IP option problem fixed in 0ddf3fb2c43d2.
From a SELinux/IP_PASSSEC point of view we need access to the skb->sp
pointer to examine the SAs. I'm posting this here without a patch
because it isn't clear to me how you would like to fix the problem; my
initial thought would be to simply make the skb_release_head_state()
conditional on the skb->sp pointer, much like the IP options fix, but
I'm not sure if you have a more clever idea.
--
paul moore
www.paul-moore.com
From: Paolo Abeni <pabeni@redhat.com> Date: 2017-07-24 12:25:19
Hi,
On Fri, 2017-07-21 at 18:19 -0400, Paul Moore wrote:
I've been seeing a SELinux regression with IP_PASSSEC on the v4.13-rcX
kernels and finally tracked the problem down to the
skb_release_head_state() call in __udp_queue_rcv_skb(). Looking at
the code and the git log it would appear that the likely culprit is
0a463c78d25b ("udp: avoid a cache miss on dequeue
"); it looks similar to IP option problem fixed in 0ddf3fb2c43d2.
Thank you for the report!
My bad, I completely missed that code path.
From a SELinux/IP_PASSSEC point of view we need access to the skb->sp
pointer to examine the SAs. I'm posting this here without a patch
because it isn't clear to me how you would like to fix the problem; my
initial thought would be to simply make the skb_release_head_state()
conditional on the skb->sp pointer, much like the IP options fix, but
I'm not sure if you have a more clever idea.
Unfortunately explicitly checking skb->sp at skb free time will defeat
completely the intended optimization.
To preserve it, something like the following patch is required, could
you please test it in your environment?
Such patch is still prone to a kind of race, as only UDP packets
enqueued to the UDP socket after the setsockopt() will carry the
relevant cmsg info.
e.g. with the following event sequence:
<an UDP packet is enqueued to the revevant socket>
setsockopt(...,IP_CMSG_PASSSEC)
recvmsg(...);
the ancillary message data will not include the IP_CMSG_PASSSEC, while
kernel pre 0a463c78d25b will provide it. Do you think such behavior
will be acceptable?
If not, I fear a revert will be needed.
Cheers,
Paolo
---
unlock_sock_fast(sk, slow);
}
- /* we cleared the head states previously only if the skb lacks any IP
- * options, see __udp_queue_rcv_skb().
+ /* In the more common cases we cleared the head states previously,
+ * see __udp_queue_rcv_skb().
*/
- if (unlikely(IPCB(skb)->opt.optlen > 0))
+ if (unlikely(udp_skb_has_head_state(skb)))
skb_release_head_state(skb);
consume_stateless_skb(skb);
}
sk_mark_napi_id_once(sk, skb);
}
- /* At recvmsg() time we need skb->dst to process IP options-related
- * cmsg, elsewhere can we clear all pending head states while they are
- * hot in the cache
+ /* At recvmsg() time we may access skb->dst or skb->sp depending on
+ * the IP options and the cmsg flags, elsewhere can we clear all
+ * pending head states while they are hot in the cache
*/
- if (likely(IPCB(skb)->opt.optlen == 0))
+ if (likely(IPCB(skb)->opt.optlen == 0 &&
+ !(inet_sk(sk)->cmsg_flags & IP_CMSG_PASSSEC)))
skb_release_head_state(skb);
rc = __udp_enqueue_schedule_skb(sk, skb);
From: Paul Moore <paul@paul-moore.com> Date: 2017-07-24 14:42:50
On Mon, Jul 24, 2017 at 8:25 AM, Paolo Abeni [off-list ref] wrote:
Hi,
On Fri, 2017-07-21 at 18:19 -0400, Paul Moore wrote:
quoted
I've been seeing a SELinux regression with IP_PASSSEC on the v4.13-rcX
kernels and finally tracked the problem down to the
skb_release_head_state() call in __udp_queue_rcv_skb(). Looking at
the code and the git log it would appear that the likely culprit is
0a463c78d25b ("udp: avoid a cache miss on dequeue
"); it looks similar to IP option problem fixed in 0ddf3fb2c43d2.
Thank you for the report!
My bad, I completely missed that code path.
Hi Paolo,
No problem, things get missed from time to time, especially when we
deal with things that cross subsystems.
quoted
From a SELinux/IP_PASSSEC point of view we need access to the skb->sp
pointer to examine the SAs. I'm posting this here without a patch
because it isn't clear to me how you would like to fix the problem; my
initial thought would be to simply make the skb_release_head_state()
conditional on the skb->sp pointer, much like the IP options fix, but
I'm not sure if you have a more clever idea.
Unfortunately explicitly checking skb->sp at skb free time will defeat
completely the intended optimization.
To preserve it, something like the following patch is required, could
you please test it in your environment?
Such patch is still prone to a kind of race, as only UDP packets
enqueued to the UDP socket after the setsockopt() will carry the
relevant cmsg info.
e.g. with the following event sequence:
<an UDP packet is enqueued to the revevant socket>
setsockopt(...,IP_CMSG_PASSSEC)
recvmsg(...);
the ancillary message data will not include the IP_CMSG_PASSSEC, while
kernel pre 0a463c78d25b will provide it. Do you think such behavior
will be acceptable?
If not, I fear a revert will be needed.
The change in behavior for userspace makes me a little nervous as
there is no way of knowing how any random application may be coded.
Even if we are confident that the majority of applications set
IP_PASSSEC before calling bind(), we are likely still stuck with a few
that will break, and that means a lot of hard to debug problem
reports.
I would feel much more comfortable if we could preserve the existing behavior.
--
paul moore
www.paul-moore.com
From: Paolo Abeni <pabeni@redhat.com> Date: 2017-07-24 16:09:17
Hi,
On Mon, 2017-07-24 at 10:42 -0400, Paul Moore wrote:
The change in behavior for userspace makes me a little nervous as
there is no way of knowing how any random application may be coded.
Even if we are confident that the majority of applications set
IP_PASSSEC before calling bind(), we are likely still stuck with a few
that will break, and that means a lot of hard to debug problem
reports.
I would feel much more comfortable if we could preserve the existing behavior.
I agree, we must preserve the original behavior.
Re-thinking about the problem, checking skb->sp in the BH, and storing
the status in the scratch area should both fix the issue in a sane way
and preserve the optimization.
Something like the code below. Could you please try in your
environment? (or point me to simple reproducer ;-)
There are some cosmetics changes vs the previous iteration, but the
only relevant difference is that now the code always preserve skb->sb,
as per the pre-0a463c78d25b kernel behavior.
Thank you!
Paolo
---
/* UDP uses skb->dev_scratch to cache as much information as possible and avoid
* possibly multiple cache miss on dequeue()
*/
-#if BITS_PER_LONG == 64
-
-/* truesize, len and the bit needed to compute skb_csum_unnecessary will be on
- * cold cache lines at recvmsg time.
- * skb->len can be stored on 16 bits since the udp header has been already
- * validated and pulled.
- */
struct udp_dev_scratch {
- u32 truesize;
+ /* skb->truesize and the stateless bit embeded in a single field;
+ * do not use a bitfield since the compiler emits better/smaller code
+ * this way
+ */
+ u32 _tsize_state;
+
+#if BITS_PER_LONG == 64
+ /* len and the bit needed to compute skb_csum_unnecessary
+ * will be on cold cache lines at recvmsg time.
+ * skb->len can be stored on 16 bits since the udp header has been
+ * already validated and pulled.
+ */
u16 len;
bool is_linear;
bool csum_unnecessary;
+#endif
};
+static inline struct udp_dev_scratch* udp_skb_scratch(struct sk_buff *skb)
+{
+ return (struct udp_dev_scratch *)&skb->dev_scratch;
+}
+
+#if BITS_PER_LONG == 64
static inline unsigned int udp_skb_len(struct sk_buff *skb)
{
- return ((struct udp_dev_scratch *)&skb->dev_scratch)->len;
+ return udp_skb_scratch(skb)->len;
}
static inline bool udp_skb_csum_unnecessary(struct sk_buff *skb)
{
- return ((struct udp_dev_scratch *)&skb->dev_scratch)->csum_unnecessary;
+ return udp_skb_scratch(skb)->csum_unnecessary;
}
static inline bool udp_skb_is_linear(struct sk_buff *skb)
{
- return ((struct udp_dev_scratch *)&skb->dev_scratch)->is_linear;
+ return udp_skb_scratch(skb)->is_linear;
}
#else
unlock_sock_fast(sk, slow);
}
- /* we cleared the head states previously only if the skb lacks any IP
- * options, see __udp_queue_rcv_skb().
+ /* In the more common cases we cleared the head states previously,
+ * see __udp_queue_rcv_skb().
*/
- if (unlikely(IPCB(skb)->opt.optlen > 0))
+ if (unlikely(udp_skb_has_head_state(skb)))
skb_release_head_state(skb);
consume_stateless_skb(skb);
}
sk_mark_napi_id_once(sk, skb);
}
- /* At recvmsg() time we need skb->dst to process IP options-related
- * cmsg, elsewhere can we clear all pending head states while they are
- * hot in the cache
+ /* At recvmsg() time we may access skb->dst or skb->sp depending on
+ * the IP options and the cmsg flags, elsewhere can we clear all
+ * pending head states while they are hot in the cache
*/
- if (likely(IPCB(skb)->opt.optlen == 0))
+ if (likely(IPCB(skb)->opt.optlen == 0 && !skb->sp))
skb_release_head_state(skb);
rc = __udp_enqueue_schedule_skb(sk, skb);
From: Paul Moore <paul@paul-moore.com> Date: 2017-07-24 19:00:04
On Mon, Jul 24, 2017 at 12:09 PM, Paolo Abeni [off-list ref] wrote:
Hi,
On Mon, 2017-07-24 at 10:42 -0400, Paul Moore wrote:
quoted
The change in behavior for userspace makes me a little nervous as
there is no way of knowing how any random application may be coded.
Even if we are confident that the majority of applications set
IP_PASSSEC before calling bind(), we are likely still stuck with a few
that will break, and that means a lot of hard to debug problem
reports.
I would feel much more comfortable if we could preserve the existing behavior.
I agree, we must preserve the original behavior.
Re-thinking about the problem, checking skb->sp in the BH, and storing
the status in the scratch area should both fix the issue in a sane way
and preserve the optimization.
Something like the code below. Could you please try in your
environment? (or point me to simple reproducer ;-)
I'm happy to test this, but if you are curious, you can find the
selinux-testsuite at the link below; the "inet_socket" tests are the
ones relevant to this problem.
* https://github.com/SELinuxProject/selinux-testsuite
However, I believe there is a problem with this patch, see below.
@@ -305,33 +305,44 @@ struct sock *udp6_lib_lookup_skb(struct sk_buff *skb,/* UDP uses skb->dev_scratch to cache as much information as possible and avoid*possiblymultiplecachemissondequeue()*/-#if BITS_PER_LONG == 64--/* truesize, len and the bit needed to compute skb_csum_unnecessary will be on-*coldcachelinesatrecvmsgtime.-*skb->lencanbestoredon16bitssincetheudpheaderhasbeenalready-*validatedandpulled.-*/structudp_dev_scratch{-u32truesize;+/* skb->truesize and the stateless bit embeded in a single field;+*donotuseabitfieldsincethecompileremitsbetter/smallercode+*thisway+*/+u32_tsize_state;++#if BITS_PER_LONG == 64+/* len and the bit needed to compute skb_csum_unnecessary+*willbeoncoldcachelinesatrecvmsgtime.+*skb->lencanbestoredon16bitssincetheudpheaderhasbeen+*alreadyvalidatedandpulled.+*/u16len;boolis_linear;boolcsum_unnecessary;+#endif};
From: Paul Moore <paul@paul-moore.com> Date: 2017-07-25 02:00:28
On Mon, Jul 24, 2017 at 3:00 PM, Paul Moore [off-list ref] wrote:
On Mon, Jul 24, 2017 at 12:09 PM, Paolo Abeni [off-list ref] wrote:
quoted
Hi,
On Mon, 2017-07-24 at 10:42 -0400, Paul Moore wrote:
quoted
The change in behavior for userspace makes me a little nervous as
there is no way of knowing how any random application may be coded.
Even if we are confident that the majority of applications set
IP_PASSSEC before calling bind(), we are likely still stuck with a few
that will break, and that means a lot of hard to debug problem
reports.
I would feel much more comfortable if we could preserve the existing behavior.
I agree, we must preserve the original behavior.
Re-thinking about the problem, checking skb->sp in the BH, and storing
the status in the scratch area should both fix the issue in a sane way
and preserve the optimization.
Something like the code below. Could you please try in your
environment? (or point me to simple reproducer ;-)
I'm happy to test this, but if you are curious, you can find the
selinux-testsuite at the link below; the "inet_socket" tests are the
ones relevant to this problem.
* https://github.com/SELinuxProject/selinux-testsuite
However, I believe there is a problem with this patch, see below.
@@ -305,33 +305,44 @@ struct sock *udp6_lib_lookup_skb(struct sk_buff *skb,/* UDP uses skb->dev_scratch to cache as much information as possible and avoid*possiblymultiplecachemissondequeue()*/-#if BITS_PER_LONG == 64--/* truesize, len and the bit needed to compute skb_csum_unnecessary will be on-*coldcachelinesatrecvmsgtime.-*skb->lencanbestoredon16bitssincetheudpheaderhasbeenalready-*validatedandpulled.-*/structudp_dev_scratch{-u32truesize;+/* skb->truesize and the stateless bit embeded in a single field;+*donotuseabitfieldsincethecompileremitsbetter/smallercode+*thisway+*/+u32_tsize_state;++#if BITS_PER_LONG == 64+/* len and the bit needed to compute skb_csum_unnecessary+*willbeoncoldcachelinesatrecvmsgtime.+*skb->lencanbestoredon16bitssincetheudpheaderhasbeen+*alreadyvalidatedandpulled.+*/u16len;boolis_linear;boolcsum_unnecessary;+#endif};
@@ -1163,34 +1163,32 @@ int udp_sendpage(struct sock *sk, struct page *page, int offset,returnret;}-#if BITS_PER_LONG == 64+#define UDP_SKB_IS_STATELESS 0x80000000+staticvoidudp_set_dev_scratch(structsk_buff*skb){-structudp_dev_scratch*scratch;+structudp_dev_scratch*scratch=udp_skb_scratch(skb);BUILD_BUG_ON(sizeof(structudp_dev_scratch)>sizeof(long));
The BUILD_BUG_ON() assertion no longer appears to be correct with this patch.
Nevermind, I just took a closer look at this and realized I made a
mistake when applying your patch (had to apply manually for some
reason). I'm building a test kernel now.
--
paul moore
www.paul-moore.com
From: Paolo Abeni <pabeni@redhat.com> Date: 2017-07-25 09:59:40
On Mon, 2017-07-24 at 22:00 -0400, Paul Moore wrote:
quoted
I'm happy to test this, but if you are curious, you can find the
selinux-testsuite at the link below; the "inet_socket" tests are the
ones relevant to this problem.
* https://github.com/SELinuxProject/selinux-testsuite
Thanks, I'll have a look.
quoted
However, I believe there is a problem with this patch, see below.
The BUILD_BUG_ON() assertion no longer appears to be correct with this patch.
Nevermind, I just took a closer look at this and realized I made a
mistake when applying your patch (had to apply manually for some
reason). I'm building a test kernel now.
Yup, I compile-tested the code, plus some basic sanity checks, so the
build breakage felt unexpected.
Thanks for testing,
Paolo
From: Paul Moore <paul@paul-moore.com> Date: 2017-07-25 14:45:57
On Tue, Jul 25, 2017 at 5:59 AM, Paolo Abeni [off-list ref] wrote:
On Mon, 2017-07-24 at 22:00 -0400, Paul Moore wrote:
quoted
quoted
I'm happy to test this, but if you are curious, you can find the
selinux-testsuite at the link below; the "inet_socket" tests are the
ones relevant to this problem.
* https://github.com/SELinuxProject/selinux-testsuite
Thanks, I'll have a look.
quoted
quoted
However, I believe there is a problem with this patch, see below.
The BUILD_BUG_ON() assertion no longer appears to be correct with this patch.
Nevermind, I just took a closer look at this and realized I made a
mistake when applying your patch (had to apply manually for some
reason). I'm building a test kernel now.
Yup, I compile-tested the code, plus some basic sanity checks, so the
build breakage felt unexpected.
Thanks for testing,
I just did a quick run through the selinux-testsuite and the
regression would appear to be fixed, thanks! I'm guessing you'll send
this to DaveM so we can get this fixed before v4.13 is released?
Tested-by: Paul Moore <paul@paul-moore.com>
--
paul moore
www.paul-moore.com
From: Paolo Abeni <pabeni@redhat.com> Date: 2017-07-25 15:37:02
On Tue, 2017-07-25 at 10:45 -0400, Paul Moore wrote:
On Tue, Jul 25, 2017 at 5:59 AM, Paolo Abeni [off-list ref] wrote:
quoted
On Mon, 2017-07-24 at 22:00 -0400, Paul Moore wrote:
quoted
quoted
I'm happy to test this, but if you are curious, you can find the
selinux-testsuite at the link below; the "inet_socket" tests are the
ones relevant to this problem.
* https://github.com/SELinuxProject/selinux-testsuite
Thanks, I'll have a look.
quoted
quoted
However, I believe there is a problem with this patch, see below.
The BUILD_BUG_ON() assertion no longer appears to be correct with this patch.
Nevermind, I just took a closer look at this and realized I made a
mistake when applying your patch (had to apply manually for some
reason). I'm building a test kernel now.
Yup, I compile-tested the code, plus some basic sanity checks, so the
build breakage felt unexpected.
Thanks for testing,
I just did a quick run through the selinux-testsuite and the
regression would appear to be fixed, thanks! I'm guessing you'll send
this to DaveM so we can get this fixed before v4.13 is released?
Tested-by: Paul Moore <paul@paul-moore.com>