SELinux/IP_PASSSEC regression in 4.13-rcX

SELinux/IP_PASSSEC regression in 4.13-rcX

[Paul Moore]

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

Re: SELinux/IP_PASSSEC regression in 4.13-rcX

[Paolo Abeni]

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
---
diff --git a/include/net/udp.h b/include/net/udp.h
index 972ce4b..f109126 100644
--- a/include/net/udp.h
+++ b/include/net/udp.h
@@ -319,19 +319,24 @@ struct udp_dev_scratch {
 	bool csum_unnecessary;
 };
 
+static inline struct udp_dev_scratch* udp_skb_scratch(struct sk_buff *skb)
+{
+	return (struct udp_dev_scratch *)&skb->dev_scratch;
+}
+
 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
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index b057653..582c13e 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -1163,32 +1163,47 @@ int udp_sendpage(struct sock *sk, struct page *page, int offset,
 	return ret;
 }
 
+#define UDP_SKB_IS_STATELESS 0x80000000
+
 #if BITS_PER_LONG == 64
 static void udp_set_dev_scratch(struct sk_buff *skb)
 {
-	struct udp_dev_scratch *scratch;
+	struct udp_dev_scratch *scratch = udp_skb_scratch(skb);
 
 	BUILD_BUG_ON(sizeof(struct udp_dev_scratch) > sizeof(long));
-	scratch = (struct udp_dev_scratch *)&skb->dev_scratch;
 	scratch->truesize = skb->truesize;
 	scratch->len = skb->len;
 	scratch->csum_unnecessary = !!skb_csum_unnecessary(skb);
 	scratch->is_linear = !skb_is_nonlinear(skb);
+	if (likely(!skb->_skb_refdst))
+		scratch->truesize |= UDP_SKB_IS_STATELESS;
 }
 
 static int udp_skb_truesize(struct sk_buff *skb)
 {
-	return ((struct udp_dev_scratch *)&skb->dev_scratch)->truesize;
+	return udp_skb_scratch(skb)->truesize & ~UDP_SKB_IS_STATELESS;
+}
+
+static bool udp_skb_has_head_state(struct sk_buff *skb)
+{
+	return !(udp_skb_scratch(skb)->truesize & UDP_SKB_IS_STATELESS);
 }
 #else
 static void udp_set_dev_scratch(struct sk_buff *skb)
 {
 	skb->dev_scratch = skb->truesize;
+	if (likely(!skb->_skb_refdst))
+		scratch->dev_scratch |= UDP_SKB_IS_STATELESS;
 }
 
 static int udp_skb_truesize(struct sk_buff *skb)
 {
-	return skb->dev_scratch;
+	return skb->dev_scratch & ~UDP_SKB_IS_STATELESS;
+}
+
+static bool udp_skb_has_head_state(struct sk_buff *skb)
+{
+	return !(skb->dev_scratch & UDP_SKB_IS_STATELESS);
 }
 #endif
 
@@ -1388,10 +1403,10 @@ void skb_consume_udp(struct sock *sk, struct sk_buff *skb, int len)
 		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);
 }
@@ -1784,11 +1799,12 @@ static int __udp_queue_rcv_skb(struct sock *sk, struct sk_buff *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);

Re: SELinux/IP_PASSSEC regression in 4.13-rcX

[Paul Moore]

On Mon, Jul 24, 2017 at 8:25 AM, Paolo Abeni <pabeni@redhat.com> wrote:
> 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.

Hi Paolo,

No problem, things get missed from time to time, especially when we
deal with things that cross subsystems.

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

Re: SELinux/IP_PASSSEC regression in 4.13-rcX

[Paolo Abeni]

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
---
diff --git a/include/net/udp.h b/include/net/udp.h
index 972ce4b..8d2c406 100644
--- a/include/net/udp.h
+++ b/include/net/udp.h
@@ -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
  * 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
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index b057653..d243772 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -1163,34 +1163,32 @@ int udp_sendpage(struct sock *sk, struct page *page, int offset,
 	return ret;
 }
 
