* 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
* Re: [PATCH] Documentation: net: phy: improve explanation when to specify the PHY ID
From: Florian Fainelli @ 2017-01-23 20:33 UTC (permalink / raw)
To: Martin Blumenstingl, andrew-g2DYL2Zd6BY,
devicetree-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA,
davem-fT/PcQaiUtIeIZ0/mPfg9Q
Cc: mark.rutland-5wv7dgnIgG8, robh+dt-DgEjT+Ai2ygdnm+yROfE0A
In-Reply-To: <20170122164132.13967-1-martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
On 01/22/2017 08:41 AM, Martin Blumenstingl wrote:
> The old description basically read like "ethernet-phy-idAAAA.BBBB" can
> be specified when you know the actual PHY ID. However, specifying this
> has a side-effect: it forces Linux to bind to a certain PHY driver (the
> one that matches the ID given in the compatible string), ignoring the ID
> which is reported by the actual PHY.
> Whenever a device is shipped with (multiple) different PHYs during it's
> production lifetime then explicitly specifying
> "ethernet-phy-idAAAA.BBBB" could break certain revisions of that device.
>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
Reviewed-by: Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
--
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] Documentation: net: phy: improve explanation when to specify the PHY ID
From: Rob Herring @ 2017-01-23 20:32 UTC (permalink / raw)
To: Martin Blumenstingl
Cc: andrew-g2DYL2Zd6BY, devicetree-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA, davem-fT/PcQaiUtIeIZ0/mPfg9Q,
f.fainelli-Re5JQEeQqe8AvxtiuMwx3w, mark.rutland-5wv7dgnIgG8
In-Reply-To: <20170122164132.13967-1-martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
On Sun, Jan 22, 2017 at 05:41:32PM +0100, Martin Blumenstingl wrote:
> The old description basically read like "ethernet-phy-idAAAA.BBBB" can
> be specified when you know the actual PHY ID. However, specifying this
> has a side-effect: it forces Linux to bind to a certain PHY driver (the
> one that matches the ID given in the compatible string), ignoring the ID
> which is reported by the actual PHY.
> Whenever a device is shipped with (multiple) different PHYs during it's
> production lifetime then explicitly specifying
> "ethernet-phy-idAAAA.BBBB" could break certain revisions of that device.
>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
> ---
> Thanks to Andrew Lunn for pointing the documentation issue out to me in:
> http://lists.infradead.org/pipermail/linux-amlogic/2017-January/002141.html
>
>
> Documentation/devicetree/bindings/net/phy.txt | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: Potential issues (security and otherwise) with the current cgroup-bpf API
From: Andy Lutomirski @ 2017-01-23 20:20 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Tejun Heo, Michal Hocko, Peter Zijlstra, David Ahern,
Andy Lutomirski, Daniel Mack, Mickaël Salaün, Kees Cook,
Jann Horn, David S. Miller, Thomas Graf, Michael Kerrisk,
Linux API, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Network Development
In-Reply-To: <20170123043154.GA93519-+o4/htvd0TDFYCXBM6kdu7fOX0fSgVTm@public.gmane.org>
On Sun, Jan 22, 2017 at 8:31 PM, Alexei Starovoitov
<alexei.starovoitov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Thu, Jan 19, 2017 at 08:04:59PM -0800, Andy Lutomirski wrote:
>> On Thu, Jan 19, 2017 at 6:39 PM, Alexei Starovoitov
>> <alexei.starovoitov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> > On Wed, Jan 18, 2017 at 06:29:22PM -0800, Andy Lutomirski wrote:
>> >> I think it could work by making a single socket cgroup controller that
>> >> handles all cgroup things that are bound to a socket. Using
>> >
>> > Such 'socket cgroup controller' would limit usability of the feature
>> > to sockets and force all other use cases like landlock to invent
>> > their own wheel, which is undesirable. Everyone will be
>> > inventing new 'foo cgroup controller', while all of them
>> > are really bpf features. They are different bpf program
>> > types that attach to different hooks and use cgroup for scoping.
>>
>> Can you elaborate on why that would be a problem? In a cgroup v1
>> world, users who want different hierarchies for different types of
>> control could easily want one hierarchy for socket hooks and a
>> different hierarchy for lsm hooks. In a cgroup v2 delegation world, I
>> could easily imagine the decision to delegate socket hooks being
>> different from the decision to delegate lsm hooks. Almost all of the
>> code would be shared between different bpf-using cgroup controllers.
>
> how do you think it can be enforced when directory is chowned?
A combination of delegation policy (a la subtree_control in the
parent) and MAC policy. I'm quite confident that apparmor can apply
policy to cgroupfs and I'm reasonably confident (although slightly
less so) that selinux can as well. The docs suck :(
But if there's only one file in there to apply MAC policy to, the
ability to differentiate between subsystems is quite limited. In the
current API, there are no files to apply policy to, so it won't work
at all.
>
> ... and open() of the directory done by the current api will preserve
> cgroup delegation when and only when bpf_prog_type_cgroup_*
> becomes unprivileged.
> I'm not proposing creating new api here.
I don't know what you mean. open() of the directory can't check
anything useful because it has to work for programs that just want to
read the directory.
>> Meanwhile, cgroup+bpf actually appears to be buggy in this regard even
>> regardless of what semantics you think are better. sk_bound_dev_if is
>> exposed as a u32 value, but sk_bound_dev_if only has meaning within a
>> given netns. The "ip vrf" stuff will straight-up malfunction if a
>> process affected by its hook runs in a different netns from the netns
>> that "ip vrf" was run in.
>
> how is that any different from normal 'ip netns exec'?
> that is expected user behavior.
# ./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
The expected behavior to me is that ip netns exec (or equivalently
unshare -n) gives a clean slate. Actually malfunctioning (which this
example using the latest iproute2 and linux just did) is not expected.
I'm done with this part of this thread and I'm sending a patch.
>
>> restricting the types of sockets that can be created, then you do want
>> the filter to work across namespaces, but seccomp can do that too and
>> the current code doesn't handle netns correctly.
>
> are you saying that seccomp supports netns filtering? please show the proof.
It can trivially restruct the types of sockets that are created by
filtering on socket(2) syscall parameters, at least on sane
architectures that don't use socketcall().
> To summarize, I think the 'prog override is not allowed' flag would be
> ok feature to have and I wouldn't mind making it the default when no 'flag'
> field is passed to bpf syscall, but it's not acceptable to remove current
> 'prog override is allowed' behavior.
> So if you insist on changing the default, please implement the flag asap.
> Though from security point of view and ABI point of view there is absolutely
> no difference whether this flag is added today or a year later while
> the default is kept as-is.
It's too late and I have too little time. I'll try to write a patch
to change the default to just deny attempts to override. Better
behavior can be added later.
IMO your suggestions about priorities are overcomplicated. For your
instrumentation needs, I can imagine that a simple "make this hook not
run if a descendent has a hook" would do it. For everything else, run
them all in tree order (depending on filter type) and let the eBPF
code do whatever it wants to do.
^ permalink raw reply
* Re: [net PATCH v5 6/6] virtio_net: XDP support for adjust_head
From: Michael S. Tsirkin @ 2017-01-23 20:09 UTC (permalink / raw)
To: John Fastabend
Cc: jasowang, john.r.fastabend, netdev, alexei.starovoitov, daniel
In-Reply-To: <20170123205957-mutt-send-email-mst@kernel.org>
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.
>
> --
> 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.
Thoughts?
--->
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 related
* [PATCH v2] virtio_net: fix PAGE_SIZE > 64k
From: Michael S. Tsirkin @ 2017-01-23 19:37 UTC (permalink / raw)
To: linux-kernel; +Cc: netdev, John Fastabend, virtualization
I don't have any guests with PAGE_SIZE > 64k but the
code seems to be clearly broken in that case
as PAGE_SIZE / MERGEABLE_BUFFER_ALIGN will need
more than 8 bit and so the code in mergeable_ctx_to_buf_address
does not give us the actual true size.
Cc: John Fastabend <john.fastabend@gmail.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
changes from v1:
fix build warnings
drivers/net/virtio_net.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 4a10500..4dc373b 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -48,8 +48,16 @@ module_param(gso, bool, 0444);
*/
DECLARE_EWMA(pkt_len, 1, 64)
+/* With mergeable buffers we align buffer address and use the low bits to
+ * encode its true size. Buffer size is up to 1 page so we need to align to
+ * square root of page size to ensure we reserve enough bits to encode the true
+ * size.
+ */
+#define MERGEABLE_BUFFER_MIN_ALIGN_SHIFT ((PAGE_SHIFT + 1) / 2)
+
/* Minimum alignment for mergeable packet buffers. */
-#define MERGEABLE_BUFFER_ALIGN max(L1_CACHE_BYTES, 256)
+#define MERGEABLE_BUFFER_ALIGN max(L1_CACHE_BYTES, \
+ 1 << MERGEABLE_BUFFER_MIN_ALIGN_SHIFT)
#define VIRTNET_DRIVER_VERSION "1.0.0"
--
MST
^ 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