* Re: [PATCH net-next 3/3] net/tcp-fastopen: Add new API support
From: Willy Tarreau @ 2017-01-23 21:37 UTC (permalink / raw)
To: Wei Wang
Cc: Wei Wang, Linux Kernel Network Developers, David Miller,
Eric Dumazet, Yuchung Cheng
In-Reply-To: <CAEA6p_DxVMAry1PCz_idmk=TGpnnTib3WpWso03FB1oMVXN+sg@mail.gmail.com>
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.
If that's possible at little cost it would be nice, because your patch
makes it so easy to enable TFO on outgoing connections now that I
expect many people will blindly run the setsockopt() before connect().
Do not hesitate to ask me to run some tests. While 4 years ago it was
not easy, here it's very simple for me. By the way I'm seeing an ~10%
performance increase on haproxy by enabling this, it's really cool!
Thanks,
Willy
^ permalink raw reply
* XDP offload to hypervisor
From: Michael S. Tsirkin @ 2017-01-23 21:40 UTC (permalink / raw)
To: John Fastabend
Cc: jasowang, john.r.fastabend, netdev, alexei.starovoitov, daniel
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.
If the result is XDP_DROP or XDP_TX we don't need to wake up the guest at all!
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?
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?
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?
Thanks!
--
MST
^ permalink raw reply
* Re: Question about veth_xmit()
From: Xiangning Yu @ 2017-01-23 21:46 UTC (permalink / raw)
To: Cong Wang; +Cc: Linux Kernel Network Developers
In-Reply-To: <CAM_iQpVXOsPZrAhBN10JYrVjOOK2_q7=gsFbsi_=s5XZLj1S+g@mail.gmail.com>
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.
Thanks,
- Xiangning
^ permalink raw reply
* Re: [PATCH v2] bpf: Restrict cgroup bpf hooks to the init netns
From: Andy Lutomirski @ 2017-01-23 21:49 UTC (permalink / raw)
To: David Ahern
Cc: Andy Lutomirski, Network Development, David S. Miller,
Daniel Borkmann, Alexei Starovoitov
In-Reply-To: <7bfbda89-1641-7089-9925-d6f751a863e1@cumulusnetworks.com>
On Mon, Jan 23, 2017 at 1:03 PM, David Ahern <dsa@cumulusnetworks.com> wrote:
> On 1/23/17 1:36 PM, Andy Lutomirski wrote:
>> To see how cgroup+bpf interacts with network namespaces, I wrote a
>> little program called show_bind that calls getsockopt(...,
>> SO_BINDTODEVICE, ...) and prints the result. It did this:
>>
>> # ./ip link add dev vrf0 type vrf table 10
>> # ./ip vrf exec vrf0 ./show_bind
>> Default binding is "vrf0"
>> # ./ip vrf exec vrf0 unshare -n ./show_bind
>> show_bind: getsockopt: No such device
>>
>> What's happening here is that "ip vrf" looks up vrf0's ifindex in
>> the init netns and installs a hook that binds sockets to that
>
> It looks up the device name in the current namespace.
>
>> ifindex. When the hook runs in a different netns, it sets
>> sk_bound_dev_if to an ifindex from the wrong netns, resulting in
>> incorrect behavior. In this particular example, the ifindex was 4
>> and there was no ifindex 4 in the new netns. If there had been,
>> this test would have malfunctioned differently
>
> While the cgroups and network namespace interaction needs improvement, a management tool can workaround the deficiencies:
>
> A shell in the default namespace, mgmt vrf (PS1 tells me the network context):
> dsa@kenny:mgmt:~$
>
> Switch to a different namespace (one that I run VMs for network testing):
> dsa@kenny:mgmt:~$ sudo ip netns exec vms su - dsa
>
> And then bind the shell to vrf2
> dsa@kenny:vms:~$ sudo ip vrf exec vrf2 su - dsa
> dsa@kenny:vms:vrf2:~$
>
> Or I can go straight to vrf2:
> dsa@kenny:mgmt:~$ sudo ip netns exec vms ip vrf exec vrf2 su - dsa
> dsa@kenny:vms:vrf2:~$
Indeed, if you're careful to set up the vrf cgroup in the same netns
that you end up using it in, it'll work. But there's a bigger footgun
there than I think is warranted, and I'm not sure how iproute2 is
supposed to do all that much better given that the eBPF program can
neither see what namespace a socket is bound to nor can it act in a
way that works correctly in any namespace.
Long-term, I think the real fix is to make the hook work on a
per-netns basis and, if needed, add an interface for a cross-netns
hook to work sensibly. But I think it's a bit late to do that for
4.10, so instead I'm proposing to limit the API to the case where it
works and the semantics are unambiguous and to leave further
improvements for later.
It's a bit unfortunate that there seems to be an impedance mismatch in
that "ip vrf" acts on cgroups and that cgroups are somewhat orthogonal
to network namespaces.
>
>
> I am testing additional iproute2 cleanups which will be sent before 4.10 is released.
>
> -----8<-----
>
>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>> index e89acea22ecf..c0bbc55e244d 100644
>> --- a/kernel/bpf/syscall.c
>> +++ b/kernel/bpf/syscall.c
>> @@ -902,6 +902,17 @@ static int bpf_prog_attach(const union bpf_attr *attr)
>> struct cgroup *cgrp;
>> enum bpf_prog_type ptype;
>>
>> + /*
>> + * For now, socket bpf hooks attached to cgroups can only be
>> + * installed in the init netns and only affect the init netns.
>> + * This could be relaxed in the future once some semantic issues
>> + * are resolved. For example, ifindexes belonging to one netns
>> + * should probably not be visible to hooks installed by programs
>> + * running in a different netns.
>> + */
>> + if (current->nsproxy->net_ns != &init_net)
>> + return -EINVAL;
>> +
>> if (!capable(CAP_NET_ADMIN))
>> return -EPERM;
>>
>
> But should this patch be taken, shouldn't the EPERM out rank the namespace check.
>
I could see that going either way. If the hook becomes per-netns,
then the capable() check could potentially become ns_capable() and it
would start succeeding. I'd be happy to change it, though.
--Andy
--
Andy Lutomirski
AMA Capital Management, LLC
^ permalink raw reply
* Re: [PULL] vhost: cleanups and fixes
From: Linus Torvalds @ 2017-01-23 21:50 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: KVM list, virtualization, Network Development,
Linux Kernel Mailing List, bhumirks, Christian Borntraeger,
Colin Ian King, Cornelia Huck, Dan Carpenter, gcampana,
Jason Wang, pasic, pmorel, silbe
In-Reply-To: <20170123170530-mutt-send-email-mst@kernel.org>
On Mon, Jan 23, 2017 at 7:05 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> virtio, vhost: fixes, cleanups
Was there a reason why you sent this twice?
Or was this *supposed* to be the ARM DMA fix pull request? Because it wasn't.
Linus
^ permalink raw reply
* 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
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