-#if BITS_PER_LONG == 64
+#define UDP_SKB_IS_STATELESS 0x80000000
+
 static void udp_set_dev_scratch(struct sk_buff *skb)
 {
-	struct udp_dev_scratch *scratch;
+	struct udp_dev_scratch *scratch = udp_skb_scratch(skb);
 
 	BUILD_BUG_ON(sizeof(struct udp_dev_scratch) > sizeof(long));
-	scratch = (struct udp_dev_scratch *)&skb->dev_scratch;
-	scratch->truesize = skb->truesize;
+	scratch->_tsize_state = skb->truesize;
+#if BITS_PER_LONG == 64
 	scratch->len = skb->len;
 	scratch->csum_unnecessary = !!skb_csum_unnecessary(skb);
 	scratch->is_linear = !skb_is_nonlinear(skb);
+#endif
+	if (likely(!skb->_skb_refdst))
+		scratch->_tsize_state |= UDP_SKB_IS_STATELESS;
 }
 
 static int udp_skb_truesize(struct sk_buff *skb)
 {
-	return ((struct udp_dev_scratch *)&skb->dev_scratch)->truesize;
-}
-#else
-static void udp_set_dev_scratch(struct sk_buff *skb)
-{
-	skb->dev_scratch = skb->truesize;
+	return udp_skb_scratch(skb)->_tsize_state & ~UDP_SKB_IS_STATELESS;
 }
 
-static int udp_skb_truesize(struct sk_buff *skb)
+static bool udp_skb_has_head_state(struct sk_buff *skb)
 {
-	return skb->dev_scratch;
+	return !(udp_skb_scratch(skb)->_tsize_state & UDP_SKB_IS_STATELESS);
 }
-#endif
 
 /* fully reclaim rmem/fwd memory allocated for skb */
 static void udp_rmem_release(struct sock *sk, int size, int partial,
@@ -1388,10 +1386,10 @@ void skb_consume_udp(struct sock *sk, struct sk_buff *skb, int len)
 		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);
 }
@@ -1784,11 +1782,11 @@ static int __udp_queue_rcv_skb(struct sock *sk, struct sk_buff *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);

Re: SELinux/IP_PASSSEC regression in 4.13-rcX

[Paul Moore]

On Mon, Jul 24, 2017 at 12:09 PM, Paolo Abeni <pabeni@redhat.com> wrote:
> 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 ;-)

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.

> ---
> diff --git a/include/net/udp.h b/include/net/udp.h
> index 972ce4b..8d2c406 100644
> --- a/include/net/udp.h
> +++ b/include/net/udp.h
> @@ -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
>   * 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
>  };

...

> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index b057653..d243772 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -1163,34 +1163,32 @@ int udp_sendpage(struct sock *sk, struct page *page, int offset,
>         return ret;
>  }
>
> -#if BITS_PER_LONG == 64
> +#define UDP_SKB_IS_STATELESS 0x80000000
> +
>  static void udp_set_dev_scratch(struct sk_buff *skb)
>  {
> -       struct udp_dev_scratch *scratch;
> +       struct udp_dev_scratch *scratch = udp_skb_scratch(skb);
>
>         BUILD_BUG_ON(sizeof(struct udp_dev_scratch) > sizeof(long));

The BUILD_BUG_ON() assertion no longer appears to be correct with this patch.

> -       scratch = (struct udp_dev_scratch *)&skb->dev_scratch;
> -       scratch->truesize = skb->truesize;
> +       scratch->_tsize_state = skb->truesize;
> +#if BITS_PER_LONG == 64
>         scratch->len = skb->len;
>         scratch->csum_unnecessary = !!skb_csum_unnecessary(skb);
>         scratch->is_linear = !skb_is_nonlinear(skb);
> +#endif
> +       if (likely(!skb->_skb_refdst))
> +               scratch->_tsize_state |= UDP_SKB_IS_STATELESS;
>  }

-- 
paul moore
www.paul-moore.com

Re: SELinux/IP_PASSSEC regression in 4.13-rcX

[Paul Moore]

