* Re: XDP offload to hypervisor
From: John Fastabend @ 2017-01-23 21:56 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: jasowang, john.r.fastabend, netdev, alexei.starovoitov, daniel
In-Reply-To: <20170123230727-mutt-send-email-mst@kernel.org>
On 17-01-23 01:40 PM, Michael S. Tsirkin wrote:
> I've been thinking about passing XDP programs from guest to the
> hypervisor. Basically, after getting an incoming packet, we could run
> an XDP program in host kernel.
>
Interesting. I am planning on adding XDP to tun driver. My use case
is we want to use XDP to restrict VM traffic. I was planning on pushing
the xdp program execution into tun_get_user(). So different then "offloading"
an xdp program into hypervisor.
> If the result is XDP_DROP or XDP_TX we don't need to wake up the guest at all!
>
nice win.
> When using tun for networking - especially with adjust_head - this
> unfortunately probably means we need to do a data copy unless there is
> enough headroom. How much is enough though?
We were looking at making headroom configurable on Intel drivers or at
least matching it with XDP headroom guidelines. (although the developers
had the same complaint about 256B being large). Then at least on supported
drivers the copy could be an exception path.
>
> Another issue is around host/guest ABI. Guest BPF could add new features
> at any point. What if hypervisor can not support it all? I guess we
> could try loading program into hypervisor and run it within guest on
> failure to load, but this ignores question of cross-version
> compatibility - someone might start guest on a new host
> then try to move to an old one. So we will need an option
> "behave like an older host" such that guest can start and then
> move to an older host later. This will likely mean
> implementing this validation of programs in qemu userspace unless linux
> can supply something like this. Is this (disabling some features)
> something that might be of interest to larger bpf community?
This is interesting to me at least. Another interesting "feature" of
running bpf in qemu userspace is it could work with vhost_user as well
presumably?
>
> With a device such as macvtap there exist configurations where a single
> guest is in control of the device (aka passthrough mode) in that case
> there's a potential to run xdp on host before host skb is built, unless
> host already has an xdp program attached. If it does we could run the
> program within guest, but what if a guest program got attached first?
> Maybe we should pass a flag in the packet "xdp passed on this packet in
> host". Then, guest can skip running it. Unless we do a full reset
> there's always a potential for packets to slip through, e.g. on xdp
> program changes. Maybe a flush command is needed, or force queue or
> device reset to make sure nothing is going on. Does this make sense?
>
Could the virtio driver pretend its "offloading" the XDP program to
hardware? This would make it explicit in VM that the program is run
before data is received by virtio_net. Then qemu is enabling the
offload framework which would be interesting.
> Thanks!
>
^ permalink raw reply
* Re: [net PATCH v5 1/6] virtio_net: use dev_kfree_skb for small buffer XDP receive
From: John Fastabend @ 2017-01-23 21:57 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: jasowang, john.r.fastabend, netdev, alexei.starovoitov, daniel
In-Reply-To: <20170123230809-mutt-send-email-mst@kernel.org>
On 17-01-23 01:08 PM, Michael S. Tsirkin wrote:
> On Tue, Jan 17, 2017 at 02:19:50PM -0800, John Fastabend wrote:
>> In the small buffer case during driver unload we currently use
>> put_page instead of dev_kfree_skb. Resolve this by adding a check
>> for virtnet mode when checking XDP queue type. Also name the
>> function so that the code reads correctly to match the additional
>> check.
>>
>> Fixes: bb91accf2733 ("virtio-net: XDP support for small buffers")
>> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
>> Acked-by: Jason Wang <jasowang@redhat.com>
>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
>
> I think we definitely want this one in -net as it's
> a bugfix.
>
Agreed, let me pull this fix out of the series and submit it for
net.
^ permalink raw reply
* Re: Question about veth_xmit()
From: Eric Dumazet @ 2017-01-23 22:05 UTC (permalink / raw)
To: Xiangning Yu; +Cc: Cong Wang, Linux Kernel Network Developers
In-Reply-To: <CAOwmpL3fiGcXHmUouzRf8vneSF1SQ5SWVqYLXGVecLTepZTNOw@mail.gmail.com>
On Mon, 2017-01-23 at 13:46 -0800, Xiangning Yu wrote:
> On Mon, Jan 23, 2017 at 12:56 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > On Mon, Jan 23, 2017 at 10:46 AM, Xiangning Yu <yuxiangning@gmail.com> wrote:
> >> Hi netdev folks,
> >>
> >> It looks like we call dev_forward_skb() in veth_xmit(), which calls
> >> netif_rx() eventually.
> >>
> >> While netif_rx() will enqueue the skb to the CPU RX backlog before the
> >> actual processing takes place. So, this actually means a TX skb has to
> >> wait some un-related RX skbs to finish. And this will happen twice for
> >> a single ping, because the veth device always works as a pair?
> >
> > For me it is more like for the completeness of network stack of each
> > netns. The /proc net.core.netdev_max_backlog etc. are per netns, which
> > means each netns, as an independent network stack, should respect it
> > too.
> >
> > Since you care about latency, why not tune net.core.dev_weight for your
> > own netns?
>
> I haven't tried that yet, thank you for the hint! Though normally one
> of the veth device will be in the global namespace.
Well, per cpu backlog are not per net ns, but per cpu.
So Cong suggestion is not going to work.
^ permalink raw reply
* Re: [PATCH net-next 3/3] net/tcp-fastopen: Add new API support
From: Willy Tarreau @ 2017-01-23 22:01 UTC (permalink / raw)
To: Wei Wang
Cc: Wei Wang, Linux Kernel Network Developers, David Miller,
Eric Dumazet, Yuchung Cheng
In-Reply-To: <20170123213732.GF20894@1wt.eu>
On Mon, Jan 23, 2017 at 10:37:32PM +0100, Willy Tarreau wrote:
> On Mon, Jan 23, 2017 at 01:28:53PM -0800, Wei Wang wrote:
> > Hi Willy,
> >
> > True. If you call connect() multiple times on a socket which already has
> > cookie without a write(), the second and onward connect() call will return
> > EINPROGRESS.
> > It is basically because the following code block in __inet_stream_connect()
> > can't distinguish if it is the first time connect() is called or not:
> >
> > case SS_CONNECTING:
> > if (inet_sk(sk)->defer_connect) <----- defer_connect will
> > be 0 only after a write() is called
> > err = -EINPROGRESS;
> > else
> > err = -EALREADY;
> > /* Fall out of switch with err, set for this state */
> > break;
>
> Ah OK that totally makes sense, thanks for the explanation!
>
> > I guess we can add some extra logic here to address this issue. So the
> > second connect() and onwards will return EALREADY.
Thinking about it a bit more, I really think it would make more
sense to return -EISCONN here if we want to match the semantics
of a connect() returning zero on the first call. This way the
caller knows it can write whenever it wants and can disable
write polling until needed.
I'm currently rebuilding a kernel with this change to see if it
behaves any better :
- err = -EINPROGRESS;
+ err = -EISCONN;
I'll keep you updated.
Thanks,
Willy
^ permalink raw reply
* Re: [net PATCH v5 6/6] virtio_net: XDP support for adjust_head
From: John Fastabend @ 2017-01-23 22:12 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: jasowang, john.r.fastabend, netdev, alexei.starovoitov, daniel
In-Reply-To: <20170123220231-mutt-send-email-mst@kernel.org>
On 17-01-23 12:09 PM, Michael S. Tsirkin wrote:
> On Mon, Jan 23, 2017 at 09:22:36PM +0200, Michael S. Tsirkin wrote:
>> On Tue, Jan 17, 2017 at 02:22:59PM -0800, John Fastabend wrote:
>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>> index 62dbf4b..3b129b4 100644
>>> --- a/drivers/net/virtio_net.c
>>> +++ b/drivers/net/virtio_net.c
>>> @@ -41,6 +41,9 @@
>>> #define GOOD_PACKET_LEN (ETH_HLEN + VLAN_HLEN + ETH_DATA_LEN)
>>> #define GOOD_COPY_LEN 128
>>>
>>> +/* Amount of XDP headroom to prepend to packets for use by xdp_adjust_head */
>>> +#define VIRTIO_XDP_HEADROOM 256
>>> +
>>> /* RX packet size EWMA. The average packet size is used to determine the packet
>>> * buffer size when refilling RX rings. As the entire RX ring may be refilled
>>> * at once, the weight is chosen so that the EWMA will be insensitive to short-
>>
>> I wonder where does this number come from? This is quite a lot and
>> means that using XDP_PASS will slow down any sockets on top of it.
>> Which in turn means people will try to remove XDP when not in use,
>> causing resets. E.g. build_skb (which I have a patch to switch to) uses
>> a much more reasonable NET_SKB_PAD.
I just used the value Alexei (or someone?) came up with. I think it needs to be
large enough to avoid copy in header encap cases. So minimum
VXLAN_HDR + OUTER_UDP + OUTER_IPV6_HDR + OUTER_MAC =
8 + 8 + 40 + 14 = 70
The choice of VXLAN hdr was sort of arbitrary but seems good for estimates. For
what its worth there is also a ndo_set_rx_headroom could we use that to set it
and choose a reasonable default.
>>
>> --
>> MST
>
>
> Let me show you a patch that I've been cooking. What is missing there
> is handling corner cases like e.g. when ring size is ~4 entries so
> using smaller buffers might mean we no longer have enough space to store
> a full packet. So it looks like I have to maintain the skb copy path
> for this hardware.
>
> With this patch, standard configuration has NET_SKB_PAD + NET_IP_ALIGN
> bytes head padding. Would this be enough for XDP? If yes we do not
> need the resets.
Based on above seems a bit small (L1_CACHE_BYTES + 2)? How tricky would it
be to add support for ndo_set_rx_headroom.
>
> Thoughts?
I'll take a look at the patch this afternoon. Thanks.
>
> --->
>
> virtio_net: switch to build_skb for mrg_rxbuf
>
> For small packets data copy was observed to
> take up about 15% CPU time. Switch to build_skb
> and avoid the copy when using mergeable rx buffers.
>
> As a bonus, medium-size skbs that fit in a page will be
> completely linear.
>
> Of course, we now need to lower the lower bound on packet size,
> to make sure a sane number of skbs fits in rx socket buffer.
> By how much? I don't know yet.
>
> It might also be useful to prefetch the packet buffer since
> net stack will likely use it soon.
>
> Lightly tested, in particular, I didn't yet test what this
> actually does to performance - sending this out for early
> feedback/flames.
>
> TODO: it appears that Linux won't handle correctly the case of first
> buffer being very small (or consisting exclusively of virtio header).
> This is already the case for current code, need to fix.
> TODO: might be unfair to the last packet in a fragment as we include
> remaining space if any in its truesize.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> ----
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index b425fa1..a6b996f 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -38,6 +38,8 @@ module_param(gso, bool, 0444);
>
> /* FIXME: MTU in config. */
> #define GOOD_PACKET_LEN (ETH_HLEN + VLAN_HLEN + ETH_DATA_LEN)
> +//#define MIN_PACKET_ALLOC GOOD_PACKET_LEN
> +#define MIN_PACKET_ALLOC 128
> #define GOOD_COPY_LEN 128
>
> /* RX packet size EWMA. The average packet size is used to determine the packet
> @@ -246,6 +248,9 @@ static void *mergeable_ctx_to_buf_address(unsigned long mrg_ctx)
> static unsigned long mergeable_buf_to_ctx(void *buf, unsigned int truesize)
> {
> unsigned int size = truesize / MERGEABLE_BUFFER_ALIGN;
> +
> + BUG_ON((unsigned long)buf & (MERGEABLE_BUFFER_ALIGN - 1));
> + BUG_ON(size - 1 >= MERGEABLE_BUFFER_ALIGN);
> return (unsigned long)buf | (size - 1);
> }
>
> @@ -354,25 +359,54 @@ static struct sk_buff *receive_big(struct net_device *dev,
> return NULL;
> }
>
> +#define VNET_SKB_PAD (NET_SKB_PAD + NET_IP_ALIGN)
> +#define VNET_SKB_BUG (VNET_SKB_PAD < sizeof(struct virtio_net_hdr_mrg_rxbuf))
> +#define VNET_SKB_LEN(len) ((len) - sizeof(struct virtio_net_hdr_mrg_rxbuf))
> +#define VNET_SKB_OFF VNET_SKB_LEN(VNET_SKB_PAD)
> +
> +static struct sk_buff *vnet_build_skb(struct virtnet_info *vi,
> + void *buf,
> + unsigned int len, unsigned int truesize)
> +{
> + struct sk_buff *skb = build_skb(buf, truesize);
> +
> + if (!skb)
> + return NULL;
> +
> + skb_reserve(skb, VNET_SKB_PAD);
> + skb_put(skb, VNET_SKB_LEN(len));
> +
> + return skb;
> +}
> +
> static struct sk_buff *receive_mergeable(struct net_device *dev,
> struct virtnet_info *vi,
> struct receive_queue *rq,
> unsigned long ctx,
> - unsigned int len)
> + unsigned int len,
> + struct virtio_net_hdr_mrg_rxbuf *hdr)
> {
> void *buf = mergeable_ctx_to_buf_address(ctx);
> - struct virtio_net_hdr_mrg_rxbuf *hdr = buf;
> - u16 num_buf = virtio16_to_cpu(vi->vdev, hdr->num_buffers);
> + u16 num_buf;
> struct page *page = virt_to_head_page(buf);
> - int offset = buf - page_address(page);
> - unsigned int truesize = max(len, mergeable_ctx_to_buf_truesize(ctx));
> + unsigned int truesize = mergeable_ctx_to_buf_truesize(ctx);
> + int offset;
> + struct sk_buff *head_skb;
> + struct sk_buff *curr_skb;
> +
> + BUG_ON(len > truesize);
>
> - struct sk_buff *head_skb = page_to_skb(vi, rq, page, offset, len,
> - truesize);
> - struct sk_buff *curr_skb = head_skb;
> + /* copy header: build_skb will overwrite it */
> + memcpy(hdr, buf + VNET_SKB_OFF, sizeof *hdr);
> +
> + head_skb = vnet_build_skb(vi, buf, len, truesize);
> + curr_skb = head_skb;
> +
> + num_buf = virtio16_to_cpu(vi->vdev, hdr->num_buffers);
>
> if (unlikely(!curr_skb))
> goto err_skb;
> +
> while (--num_buf) {
> int num_skb_frags;
>
> @@ -386,7 +420,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> goto err_buf;
> }
>
> - buf = mergeable_ctx_to_buf_address(ctx);
> + buf = mergeable_ctx_to_buf_address(ctx) + VNET_SKB_OFF;
> page = virt_to_head_page(buf);
>
> num_skb_frags = skb_shinfo(curr_skb)->nr_frags;
> @@ -403,7 +437,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> head_skb->truesize += nskb->truesize;
> num_skb_frags = 0;
> }
> - truesize = max(len, mergeable_ctx_to_buf_truesize(ctx));
> + truesize = mergeable_ctx_to_buf_truesize(ctx);
> + BUG_ON(len > truesize);
> if (curr_skb != head_skb) {
> head_skb->data_len += len;
> head_skb->len += len;
> @@ -449,6 +484,7 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
> struct virtnet_stats *stats = this_cpu_ptr(vi->stats);
> struct sk_buff *skb;
> struct virtio_net_hdr_mrg_rxbuf *hdr;
> + struct virtio_net_hdr_mrg_rxbuf hdr0;
>
> if (unlikely(len < vi->hdr_len + ETH_HLEN)) {
> pr_debug("%s: short packet %i\n", dev->name, len);
> @@ -465,17 +501,24 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
> return;
> }
>
> - if (vi->mergeable_rx_bufs)
> - skb = receive_mergeable(dev, vi, rq, (unsigned long)buf, len);
> - else if (vi->big_packets)
> + if (vi->mergeable_rx_bufs) {
> + skb = receive_mergeable(dev, vi, rq, (unsigned long)buf, len,
> + &hdr0);
> + if (unlikely(!skb))
> + return;
> + hdr = &hdr0;
> + } else if (vi->big_packets) {
> skb = receive_big(dev, vi, rq, buf, len);
> - else
> + if (unlikely(!skb))
> + return;
> + hdr = skb_vnet_hdr(skb);
> + } else {
> skb = receive_small(vi, buf, len);
> + if (unlikely(!skb))
> + return;
> + hdr = skb_vnet_hdr(skb);
> + }
>
> - if (unlikely(!skb))
> - return;
> -
> - hdr = skb_vnet_hdr(skb);
>
> u64_stats_update_begin(&stats->rx_syncp);
> stats->rx_bytes += skb->len;
> @@ -581,11 +624,14 @@ static int add_recvbuf_big(struct virtnet_info *vi, struct receive_queue *rq,
>
> static unsigned int get_mergeable_buf_len(struct ewma_pkt_len *avg_pkt_len)
> {
> - const size_t hdr_len = sizeof(struct virtio_net_hdr_mrg_rxbuf);
> + unsigned int hdr;
> unsigned int len;
>
> - len = hdr_len + clamp_t(unsigned int, ewma_pkt_len_read(avg_pkt_len),
> - GOOD_PACKET_LEN, PAGE_SIZE - hdr_len);
> + hdr = ALIGN(VNET_SKB_PAD + SKB_DATA_ALIGN(sizeof(struct skb_shared_info)),
> + MERGEABLE_BUFFER_ALIGN);
> +
> + len = hdr + clamp_t(unsigned int, ewma_pkt_len_read(avg_pkt_len),
> + MIN_PACKET_ALLOC, PAGE_SIZE - hdr);
> return ALIGN(len, MERGEABLE_BUFFER_ALIGN);
> }
>
> @@ -601,8 +647,11 @@ static int add_recvbuf_mergeable(struct receive_queue *rq, gfp_t gfp)
> if (unlikely(!skb_page_frag_refill(len, alloc_frag, gfp)))
> return -ENOMEM;
>
> - buf = (char *)page_address(alloc_frag->page) + alloc_frag->offset;
> - ctx = mergeable_buf_to_ctx(buf, len);
> + BUILD_BUG_ON(VNET_SKB_BUG);
> +
> + buf = (char *)page_address(alloc_frag->page) + alloc_frag->offset +
> + VNET_SKB_OFF;
> + //ctx = mergeable_buf_to_ctx(buf - VNET_SKB_OFF, len);
> get_page(alloc_frag->page);
> alloc_frag->offset += len;
> hole = alloc_frag->size - alloc_frag->offset;
> @@ -615,8 +664,10 @@ static int add_recvbuf_mergeable(struct receive_queue *rq, gfp_t gfp)
> len += hole;
> alloc_frag->offset += hole;
> }
> + ctx = mergeable_buf_to_ctx(buf - VNET_SKB_OFF, len);
>
> - sg_init_one(rq->sg, buf, len);
> + sg_init_one(rq->sg, buf,
> + len - VNET_SKB_OFF - SKB_DATA_ALIGN(sizeof(struct skb_shared_info)));
> err = virtqueue_add_inbuf(rq->vq, rq->sg, 1, (void *)ctx, gfp);
> if (err < 0)
> put_page(virt_to_head_page(buf));
>
^ permalink raw reply
* Re: XDP offload to hypervisor
From: Michael S. Tsirkin @ 2017-01-23 22:26 UTC (permalink / raw)
To: John Fastabend
Cc: jasowang, john.r.fastabend, netdev, alexei.starovoitov, daniel
In-Reply-To: <58867C00.1060901@gmail.com>
On Mon, Jan 23, 2017 at 01:56:16PM -0800, John Fastabend wrote:
> On 17-01-23 01:40 PM, Michael S. Tsirkin wrote:
> > I've been thinking about passing XDP programs from guest to the
> > hypervisor. Basically, after getting an incoming packet, we could run
> > an XDP program in host kernel.
> >
>
> Interesting. I am planning on adding XDP to tun driver. My use case
> is we want to use XDP to restrict VM traffic. I was planning on pushing
> the xdp program execution into tun_get_user(). So different then "offloading"
> an xdp program into hypervisor.
tun currently supports TUNATTACHFILTER. Do you plan to extend it then?
So maybe there's need to support more than one program.
Would it work if we run one (host-supplied)
and then if we get XDP_PASS run another (guest supplied)
otherwise don't wake up guest?
> > If the result is XDP_DROP or XDP_TX we don't need to wake up the guest at all!
> >
>
> nice win.
>
> > When using tun for networking - especially with adjust_head - this
> > unfortunately probably means we need to do a data copy unless there is
> > enough headroom. How much is enough though?
>
> We were looking at making headroom configurable on Intel drivers or at
> least matching it with XDP headroom guidelines. (although the developers
> had the same complaint about 256B being large).
> Then at least on supported
> drivers the copy could be an exception path.
So I am concerned that userspace comes to depend on support for 256byte
headroom that this patchset enables. How about
-#define XDP_PACKET_HEADROOM 256
+#define XDP_PACKET_HEADROOM 64
so we start with a concervative value?
In fact NET_SKB_PAD would be ideal I think but it's
platform dependent.
Or at least do the equivalent for virtio only ...
> >
> > Another issue is around host/guest ABI. Guest BPF could add new features
> > at any point. What if hypervisor can not support it all? I guess we
> > could try loading program into hypervisor and run it within guest on
> > failure to load, but this ignores question of cross-version
> > compatibility - someone might start guest on a new host
> > then try to move to an old one. So we will need an option
> > "behave like an older host" such that guest can start and then
> > move to an older host later. This will likely mean
> > implementing this validation of programs in qemu userspace unless linux
> > can supply something like this. Is this (disabling some features)
> > something that might be of interest to larger bpf community?
>
> This is interesting to me at least. Another interesting "feature" of
> running bpf in qemu userspace is it could work with vhost_user as well
> presumably?
I think with vhost user you would want to push it out
to the switch, not run it in qemu.
IOW qemu gets the program and sends it to the switch.
Response is sent to guest so it knows whether switch can support it.
> >
> > With a device such as macvtap there exist configurations where a single
> > guest is in control of the device (aka passthrough mode) in that case
> > there's a potential to run xdp on host before host skb is built, unless
> > host already has an xdp program attached. If it does we could run the
> > program within guest, but what if a guest program got attached first?
> > Maybe we should pass a flag in the packet "xdp passed on this packet in
> > host". Then, guest can skip running it. Unless we do a full reset
> > there's always a potential for packets to slip through, e.g. on xdp
> > program changes. Maybe a flush command is needed, or force queue or
> > device reset to make sure nothing is going on. Does this make sense?
> >
>
> Could the virtio driver pretend its "offloading" the XDP program to
> hardware? This would make it explicit in VM that the program is run
> before data is received by virtio_net. Then qemu is enabling the
> offload framework which would be interesting.
On qemu side this is not a problem a command causes
a trap and qemu could flush the queue. But the packets
are still in the rx queue and get processed by napi later.
I think the cleanest interface for it might be a command consuming
an rx buffer and writing a pre-defined pattern into it.
This way guest can figure out how far did device get in the rx queue.
> > Thanks!
> >
^ permalink raw reply
* Re: [net PATCH v5 6/6] virtio_net: XDP support for adjust_head
From: Michael S. Tsirkin @ 2017-01-23 22:28 UTC (permalink / raw)
To: John Fastabend
Cc: jasowang, john.r.fastabend, netdev, alexei.starovoitov, daniel
In-Reply-To: <58867FDF.4070708@gmail.com>
On Mon, Jan 23, 2017 at 02:12:47PM -0800, John Fastabend wrote:
> On 17-01-23 12:09 PM, Michael S. Tsirkin wrote:
> > On Mon, Jan 23, 2017 at 09:22:36PM +0200, Michael S. Tsirkin wrote:
> >> On Tue, Jan 17, 2017 at 02:22:59PM -0800, John Fastabend wrote:
> >>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> >>> index 62dbf4b..3b129b4 100644
> >>> --- a/drivers/net/virtio_net.c
> >>> +++ b/drivers/net/virtio_net.c
> >>> @@ -41,6 +41,9 @@
> >>> #define GOOD_PACKET_LEN (ETH_HLEN + VLAN_HLEN + ETH_DATA_LEN)
> >>> #define GOOD_COPY_LEN 128
> >>>
> >>> +/* Amount of XDP headroom to prepend to packets for use by xdp_adjust_head */
> >>> +#define VIRTIO_XDP_HEADROOM 256
> >>> +
> >>> /* RX packet size EWMA. The average packet size is used to determine the packet
> >>> * buffer size when refilling RX rings. As the entire RX ring may be refilled
> >>> * at once, the weight is chosen so that the EWMA will be insensitive to short-
> >>
> >> I wonder where does this number come from? This is quite a lot and
> >> means that using XDP_PASS will slow down any sockets on top of it.
> >> Which in turn means people will try to remove XDP when not in use,
> >> causing resets. E.g. build_skb (which I have a patch to switch to) uses
> >> a much more reasonable NET_SKB_PAD.
>
> I just used the value Alexei (or someone?) came up with. I think it needs to be
> large enough to avoid copy in header encap cases. So minimum
>
> VXLAN_HDR + OUTER_UDP + OUTER_IPV6_HDR + OUTER_MAC =
> 8 + 8 + 40 + 14 = 70
>
> The choice of VXLAN hdr was sort of arbitrary but seems good for estimates. For
> what its worth there is also a ndo_set_rx_headroom could we use that to set it
> and choose a reasonable default.
>
> >>
> >> --
> >> MST
> >
> >
> > Let me show you a patch that I've been cooking. What is missing there
> > is handling corner cases like e.g. when ring size is ~4 entries so
> > using smaller buffers might mean we no longer have enough space to store
> > a full packet. So it looks like I have to maintain the skb copy path
> > for this hardware.
> >
> > With this patch, standard configuration has NET_SKB_PAD + NET_IP_ALIGN
> > bytes head padding. Would this be enough for XDP? If yes we do not
> > need the resets.
>
> Based on above seems a bit small (L1_CACHE_BYTES + 2)? How tricky would it
> be to add support for ndo_set_rx_headroom.
Donnu but then what? Expose it to userspace and let admin
make the decision for us?
> >
> > Thoughts?
>
> I'll take a look at the patch this afternoon. Thanks.
>
> >
> > --->
> >
> > virtio_net: switch to build_skb for mrg_rxbuf
> >
> > For small packets data copy was observed to
> > take up about 15% CPU time. Switch to build_skb
> > and avoid the copy when using mergeable rx buffers.
> >
> > As a bonus, medium-size skbs that fit in a page will be
> > completely linear.
> >
> > Of course, we now need to lower the lower bound on packet size,
> > to make sure a sane number of skbs fits in rx socket buffer.
> > By how much? I don't know yet.
> >
> > It might also be useful to prefetch the packet buffer since
> > net stack will likely use it soon.
> >
> > Lightly tested, in particular, I didn't yet test what this
> > actually does to performance - sending this out for early
> > feedback/flames.
> >
> > TODO: it appears that Linux won't handle correctly the case of first
> > buffer being very small (or consisting exclusively of virtio header).
> > This is already the case for current code, need to fix.
> > TODO: might be unfair to the last packet in a fragment as we include
> > remaining space if any in its truesize.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >
> > ----
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index b425fa1..a6b996f 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -38,6 +38,8 @@ module_param(gso, bool, 0444);
> >
> > /* FIXME: MTU in config. */
> > #define GOOD_PACKET_LEN (ETH_HLEN + VLAN_HLEN + ETH_DATA_LEN)
> > +//#define MIN_PACKET_ALLOC GOOD_PACKET_LEN
> > +#define MIN_PACKET_ALLOC 128
> > #define GOOD_COPY_LEN 128
> >
> > /* RX packet size EWMA. The average packet size is used to determine the packet
> > @@ -246,6 +248,9 @@ static void *mergeable_ctx_to_buf_address(unsigned long mrg_ctx)
> > static unsigned long mergeable_buf_to_ctx(void *buf, unsigned int truesize)
> > {
> > unsigned int size = truesize / MERGEABLE_BUFFER_ALIGN;
> > +
> > + BUG_ON((unsigned long)buf & (MERGEABLE_BUFFER_ALIGN - 1));
> > + BUG_ON(size - 1 >= MERGEABLE_BUFFER_ALIGN);
> > return (unsigned long)buf | (size - 1);
> > }
> >
> > @@ -354,25 +359,54 @@ static struct sk_buff *receive_big(struct net_device *dev,
> > return NULL;
> > }
> >
> > +#define VNET_SKB_PAD (NET_SKB_PAD + NET_IP_ALIGN)
> > +#define VNET_SKB_BUG (VNET_SKB_PAD < sizeof(struct virtio_net_hdr_mrg_rxbuf))
> > +#define VNET_SKB_LEN(len) ((len) - sizeof(struct virtio_net_hdr_mrg_rxbuf))
> > +#define VNET_SKB_OFF VNET_SKB_LEN(VNET_SKB_PAD)
> > +
> > +static struct sk_buff *vnet_build_skb(struct virtnet_info *vi,
> > + void *buf,
> > + unsigned int len, unsigned int truesize)
> > +{
> > + struct sk_buff *skb = build_skb(buf, truesize);
> > +
> > + if (!skb)
> > + return NULL;
> > +
> > + skb_reserve(skb, VNET_SKB_PAD);
> > + skb_put(skb, VNET_SKB_LEN(len));
> > +
> > + return skb;
> > +}
> > +
> > static struct sk_buff *receive_mergeable(struct net_device *dev,
> > struct virtnet_info *vi,
> > struct receive_queue *rq,
> > unsigned long ctx,
> > - unsigned int len)
> > + unsigned int len,
> > + struct virtio_net_hdr_mrg_rxbuf *hdr)
> > {
> > void *buf = mergeable_ctx_to_buf_address(ctx);
> > - struct virtio_net_hdr_mrg_rxbuf *hdr = buf;
> > - u16 num_buf = virtio16_to_cpu(vi->vdev, hdr->num_buffers);
> > + u16 num_buf;
> > struct page *page = virt_to_head_page(buf);
> > - int offset = buf - page_address(page);
> > - unsigned int truesize = max(len, mergeable_ctx_to_buf_truesize(ctx));
> > + unsigned int truesize = mergeable_ctx_to_buf_truesize(ctx);
> > + int offset;
> > + struct sk_buff *head_skb;
> > + struct sk_buff *curr_skb;
> > +
> > + BUG_ON(len > truesize);
> >
> > - struct sk_buff *head_skb = page_to_skb(vi, rq, page, offset, len,
> > - truesize);
> > - struct sk_buff *curr_skb = head_skb;
> > + /* copy header: build_skb will overwrite it */
> > + memcpy(hdr, buf + VNET_SKB_OFF, sizeof *hdr);
> > +
> > + head_skb = vnet_build_skb(vi, buf, len, truesize);
> > + curr_skb = head_skb;
> > +
> > + num_buf = virtio16_to_cpu(vi->vdev, hdr->num_buffers);
> >
> > if (unlikely(!curr_skb))
> > goto err_skb;
> > +
> > while (--num_buf) {
> > int num_skb_frags;
> >
> > @@ -386,7 +420,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> > goto err_buf;
> > }
> >
> > - buf = mergeable_ctx_to_buf_address(ctx);
> > + buf = mergeable_ctx_to_buf_address(ctx) + VNET_SKB_OFF;
> > page = virt_to_head_page(buf);
> >
> > num_skb_frags = skb_shinfo(curr_skb)->nr_frags;
> > @@ -403,7 +437,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> > head_skb->truesize += nskb->truesize;
> > num_skb_frags = 0;
> > }
> > - truesize = max(len, mergeable_ctx_to_buf_truesize(ctx));
> > + truesize = mergeable_ctx_to_buf_truesize(ctx);
> > + BUG_ON(len > truesize);
> > if (curr_skb != head_skb) {
> > head_skb->data_len += len;
> > head_skb->len += len;
> > @@ -449,6 +484,7 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
> > struct virtnet_stats *stats = this_cpu_ptr(vi->stats);
> > struct sk_buff *skb;
> > struct virtio_net_hdr_mrg_rxbuf *hdr;
> > + struct virtio_net_hdr_mrg_rxbuf hdr0;
> >
> > if (unlikely(len < vi->hdr_len + ETH_HLEN)) {
> > pr_debug("%s: short packet %i\n", dev->name, len);
> > @@ -465,17 +501,24 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
> > return;
> > }
> >
> > - if (vi->mergeable_rx_bufs)
> > - skb = receive_mergeable(dev, vi, rq, (unsigned long)buf, len);
> > - else if (vi->big_packets)
> > + if (vi->mergeable_rx_bufs) {
> > + skb = receive_mergeable(dev, vi, rq, (unsigned long)buf, len,
> > + &hdr0);
> > + if (unlikely(!skb))
> > + return;
> > + hdr = &hdr0;
> > + } else if (vi->big_packets) {
> > skb = receive_big(dev, vi, rq, buf, len);
> > - else
> > + if (unlikely(!skb))
> > + return;
> > + hdr = skb_vnet_hdr(skb);
> > + } else {
> > skb = receive_small(vi, buf, len);
> > + if (unlikely(!skb))
> > + return;
> > + hdr = skb_vnet_hdr(skb);
> > + }
> >
> > - if (unlikely(!skb))
> > - return;
> > -
> > - hdr = skb_vnet_hdr(skb);
> >
> > u64_stats_update_begin(&stats->rx_syncp);
> > stats->rx_bytes += skb->len;
> > @@ -581,11 +624,14 @@ static int add_recvbuf_big(struct virtnet_info *vi, struct receive_queue *rq,
> >
> > static unsigned int get_mergeable_buf_len(struct ewma_pkt_len *avg_pkt_len)
> > {
> > - const size_t hdr_len = sizeof(struct virtio_net_hdr_mrg_rxbuf);
> > + unsigned int hdr;
> > unsigned int len;
> >
> > - len = hdr_len + clamp_t(unsigned int, ewma_pkt_len_read(avg_pkt_len),
> > - GOOD_PACKET_LEN, PAGE_SIZE - hdr_len);
> > + hdr = ALIGN(VNET_SKB_PAD + SKB_DATA_ALIGN(sizeof(struct skb_shared_info)),
> > + MERGEABLE_BUFFER_ALIGN);
> > +
> > + len = hdr + clamp_t(unsigned int, ewma_pkt_len_read(avg_pkt_len),
> > + MIN_PACKET_ALLOC, PAGE_SIZE - hdr);
> > return ALIGN(len, MERGEABLE_BUFFER_ALIGN);
> > }
> >
> > @@ -601,8 +647,11 @@ static int add_recvbuf_mergeable(struct receive_queue *rq, gfp_t gfp)
> > if (unlikely(!skb_page_frag_refill(len, alloc_frag, gfp)))
> > return -ENOMEM;
> >
> > - buf = (char *)page_address(alloc_frag->page) + alloc_frag->offset;
> > - ctx = mergeable_buf_to_ctx(buf, len);
> > + BUILD_BUG_ON(VNET_SKB_BUG);
> > +
> > + buf = (char *)page_address(alloc_frag->page) + alloc_frag->offset +
> > + VNET_SKB_OFF;
> > + //ctx = mergeable_buf_to_ctx(buf - VNET_SKB_OFF, len);
> > get_page(alloc_frag->page);
> > alloc_frag->offset += len;
> > hole = alloc_frag->size - alloc_frag->offset;
> > @@ -615,8 +664,10 @@ static int add_recvbuf_mergeable(struct receive_queue *rq, gfp_t gfp)
> > len += hole;
> > alloc_frag->offset += hole;
> > }
> > + ctx = mergeable_buf_to_ctx(buf - VNET_SKB_OFF, len);
> >
> > - sg_init_one(rq->sg, buf, len);
> > + sg_init_one(rq->sg, buf,
> > + len - VNET_SKB_OFF - SKB_DATA_ALIGN(sizeof(struct skb_shared_info)));
> > err = virtqueue_add_inbuf(rq->vq, rq->sg, 1, (void *)ctx, gfp);
> > if (err < 0)
> > put_page(virt_to_head_page(buf));
> >
^ permalink raw reply
* Re: [PATCH net-next 3/3] net/tcp-fastopen: Add new API support
From: Willy Tarreau @ 2017-01-23 22:33 UTC (permalink / raw)
To: Wei Wang
Cc: Wei Wang, Linux Kernel Network Developers, David Miller,
Eric Dumazet, Yuchung Cheng
In-Reply-To: <20170123220121.GG20894@1wt.eu>
On Mon, Jan 23, 2017 at 11:01:21PM +0100, Willy Tarreau wrote:
> On Mon, Jan 23, 2017 at 10:37:32PM +0100, Willy Tarreau wrote:
> > On Mon, Jan 23, 2017 at 01:28:53PM -0800, Wei Wang wrote:
> > > Hi Willy,
> > >
> > > True. If you call connect() multiple times on a socket which already has
> > > cookie without a write(), the second and onward connect() call will return
> > > EINPROGRESS.
> > > It is basically because the following code block in __inet_stream_connect()
> > > can't distinguish if it is the first time connect() is called or not:
> > >
> > > case SS_CONNECTING:
> > > if (inet_sk(sk)->defer_connect) <----- defer_connect will
> > > be 0 only after a write() is called
> > > err = -EINPROGRESS;
> > > else
> > > err = -EALREADY;
> > > /* Fall out of switch with err, set for this state */
> > > break;
> >
> > Ah OK that totally makes sense, thanks for the explanation!
> >
> > > I guess we can add some extra logic here to address this issue. So the
> > > second connect() and onwards will return EALREADY.
>
> Thinking about it a bit more, I really think it would make more
> sense to return -EISCONN here if we want to match the semantics
> of a connect() returning zero on the first call. This way the
> caller knows it can write whenever it wants and can disable
> write polling until needed.
>
> I'm currently rebuilding a kernel with this change to see if it
> behaves any better :
>
> - err = -EINPROGRESS;
> + err = -EISCONN;
OK so obviously it didn't work since sendmsg() goes there as well.
But that made me realize that there really are 3 states, not 2 :
- after connect() and before sendmsg() :
defer_accept = 1, we want to lie to userland and pretend we're
connected so that userland can call send(). A connect() must
return either zero or -EISCONN.
- during first sendmsg(), still connecting :
the connection is in progress, EINPROGRESS must be returned to
the first sendmsg().
- after the first sendmsg() :
defer_accept = 0 ; connect() must return -EALREADY. We want to
return real socket states from now on.
Thus I modified defer_accept to take two bits to represent the extra
state we need to indicate the transition. Now sendmsg() upgrades
defer_accept from 1 to 2 before calling __inet_stream_connect(), which
then knows it must return EINPROGRESS to sendmsg().
This way we correctly cover all these situations. Even if we call
connect() again after the first connect() attempt it still matches
the first result :
accept4(7, {sa_family=AF_INET, sin_port=htons(36860), sin_addr=inet_addr("192.168.0.176")}, [128->16], SOCK_NONBLOCK) = 1
setsockopt(1, SOL_TCP, TCP_NODELAY, [1], 4) = 0
accept4(7, 0x7ffc2282fcb0, [128], SOCK_NONBLOCK) = -1 EAGAIN (Resource temporarily unavailable)
recvfrom(1, 0x7b53a4, 7006, 0, NULL, NULL) = -1 EAGAIN (Resource temporarily unavailable)
socket(AF_INET, SOCK_STREAM, IPPROTO_TCP) = 2
fcntl(2, F_SETFL, O_RDONLY|O_NONBLOCK) = 0
setsockopt(2, SOL_TCP, TCP_NODELAY, [1], 4) = 0
setsockopt(2, SOL_TCP, 0x1e /* TCP_??? */, [1], 4) = 0
connect(2, {sa_family=AF_INET, sin_port=htons(8001), sin_addr=inet_addr("127.0.0.1")}, 16) = 0
epoll_ctl(0, EPOLL_CTL_ADD, 1, {EPOLLIN|EPOLLRDHUP, {u32=1, u64=1}}) = 0
epoll_wait(0, [], 200, 0) = 0
connect(2, {sa_family=AF_INET, sin_port=htons(8001), sin_addr=inet_addr("127.0.0.1")}, 16) = -1 EISCONN (Transport endpoint is already connected)
epoll_wait(0, [], 200, 0) = 0
recvfrom(2, 0x7b53a4, 8030, 0, NULL, NULL) = -1 EAGAIN (Resource temporarily unavailable)
epoll_ctl(0, EPOLL_CTL_ADD, 2, {EPOLLIN|EPOLLRDHUP, {u32=2, u64=2}}) = 0
epoll_wait(0, [], 200, 1000) = 0
epoll_wait(0, [], 200, 1000) = 0
epoll_wait(0, [], 200, 1000) = 0
epoll_wait(0, [{EPOLLIN, {u32=1, u64=1}}], 200, 1000) = 1
recvfrom(1, "GET / HTTP/1.1\r\n", 8030, 0, NULL, NULL) = 16
sendto(2, "GET / HTTP/1.1\r\n", 16, MSG_DONTWAIT|MSG_NOSIGNAL, NULL, 0) = 16
epoll_wait(0, [{EPOLLIN, {u32=1, u64=1}}], 200, 1000) = 1
recvfrom(1, "\r\n", 8030, 0, NULL, NULL) = 2
sendto(2, "\r\n", 2, MSG_DONTWAIT|MSG_NOSIGNAL, NULL, 0) = 2
epoll_wait(0, [{EPOLLIN|EPOLLRDHUP, {u32=2, u64=2}}], 200, 1000) = 1
recvfrom(2, "HTTP/1.1 302 Found\r\nCache-Contro"..., 8030, 0, NULL, NULL) = 98
sendto(1, "HTTP/1.1 302 Found\r\nCache-Contro"..., 98, MSG_DONTWAIT|MSG_NOSIGNAL|MSG_MORE, NULL, 0) = 98
shutdown(1, SHUT_WR) = 0
epoll_ctl(0, EPOLL_CTL_DEL, 2, 0x6ff55c) = 0
epoll_wait(0, [{EPOLLIN|EPOLLHUP|EPOLLRDHUP, {u32=1, u64=1}}], 200, 1000) = 1
recvfrom(1, "", 8030, 0, NULL, NULL) = 0
close(1) = 0
shutdown(2, SHUT_WR) = 0
close(2) = 0
Here's what I changed on top of your patchset :
diff --git a/include/net/inet_sock.h b/include/net/inet_sock.h
index 0042fed..dc53f7f 100644
--- a/include/net/inet_sock.h
+++ b/include/net/inet_sock.h
@@ -207,9 +207,10 @@ struct inet_sock {
mc_all:1,
nodefrag:1;
__u8 bind_address_no_port:1,
- defer_connect:1; /* Indicates that fastopen_connect is set
+ defer_connect:2; /* Indicates that fastopen_connect is set
* and cookie exists so we defer connect
- * until first data frame is written
+ * until first data frame is written.
+ * Switches from 1 to 2 during first write()
*/
__u8 rcv_tos;
__u8 convert_csum;
diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index e67d572..6dda9d5a 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -594,8 +594,10 @@ int __inet_stream_connect(struct socket *sock, struct sockaddr *uaddr,
err = -EISCONN;
goto out;
case SS_CONNECTING:
- if (inet_sk(sk)->defer_connect)
- err = -EINPROGRESS;
+ if (inet_sk(sk)->defer_connect == 2)
+ err = -EINPROGRESS; /* sendmsg started */
+ else if (inet_sk(sk)->defer_connect == 1)
+ err = -EISCONN; /* suggest to send now */
else
err = -EALREADY;
/* Fall out of switch with err, set for this state */
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 3c8938d..bd71f60 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -1103,6 +1103,10 @@ static int tcp_sendmsg_fastopen(struct sock *sk, struct msghdr *msg,
inet->inet_dport = 0;
sk->sk_route_caps = 0;
}
+ /* tell __inet_stream_connect() that we're doing the
+ * first write.
+ */
+ inet->defer_connect = 2;
}
flags = (msg->msg_flags & MSG_DONTWAIT) ? O_NONBLOCK : 0;
err = __inet_stream_connect(sk->sk_socket, msg->msg_name,
Is this also what you had in mind ?
thanks,
Willy
^ permalink raw reply related
* Re: Question about veth_xmit()
From: Cong Wang @ 2017-01-23 22:42 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Xiangning Yu, Linux Kernel Network Developers
In-Reply-To: <1485209145.16328.214.camel@edumazet-glaptop3.roam.corp.google.com>
On Mon, Jan 23, 2017 at 2:05 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Mon, 2017-01-23 at 13:46 -0800, Xiangning Yu wrote:
>> On Mon, Jan 23, 2017 at 12:56 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> > On Mon, Jan 23, 2017 at 10:46 AM, Xiangning Yu <yuxiangning@gmail.com> wrote:
>> >> Hi netdev folks,
>> >>
>> >> It looks like we call dev_forward_skb() in veth_xmit(), which calls
>> >> netif_rx() eventually.
>> >>
>> >> While netif_rx() will enqueue the skb to the CPU RX backlog before the
>> >> actual processing takes place. So, this actually means a TX skb has to
>> >> wait some un-related RX skbs to finish. And this will happen twice for
>> >> a single ping, because the veth device always works as a pair?
>> >
>> > For me it is more like for the completeness of network stack of each
>> > netns. The /proc net.core.netdev_max_backlog etc. are per netns, which
>> > means each netns, as an independent network stack, should respect it
>> > too.
>> >
>> > Since you care about latency, why not tune net.core.dev_weight for your
>> > own netns?
>>
>> I haven't tried that yet, thank you for the hint! Though normally one
>> of the veth device will be in the global namespace.
>
> Well, per cpu backlog are not per net ns, but per cpu.
Right, they all point to a same weight_p. But weight_p itself
is not per cpu, softnet_data is. However, if a container uses
cpuset and netns for isolations, per cpu will turn into per netns.
The point of network stack completeness still stands.
^ permalink raw reply
* Re: [PATCH net-next 3/3] net/tcp-fastopen: Add new API support
From: Willy Tarreau @ 2017-01-23 23:01 UTC (permalink / raw)
To: Wei Wang
Cc: Wei Wang, Linux Kernel Network Developers, David Miller,
Eric Dumazet, Yuchung Cheng
In-Reply-To: <CAC15z3g8OxZNET+OnvcyYwHYuHq7QBamgGmjkBHzDr-XnUSGDQ@mail.gmail.com>
On Mon, Jan 23, 2017 at 02:57:31PM -0800, Wei Wang wrote:
> Yes. That seems to be a valid fix to it.
> Let me try it with my existing test cases as well to see if it works for
> all scenarios I have.
Perfect. Note that since the state 2 is transient I initially thought
about abusing the flags passed to __inet_stream_connect() to say "hey
I'm sendmsg() and not connect()" but that would have been a ugly hack
while here we really have the 3 socket states represented eventhough
one changes twice around a call.
Thanks,
Willy
^ permalink raw reply
* Re: [PATCH 2/3] ath10k: use dma_zalloc_coherent()
From: Joe Perches @ 2017-01-23 23:19 UTC (permalink / raw)
To: Srinivas Kandagatla, Kalle Valo
Cc: ath10k, linux-wireless, netdev, linux-kernel
In-Reply-To: <1485183876-27080-2-git-send-email-srinivas.kandagatla@linaro.org>
On Mon, 2017-01-23 at 15:04 +0000, Srinivas Kandagatla wrote:
> use dma_zalloc_coherent() instead of dma_alloc_coherent and memset().
[]
> diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
[]
> @@ -896,7 +896,7 @@ static int ath10k_pci_diag_read_mem(struct ath10k *ar, u32 address, void *data,
> */
> alloc_nbytes = min_t(unsigned int, nbytes, DIAG_TRANSFER_LIMIT);
>
> - data_buf = (unsigned char *)dma_alloc_coherent(ar->dev,
> + data_buf = (unsigned char *)dma_zalloc_coherent(ar->dev,
> alloc_nbytes,
> &ce_data_base,
> GFP_ATOMIC);
trivia:
Nicer to realign arguments and remove the unnecessary cast.
Perhaps:
data_buf = dma_zalloc_coherent(ar->dev, alloc_nbytes, &ce_data_base,
GFP_ATOMIC);
^ permalink raw reply
* sk_buff and reference counting netdev pointers
From: Joel Cunningham @ 2017-01-23 22:37 UTC (permalink / raw)
To: netdev
Hi,
I’m working on a research effort to understand the synchronization mechanisms for accessing and modifying a struct net_device object. One area that isn’t clear is the net device pointer (dev) stored in a struct sk_buff. From my investigation, the pointer appears to be assigned without increasing the struct net_device’s reference count (example __netdev_alloc_skb doesn’t call dev_hold) and also when the sk_buff is freed (kfree_skb) no call to dev_put() is made. This seems to leave a possibility of an skb referencing a stale net device unless something is cleaning up all the skbs during unregister_netdevice() (which waits for all outstanding references to be released). Any insight in understanding how this is working would be appreciated!
Joel
^ permalink raw reply
* Re: sk_buff and reference counting netdev pointers
From: Cong Wang @ 2017-01-23 23:45 UTC (permalink / raw)
To: Joel Cunningham; +Cc: Linux Kernel Network Developers
In-Reply-To: <E593AEBC-5FD1-4CEF-A119-2C032DB574FC@me.com>
On Mon, Jan 23, 2017 at 2:37 PM, Joel Cunningham <joel.cunningham@me.com> wrote:
> Hi,
>
> I’m working on a research effort to understand the synchronization mechanisms for accessing and modifying a struct net_device object. One area that isn’t clear is the net device pointer (dev) stored in a struct sk_buff. From my investigation, the pointer appears to be assigned without increasing the struct net_device’s reference count (example __netdev_alloc_skb doesn’t call dev_hold) and also when the sk_buff is freed (kfree_skb) no call to dev_put() is made. This seems to leave a possibility of an skb referencing a stale net device unless something is cleaning up all the skbs during unregister_netdevice() (which waits for all outstanding references to be released). Any insight in understanding how this is working would be appreciated!
>
This is a very common question.
synchronize_net() is supposed to wait for on-flying packets, since
both for TX and RX paths we acquire RCU read lock.
^ permalink raw reply
* [RFC PATCH] mlx5: Fix page rfcnt issue
From: Tom Herbert @ 2017-01-23 23:56 UTC (permalink / raw)
To: netdev, tariqt, saeedm; +Cc: kernel-team
This patch is an FYI about possible issuses in mlx5.
There are two issues we discovered in the mlx5 backport from 4.9 to
4.6...
The bad behaviours we were seeing was a refcnts going to less than zero
and eventually killing hosts. We've only seen this running a real
application work load and it does take a while to trigger. The root
cause is unclear, however this patch seems to circumvent the issue. I
have not been able to reproduce this issue upstream because I haven't
been able to get the app running cleanly. At this point I cannot verify
if it is an upstream issue or not.
Specific issues that I have identified are:
1) In mlx5e_free_rx_mpwqe we are seeing cases where wi->skbs_frags[i] >
pg_strides so that a negative refcnt is being subtracted. The warning
at en_rx.c:409 of this patch does trigger.
2) Checksum faults are occurring. I have previously described this
problem.
As for the checksum faults that seems to be an unrelated issue and I do
believe that was an upstream issue in 4.9, but may have been fixed in
4.10. This is easier to reproduce than the page refcnt issue-- just a
few concurrent netperf TCP_STREAMs was able to trigger the faults.
One other potential problem in the driver is the use of put_page in
release pages. Comparing how the allocation is done in other drivers
(for instance comparing to ixgbe) some seem to use __free_pages instead.
I don't know which is correct to use, but somehow it doesn't seem like
they can both be right.
This patch:
1) We no longer do the page_ref_sub page_ref_add, instead we take the
more traditional approach of just doing get_page when giving the
page to an skb.
2) Add warnings to catch cases where operations on page refcnts are
nonsensical
3) Comment out checksum-complete processing in order to squelch the
checksum faults.
---
drivers/net/ethernet/mellanox/mlx5/core/en_rx.c | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
index 20f116f..b24c2b8 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
@@ -209,6 +209,7 @@ static inline bool mlx5e_rx_cache_get(struct mlx5e_rq *rq,
}
if (page_ref_count(cache->page_cache[cache->head].page) != 1) {
+ WARN_ON(page_ref_count(cache->page_cache[cache->head].page) <= 0);
rq->stats.cache_busy++;
return false;
}
@@ -292,6 +293,13 @@ static inline void mlx5e_add_skb_frag_mpwqe(struct mlx5e_rq *rq,
wi->umr.dma_info[page_idx].addr + frag_offset,
len, DMA_FROM_DEVICE);
wi->skbs_frags[page_idx]++;
+ WARN_ON(wi->skbs_frags[page_idx] > mlx5e_mpwqe_strides_per_page(rq));
+
+ /* Take a page reference every time we give page to skb (alternative
+ * to original mlx code).
+ */
+ get_page(wi->umr.dma_info[page_idx].page);
+
skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags,
wi->umr.dma_info[page_idx].page, frag_offset,
len, truesize);
@@ -372,7 +380,6 @@ static int mlx5e_alloc_rx_umr_mpwqe(struct mlx5e_rq *rq,
if (unlikely(err))
goto err_unmap;
wi->umr.mtt[i] = cpu_to_be64(dma_info->addr | MLX5_EN_WR);
- page_ref_add(dma_info->page, pg_strides);
wi->skbs_frags[i] = 0;
}
@@ -385,7 +392,6 @@ static int mlx5e_alloc_rx_umr_mpwqe(struct mlx5e_rq *rq,
while (--i >= 0) {
struct mlx5e_dma_info *dma_info = &wi->umr.dma_info[i];
- page_ref_sub(dma_info->page, pg_strides);
mlx5e_page_release(rq, dma_info, true);
}
@@ -400,7 +406,7 @@ void mlx5e_free_rx_mpwqe(struct mlx5e_rq *rq, struct mlx5e_mpw_info *wi)
for (i = 0; i < MLX5_MPWRQ_PAGES_PER_WQE; i++) {
struct mlx5e_dma_info *dma_info = &wi->umr.dma_info[i];
- page_ref_sub(dma_info->page, pg_strides - wi->skbs_frags[i]);
+ WARN_ON(pg_strides - wi->skbs_frags[i] < 0);
mlx5e_page_release(rq, dma_info, true);
}
}
@@ -565,12 +571,17 @@ static inline void mlx5e_handle_csum(struct net_device *netdev,
return;
}
+#if 0
+ /* Disabled since we are seeing checksum faults occurring. This should
+ * not have any noticeable impact (in the short term).
+ */
if (is_first_ethertype_ip(skb)) {
skb->ip_summed = CHECKSUM_COMPLETE;
skb->csum = csum_unfold((__force __sum16)cqe->check_sum);
rq->stats.csum_complete++;
return;
}
+#endif
if (likely((cqe->hds_ip_ext & CQE_L3_OK) &&
(cqe->hds_ip_ext & CQE_L4_OK))) {
@@ -929,6 +940,7 @@ void mlx5e_handle_rx_cqe_mpwrq(struct mlx5e_rq *rq, struct mlx5_cqe64 *cqe)
if (likely(wi->consumed_strides < rq->mpwqe_num_strides))
return;
+ WARN_ON(wi->consumed_strides > rq->mpwqe_num_strides);
mlx5e_free_rx_mpwqe(rq, wi);
mlx5_wq_ll_pop(&rq->wq, cqe->wqe_id, &wqe->next.next_wqe_index);
}
--
2.9.3
^ permalink raw reply related
* [PATCH net-next 1/2] ip6_tunnel: must reload ipv6h in ip6ip6_tnl_xmit()
From: Eric Dumazet @ 2017-01-23 23:58 UTC (permalink / raw)
To: David S . Miller
Cc: netdev, Eric Dumazet, Willem de Bruijn, Alexei Starovoitov,
Eric Dumazet, Dmitry Kozlov
In-Reply-To: <20170123235804.29342-1-edumazet@google.com>
Since ip6_tnl_parse_tlv_enc_lim() can call pskb_may_pull(),
we must reload any pointer that was related to skb->head
(or skb->data), or risk use after free.
Fixes: c12b395a4664 ("gre: Support GRE over IPv6")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Dmitry Kozlov <xeb@mail.ru>
---
net/ipv6/ip6_gre.c | 3 +++
net/ipv6/ip6_tunnel.c | 2 ++
2 files changed, 5 insertions(+)
diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
index 75b6108234dd05a54af0ae51c7c11eaf1ca2..558631860d91bbcb1321a4b566b6e92ddcaf 100644
--- a/net/ipv6/ip6_gre.c
+++ b/net/ipv6/ip6_gre.c
@@ -582,6 +582,9 @@ static inline int ip6gre_xmit_ipv6(struct sk_buff *skb, struct net_device *dev)
return -1;
offset = ip6_tnl_parse_tlv_enc_lim(skb, skb_network_header(skb));
+ /* ip6_tnl_parse_tlv_enc_lim() might have reallocated skb->head */
+ ipv6h = ipv6_hdr(skb);
+
if (offset > 0) {
struct ipv6_tlv_tnl_enc_lim *tel;
tel = (struct ipv6_tlv_tnl_enc_lim *)&skb_network_header(skb)[offset];
diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index 753d6d0860fb14c100ab8b20799782ab8160..02923f956ac8ba5f3270e8d5282fd9637c2d 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -1303,6 +1303,8 @@ ip6ip6_tnl_xmit(struct sk_buff *skb, struct net_device *dev)
fl6.flowlabel = key->label;
} else {
offset = ip6_tnl_parse_tlv_enc_lim(skb, skb_network_header(skb));
+ /* ip6_tnl_parse_tlv_enc_lim() might have reallocated skb->head */
+ ipv6h = ipv6_hdr(skb);
if (offset > 0) {
struct ipv6_tlv_tnl_enc_lim *tel;
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* [PATCH net 0/2] ipv6: fix ip6_tnl_parse_tlv_enc_lim() issues
From: Eric Dumazet @ 2017-01-23 23:58 UTC (permalink / raw)
To: David S . Miller
Cc: netdev, Eric Dumazet, Willem de Bruijn, Alexei Starovoitov,
Eric Dumazet
First patch fixes ip6_tnl_parse_tlv_enc_lim() callers,
bug added in linux-3.7
Second patch fixes ip6_tnl_parse_tlv_enc_lim() itself,
bug predates linux-2.6.12
Based on a report from Dmitry Vyukov, thanks to KASAN.
Eric Dumazet (2):
ip6_tunnel: must reload ipv6h in ip6ip6_tnl_xmit()
ipv6: fix ip6_tnl_parse_tlv_enc_lim()
net/ipv6/ip6_gre.c | 3 +++
net/ipv6/ip6_tunnel.c | 34 +++++++++++++++++++++++-----------
2 files changed, 26 insertions(+), 11 deletions(-)
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply
* [PATCH net-next 2/2] ipv6: fix ip6_tnl_parse_tlv_enc_lim()
From: Eric Dumazet @ 2017-01-23 23:58 UTC (permalink / raw)
To: David S . Miller
Cc: netdev, Eric Dumazet, Willem de Bruijn, Alexei Starovoitov,
Eric Dumazet
In-Reply-To: <20170123235804.29342-1-edumazet@google.com>
This function suffers from multiple issues.
First one is that pskb_may_pull() may reallocate skb->head,
so the 'raw' pointer needs either to be reloaded or not used at all.
Second issue is that NEXTHDR_DEST handling does not validate
that the options are present in skb->data, so we might read
garbage or access non existent memory.
With help from Willem de Bruijn.
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Dmitry Vyukov <dvyukov@google.com>
Cc: Willem de Bruijn <willemb@google.com>
---
net/ipv6/ip6_tunnel.c | 32 +++++++++++++++++++++-----------
1 file changed, 21 insertions(+), 11 deletions(-)
diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index 02923f956ac8ba5f3270e8d5282fd9637c2d..f9c43c9043f7e386108bca2f2aeb866cfb76 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -400,18 +400,18 @@ ip6_tnl_dev_uninit(struct net_device *dev)
__u16 ip6_tnl_parse_tlv_enc_lim(struct sk_buff *skb, __u8 *raw)
{
- const struct ipv6hdr *ipv6h = (const struct ipv6hdr *) raw;
- __u8 nexthdr = ipv6h->nexthdr;
- __u16 off = sizeof(*ipv6h);
+ const struct ipv6hdr *ipv6h = (const struct ipv6hdr *)raw;
+ unsigned int nhoff = (raw - skb->data) + sizeof(*ipv6h);
+ u8 next, nexthdr = ipv6h->nexthdr;
while (ipv6_ext_hdr(nexthdr) && nexthdr != NEXTHDR_NONE) {
- __u16 optlen = 0;
struct ipv6_opt_hdr *hdr;
- if (raw + off + sizeof(*hdr) > skb->data &&
- !pskb_may_pull(skb, raw - skb->data + off + sizeof (*hdr)))
+ u16 optlen;
+
+ if (!pskb_may_pull(skb, nhoff + sizeof(*hdr)))
break;
- hdr = (struct ipv6_opt_hdr *) (raw + off);
+ hdr = (struct ipv6_opt_hdr *)(skb->data + nhoff);
if (nexthdr == NEXTHDR_FRAGMENT) {
struct frag_hdr *frag_hdr = (struct frag_hdr *) hdr;
if (frag_hdr->frag_off)
@@ -422,13 +422,23 @@ __u16 ip6_tnl_parse_tlv_enc_lim(struct sk_buff *skb, __u8 *raw)
} else {
optlen = ipv6_optlen(hdr);
}
+ /* cache hdr->nexthdr, since pskb_may_pull() might
+ * invalidate hdr
+ */
+ next = hdr->nexthdr;
if (nexthdr == NEXTHDR_DEST) {
- __u16 i = off + 2;
+ u16 i = 2;
+
+ /* Remember : hdr is no longer valid at this point. */
+ if (!pskb_may_pull(skb, nhoff + optlen))
+ break;
+
+ raw = skb->data + nhoff;
while (1) {
struct ipv6_tlv_tnl_enc_lim *tel;
/* No more room for encapsulation limit */
- if (i + sizeof (*tel) > off + optlen)
+ if (i + sizeof(*tel) > optlen)
break;
tel = (struct ipv6_tlv_tnl_enc_lim *) &raw[i];
@@ -443,8 +453,8 @@ __u16 ip6_tnl_parse_tlv_enc_lim(struct sk_buff *skb, __u8 *raw)
i++;
}
}
- nexthdr = hdr->nexthdr;
- off += optlen;
+ nexthdr = next;
+ nhoff += optlen;
}
return 0;
}
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* Re: [patch net-next 1/4] net: Introduce psample, a new genetlink channel for packet sampling
From: Stephen Hemminger @ 2017-01-23 23:57 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev, davem, yotamg, idosch, eladr, nogahf, ogerlitz, jhs,
geert+renesas, xiyou.wangcong, linux, roopa, john.fastabend,
simon.horman, mrv
In-Reply-To: <1485085487-2652-2-git-send-email-jiri@resnulli.us>
On Sun, 22 Jan 2017 12:44:44 +0100
Jiri Pirko <jiri@resnulli.us> wrote:
> +static LIST_HEAD(psample_groups_list);
> +static DEFINE_SPINLOCK(psample_groups_lock);
> +
Why not a mutex? You aren't acquiring this in IRQ context?
^ permalink raw reply
* Re: [RFC PATCH net-next 0/5] bridge: per vlan lwt and dst_metadata support
From: Roopa Prabhu @ 2017-01-24 0:00 UTC (permalink / raw)
To: Jiri Benc
Cc: Jiri Pirko, netdev, davem, stephen, nikolay, tgraf, hannes,
pshelar, dsa, hadi
In-Reply-To: <20170123172458.5c849171@griffin>
On 1/23/17, 8:24 AM, Jiri Benc wrote:
> On Mon, 23 Jan 2017 08:13:30 -0800, Roopa Prabhu wrote:
>> And, a 'vlan-to-tunid' mapping is a very common configuration in L2 ethernet vpn configurations.
> You have one particular and narrow use case in mind and are proposing a
> rather large patchset to add support for that (and only that) single
> use case, while we already have a generic mechanism in place to address
> this and many similar (and dissimilar, too) use cases. That doesn't
> sound right.
Let me clarify:
the generic mechanism you are talking about is dst_metadata infra. Any subsystem can use it.
tc vlan and dst_metadata wrapper/filter provide a creative way to use it inside the tc subsystem and is very
useful for people using tc all-around.
What I am proposing here is hooks in bridge to use the dst_metadata for pure L2 networks who
use the bridge driver. This is similar to how we have lwt plugged into the L3 (routing) code.
If you are using the bridge driver for vlan config and filtering, I don't see why one
has to duplicate vlan config using tc. Its painful trying to deploy l2 networks with vlan config spanning
multiple subsystems and apis.
Regarding the patch-set size, let me give you a breakdown:
If i used tc for passing dst_metadata (assume 4k vlans that are participating in l2 ethernet vpn):
(a) configure bridging/vlan filtering using bridge driver (4k vlans)
(b) configure tc rules to map vlans to tunnel-id (Additional patch to tc to only allow tunnel-id in dst_metadata: ingress + egress = 8k tc rules)
(c) vxlan driver patch to make it bridge friendly (my patch in this series is required regardless if i use tc or bridge driver for dst_metadata because vxlan driver learns and needs to carry the forwarding information database)
(d) ethernet vpn controller (quagga bgp) looks at 'bridge api + vxlan api + tc filtering rules'
My current series:
(a) configure bridging/vlan filtering using bridge driver (4k vlans with tunnel info)
(b) vxlan driver patch to make it bridge friendly (my patch in this series is required regardless if
i use tc or bridge driver for dst_metadata because vxlan driver learns and needs to carry the forwarding information database)
(c) ethernet vpn controller (quagga bgp) looks at 'bridge api + vxlan api'
And btw, most of the functions that i am adding in the bridge driver are related to vlan range handling.
vlan ranges code is tricky and i am trying to also support vlan-tunnelid mapping in ranges, and i have tried
to rewrite my own vlan range code (added long back) to include tunnel info. The rest is just use of the dst_metadata infra
to store and use dst_metadata per vlan.
>
> If the current generic mechanisms have bottlenecks for your use case,
> let's work on removing those bottlenecks. That way, everybody benefits,
> not just a single use case.
For people using all tc, the tc wrapper for dst_metadata is a good fit.
I see my series as still using the generic 'dst_metadata' mechanism/infra for a newer use case.
like i say above, I see this similar to how we have plugged dst_metadata into the L3 (routing) code.
This does it in the bridging code (for L2 networks).
Thanks,
Roopa
^ permalink raw reply
* Re: [PATCH net-next 2/2] ipv6: fix ip6_tnl_parse_tlv_enc_lim()
From: Eric Dumazet @ 2017-01-24 0:06 UTC (permalink / raw)
To: David S . Miller
Cc: netdev, Eric Dumazet, Willem de Bruijn, Alexei Starovoitov,
Eric Dumazet
In-Reply-To: <20170123235804.29342-3-edumazet@google.com>
On Mon, Jan 23, 2017 at 3:58 PM, Eric Dumazet <edumazet@google.com> wrote:
> This function suffers from multiple issues.
>
> First one is that pskb_may_pull() may reallocate skb->head,
> so the 'raw' pointer needs either to be reloaded or not used at all.
>
> Second issue is that NEXTHDR_DEST handling does not validate
> that the options are present in skb->data, so we might read
> garbage or access non existent memory.
>
> With help from Willem de Bruijn.
Hmm, I've added a bug. Will send a V2, sorry for this.
^ permalink raw reply
* [PATCH net-next 2/5] bpf: enable load bytes helper for filter/reuseport progs
From: Daniel Borkmann @ 2017-01-24 0:06 UTC (permalink / raw)
To: davem; +Cc: netdev, ast, Daniel Borkmann
In-Reply-To: <cover.1485214851.git.daniel@iogearbox.net>
BPF_PROG_TYPE_SOCKET_FILTER are used in various facilities such as
for SO_REUSEPORT and packet fanout demuxing, packet filtering, kcm,
etc, and yet the only facility they can use is BPF_LD with {BPF_ABS,
BPF_IND} for single byte/half/word access.
Direct packet access is only restricted to tc programs right now,
but we can still facilitate usage by allowing skb_load_bytes() helper
added back then in 05c74e5e53f6 ("bpf: add bpf_skb_load_bytes helper")
that calls skb_header_pointer() similarly to bpf_load_pointer(), but
for stack buffers with larger access size.
Name the previous sk_filter_func_proto() as bpf_base_func_proto()
since this is used everywhere else as well, similarly for the ctx
converter, that is, bpf_convert_ctx_access().
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Acked-by: Alexei Starovoitov <ast@kernel.org>
---
net/core/filter.c | 41 ++++++++++++++++++++++++++---------------
1 file changed, 26 insertions(+), 15 deletions(-)
diff --git a/net/core/filter.c b/net/core/filter.c
index 883975f..e2263da 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -2598,7 +2598,7 @@ static unsigned long bpf_xdp_copy(void *dst_buff, const void *src_buff,
};
static const struct bpf_func_proto *
-sk_filter_func_proto(enum bpf_func_id func_id)
+bpf_base_func_proto(enum bpf_func_id func_id)
{
switch (func_id) {
case BPF_FUNC_map_lookup_elem:
@@ -2626,6 +2626,17 @@ static unsigned long bpf_xdp_copy(void *dst_buff, const void *src_buff,
}
static const struct bpf_func_proto *
+sk_filter_func_proto(enum bpf_func_id func_id)
+{
+ switch (func_id) {
+ case BPF_FUNC_skb_load_bytes:
+ return &bpf_skb_load_bytes_proto;
+ default:
+ return bpf_base_func_proto(func_id);
+ }
+}
+
+static const struct bpf_func_proto *
tc_cls_act_func_proto(enum bpf_func_id func_id)
{
switch (func_id) {
@@ -2680,7 +2691,7 @@ static unsigned long bpf_xdp_copy(void *dst_buff, const void *src_buff,
case BPF_FUNC_skb_under_cgroup:
return &bpf_skb_under_cgroup_proto;
default:
- return sk_filter_func_proto(func_id);
+ return bpf_base_func_proto(func_id);
}
}
@@ -2695,7 +2706,7 @@ static unsigned long bpf_xdp_copy(void *dst_buff, const void *src_buff,
case BPF_FUNC_xdp_adjust_head:
return &bpf_xdp_adjust_head_proto;
default:
- return sk_filter_func_proto(func_id);
+ return bpf_base_func_proto(func_id);
}
}
@@ -2706,7 +2717,7 @@ static unsigned long bpf_xdp_copy(void *dst_buff, const void *src_buff,
case BPF_FUNC_skb_load_bytes:
return &bpf_skb_load_bytes_proto;
default:
- return sk_filter_func_proto(func_id);
+ return bpf_base_func_proto(func_id);
}
}
@@ -2733,7 +2744,7 @@ static unsigned long bpf_xdp_copy(void *dst_buff, const void *src_buff,
case BPF_FUNC_skb_under_cgroup:
return &bpf_skb_under_cgroup_proto;
default:
- return sk_filter_func_proto(func_id);
+ return bpf_base_func_proto(func_id);
}
}
@@ -2983,10 +2994,10 @@ void bpf_warn_invalid_xdp_action(u32 act)
}
EXPORT_SYMBOL_GPL(bpf_warn_invalid_xdp_action);
-static u32 sk_filter_convert_ctx_access(enum bpf_access_type type,
- const struct bpf_insn *si,
- struct bpf_insn *insn_buf,
- struct bpf_prog *prog)
+static u32 bpf_convert_ctx_access(enum bpf_access_type type,
+ const struct bpf_insn *si,
+ struct bpf_insn *insn_buf,
+ struct bpf_prog *prog)
{
struct bpf_insn *insn = insn_buf;
int off;
@@ -3210,7 +3221,7 @@ static u32 tc_cls_act_convert_ctx_access(enum bpf_access_type type,
offsetof(struct net_device, ifindex));
break;
default:
- return sk_filter_convert_ctx_access(type, si, insn_buf, prog);
+ return bpf_convert_ctx_access(type, si, insn_buf, prog);
}
return insn - insn_buf;
@@ -3242,7 +3253,7 @@ static u32 xdp_convert_ctx_access(enum bpf_access_type type,
static const struct bpf_verifier_ops sk_filter_ops = {
.get_func_proto = sk_filter_func_proto,
.is_valid_access = sk_filter_is_valid_access,
- .convert_ctx_access = sk_filter_convert_ctx_access,
+ .convert_ctx_access = bpf_convert_ctx_access,
};
static const struct bpf_verifier_ops tc_cls_act_ops = {
@@ -3261,24 +3272,24 @@ static u32 xdp_convert_ctx_access(enum bpf_access_type type,
static const struct bpf_verifier_ops cg_skb_ops = {
.get_func_proto = cg_skb_func_proto,
.is_valid_access = sk_filter_is_valid_access,
- .convert_ctx_access = sk_filter_convert_ctx_access,
+ .convert_ctx_access = bpf_convert_ctx_access,
};
static const struct bpf_verifier_ops lwt_inout_ops = {
.get_func_proto = lwt_inout_func_proto,
.is_valid_access = lwt_is_valid_access,
- .convert_ctx_access = sk_filter_convert_ctx_access,
+ .convert_ctx_access = bpf_convert_ctx_access,
};
static const struct bpf_verifier_ops lwt_xmit_ops = {
.get_func_proto = lwt_xmit_func_proto,
.is_valid_access = lwt_is_valid_access,
- .convert_ctx_access = sk_filter_convert_ctx_access,
+ .convert_ctx_access = bpf_convert_ctx_access,
.gen_prologue = tc_cls_act_prologue,
};
static const struct bpf_verifier_ops cg_sock_ops = {
- .get_func_proto = sk_filter_func_proto,
+ .get_func_proto = bpf_base_func_proto,
.is_valid_access = sock_filter_is_valid_access,
.convert_ctx_access = sock_filter_convert_ctx_access,
};
--
1.9.3
^ permalink raw reply related
* [PATCH net-next 4/5] bpf: add prog tag test case to bpf selftests
From: Daniel Borkmann @ 2017-01-24 0:06 UTC (permalink / raw)
To: davem; +Cc: netdev, ast, Daniel Borkmann
In-Reply-To: <cover.1485214851.git.daniel@iogearbox.net>
Add the test case used to compare the results from fdinfo with
af_alg's output on the tag. Tests are from min to max sized
programs, with and without maps included.
# ./test_tag
test_tag: OK (40945 tests)
Tested on x86_64 and s390x.
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Acked-by: Alexei Starovoitov <ast@kernel.org>
---
tools/testing/selftests/bpf/Makefile | 4 +-
tools/testing/selftests/bpf/test_tag.c | 202 +++++++++++++++++++++++++++++++++
2 files changed, 204 insertions(+), 2 deletions(-)
create mode 100644 tools/testing/selftests/bpf/test_tag.c
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 064a3e5..769a6cb 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -1,8 +1,8 @@
CFLAGS += -Wall -O2 -I../../../../usr/include
-test_objs = test_verifier test_maps test_lru_map test_lpm_map
+test_objs = test_verifier test_tag test_maps test_lru_map test_lpm_map
-TEST_PROGS := test_verifier test_maps test_lru_map test_lpm_map test_kmod.sh
+TEST_PROGS := $(test_objs) test_kmod.sh
TEST_FILES := $(test_objs)
all: $(test_objs)
diff --git a/tools/testing/selftests/bpf/test_tag.c b/tools/testing/selftests/bpf/test_tag.c
new file mode 100644
index 0000000..6ab4793
--- /dev/null
+++ b/tools/testing/selftests/bpf/test_tag.c
@@ -0,0 +1,202 @@
+#include <stdio.h>
+#include <stdlib.h>
+#include <ctype.h>
+#include <time.h>
+#include <errno.h>
+#include <unistd.h>
+#include <string.h>
+#include <sched.h>
+#include <limits.h>
+#include <assert.h>
+
+#include <sys/socket.h>
+#include <sys/resource.h>
+
+#include <linux/filter.h>
+#include <linux/bpf.h>
+#include <linux/if_alg.h>
+
+#include "../../../include/linux/filter.h"
+
+#include "bpf_sys.h"
+
+static struct bpf_insn prog[BPF_MAXINSNS];
+
+static void bpf_gen_imm_prog(unsigned int insns, int fd_map)
+{
+ int i;
+
+ srand(time(NULL));
+ for (i = 0; i < insns; i++)
+ prog[i] = BPF_ALU64_IMM(BPF_MOV, i % BPF_REG_10, rand());
+ prog[i - 1] = BPF_EXIT_INSN();
+}
+
+static void bpf_gen_map_prog(unsigned int insns, int fd_map)
+{
+ int i, j = 0;
+
+ for (i = 0; i + 1 < insns; i += 2) {
+ struct bpf_insn tmp[] = {
+ BPF_LD_MAP_FD(j++ % BPF_REG_10, fd_map)
+ };
+
+ memcpy(&prog[i], tmp, sizeof(tmp));
+ }
+ if (insns % 2 == 0)
+ prog[insns - 2] = BPF_ALU64_IMM(BPF_MOV, i % BPF_REG_10, 42);
+ prog[insns - 1] = BPF_EXIT_INSN();
+}
+
+static int bpf_try_load_prog(int insns, int fd_map,
+ void (*bpf_filler)(unsigned int insns,
+ int fd_map))
+{
+ int fd_prog;
+
+ bpf_filler(insns, fd_map);
+ fd_prog = bpf_prog_load(BPF_PROG_TYPE_SCHED_CLS, prog, insns *
+ sizeof(struct bpf_insn), "", NULL, 0);
+ assert(fd_prog > 0);
+ if (fd_map > 0)
+ bpf_filler(insns, 0);
+ return fd_prog;
+}
+
+static int __hex2bin(char ch)
+{
+ if ((ch >= '0') && (ch <= '9'))
+ return ch - '0';
+ ch = tolower(ch);
+ if ((ch >= 'a') && (ch <= 'f'))
+ return ch - 'a' + 10;
+ return -1;
+}
+
+static int hex2bin(uint8_t *dst, const char *src, size_t count)
+{
+ while (count--) {
+ int hi = __hex2bin(*src++);
+ int lo = __hex2bin(*src++);
+
+ if ((hi < 0) || (lo < 0))
+ return -1;
+ *dst++ = (hi << 4) | lo;
+ }
+ return 0;
+}
+
+static void tag_from_fdinfo(int fd_prog, uint8_t *tag, uint32_t len)
+{
+ const int prefix_len = sizeof("prog_tag:\t") - 1;
+ char buff[256];
+ int ret = -1;
+ FILE *fp;
+
+ snprintf(buff, sizeof(buff), "/proc/%d/fdinfo/%d", getpid(),
+ fd_prog);
+ fp = fopen(buff, "r");
+ assert(fp);
+
+ while (fgets(buff, sizeof(buff), fp)) {
+ if (strncmp(buff, "prog_tag:\t", len))
+ continue;
+ ret = hex2bin(tag, buff + prefix_len, len);
+ break;
+ }
+
+ fclose(fp);
+ assert(!ret);
+}
+
+static void tag_from_alg(int insns, uint8_t *tag, uint32_t len)
+{
+ static const struct sockaddr_alg alg = {
+ .salg_family = AF_ALG,
+ .salg_type = "hash",
+ .salg_name = "sha1",
+ };
+ int fd_base, fd_alg, ret;
+ ssize_t size;
+
+ fd_base = socket(AF_ALG, SOCK_SEQPACKET, 0);
+ assert(fd_base > 0);
+
+ ret = bind(fd_base, (struct sockaddr *)&alg, sizeof(alg));
+ assert(!ret);
+
+ fd_alg = accept(fd_base, NULL, 0);
+ assert(fd_alg > 0);
+
+ insns *= sizeof(struct bpf_insn);
+ size = write(fd_alg, prog, insns);
+ assert(size == insns);
+
+ size = read(fd_alg, tag, len);
+ assert(size == len);
+
+ close(fd_alg);
+ close(fd_base);
+}
+
+static void tag_dump(const char *prefix, uint8_t *tag, uint32_t len)
+{
+ int i;
+
+ printf("%s", prefix);
+ for (i = 0; i < len; i++)
+ printf("%02x", tag[i]);
+ printf("\n");
+}
+
+static void tag_exit_report(int insns, int fd_map, uint8_t *ftag,
+ uint8_t *atag, uint32_t len)
+{
+ printf("Program tag mismatch for %d insns%s!\n", insns,
+ fd_map < 0 ? "" : " with map");
+
+ tag_dump(" fdinfo result: ", ftag, len);
+ tag_dump(" af_alg result: ", atag, len);
+ exit(1);
+}
+
+static void do_test(uint32_t *tests, int start_insns, int fd_map,
+ void (*bpf_filler)(unsigned int insns, int fd))
+{
+ int i, fd_prog;
+
+ for (i = start_insns; i <= BPF_MAXINSNS; i++) {
+ uint8_t ftag[8], atag[sizeof(ftag)];
+
+ fd_prog = bpf_try_load_prog(i, fd_map, bpf_filler);
+ tag_from_fdinfo(fd_prog, ftag, sizeof(ftag));
+ tag_from_alg(i, atag, sizeof(atag));
+ if (memcmp(ftag, atag, sizeof(ftag)))
+ tag_exit_report(i, fd_map, ftag, atag, sizeof(ftag));
+
+ close(fd_prog);
+ sched_yield();
+ (*tests)++;
+ }
+}
+
+int main(void)
+{
+ struct rlimit rinf = { RLIM_INFINITY, RLIM_INFINITY };
+ uint32_t tests = 0;
+ int i, fd_map;
+
+ setrlimit(RLIMIT_MEMLOCK, &rinf);
+ fd_map = bpf_map_create(BPF_MAP_TYPE_HASH, sizeof(int),
+ sizeof(int), 1, BPF_F_NO_PREALLOC);
+ assert(fd_map > 0);
+
+ for (i = 0; i < 5; i++) {
+ do_test(&tests, 2, -1, bpf_gen_imm_prog);
+ do_test(&tests, 3, fd_map, bpf_gen_map_prog);
+ }
+
+ printf("test_tag: OK (%u tests)\n", tests);
+ close(fd_map);
+ return 0;
+}
--
1.9.3
^ permalink raw reply related
* [PATCH net-next 0/5] Misc BPF improvements
From: Daniel Borkmann @ 2017-01-24 0:06 UTC (permalink / raw)
To: davem; +Cc: netdev, ast, Daniel Borkmann
This series adds various misc improvements to BPF, f.e. allowing
skb_load_bytes() helper to be used with filter/reuseport programs
to facilitate programming, test cases for program tag, etc. For
details, please see individual patches.
Thanks!
Daniel Borkmann (5):
bpf: simplify __is_valid_access test on cb
bpf: enable load bytes helper for filter/reuseport progs
bpf: allow option for setting bpf_l4_csum_replace from scratch
bpf: add prog tag test case to bpf selftests
bpf: enable verifier to better track const alu ops
include/uapi/linux/bpf.h | 1 +
kernel/bpf/verifier.c | 64 ++++++---
net/core/filter.c | 63 ++++-----
tools/testing/selftests/bpf/Makefile | 4 +-
tools/testing/selftests/bpf/test_tag.c | 202 ++++++++++++++++++++++++++++
tools/testing/selftests/bpf/test_verifier.c | 82 +++++++++++
6 files changed, 364 insertions(+), 52 deletions(-)
create mode 100644 tools/testing/selftests/bpf/test_tag.c
--
1.9.3
^ permalink raw reply
* [PATCH net-next 5/5] bpf: enable verifier to better track const alu ops
From: Daniel Borkmann @ 2017-01-24 0:06 UTC (permalink / raw)
To: davem; +Cc: netdev, ast, Daniel Borkmann, Gianluca Borello, William Tu
In-Reply-To: <cover.1485214851.git.daniel@iogearbox.net>
William reported couple of issues in relation to direct packet
access. Typical scheme is to check for data + [off] <= data_end,
where [off] can be either immediate or coming from a tracked
register that contains an immediate, depending on the branch, we
can then access the data. However, in case of calculating [off]
for either the mentioned test itself or for access after the test
in a more "complex" way, then the verifier will stop tracking the
CONST_IMM marked register and will mark it as UNKNOWN_VALUE one.
Adding that UNKNOWN_VALUE typed register to a pkt() marked
register, the verifier then bails out in check_packet_ptr_add()
as it finds the registers imm value below 48. In the first below
example, that is due to evaluate_reg_imm_alu() not handling right
shifts and thus marking the register as UNKNOWN_VALUE via helper
__mark_reg_unknown_value() that resets imm to 0.
In the second case the same happens at the time when r4 is set
to r4 &= r5, where it transitions to UNKNOWN_VALUE from
evaluate_reg_imm_alu(). Later on r4 we shift right by 3 inside
evaluate_reg_alu(), where the register's imm turns into 3. That
is, for registers with type UNKNOWN_VALUE, imm of 0 means that
we don't know what value the register has, and for imm > 0 it
means that the value has [imm] upper zero bits. F.e. when shifting
an UNKNOWN_VALUE register by 3 to the right, no matter what value
it had, we know that the 3 upper most bits must be zero now.
This is to make sure that ALU operations with unknown registers
don't overflow. Meaning, once we know that we have more than 48
upper zero bits, or, in other words cannot go beyond 0xffff offset
with ALU ops, such an addition will track the target register
as a new pkt() register with a new id, but 0 offset and 0 range,
so for that a new data/data_end test will be required. Is the source
register a CONST_IMM one that is to be added to the pkt() register,
or the source instruction is an add instruction with immediate
value, then it will get added if it stays within max 0xffff bounds.
>From there, pkt() type, can be accessed should reg->off + imm be
within the access range of pkt().
[...]
from 28 to 30: R0=imm1,min_value=1,max_value=1
R1=pkt(id=0,off=0,r=22) R2=pkt_end
R3=imm144,min_value=144,max_value=144
R4=imm0,min_value=0,max_value=0
R5=inv48,min_value=2054,max_value=2054 R10=fp
30: (bf) r5 = r3
31: (07) r5 += 23
32: (77) r5 >>= 3
33: (bf) r6 = r1
34: (0f) r6 += r5
cannot add integer value with 0 upper zero bits to ptr_to_packet
[...]
from 52 to 80: R0=imm1,min_value=1,max_value=1
R1=pkt(id=0,off=0,r=34) R2=pkt_end R3=inv
R4=imm272 R5=inv56,min_value=17,max_value=17
R6=pkt(id=0,off=26,r=34) R10=fp
80: (07) r4 += 71
81: (18) r5 = 0xfffffff8
83: (5f) r4 &= r5
84: (77) r4 >>= 3
85: (0f) r1 += r4
cannot add integer value with 3 upper zero bits to ptr_to_packet
Thus to get above use-cases working, evaluate_reg_imm_alu() has
been extended for further ALU ops. This is fine, because we only
operate strictly within realm of CONST_IMM types, so here we don't
care about overflows as they will happen in the simulated but also
real execution and interaction with pkt() in check_packet_ptr_add()
will check actual imm value once added to pkt(), but it's irrelevant
before.
With regards to 06c1c049721a ("bpf: allow helpers access to variable
memory") that works on UNKNOWN_VALUE registers, the verifier becomes
now a bit smarter as it can better resolve ALU ops, so we need to
adapt two test cases there, as min/max bound tracking only becomes
necessary when registers were spilled to stack. So while mask was
set before to track upper bound for UNKNOWN_VALUE case, it's now
resolved directly as CONST_IMM, and such contructs are only necessary
when f.e. registers are spilled.
For commit 6b17387307ba ("bpf: recognize 64bit immediate loads as
consts") that initially enabled dw load tracking only for nfp jit/
analyzer, I did couple of tests on large, complex programs and we
don't increase complexity badly (my tests were in ~3% range on avg).
I've added a couple of tests similar to affected code above, and
it works fine with verifier now.
Reported-by: William Tu <u9012063@gmail.com>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Cc: Gianluca Borello <g.borello@gmail.com>
Cc: William Tu <u9012063@gmail.com>
Acked-by: Alexei Starovoitov <ast@kernel.org>
---
kernel/bpf/verifier.c | 64 +++++++++++++++-------
tools/testing/selftests/bpf/test_verifier.c | 82 +++++++++++++++++++++++++++++
2 files changed, 127 insertions(+), 19 deletions(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 8f69df7..fb3513b 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -1566,22 +1566,54 @@ static int evaluate_reg_imm_alu(struct bpf_verifier_env *env,
struct bpf_reg_state *dst_reg = ®s[insn->dst_reg];
struct bpf_reg_state *src_reg = ®s[insn->src_reg];
u8 opcode = BPF_OP(insn->code);
+ u64 dst_imm = dst_reg->imm;
- /* dst_reg->type == CONST_IMM here, simulate execution of 'add'/'or'
- * insn. Don't care about overflow or negative values, just add them
+ /* dst_reg->type == CONST_IMM here. Simulate execution of insns
+ * containing ALU ops. Don't care about overflow or negative
+ * values, just add/sub/... them; registers are in u64.
*/
- if (opcode == BPF_ADD && BPF_SRC(insn->code) == BPF_K)
- dst_reg->imm += insn->imm;
- else if (opcode == BPF_ADD && BPF_SRC(insn->code) == BPF_X &&
- src_reg->type == CONST_IMM)
- dst_reg->imm += src_reg->imm;
- else if (opcode == BPF_OR && BPF_SRC(insn->code) == BPF_K)
- dst_reg->imm |= insn->imm;
- else if (opcode == BPF_OR && BPF_SRC(insn->code) == BPF_X &&
- src_reg->type == CONST_IMM)
- dst_reg->imm |= src_reg->imm;
- else
+ if (opcode == BPF_ADD && BPF_SRC(insn->code) == BPF_K) {
+ dst_imm += insn->imm;
+ } else if (opcode == BPF_ADD && BPF_SRC(insn->code) == BPF_X &&
+ src_reg->type == CONST_IMM) {
+ dst_imm += src_reg->imm;
+ } else if (opcode == BPF_SUB && BPF_SRC(insn->code) == BPF_K) {
+ dst_imm -= insn->imm;
+ } else if (opcode == BPF_SUB && BPF_SRC(insn->code) == BPF_X &&
+ src_reg->type == CONST_IMM) {
+ dst_imm -= src_reg->imm;
+ } else if (opcode == BPF_MUL && BPF_SRC(insn->code) == BPF_K) {
+ dst_imm *= insn->imm;
+ } else if (opcode == BPF_MUL && BPF_SRC(insn->code) == BPF_X &&
+ src_reg->type == CONST_IMM) {
+ dst_imm *= src_reg->imm;
+ } else if (opcode == BPF_OR && BPF_SRC(insn->code) == BPF_K) {
+ dst_imm |= insn->imm;
+ } else if (opcode == BPF_OR && BPF_SRC(insn->code) == BPF_X &&
+ src_reg->type == CONST_IMM) {
+ dst_imm |= src_reg->imm;
+ } else if (opcode == BPF_AND && BPF_SRC(insn->code) == BPF_K) {
+ dst_imm &= insn->imm;
+ } else if (opcode == BPF_AND && BPF_SRC(insn->code) == BPF_X &&
+ src_reg->type == CONST_IMM) {
+ dst_imm &= src_reg->imm;
+ } else if (opcode == BPF_RSH && BPF_SRC(insn->code) == BPF_K) {
+ dst_imm >>= insn->imm;
+ } else if (opcode == BPF_RSH && BPF_SRC(insn->code) == BPF_X &&
+ src_reg->type == CONST_IMM) {
+ dst_imm >>= src_reg->imm;
+ } else if (opcode == BPF_LSH && BPF_SRC(insn->code) == BPF_K) {
+ dst_imm <<= insn->imm;
+ } else if (opcode == BPF_LSH && BPF_SRC(insn->code) == BPF_X &&
+ src_reg->type == CONST_IMM) {
+ dst_imm <<= src_reg->imm;
+ } else {
mark_reg_unknown_value(regs, insn->dst_reg);
+ goto out;
+ }
+
+ dst_reg->imm = dst_imm;
+out:
return 0;
}
@@ -2225,14 +2257,8 @@ static int check_ld_imm(struct bpf_verifier_env *env, struct bpf_insn *insn)
return err;
if (insn->src_reg == 0) {
- /* generic move 64-bit immediate into a register,
- * only analyzer needs to collect the ld_imm value.
- */
u64 imm = ((u64)(insn + 1)->imm << 32) | (u32)insn->imm;
- if (!env->analyzer_ops)
- return 0;
-
regs[insn->dst_reg].type = CONST_IMM;
regs[insn->dst_reg].imm = imm;
return 0;
diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index 1aa7324..0d0912c 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -2326,6 +2326,84 @@ struct test_val {
.prog_type = BPF_PROG_TYPE_SCHED_CLS,
},
{
+ "direct packet access: test11 (shift, good access)",
+ .insns = {
+ BPF_LDX_MEM(BPF_W, BPF_REG_2, BPF_REG_1,
+ offsetof(struct __sk_buff, data)),
+ BPF_LDX_MEM(BPF_W, BPF_REG_3, BPF_REG_1,
+ offsetof(struct __sk_buff, data_end)),
+ BPF_MOV64_REG(BPF_REG_0, BPF_REG_2),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_0, 22),
+ BPF_JMP_REG(BPF_JGT, BPF_REG_0, BPF_REG_3, 8),
+ BPF_MOV64_IMM(BPF_REG_3, 144),
+ BPF_MOV64_REG(BPF_REG_5, BPF_REG_3),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_5, 23),
+ BPF_ALU64_IMM(BPF_RSH, BPF_REG_5, 3),
+ BPF_MOV64_REG(BPF_REG_6, BPF_REG_2),
+ BPF_ALU64_REG(BPF_ADD, BPF_REG_6, BPF_REG_5),
+ BPF_MOV64_IMM(BPF_REG_0, 1),
+ BPF_EXIT_INSN(),
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+ },
+ .result = ACCEPT,
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ },
+ {
+ "direct packet access: test12 (and, good access)",
+ .insns = {
+ BPF_LDX_MEM(BPF_W, BPF_REG_2, BPF_REG_1,
+ offsetof(struct __sk_buff, data)),
+ BPF_LDX_MEM(BPF_W, BPF_REG_3, BPF_REG_1,
+ offsetof(struct __sk_buff, data_end)),
+ BPF_MOV64_REG(BPF_REG_0, BPF_REG_2),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_0, 22),
+ BPF_JMP_REG(BPF_JGT, BPF_REG_0, BPF_REG_3, 8),
+ BPF_MOV64_IMM(BPF_REG_3, 144),
+ BPF_MOV64_REG(BPF_REG_5, BPF_REG_3),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_5, 23),
+ BPF_ALU64_IMM(BPF_AND, BPF_REG_5, 15),
+ BPF_MOV64_REG(BPF_REG_6, BPF_REG_2),
+ BPF_ALU64_REG(BPF_ADD, BPF_REG_6, BPF_REG_5),
+ BPF_MOV64_IMM(BPF_REG_0, 1),
+ BPF_EXIT_INSN(),
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+ },
+ .result = ACCEPT,
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ },
+ {
+ "direct packet access: test13 (branches, good access)",
+ .insns = {
+ BPF_LDX_MEM(BPF_W, BPF_REG_2, BPF_REG_1,
+ offsetof(struct __sk_buff, data)),
+ BPF_LDX_MEM(BPF_W, BPF_REG_3, BPF_REG_1,
+ offsetof(struct __sk_buff, data_end)),
+ BPF_MOV64_REG(BPF_REG_0, BPF_REG_2),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_0, 22),
+ BPF_JMP_REG(BPF_JGT, BPF_REG_0, BPF_REG_3, 13),
+ BPF_LDX_MEM(BPF_W, BPF_REG_3, BPF_REG_1,
+ offsetof(struct __sk_buff, mark)),
+ BPF_MOV64_IMM(BPF_REG_4, 1),
+ BPF_JMP_REG(BPF_JGT, BPF_REG_3, BPF_REG_4, 2),
+ BPF_MOV64_IMM(BPF_REG_3, 14),
+ BPF_JMP_IMM(BPF_JA, 0, 0, 1),
+ BPF_MOV64_IMM(BPF_REG_3, 24),
+ BPF_MOV64_REG(BPF_REG_5, BPF_REG_3),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_5, 23),
+ BPF_ALU64_IMM(BPF_AND, BPF_REG_5, 15),
+ BPF_MOV64_REG(BPF_REG_6, BPF_REG_2),
+ BPF_ALU64_REG(BPF_ADD, BPF_REG_6, BPF_REG_5),
+ BPF_MOV64_IMM(BPF_REG_0, 1),
+ BPF_EXIT_INSN(),
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+ },
+ .result = ACCEPT,
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ },
+ {
"helper access to packet: test1, valid packet_ptr range",
.insns = {
BPF_LDX_MEM(BPF_W, BPF_REG_2, BPF_REG_1,
@@ -4208,6 +4286,8 @@ struct test_val {
.insns = {
BPF_MOV64_IMM(BPF_REG_1, 0),
BPF_MOV64_IMM(BPF_REG_2, 0),
+ BPF_STX_MEM(BPF_DW, BPF_REG_10, BPF_REG_2, -128),
+ BPF_LDX_MEM(BPF_DW, BPF_REG_2, BPF_REG_10, -128),
BPF_ALU64_IMM(BPF_AND, BPF_REG_2, 64),
BPF_MOV64_IMM(BPF_REG_3, 0),
BPF_MOV64_IMM(BPF_REG_4, 0),
@@ -4251,6 +4331,8 @@ struct test_val {
BPF_STX_MEM(BPF_DW, BPF_REG_10, BPF_REG_0, -16),
BPF_STX_MEM(BPF_DW, BPF_REG_10, BPF_REG_0, -8),
BPF_MOV64_IMM(BPF_REG_2, 0),
+ BPF_STX_MEM(BPF_DW, BPF_REG_10, BPF_REG_2, -128),
+ BPF_LDX_MEM(BPF_DW, BPF_REG_2, BPF_REG_10, -128),
BPF_ALU64_IMM(BPF_AND, BPF_REG_2, 63),
BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, 1),
BPF_MOV64_IMM(BPF_REG_3, 0),
--
1.9.3
^ permalink raw reply related
* [PATCH net-next 1/5] bpf: simplify __is_valid_access test on cb
From: Daniel Borkmann @ 2017-01-24 0:06 UTC (permalink / raw)
To: davem; +Cc: netdev, ast, Daniel Borkmann
In-Reply-To: <cover.1485214851.git.daniel@iogearbox.net>
The __is_valid_access() test for cb[] from 62c7989b24db ("bpf: allow
b/h/w/dw access for bpf's cb in ctx") was done unnecessarily complex,
we can just simplify it the same way as recent fix from 2d071c643f1c
("bpf, trace: make ctx access checks more robust") did. Overflow can
never happen as size is 1/2/4/8 depending on access.
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Acked-by: Alexei Starovoitov <ast@kernel.org>
---
net/core/filter.c | 15 ++-------------
1 file changed, 2 insertions(+), 13 deletions(-)
diff --git a/net/core/filter.c b/net/core/filter.c
index 9038386..883975f 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -2784,19 +2784,8 @@ static bool __is_valid_access(int off, int size)
switch (off) {
case offsetof(struct __sk_buff, cb[0]) ...
offsetof(struct __sk_buff, cb[4]) + sizeof(__u32) - 1:
- if (size == sizeof(__u16) &&
- off > offsetof(struct __sk_buff, cb[4]) + sizeof(__u16))
- return false;
- if (size == sizeof(__u32) &&
- off > offsetof(struct __sk_buff, cb[4]))
- return false;
- if (size == sizeof(__u64) &&
- off > offsetof(struct __sk_buff, cb[2]))
- return false;
- if (size != sizeof(__u8) &&
- size != sizeof(__u16) &&
- size != sizeof(__u32) &&
- size != sizeof(__u64))
+ if (off + size >
+ offsetof(struct __sk_buff, cb[4]) + sizeof(__u32))
return false;
break;
default:
--
1.9.3
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox