Re: SELinux/IP_PASSSEC regression in 4.13-rcX

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.

quoted

---

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

...

quoted

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