On Mon, Jul 24, 2017 at 3:00 PM, Paul Moore <paul@paul-moore.com> wrote:
> On Mon, Jul 24, 2017 at 12:09 PM, Paolo Abeni <pabeni@redhat.com> wrote:
>> 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 ;-)
>
> 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.
>
>> ---
>> diff --git a/include/net/udp.h b/include/net/udp.h
>> index 972ce4b..8d2c406 100644
>> --- a/include/net/udp.h
>> +++ b/include/net/udp.h
>> @@ -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
>>   * 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
>>  };
>
> ...
>
>> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
>> index b057653..d243772 100644
>> --- a/net/ipv4/udp.c
>> +++ b/net/ipv4/udp.c
>> @@ -1163,34 +1163,32 @@ int udp_sendpage(struct sock *sk, struct page *page, int offset,
>>         return ret;
>>  }
>>
>> -#if BITS_PER_LONG == 64
>> +#define UDP_SKB_IS_STATELESS 0x80000000
>> +
>>  static void udp_set_dev_scratch(struct sk_buff *skb)
>>  {
>> -       struct udp_dev_scratch *scratch;
>> +       struct udp_dev_scratch *scratch = udp_skb_scratch(skb);
>>
>>         BUILD_BUG_ON(sizeof(struct udp_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

Re: SELinux/IP_PASSSEC regression in 4.13-rcX

[Paolo Abeni]

On Mon, 2017-07-24 at 22:00 -0400, Paul Moore wrote:
> > 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.

> > However, I believe there is a problem with this patch, see below.

[...]

> > > -#if BITS_PER_LONG == 64
> > > +#define UDP_SKB_IS_STATELESS 0x80000000
> > > +
> > >  static void udp_set_dev_scratch(struct sk_buff *skb)
> > >  {
> > > -       struct udp_dev_scratch *scratch;
> > > +       struct udp_dev_scratch *scratch = udp_skb_scratch(skb);
> > > 
> > >         BUILD_BUG_ON(sizeof(struct udp_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.

Yup, I compile-tested the code, plus some basic sanity checks, so the
build breakage felt unexpected.

Thanks for testing,

Paolo

Re: SELinux/IP_PASSSEC regression in 4.13-rcX

[Paul Moore]

On Tue, Jul 25, 2017 at 5:59 AM, Paolo Abeni <pabeni@redhat.com> wrote:
> On Mon, 2017-07-24 at 22:00 -0400, Paul Moore wrote:
>> > 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.
>
>> > However, I believe there is a problem with this patch, see below.
>
> [...]
>
>> > > -#if BITS_PER_LONG == 64
>> > > +#define UDP_SKB_IS_STATELESS 0x80000000
>> > > +
>> > >  static void udp_set_dev_scratch(struct sk_buff *skb)
>> > >  {
>> > > -       struct udp_dev_scratch *scratch;
>> > > +       struct udp_dev_scratch *scratch = udp_skb_scratch(skb);
>> > >
>> > >         BUILD_BUG_ON(sizeof(struct udp_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.
>
> 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

Re: SELinux/IP_PASSSEC regression in 4.13-rcX

[Paolo Abeni]

On Tue, 2017-07-25 at 10:45 -0400, Paul Moore wrote:
> On Tue, Jul 25, 2017 at 5:59 AM, Paolo Abeni <pabeni@redhat.com> wrote:
> > On Mon, 2017-07-24 at 22:00 -0400, Paul Moore wrote:
> > > > 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.
> > 
> > > > However, I believe there is a problem with this patch, see below.
> > 
> > [...]
> > 
> > > > > -#if BITS_PER_LONG == 64
> > > > > +#define UDP_SKB_IS_STATELESS 0x80000000
> > > > > +
> > > > >  static void udp_set_dev_scratch(struct sk_buff *skb)
> > > > >  {
> > > > > -       struct udp_dev_scratch *scratch;
> > > > > +       struct udp_dev_scratch *scratch = udp_skb_scratch(skb);
> > > > > 
> > > > >         BUILD_BUG_ON(sizeof(struct udp_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.
> > 
> > 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>

Sure. I'll submit soon for -net.

Cheers,

Paolo