On Mon, Jul 24, 2017 at 12:09 PM, Paolo Abeni <pabeni@xxxxxxxxxx> 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
