* 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: [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: [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: 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
* 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: [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
* Re: [PATCH net-next 3/3] net/tcp-fastopen: Add new API support
From: Willy Tarreau @ 2017-01-23 21:16 UTC (permalink / raw)
To: Wei Wang; +Cc: netdev, David Miller, Eric Dumazet, Yuchung Cheng, Wei Wang
In-Reply-To: <20170123185922.48046-4-tracywwnj@gmail.com>
Hi Wei,
first, thanks a lot for doing this, it's really awesome!
I'm testing it on 4.9 on haproxy and I met a corner case : when I
perform a connect() to a server and I have nothing to send, upon
POLLOUT notification since I have nothing to send I simply probe the
connection using connect() again to see if it returns EISCONN or
anything else. But here now I'm seeing EINPROGRESS loops.
To illustrate this, here's what I'm doing :
:8000 :8001
[ client ] ---> [ proxy ] ---> [ server ]
The proxy is configured to enable TFO to the server and the server
supports TFO as well. The proxy and the server are in fact two proxy
instances in haproxy running in the same process for convenience.
When I already have data to send here's what I'm seeing (so it works fine) :
06:29:16.861190 accept4(7, {sa_family=AF_INET, sin_port=htons(33986), sin_addr=inet_addr("192.168.0.176")}, [128->16], SOCK
_NONBLOCK) = 9
06:29:16.861277 setsockopt(9, SOL_TCP, TCP_NODELAY, [1], 4) = 0
06:29:16.861342 accept4(7, 0x7ffd0d794430, [128], SOCK_NONBLOCK) = -1 EAGAIN (Resource temporarily unavailable)
06:29:16.861417 recvfrom(9, "BLAH\n", 7006, 0, NULL, NULL) = 5
06:29:16.861509 recvfrom(9, 0x2619329, 7001, 0, NULL, NULL) = -1 EAGAIN (Resource temporarily unavailable)
06:29:16.861657 socket(AF_INET, SOCK_STREAM, IPPROTO_TCP) = 10
06:29:16.861730 fcntl(10, F_SETFL, O_RDONLY|O_NONBLOCK) = 0
06:29:16.861779 setsockopt(10, SOL_TCP, TCP_NODELAY, [1], 4) = 0
06:29:16.861882 setsockopt(10, SOL_TCP, 0x1e /* TCP_??? */, [1], 4) = 0
06:29:16.861942 connect(10, {sa_family=AF_INET, sin_port=htons(8001), sin_addr=inet_addr("127.0.0.1")}, 16) = 0
06:29:16.862015 epoll_ctl(3, EPOLL_CTL_ADD, 9, {EPOLLIN|EPOLLRDHUP, {u32=9, u64=9}}) = 0
06:29:16.862072 epoll_wait(3, [], 200, 0) = 0
06:29:16.862126 sendto(10, "BLAH\n", 5, MSG_DONTWAIT|MSG_NOSIGNAL, NULL, 0) = 5
06:29:16.862281 epoll_wait(3, [{EPOLLIN, {u32=8, u64=8}}], 200, 0) = 1
06:29:16.862334 recvfrom(10, 0x26173a4, 8030, 0, NULL, NULL) = -1 EAGAIN (Resource temporarily unavailable)
06:29:16.862385 accept4(8, {sa_family=AF_INET, sin_port=htons(46760), sin_addr=inet_addr("127.0.0.1")}, [128->16], SOCK_NON
BLOCK) = 11
06:29:16.862450 setsockopt(11, SOL_TCP, TCP_NODELAY, [1], 4) = 0
06:29:16.862504 accept4(8, 0x7ffd0d794430, [128], SOCK_NONBLOCK) = -1 EAGAIN (Resource temporarily unavailable)
06:29:16.862564 recvfrom(11, "BLAH\n", 7006, 0, NULL, NULL) = 5
When I don't have data, here's what I'm seeing :
06:29:24.047801 accept4(7, {sa_family=AF_INET, sin_port=htons(33988), sin_addr=inet_addr("192.168.0.176")}, [128->16], SOCK
_NONBLOCK) = 9
06:29:24.047899 setsockopt(9, SOL_TCP, TCP_NODELAY, [1], 4) = 0
06:29:24.047966 accept4(7, 0x7ffdedb2c7f0, [128], SOCK_NONBLOCK) = -1 EAGAIN (Resource temporarily unavailable)
06:29:24.048043 recvfrom(9, 0xd31324, 7006, 0, NULL, NULL) = -1 EAGAIN (Resource temporarily unavailable)
06:29:24.048281 socket(AF_INET, SOCK_STREAM, IPPROTO_TCP) = 10
06:29:24.048342 fcntl(10, F_SETFL, O_RDONLY|O_NONBLOCK) = 0
06:29:24.048392 setsockopt(10, SOL_TCP, TCP_NODELAY, [1], 4) = 0
06:29:24.048447 setsockopt(10, SOL_TCP, 0x1e /* TCP_??? */, [1], 4) = 0
06:29:24.048508 connect(10, {sa_family=AF_INET, sin_port=htons(8001), sin_addr=inet_addr("127.0.0.1")}, 16) = 0
06:29:24.048593 epoll_ctl(3, EPOLL_CTL_ADD, 9, {EPOLLIN|EPOLLRDHUP, {u32=9, u64=9}}) = 0
06:29:24.048651 epoll_wait(3, [], 200, 0) = 0
06:29:24.048699 getsockopt(10, SOL_SOCKET, SO_ERROR, [0], [4]) = 0
06:29:24.048751 connect(10, {sa_family=AF_INET, sin_port=htons(8001), sin_addr=inet_addr("127.0.0.1")}, 16) = -1 EINPROGRES
S (Operation now in progress)
06:29:24.048808 epoll_ctl(3, EPOLL_CTL_ADD, 10, {EPOLLOUT, {u32=10, u64=10}}) = 0
06:29:24.048860 epoll_wait(3, [{EPOLLOUT, {u32=10, u64=10}}], 200, 1000) = 1
06:29:24.048912 getsockopt(10, SOL_SOCKET, SO_ERROR, [0], [4]) = 0
06:29:24.048963 connect(10, {sa_family=AF_INET, sin_port=htons(8001), sin_addr=inet_addr("127.0.0.1")}, 16) = -1 EINPROGRES
S (Operation now in progress)
06:29:24.049018 epoll_wait(3, [{EPOLLOUT, {u32=10, u64=10}}], 200, 1000) = 1
06:29:24.049072 getsockopt(10, SOL_SOCKET, SO_ERROR, [0], [4]) = 0
06:29:24.049122 connect(10, {sa_family=AF_INET, sin_port=htons(8001), sin_addr=inet_addr("127.0.0.1")}, 16) = -1 EINPROGRES
S (Operation now in progress)
I theorically understand why but I think we have something wrong here
and instead we should have -1 EISCONN (to pretend the connection is
established) or return EALREADY (to mention that a previous request was
already made and that we're waiting for the next step).
While I can instrument my connect() *not* to use TFO when connecting
without any pending data, I don't always know this (eg when I use
openssl and cross fingers so that it decides to quickly send something
on the next round).
I think it's easy to fall into this tricky corner case and am wondering
what can be done about it. Does the EINPROGRESS happen only because there
is no cookie yet ? If so, shouldn't the connect's status change in this
case ?
Thanks,
Willy
^ permalink raw reply
* Re: [PATCH v4 0/3] bpf: add longest prefix match map
From: David Miller @ 2017-01-23 21:11 UTC (permalink / raw)
To: daniel; +Cc: ast, dh.herrmann, daniel, netdev
In-Reply-To: <20170121162613.4159-1-daniel@zonque.org>
From: Daniel Mack <daniel@zonque.org>
Date: Sat, 21 Jan 2017 17:26:10 +0100
> This patch set adds a longest prefix match algorithm that can be used
> to match IP addresses to a stored set of ranges. It is exposed as a
> bpf map type.
>
> Internally, data is stored in an unbalanced tree of nodes that has a
> maximum height of n, where n is the prefixlen the trie was created
> with.
>
> Note that this has nothing to do with fib or fib6 and is in no way meant
> to replace or share code with it. It's rather a much simpler
> implementation that is specifically written with bpf maps in mind.
>
> Patch 1/2 adds the implementation, 2/2 an extensive test suite and 3/3
> has benchmarking code for the new trie type.
>
> Feedback is much appreciated.
Series applied, thank you.
^ permalink raw reply
* Re: [net PATCH v5 1/6] virtio_net: use dev_kfree_skb for small buffer XDP receive
From: Michael S. Tsirkin @ 2017-01-23 21:08 UTC (permalink / raw)
To: John Fastabend
Cc: jasowang, john.r.fastabend, netdev, alexei.starovoitov, daniel
In-Reply-To: <20170117221950.20280.39496.stgit@john-Precision-Tower-5810>
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.
> ---
> drivers/net/virtio_net.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 4a10500..d97bb71 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1890,8 +1890,12 @@ static void free_receive_page_frags(struct virtnet_info *vi)
> put_page(vi->rq[i].alloc_frag.page);
> }
>
> -static bool is_xdp_queue(struct virtnet_info *vi, int q)
> +static bool is_xdp_raw_buffer_queue(struct virtnet_info *vi, int q)
> {
> + /* For small receive mode always use kfree_skb variants */
> + if (!vi->mergeable_rx_bufs)
> + return false;
> +
> if (q < (vi->curr_queue_pairs - vi->xdp_queue_pairs))
> return false;
> else if (q < vi->curr_queue_pairs)
> @@ -1908,7 +1912,7 @@ static void free_unused_bufs(struct virtnet_info *vi)
> for (i = 0; i < vi->max_queue_pairs; i++) {
> struct virtqueue *vq = vi->sq[i].vq;
> while ((buf = virtqueue_detach_unused_buf(vq)) != NULL) {
> - if (!is_xdp_queue(vi, i))
> + if (!is_xdp_raw_buffer_queue(vi, i))
> dev_kfree_skb(buf);
> else
> put_page(virt_to_head_page(buf));
^ permalink raw reply
* Re: [PATCH net-next 1/2] vxlan: don't flush static fdb entries on admin down
From: David Miller @ 2017-01-23 21:07 UTC (permalink / raw)
To: roopa; +Cc: netdev, ramanb, stephen, jbenc, pshelar
In-Reply-To: <1484984599-16712-2-git-send-email-roopa@cumulusnetworks.com>
From: Roopa Prabhu <roopa@cumulusnetworks.com>
Date: Fri, 20 Jan 2017 23:43:18 -0800
> /* Purge the forwarding table */
> -static void vxlan_flush(struct vxlan_dev *vxlan)
> +static void vxlan_flush(struct vxlan_dev *vxlan, int do_all)
Please use 'bool' and true/false for this new argument.
FWIW, I am fine with changing the default behavior in this way.
^ permalink raw reply
* Re: [PATCH v2] bpf: Restrict cgroup bpf hooks to the init netns
From: David Ahern @ 2017-01-23 21:03 UTC (permalink / raw)
To: Andy Lutomirski, Network Development, David S. Miller
Cc: Daniel Borkmann, Alexei Starovoitov
In-Reply-To: <ddd9e097f54e894197a2925731de02d1d3cae137.1485203708.git.luto@kernel.org>
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:~$
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.
^ permalink raw reply
* Re: [PATCH v2] net: xilinx: constify net_device_ops structure
From: David Miller @ 2017-01-23 21:00 UTC (permalink / raw)
To: bhumirks
Cc: julia.lawall, michal.simek, soren.brinkmann, netdev,
linux-arm-kernel, linux-kernel
In-Reply-To: <1484981938-29829-1-git-send-email-bhumirks@gmail.com>
From: Bhumika Goyal <bhumirks@gmail.com>
Date: Sat, 21 Jan 2017 12:28:58 +0530
> Declare net_device_ops structure as const as it is only stored in
> the netdev_ops field of a net_device structure. This field is of type
> const, so net_device_ops structures having same properties can be made
> const too.
> Done using Coccinelle:
...
> Signed-off-by: Bhumika Goyal <bhumirks@gmail.com>
Applied.
^ permalink raw reply
* Re: [PATCH v2] net: moxa: constify net_device_ops structures
From: David Miller @ 2017-01-23 21:00 UTC (permalink / raw)
To: bhumirks; +Cc: julia.lawall, dan.carpenter, netdev, linux-kernel
In-Reply-To: <1484981846-29736-1-git-send-email-bhumirks@gmail.com>
From: Bhumika Goyal <bhumirks@gmail.com>
Date: Sat, 21 Jan 2017 12:27:26 +0530
> Declare net_device_ops structure as const as it is only stored in
> the netdev_ops field of a net_device structure. This field is of type
> const, so net_device_ops structures having same properties can be made
> const too.
> Done using Coccinelle:
...
> Signed-off-by: Bhumika Goyal <bhumirks@gmail.com>
Applied.
^ permalink raw reply
* Re: [RFC PATCH net-next 2/5] net: split skb_checksum_help
From: Tom Herbert @ 2017-01-23 20:59 UTC (permalink / raw)
To: Davide Caratti
Cc: David S. Miller, Linux Kernel Network Developers, linux-sctp,
Marcelo Ricardo Leitner
In-Reply-To: <532e1db70b6c40f1b1e5c60b5b51be9f3437a30b.1485177252.git.dcaratti@redhat.com>
On Mon, Jan 23, 2017 at 8:52 AM, Davide Caratti <dcaratti@redhat.com> wrote:
> skb_checksum_help is designed to compute the Internet Checksum only. To
> avoid duplicating code when other checksumming algorithms (e.g. crc32c)
> are used, separate common part from RFC1624-specific part.
>
> Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> Signed-off-by: Davide Caratti <dcaratti@redhat.com>
> ---
> net/core/dev.c | 51 +++++++++++++++++++++++++++++++++++----------------
> 1 file changed, 35 insertions(+), 16 deletions(-)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index ad5959e..6742160 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -2532,13 +2532,36 @@ static void skb_warn_bad_offload(const struct sk_buff *skb)
> skb_shinfo(skb)->gso_type, skb->ip_summed);
> }
>
> -/*
> - * Invalidate hardware checksum when packet is to be mangled, and
> +/* compute 16-bit RFC1624 checksum and store it at skb->data + offset */
> +static int skb_rfc1624_csum(struct sk_buff *skb, int offset)
> +{
> + __wsum csum;
> + int ret = 0;
> +
> + csum = skb_checksum(skb, offset, skb->len - offset, 0);
> +
> + offset += skb->csum_offset;
> + BUG_ON(offset + sizeof(__sum16) > skb_headlen(skb));
> +
> + if (skb_cloned(skb) &&
> + !skb_clone_writable(skb, offset + sizeof(__sum16))) {
> + ret = pskb_expand_head(skb, 0, 0, GFP_ATOMIC);
> + if (ret)
> + goto out;
> + }
> + *(__sum16 *)(skb->data + offset) = csum_fold(csum) ?: CSUM_MANGLED_0;
> +out:
> + return ret;
> +}
> +
> +/* Invalidate hardware checksum when packet is to be mangled, and
> * complete checksum manually on outgoing path.
> + * @skb - buffer that needs checksum
> + * @csum_algo(skb, offset) - function used to compute the checksum
> */
> -int skb_checksum_help(struct sk_buff *skb)
> +static int __skb_checksum_help(struct sk_buff *skb,
> + int (*csum_algo)(struct sk_buff *, int))
> {
> - __wsum csum;
> int ret = 0, offset;
>
> if (skb->ip_summed == CHECKSUM_COMPLETE)
skb_checksum_help is specific to the Internet checksum. For instance,
CHECKSUM_COMPLETE can _only_ refer to Internet checksum calculation
nothing else will work. Checksums and CRCs are very different things
with very different processing. They are not interchangeable, have
very different properties, and hence it is a mistake to try to shoe
horn things so that they use a common infrastructure.
It might make sense to create some CRC helper functions, but last time
I checked there are so few users of CRC in skbufs I'm not even sure
that would make sense.
Tom
> @@ -2560,24 +2583,20 @@ int skb_checksum_help(struct sk_buff *skb)
>
> offset = skb_checksum_start_offset(skb);
> BUG_ON(offset >= skb_headlen(skb));
> - csum = skb_checksum(skb, offset, skb->len - offset, 0);
> -
> - offset += skb->csum_offset;
> - BUG_ON(offset + sizeof(__sum16) > skb_headlen(skb));
> -
> - if (skb_cloned(skb) &&
> - !skb_clone_writable(skb, offset + sizeof(__sum16))) {
> - ret = pskb_expand_head(skb, 0, 0, GFP_ATOMIC);
> - if (ret)
> - goto out;
> - }
>
> - *(__sum16 *)(skb->data + offset) = csum_fold(csum) ?: CSUM_MANGLED_0;
> + ret = csum_algo(skb, offset);
> + if (ret)
> + goto out;
> out_set_summed:
> skb->ip_summed = CHECKSUM_NONE;
> out:
> return ret;
> }
> +
> +int skb_checksum_help(struct sk_buff *skb)
> +{
> + return __skb_checksum_help(skb, skb_rfc1624_csum);
> +}
> EXPORT_SYMBOL(skb_checksum_help);
>
> __be16 skb_network_protocol(struct sk_buff *skb, int *depth)
> --
> 2.7.4
>
^ permalink raw reply
* Re: [PATCH net-next 3/3] net/tcp-fastopen: Add new API support
From: Yuchung Cheng @ 2017-01-23 20:55 UTC (permalink / raw)
To: Wei Wang; +Cc: netdev, David Miller, Eric Dumazet, Wei Wang
In-Reply-To: <20170123185922.48046-4-tracywwnj@gmail.com>
On Mon, Jan 23, 2017 at 10:59 AM, Wei Wang <tracywwnj@gmail.com> wrote:
> From: Wei Wang <weiwan@google.com>
>
> This patch adds a new socket option, TCP_FASTOPEN_CONNECT, as an
> alternative way to perform Fast Open on the active side (client). Prior
> to this patch, a client needs to replace the connect() call with
> sendto(MSG_FASTOPEN). This can be cumbersome for applications who want
> to use Fast Open: these socket operations are often done in lower layer
> libraries used by many other applications. Changing these libraries
> and/or the socket call sequences are not trivial. A more convenient
> approach is to perform Fast Open by simply enabling a socket option when
> the socket is created w/o changing other socket calls sequence:
> s = socket()
> create a new socket
> setsockopt(s, IPPROTO_TCP, TCP_FASTOPEN_CONNECT …);
> newly introduced sockopt
> If set, new functionality described below will be used.
> Return ENOTSUPP if TFO is not supported or not enabled in the
> kernel.
>
> connect()
> With cookie present, return 0 immediately.
> With no cookie, initiate 3WHS with TFO cookie-request option and
> return -1 with errno = EINPROGRESS.
>
> write()/sendmsg()
> With cookie present, send out SYN with data and return the number of
> bytes buffered.
> With no cookie, and 3WHS not yet completed, return -1 with errno =
> EINPROGRESS.
> No MSG_FASTOPEN flag is needed.
>
> read()
> Return -1 with errno = EWOULDBLOCK/EAGAIN if connect() is called but
> write() is not called yet.
> Return -1 with errno = EWOULDBLOCK/EAGAIN if connection is
> established but no msg is received yet.
> Return number of bytes read if socket is established and there is
> msg received.
>
> The new API simplifies life for applications that always perform a write()
> immediately after a successful connect(). Such applications can now take
> advantage of Fast Open by merely making one new setsockopt() call at the time
> of creating the socket. Nothing else about the application's socket call
> sequence needs to change.
>
> Signed-off-by: Wei Wang <weiwan@google.com>
> ---
Acked-by: Yuchung Cheng <ycheng@google.com>
Thanks for making this happen.
> include/linux/tcp.h | 3 ++-
> include/net/inet_sock.h | 6 +++++-
> include/net/tcp.h | 1 +
> include/uapi/linux/tcp.h | 1 +
> net/ipv4/af_inet.c | 31 ++++++++++++++++++++++++-------
> net/ipv4/tcp.c | 35 ++++++++++++++++++++++++++++++++++-
> net/ipv4/tcp_fastopen.c | 33 +++++++++++++++++++++++++++++++++
> net/ipv4/tcp_ipv4.c | 7 ++++++-
> net/ipv6/tcp_ipv6.c | 5 +++++
> 9 files changed, 111 insertions(+), 11 deletions(-)
>
> diff --git a/include/linux/tcp.h b/include/linux/tcp.h
> index 5371b3d70cfe..f88f4649ba6f 100644
> --- a/include/linux/tcp.h
> +++ b/include/linux/tcp.h
> @@ -222,7 +222,8 @@ struct tcp_sock {
> u32 chrono_stat[3]; /* Time in jiffies for chrono_stat stats */
> u8 chrono_type:2, /* current chronograph type */
> rate_app_limited:1, /* rate_{delivered,interval_us} limited? */
> - unused:5;
> + fastopen_connect:1, /* FASTOPEN_CONNECT sockopt */
> + unused:4;
> u8 nonagle : 4,/* Disable Nagle algorithm? */
> thin_lto : 1,/* Use linear timeouts for thin streams */
> unused1 : 1,
> diff --git a/include/net/inet_sock.h b/include/net/inet_sock.h
> index c9cff977a7fb..aa95053dfc78 100644
> --- a/include/net/inet_sock.h
> +++ b/include/net/inet_sock.h
> @@ -206,7 +206,11 @@ struct inet_sock {
> transparent:1,
> mc_all:1,
> nodefrag:1;
> - __u8 bind_address_no_port:1;
> + __u8 bind_address_no_port:1,
> + defer_connect:1; /* Indicates that fastopen_connect is set
> + * and cookie exists so we defer connect
> + * until first data frame is written
> + */
> __u8 rcv_tos;
> __u8 convert_csum;
> int uc_index;
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index de67541d7adf..6ec4ea652f3f 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -1495,6 +1495,7 @@ struct sock *tcp_try_fastopen(struct sock *sk, struct sk_buff *skb,
> void tcp_fastopen_init_key_once(bool publish);
> bool tcp_fastopen_cookie_check(struct sock *sk, u16 *mss,
> struct tcp_fastopen_cookie *cookie);
> +bool tcp_fastopen_defer_connect(struct sock *sk, int *err);
> #define TCP_FASTOPEN_KEY_LENGTH 16
>
> /* Fastopen key context */
> diff --git a/include/uapi/linux/tcp.h b/include/uapi/linux/tcp.h
> index c53de2691cec..6ff35eb48d10 100644
> --- a/include/uapi/linux/tcp.h
> +++ b/include/uapi/linux/tcp.h
> @@ -116,6 +116,7 @@ enum {
> #define TCP_SAVE_SYN 27 /* Record SYN headers for new connections */
> #define TCP_SAVED_SYN 28 /* Get SYN headers recorded for connection */
> #define TCP_REPAIR_WINDOW 29 /* Get/set window parameters */
> +#define TCP_FASTOPEN_CONNECT 30 /* Attempt FastOpen with connect */
>
> struct tcp_repair_opt {
> __u32 opt_code;
> diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
> index aae410bb655a..d8a0dc076f97 100644
> --- a/net/ipv4/af_inet.c
> +++ b/net/ipv4/af_inet.c
> @@ -576,13 +576,24 @@ int __inet_stream_connect(struct socket *sock, struct sockaddr *uaddr,
> int err;
> long timeo;
>
> - if (addr_len < sizeof(uaddr->sa_family))
> - return -EINVAL;
> + /*
> + * uaddr can be NULL and addr_len can be 0 if:
> + * sk is a TCP fastopen active socket and
> + * TCP_FASTOPEN_CONNECT sockopt is set and
> + * we already have a valid cookie for this socket.
> + * In this case, user can call write() after connect().
> + * write() will invoke tcp_sendmsg_fastopen() which calls
> + * __inet_stream_connect().
> + */
> + if (uaddr) {
> + if (addr_len < sizeof(uaddr->sa_family))
> + return -EINVAL;
>
> - if (uaddr->sa_family == AF_UNSPEC) {
> - err = sk->sk_prot->disconnect(sk, flags);
> - sock->state = err ? SS_DISCONNECTING : SS_UNCONNECTED;
> - goto out;
> + if (uaddr->sa_family == AF_UNSPEC) {
> + err = sk->sk_prot->disconnect(sk, flags);
> + sock->state = err ? SS_DISCONNECTING : SS_UNCONNECTED;
> + goto out;
> + }
> }
>
> switch (sock->state) {
> @@ -593,7 +604,10 @@ int __inet_stream_connect(struct socket *sock, struct sockaddr *uaddr,
> err = -EISCONN;
> goto out;
> case SS_CONNECTING:
> - err = -EALREADY;
> + if (inet_sk(sk)->defer_connect)
> + err = -EINPROGRESS;
> + else
> + err = -EALREADY;
> /* Fall out of switch with err, set for this state */
> break;
> case SS_UNCONNECTED:
> @@ -607,6 +621,9 @@ int __inet_stream_connect(struct socket *sock, struct sockaddr *uaddr,
>
> sock->state = SS_CONNECTING;
>
> + if (!err && inet_sk(sk)->defer_connect)
> + goto out;
> +
> /* Just entered SS_CONNECTING state; the only
> * difference is that return value in non-blocking
> * case is EINPROGRESS, rather than EALREADY.
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index c43eb1a831d7..d9735b76d073 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -533,6 +533,12 @@ unsigned int tcp_poll(struct file *file, struct socket *sock, poll_table *wait)
>
> if (tp->urg_data & TCP_URG_VALID)
> mask |= POLLPRI;
> + } else if (sk->sk_state == TCP_SYN_SENT && inet_sk(sk)->defer_connect) {
> + /* Active TCP fastopen socket with defer_connect
> + * Return POLLOUT so application can call write()
> + * in order for kernel to generate SYN+data
> + */
> + mask |= POLLOUT | POLLWRNORM;
> }
> /* This barrier is coupled with smp_wmb() in tcp_reset() */
> smp_rmb();
> @@ -1071,6 +1077,7 @@ static int tcp_sendmsg_fastopen(struct sock *sk, struct msghdr *msg,
> int *copied, size_t size)
> {
> struct tcp_sock *tp = tcp_sk(sk);
> + struct inet_sock *inet = inet_sk(sk);
> int err, flags;
>
> if (!(sysctl_tcp_fastopen & TFO_CLIENT_ENABLE))
> @@ -1085,9 +1092,19 @@ static int tcp_sendmsg_fastopen(struct sock *sk, struct msghdr *msg,
> tp->fastopen_req->data = msg;
> tp->fastopen_req->size = size;
>
> + if (inet->defer_connect) {
> + err = tcp_connect(sk);
> + /* Same failure procedure as in tcp_v4/6_connect */
> + if (err) {
> + tcp_set_state(sk, TCP_CLOSE);
> + inet->inet_dport = 0;
> + sk->sk_route_caps = 0;
> + }
> + }
> flags = (msg->msg_flags & MSG_DONTWAIT) ? O_NONBLOCK : 0;
> err = __inet_stream_connect(sk->sk_socket, msg->msg_name,
> msg->msg_namelen, flags);
> + inet->defer_connect = 0;
> *copied = tp->fastopen_req->copied;
> tcp_free_fastopen_req(tp);
> return err;
> @@ -1107,7 +1124,7 @@ int tcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
> lock_sock(sk);
>
> flags = msg->msg_flags;
> - if (flags & MSG_FASTOPEN) {
> + if (unlikely(flags & MSG_FASTOPEN || inet_sk(sk)->defer_connect)) {
> err = tcp_sendmsg_fastopen(sk, msg, &copied_syn, size);
> if (err == -EINPROGRESS && copied_syn > 0)
> goto out;
> @@ -2656,6 +2673,18 @@ static int do_tcp_setsockopt(struct sock *sk, int level,
> err = -EINVAL;
> }
> break;
> + case TCP_FASTOPEN_CONNECT:
> + if (val > 1 || val < 0) {
> + err = -EINVAL;
> + } else if (sysctl_tcp_fastopen & TFO_CLIENT_ENABLE) {
> + if (sk->sk_state == TCP_CLOSE)
> + tp->fastopen_connect = val;
> + else
> + err = -EINVAL;
> + } else {
> + err = -EOPNOTSUPP;
> + }
> + break;
> case TCP_TIMESTAMP:
> if (!tp->repair)
> err = -EPERM;
> @@ -3016,6 +3045,10 @@ static int do_tcp_getsockopt(struct sock *sk, int level,
> val = icsk->icsk_accept_queue.fastopenq.max_qlen;
> break;
>
> + case TCP_FASTOPEN_CONNECT:
> + val = tp->fastopen_connect;
> + break;
> +
> case TCP_TIMESTAMP:
> val = tcp_time_stamp + tp->tsoffset;
> break;
> diff --git a/net/ipv4/tcp_fastopen.c b/net/ipv4/tcp_fastopen.c
> index f90e09e1ff4c..9674bec4a0f8 100644
> --- a/net/ipv4/tcp_fastopen.c
> +++ b/net/ipv4/tcp_fastopen.c
> @@ -346,3 +346,36 @@ bool tcp_fastopen_cookie_check(struct sock *sk, u16 *mss,
> }
> return cookie->len > 0;
> }
> +
> +/* This function checks if we want to defer sending SYN until the first
> + * write(). We defer under the following conditions:
> + * 1. fastopen_connect sockopt is set
> + * 2. we have a valid cookie
> + * Return value: return true if we want to defer until application writes data
> + * return false if we want to send out SYN immediately
> + */
> +bool tcp_fastopen_defer_connect(struct sock *sk, int *err)
> +{
> + struct tcp_fastopen_cookie cookie = { .len = 0 };
> + struct tcp_sock *tp = tcp_sk(sk);
> + u16 mss;
> +
> + if (tp->fastopen_connect && !tp->fastopen_req) {
> + if (tcp_fastopen_cookie_check(sk, &mss, &cookie)) {
> + inet_sk(sk)->defer_connect = 1;
> + return true;
> + }
> +
> + /* Alloc fastopen_req in order for FO option to be included
> + * in SYN
> + */
> + tp->fastopen_req = kzalloc(sizeof(*tp->fastopen_req),
> + sk->sk_allocation);
> + if (tp->fastopen_req)
> + tp->fastopen_req->cookie = cookie;
> + else
> + *err = -ENOBUFS;
> + }
> + return false;
> +}
> +EXPORT_SYMBOL(tcp_fastopen_defer_connect);
> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> index f7325b25b06e..27b726c96459 100644
> --- a/net/ipv4/tcp_ipv4.c
> +++ b/net/ipv4/tcp_ipv4.c
> @@ -232,6 +232,7 @@ int tcp_v4_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len)
> /* OK, now commit destination to socket. */
> sk->sk_gso_type = SKB_GSO_TCPV4;
> sk_setup_caps(sk, &rt->dst);
> + rt = NULL;
>
> if (!tp->write_seq && likely(!tp->repair))
> tp->write_seq = secure_tcp_sequence_number(inet->inet_saddr,
> @@ -242,9 +243,13 @@ int tcp_v4_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len)
>
> inet->inet_id = tp->write_seq ^ jiffies;
>
> + if (tcp_fastopen_defer_connect(sk, &err))
> + return err;
> + if (err)
> + goto failure;
> +
> err = tcp_connect(sk);
>
> - rt = NULL;
> if (err)
> goto failure;
>
> diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
> index 0b7cd3d009b6..95c05e5293b1 100644
> --- a/net/ipv6/tcp_ipv6.c
> +++ b/net/ipv6/tcp_ipv6.c
> @@ -287,6 +287,11 @@ static int tcp_v6_connect(struct sock *sk, struct sockaddr *uaddr,
> inet->inet_dport,
> &tp->tsoffset);
>
> + if (tcp_fastopen_defer_connect(sk, &err))
> + return err;
> + if (err)
> + goto late_failure;
> +
> err = tcp_connect(sk);
> if (err)
> goto late_failure;
> --
> 2.11.0.483.g087da7b7c-goog
>
^ permalink raw reply
* Re: Question about veth_xmit()
From: Cong Wang @ 2017-01-23 20:56 UTC (permalink / raw)
To: Xiangning Yu; +Cc: Linux Kernel Network Developers
In-Reply-To: <CAOwmpL3vg56ZqHaYVBy+9dJJ1=GCbgy-m8NY0JrNCV-Z42R_eg@mail.gmail.com>
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?
^ permalink raw reply
* Re: [PATCH net-next 1/3] net/tcp-fastopen: refactor cookie check logic
From: Yuchung Cheng @ 2017-01-23 20:51 UTC (permalink / raw)
To: Wei Wang; +Cc: netdev, David Miller, Eric Dumazet, Wei Wang
In-Reply-To: <20170123185922.48046-2-tracywwnj@gmail.com>
On Mon, Jan 23, 2017 at 10:59 AM, Wei Wang <tracywwnj@gmail.com> wrote:
>
> From: Wei Wang <weiwan@google.com>
>
> Refactor the cookie check logic in tcp_send_syn_data() into a function.
> This function will be called else where in later changes.
>
> Signed-off-by: Wei Wang <weiwan@google.com>
> ---
Acked-by: Yuchung Cheng <ycheng@google.com>
> include/net/tcp.h | 2 ++
> net/ipv4/tcp_fastopen.c | 21 +++++++++++++++++++++
> net/ipv4/tcp_output.c | 16 ++--------------
> 3 files changed, 25 insertions(+), 14 deletions(-)
>
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index c55d65f74f7f..de67541d7adf 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -1493,6 +1493,8 @@ struct sock *tcp_try_fastopen(struct sock *sk, struct sk_buff *skb,
> struct tcp_fastopen_cookie *foc,
> struct dst_entry *dst);
> void tcp_fastopen_init_key_once(bool publish);
> +bool tcp_fastopen_cookie_check(struct sock *sk, u16 *mss,
> + struct tcp_fastopen_cookie *cookie);
> #define TCP_FASTOPEN_KEY_LENGTH 16
>
> /* Fastopen key context */
> diff --git a/net/ipv4/tcp_fastopen.c b/net/ipv4/tcp_fastopen.c
> index f51919535ca7..f90e09e1ff4c 100644
> --- a/net/ipv4/tcp_fastopen.c
> +++ b/net/ipv4/tcp_fastopen.c
> @@ -325,3 +325,24 @@ struct sock *tcp_try_fastopen(struct sock *sk, struct sk_buff *skb,
> *foc = valid_foc;
> return NULL;
> }
> +
> +bool tcp_fastopen_cookie_check(struct sock *sk, u16 *mss,
> + struct tcp_fastopen_cookie *cookie)
> +{
> + unsigned long last_syn_loss = 0;
> + int syn_loss = 0;
> +
> + tcp_fastopen_cache_get(sk, mss, cookie, &syn_loss, &last_syn_loss);
> +
> + /* Recurring FO SYN losses: no cookie or data in SYN */
> + if (syn_loss > 1 &&
> + time_before(jiffies, last_syn_loss + (60*HZ << syn_loss))) {
> + cookie->len = -1;
> + return false;
> + }
> + if (sysctl_tcp_fastopen & TFO_CLIENT_NO_COOKIE) {
> + cookie->len = -1;
> + return true;
> + }
> + return cookie->len > 0;
> +}
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index 9a1a1494b9dd..671c69535671 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -3267,23 +3267,11 @@ static int tcp_send_syn_data(struct sock *sk, struct sk_buff *syn)
> {
> struct tcp_sock *tp = tcp_sk(sk);
> struct tcp_fastopen_request *fo = tp->fastopen_req;
> - int syn_loss = 0, space, err = 0;
> - unsigned long last_syn_loss = 0;
> + int space, err = 0;
> struct sk_buff *syn_data;
>
> tp->rx_opt.mss_clamp = tp->advmss; /* If MSS is not cached */
> - tcp_fastopen_cache_get(sk, &tp->rx_opt.mss_clamp, &fo->cookie,
> - &syn_loss, &last_syn_loss);
> - /* Recurring FO SYN losses: revert to regular handshake temporarily */
> - if (syn_loss > 1 &&
> - time_before(jiffies, last_syn_loss + (60*HZ << syn_loss))) {
> - fo->cookie.len = -1;
> - goto fallback;
> - }
> -
> - if (sysctl_tcp_fastopen & TFO_CLIENT_NO_COOKIE)
> - fo->cookie.len = -1;
> - else if (fo->cookie.len <= 0)
> + if (!tcp_fastopen_cookie_check(sk, &tp->rx_opt.mss_clamp, &fo->cookie))
> goto fallback;
>
> /* MSS for SYN-data is based on cached MSS and bounded by PMTU and
> --
> 2.11.0.483.g087da7b7c-goog
>
^ permalink raw reply
* Re: [PATCH] virtio_net: fix PAGE_SIZE > 64k
From: kbuild test robot @ 2017-01-23 20:46 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: kbuild-all, linux-kernel, Jason Wang, virtualization, netdev
In-Reply-To: <1485189896-20327-1-git-send-email-mst@redhat.com>
Hi Michael,
[auto build test WARNING on net-next/master]
[also build test WARNING on v4.10-rc5 next-20170123]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Michael-S-Tsirkin/virtio_net-fix-PAGE_SIZE-64k/20170124-023805
reproduce:
# apt-get install sparse
make ARCH=x86_64 allmodconfig
make C=1 CF=-D__CHECK_ENDIAN__
sparse warnings: (new ones prefixed by >>)
include/linux/compiler.h:253:8: sparse: attribute 'no_sanitize_address': unknown attribute
>> drivers/net/virtio_net.c:250:44: sparse: incompatible types in comparison expression (different signedness)
drivers/net/virtio_net.c:251:33: sparse: incompatible types in comparison expression (different signedness)
drivers/net/virtio_net.c:256:36: sparse: incompatible types in comparison expression (different signedness)
drivers/net/virtio_net.c:262:40: sparse: incompatible types in comparison expression (different signedness)
drivers/net/virtio_net.c:861:16: sparse: incompatible types in comparison expression (different signedness)
drivers/net/virtio_net.c:861:16: sparse: incompatible types in comparison expression (different signedness)
In file included from include/linux/list.h:8:0,
from include/linux/timer.h:4,
from include/linux/netdevice.h:28,
from drivers/net/virtio_net.c:19:
drivers/net/virtio_net.c: In function 'mergeable_ctx_to_buf_truesize':
include/linux/kernel.h:763:16: warning: comparison of distinct pointer types lacks a cast
(void) (&max1 == &max2); \
^
include/linux/kernel.h:766:2: note: in expansion of macro '__max'
__max(typeof(x), typeof(y), \
^~~~~
drivers/net/virtio_net.c:59:32: note: in expansion of macro 'max'
#define MERGEABLE_BUFFER_ALIGN max(L1_CACHE_BYTES, \
^~~
drivers/net/virtio_net.c:250:37: note: in expansion of macro 'MERGEABLE_BUFFER_ALIGN'
unsigned int truesize = mrg_ctx & (MERGEABLE_BUFFER_ALIGN - 1);
^~~~~~~~~~~~~~~~~~~~~~
include/linux/kernel.h:763:16: warning: comparison of distinct pointer types lacks a cast
(void) (&max1 == &max2); \
^
include/linux/kernel.h:766:2: note: in expansion of macro '__max'
__max(typeof(x), typeof(y), \
^~~~~
drivers/net/virtio_net.c:59:32: note: in expansion of macro 'max'
#define MERGEABLE_BUFFER_ALIGN max(L1_CACHE_BYTES, \
^~~
drivers/net/virtio_net.c:251:26: note: in expansion of macro 'MERGEABLE_BUFFER_ALIGN'
return (truesize + 1) * MERGEABLE_BUFFER_ALIGN;
^~~~~~~~~~~~~~~~~~~~~~
drivers/net/virtio_net.c: In function 'mergeable_ctx_to_buf_address':
include/linux/kernel.h:763:16: warning: comparison of distinct pointer types lacks a cast
(void) (&max1 == &max2); \
^
include/linux/kernel.h:766:2: note: in expansion of macro '__max'
__max(typeof(x), typeof(y), \
^~~~~
drivers/net/virtio_net.c:59:32: note: in expansion of macro 'max'
#define MERGEABLE_BUFFER_ALIGN max(L1_CACHE_BYTES, \
^~~
drivers/net/virtio_net.c:256:29: note: in expansion of macro 'MERGEABLE_BUFFER_ALIGN'
return (void *)(mrg_ctx & -MERGEABLE_BUFFER_ALIGN);
^~~~~~~~~~~~~~~~~~~~~~
drivers/net/virtio_net.c: In function 'mergeable_buf_to_ctx':
include/linux/kernel.h:763:16: warning: comparison of distinct pointer types lacks a cast
(void) (&max1 == &max2); \
^
include/linux/kernel.h:766:2: note: in expansion of macro '__max'
__max(typeof(x), typeof(y), \
^~~~~
drivers/net/virtio_net.c:59:32: note: in expansion of macro 'max'
#define MERGEABLE_BUFFER_ALIGN max(L1_CACHE_BYTES, \
^~~
drivers/net/virtio_net.c:262:33: note: in expansion of macro 'MERGEABLE_BUFFER_ALIGN'
unsigned int size = truesize / MERGEABLE_BUFFER_ALIGN;
^~~~~~~~~~~~~~~~~~~~~~
In file included from include/linux/cache.h:4:0,
from include/linux/printk.h:8,
from include/linux/kernel.h:13,
from include/linux/list.h:8,
from include/linux/timer.h:4,
from include/linux/netdevice.h:28,
from drivers/net/virtio_net.c:19:
drivers/net/virtio_net.c: In function 'get_mergeable_buf_len':
include/linux/kernel.h:763:16: warning: comparison of distinct pointer types lacks a cast
(void) (&max1 == &max2); \
^
include/uapi/linux/kernel.h:10:47: note: in definition of macro '__ALIGN_KERNEL_MASK'
#define __ALIGN_KERNEL_MASK(x, mask) (((x) + (mask)) & ~(mask))
^~~~
include/linux/kernel.h:49:22: note: in expansion of macro '__ALIGN_KERNEL'
#define ALIGN(x, a) __ALIGN_KERNEL((x), (a))
^~~~~~~~~~~~~~
drivers/net/virtio_net.c:861:9: note: in expansion of macro 'ALIGN'
return ALIGN(len, MERGEABLE_BUFFER_ALIGN);
^~~~~
include/linux/kernel.h:766:2: note: in expansion of macro '__max'
__max(typeof(x), typeof(y), \
^~~~~
drivers/net/virtio_net.c:59:32: note: in expansion of macro 'max'
#define MERGEABLE_BUFFER_ALIGN max(L1_CACHE_BYTES, \
^~~
drivers/net/virtio_net.c:861:20: note: in expansion of macro 'MERGEABLE_BUFFER_ALIGN'
return ALIGN(len, MERGEABLE_BUFFER_ALIGN);
^~~~~~~~~~~~~~~~~~~~~~
include/linux/kernel.h:763:16: warning: comparison of distinct pointer types lacks a cast
(void) (&max1 == &max2); \
^
include/uapi/linux/kernel.h:10:58: note: in definition of macro '__ALIGN_KERNEL_MASK'
#define __ALIGN_KERNEL_MASK(x, mask) (((x) + (mask)) & ~(mask))
^~~~
include/linux/kernel.h:49:22: note: in expansion of macro '__ALIGN_KERNEL'
#define ALIGN(x, a) __ALIGN_KERNEL((x), (a))
^~~~~~~~~~~~~~
drivers/net/virtio_net.c:861:9: note: in expansion of macro 'ALIGN'
return ALIGN(len, MERGEABLE_BUFFER_ALIGN);
vim +250 drivers/net/virtio_net.c
fb6813f4 Rusty Russell 2008-07-25 234 return p;
fb6813f4 Rusty Russell 2008-07-25 235 }
fb6813f4 Rusty Russell 2008-07-25 236
e9d7417b Jason Wang 2012-12-07 237 static void skb_xmit_done(struct virtqueue *vq)
296f96fc Rusty Russell 2007-10-22 238 {
e9d7417b Jason Wang 2012-12-07 239 struct virtnet_info *vi = vq->vdev->priv;
296f96fc Rusty Russell 2007-10-22 240
2cb9c6ba Rusty Russell 2008-02-04 241 /* Suppress further interrupts. */
e9d7417b Jason Wang 2012-12-07 242 virtqueue_disable_cb(vq);
11a3a154 Rusty Russell 2008-05-26 243
363f1514 Rusty Russell 2008-06-08 244 /* We were probably waiting for more output buffers. */
986a4f4d Jason Wang 2012-12-07 245 netif_wake_subqueue(vi->dev, vq2txq(vq));
296f96fc Rusty Russell 2007-10-22 246 }
296f96fc Rusty Russell 2007-10-22 247
ab7db917 Michael Dalton 2014-01-16 248 static unsigned int mergeable_ctx_to_buf_truesize(unsigned long mrg_ctx)
ab7db917 Michael Dalton 2014-01-16 249 {
ab7db917 Michael Dalton 2014-01-16 @250 unsigned int truesize = mrg_ctx & (MERGEABLE_BUFFER_ALIGN - 1);
ab7db917 Michael Dalton 2014-01-16 251 return (truesize + 1) * MERGEABLE_BUFFER_ALIGN;
ab7db917 Michael Dalton 2014-01-16 252 }
ab7db917 Michael Dalton 2014-01-16 253
ab7db917 Michael Dalton 2014-01-16 254 static void *mergeable_ctx_to_buf_address(unsigned long mrg_ctx)
ab7db917 Michael Dalton 2014-01-16 255 {
ab7db917 Michael Dalton 2014-01-16 256 return (void *)(mrg_ctx & -MERGEABLE_BUFFER_ALIGN);
ab7db917 Michael Dalton 2014-01-16 257
ab7db917 Michael Dalton 2014-01-16 258 }
:::::: The code at line 250 was first introduced by commit
:::::: ab7db91705e95ed1bba1304388936fccfa58c992 virtio-net: auto-tune mergeable rx buffer size for improved performance
:::::: TO: Michael Dalton <mwdalton@google.com>
:::::: CC: David S. Miller <davem@davemloft.net>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
^ permalink raw reply
* RE: iproute2: using .maxattr = IFLA_VLAN_MAX??
From: Rosen, Rami @ 2017-01-23 20:45 UTC (permalink / raw)
To: Murali Karicheri, open list:TI NETCP ETHERNET DRIVER
In-Reply-To: <58864115.4060608@ti.com>
Hi, Murali,
> This appears to be a cut-n-paste bug as the source is based on ip/iplink_vlan.c and should be fixed to IFLA_HSR_MAX.
You are right.
The "HSR section" indeed defines IFLA_HSR_MAX in include/linux/if_link.h of iproute2 as the max attributes number.
And in the top of ip/iplink_hsr.c you will find this comment, which enhances your assumption about the origin of this mistake:
/*
...
Based on iplink_vlan.c by Patrick McHardy <kaber@trash.net
...
*/
Regards,
Rami Rosen
^ permalink raw reply
* Re: [PATCH net] net: dsa: Check return value of phy_connect_direct()
From: David Miller @ 2017-01-23 20:43 UTC (permalink / raw)
To: f.fainelli; +Cc: netdev, vivien.didelot, andrew
In-Reply-To: <20170121000505.15135-1-f.fainelli@gmail.com>
From: Florian Fainelli <f.fainelli@gmail.com>
Date: Fri, 20 Jan 2017 16:05:05 -0800
> We need to check the return value of phy_connect_direct() in
> dsa_slave_phy_connect() otherwise we may be continuing the
> initialization of a slave network device with a PHY that already
> attached somewhere else and which will soon be in error because the PHY
> device is in error.
>
> The conditions for such an error to occur are that we have a port of our
> switch that is not disabled, and has the same port number as a PHY
> address (say both 5) that can be probed using the DSA slave MII bus. We
> end-up having this slave network device find a PHY at the same address
> as our port number, and we try to attach to it.
>
> A slave network (e.g: port 0) has already attached to our PHY device,
> and we try to re-attach it with a different network device, but since we
> ignore the error we would end-up initializating incorrect device
> references by the time the slave network interface is opened.
>
> The code has been (re)organized several times, making it hard to provide
> an exact Fixes tag, this is a bugfix nonetheless.
>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
Applied, thanks Florian.
^ permalink raw reply
* Re: [PATCH net v2 2/2] lwtunnel: Fix oops on state free after encap module unload
From: David Miller @ 2017-01-23 20:42 UTC (permalink / raw)
To: rshearma; +Cc: netdev, tom, roopa, david.lebrun, tgraf, dsa
In-Reply-To: <1484958086-6151-3-git-send-email-rshearma@brocade.com>
From: Robert Shearman <rshearma@brocade.com>
Date: Sat, 21 Jan 2017 00:21:26 +0000
> @@ -115,8 +115,12 @@ int lwtunnel_build_state(struct net_device *dev, u16 encap_type,
> ret = -EOPNOTSUPP;
> rcu_read_lock();
Here 'ret' equals -EOPNOTSUPP
> ops = rcu_dereference(lwtun_encaps[encap_type]);
> - if (likely(ops && ops->build_state))
> - ret = ops->build_state(dev, encap, family, cfg, lws);
> + if (likely(ops)) {
> + if (likely(try_module_get(ops->owner) && ops->build_state))
> + ret = ops->build_state(dev, encap, family, cfg, lws);
> + if (ret)
> + module_put(ops->owner);
If try_module_get() fails, 'ret' will still be -EOPNOTSUPP and we will
module_put() on a module we did not grab a reference to.
I think you need to adjust the logic here. You only want to 'put' if
try_module_get() succeeds and ->build_state() returns an error.
^ permalink raw reply
* Re: [PATCH v3 net-next] Introduce a sysctl that modifies the value of PROT_SOCK.
From: David Miller @ 2017-01-23 20:39 UTC (permalink / raw)
To: kjlx; +Cc: eric.dumazet, stephen, netdev
In-Reply-To: <20170121014911.GB2598@templeofstupid.com>
From: Krister Johansen <kjlx@templeofstupid.com>
Date: Fri, 20 Jan 2017 17:49:11 -0800
> Add net.ipv4.ip_unprivileged_port_start, which is a per namespace sysctl
> that denotes the first unprivileged inet port in the namespace. To
> disable all privileged ports set this to zero. It also checks for
> overlap with the local port range. The privileged and local range may
> not overlap.
>
> The use case for this change is to allow containerized processes to bind
> to priviliged ports, but prevent them from ever being allowed to modify
> their container's network configuration. The latter is accomplished by
> ensuring that the network namespace is not a child of the user
> namespace. This modification was needed to allow the container manager
> to disable a namespace's priviliged port restrictions without exposing
> control of the network namespace to processes in the user namespace.
>
> Signed-off-by: Krister Johansen <kjlx@templeofstupid.com>
I'm not ignoring this change, I just want to think about it some more.
Just FYI...
^ permalink raw reply
* Re: [PATCH net] net: phy: Avoid deadlock during phy_error()
From: David Miller @ 2017-01-23 20:37 UTC (permalink / raw)
To: f.fainelli; +Cc: netdev, andrew, kyle.roeschley, rmk+kernel
In-Reply-To: <20170120233152.16244-1-f.fainelli@gmail.com>
From: Florian Fainelli <f.fainelli@gmail.com>
Date: Fri, 20 Jan 2017 15:31:52 -0800
> phy_error() is called in the PHY state machine workqueue context, and
> calls phy_trigger_machine() which does a cancel_delayed_work_sync() of
> the workqueue we execute from, causing a deadlock situation.
>
> Augment phy_trigger_machine() machine with a sync boolean indicating
> whether we should use cancel_*_sync() or just cancel_*_work().
>
> Fixes: 3c293f4e08b5 ("net: phy: Trigger state machine on state change and not polling.")
> Reported-by: Russell King <rmk+kernel@armlinux.org.uk>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
Applied, thanks Florian.
^ permalink raw reply
* [PATCH v2] bpf: Restrict cgroup bpf hooks to the init netns
From: Andy Lutomirski @ 2017-01-23 20:36 UTC (permalink / raw)
To: Network Development, David S. Miller
Cc: Andy Lutomirski, Daniel Borkmann, Alexei Starovoitov, David Ahern
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
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
Since it's rather late in the release cycle, let's punt. This patch
makes it impossible to install cgroup+bpf hooks outside the init
netns and makes them not run on sockets that aren't in the init
netns.
In a future release, it should be relatively straightforward to make
these hooks be local to a netns and, if needed, to add a flag so
that hooks can be made global if necessary. Global hooks should
presumably be constrained so that they can't write to any ifindex
fields.
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: David Ahern <dsa@cumulusnetworks.com>
Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
DaveM, this mitigates a bug in a feature that's new in 4.10, and the
bug can be hit using current iproute2 -git. please consider this for
-net.
Changes from v1:
- Fix the commit message. 'git commit' was very clever and thought that
all the interesting bits of the test case were intended to be comments
and stripped them. Whoops!
kernel/bpf/cgroup.c | 21 +++++++++++++++++++++
kernel/bpf/syscall.c | 11 +++++++++++
2 files changed, 32 insertions(+)
diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
index a515f7b007c6..a824f543de69 100644
--- a/kernel/bpf/cgroup.c
+++ b/kernel/bpf/cgroup.c
@@ -143,6 +143,17 @@ int __cgroup_bpf_run_filter_skb(struct sock *sk,
if (!sk || !sk_fullsock(sk))
return 0;
+ /*
+ * 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 (sock_net(sk) != &init_net)
+ return 0;
+
if (sk->sk_family != AF_INET &&
sk->sk_family != AF_INET6)
return 0;
@@ -186,6 +197,16 @@ int __cgroup_bpf_run_filter_sk(struct sock *sk,
struct bpf_prog *prog;
int ret = 0;
+ /*
+ * 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 (sock_net(sk) != &init_net)
+ return 0;
rcu_read_lock();
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;
--
2.9.3
^ permalink raw reply related
* [PATCH] bpf: Restrict cgroup bpf hooks to the init netns
From: Andy Lutomirski @ 2017-01-23 20:33 UTC (permalink / raw)
To: Network Development, David S. Miller
Cc: Andy Lutomirski, Daniel Borkmann, Alexei Starovoitov, David Ahern
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:
Default binding is "vrf0"
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
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
Since it's rather late in the release cycle, let's punt. This patch
makes it impossible to install cgroup+bpf hooks outside the init
netns and makes them not run on sockets that aren't in the init
netns.
In a future release, it should be relatively straightforward to make
these hooks be local to a netns and, if needed, to add a flag so
that hooks can be made global if necessary. Global hooks should
presumably be constrained so that they can't write to any ifindex
fields.
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: David Ahern <dsa@cumulusnetworks.com>
Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
DaveM, this mitigates a bug in a feature that's new in 4.10, and the
bug can be hit using current iproute2 -git. please consider this for
-net.
kernel/bpf/cgroup.c | 21 +++++++++++++++++++++
kernel/bpf/syscall.c | 11 +++++++++++
2 files changed, 32 insertions(+)
diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
index a515f7b007c6..a824f543de69 100644
--- a/kernel/bpf/cgroup.c
+++ b/kernel/bpf/cgroup.c
@@ -143,6 +143,17 @@ int __cgroup_bpf_run_filter_skb(struct sock *sk,
if (!sk || !sk_fullsock(sk))
return 0;
+ /*
+ * 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 (sock_net(sk) != &init_net)
+ return 0;
+
if (sk->sk_family != AF_INET &&
sk->sk_family != AF_INET6)
return 0;
@@ -186,6 +197,16 @@ int __cgroup_bpf_run_filter_sk(struct sock *sk,
struct bpf_prog *prog;
int ret = 0;
+ /*
+ * 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 (sock_net(sk) != &init_net)
+ return 0;
rcu_read_lock();
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;
--
2.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