* Re: WoL broken in r8169.c since kernel 4.19
From: Marc Haber @ 2019-01-28 20:22 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: netdev@vger.kernel.org
In-Reply-To: <20190128073010.GG27062@torres.zugschlus.de>
On Mon, Jan 28, 2019 at 08:30:10AM +0100, Marc Haber wrote:
> On Sun, Jan 27, 2019 at 10:09:51PM +0100, Heiner Kallweit wrote:
> > Yes. All you have to do after each "git bisect good/bad" is build again,
> > test, and make current build as good or bad.
>
> Will report back if I get any results. When I bisected last time, I
> ended up with a kernel that didn't even boot, but with 5 steps this is
> probably manageable. Will take most of the week though.
[3/4995]mh@fan:~/linux/git/linux ((6fcf9b1d4d6c...) *%|BISECTING) $ git bislog
git bisect start 'drivers/net/ethernet/realtek/r8169.c'
# good: [4ff36466281428734791d3cc6331b8cca7c76019] r8169: replace get_protocol with vlan_get_protocol
git bisect good 4ff36466281428734791d3cc6331b8cca7c76019
# bad: [649f0837a8cc2b39329f2de00fa0d04b029291c5] r8169: fix broken Wake-on-LAN from S5 (poweroff)
git bisect bad 649f0837a8cc2b39329f2de00fa0d04b029291c5
# bad: [098b01ad9837b4d4d0022f407300f069a999e55a] r8169: don't include asm headers directly
git bisect bad 098b01ad9837b4d4d0022f407300f069a999e55a
[4/4996]mh@fan:~/linux/git/linux ((6fcf9b1d4d6c...) *%|BISECTING) $
The kernel that I now have is 6fcf9b1d4d6cd38202247de5c0ac7d85c4483abb and
that one throws oopses on booting and won't suspend at all.
Can I continue from here while still making sense?
Greetings
Marc
--
-----------------------------------------------------------------------------
Marc Haber | "I don't trust Computers. They | Mailadresse im Header
Leimen, Germany | lose things." Winona Ryder | Fon: *49 6224 1600402
Nordisch by Nature | How to make an American Quilt | Fax: *49 6224 1600421
^ permalink raw reply
* Re: WoL broken in r8169.c since kernel 4.19
From: Heiner Kallweit @ 2019-01-28 20:28 UTC (permalink / raw)
To: Marc Haber; +Cc: netdev@vger.kernel.org
In-Reply-To: <20190128202235.GH27062@torres.zugschlus.de>
On 28.01.2019 21:22, Marc Haber wrote:
> On Mon, Jan 28, 2019 at 08:30:10AM +0100, Marc Haber wrote:
>> On Sun, Jan 27, 2019 at 10:09:51PM +0100, Heiner Kallweit wrote:
>>> Yes. All you have to do after each "git bisect good/bad" is build again,
>>> test, and make current build as good or bad.
>>
>> Will report back if I get any results. When I bisected last time, I
>> ended up with a kernel that didn't even boot, but with 5 steps this is
>> probably manageable. Will take most of the week though.
>
> [3/4995]mh@fan:~/linux/git/linux ((6fcf9b1d4d6c...) *%|BISECTING) $ git bislog
> git bisect start 'drivers/net/ethernet/realtek/r8169.c'
> # good: [4ff36466281428734791d3cc6331b8cca7c76019] r8169: replace get_protocol with vlan_get_protocol
> git bisect good 4ff36466281428734791d3cc6331b8cca7c76019
> # bad: [649f0837a8cc2b39329f2de00fa0d04b029291c5] r8169: fix broken Wake-on-LAN from S5 (poweroff)
> git bisect bad 649f0837a8cc2b39329f2de00fa0d04b029291c5
> # bad: [098b01ad9837b4d4d0022f407300f069a999e55a] r8169: don't include asm headers directly
> git bisect bad 098b01ad9837b4d4d0022f407300f069a999e55a
> [4/4996]mh@fan:~/linux/git/linux ((6fcf9b1d4d6c...) *%|BISECTING) $
>
> The kernel that I now have is 6fcf9b1d4d6cd38202247de5c0ac7d85c4483abb and
> that one throws oopses on booting and won't suspend at all.
>
> Can I continue from here while still making sense?
>
Not w/o knowing whether this version is good or bad.
Because we're interested in file r8169.c only, you could test r8169.c from the
oops-ing kernel on top of a working kernel version.
> Greetings
> Marc
>
Heiner
^ permalink raw reply
* Re: [PATCH bpf-next v2 3/3] selftests: bpf: add test_lwt_ip_encap selftest
From: David Ahern @ 2019-01-28 20:31 UTC (permalink / raw)
To: Peter Oskolkov, Alexei Starovoitov, Daniel Borkmann, netdev
Cc: Peter Oskolkov, Willem de Bruijn
In-Reply-To: <20190124193418.81674-4-posk@google.com>
On 1/24/19 12:34 PM, Peter Oskolkov wrote:
> This patch adds a bpf self-test to cover BPF_LWT_ENCAP_IP mode
> in bpf_lwt_push_encap.
>
> Covered:
> - encapping in LWT_IN and LWT_XMIT
> - IPv4 and IPv6
>
> Signed-off-by: Peter Oskolkov <posk@google.com>
> ---
> tools/testing/selftests/bpf/Makefile | 5 +-
> .../testing/selftests/bpf/test_lwt_ip_encap.c | 125 +++++++
> .../selftests/bpf/test_lwt_ip_encap.sh | 316 ++++++++++++++++++
> 3 files changed, 444 insertions(+), 2 deletions(-)
> create mode 100644 tools/testing/selftests/bpf/test_lwt_ip_encap.c
> create mode 100755 tools/testing/selftests/bpf/test_lwt_ip_encap.sh
>
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index 70229de510f5..407c51cc5f07 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -39,7 +39,7 @@ TEST_GEN_FILES = test_pkt_access.o test_xdp.o test_l4lb.o test_tcp_estats.o test
> get_cgroup_id_kern.o socket_cookie_prog.o test_select_reuseport_kern.o \
> test_skb_cgroup_id_kern.o bpf_flow.o netcnt_prog.o \
> test_sk_lookup_kern.o test_xdp_vlan.o test_queue_map.o test_stack_map.o \
> - xdp_dummy.o test_map_in_map.o
> + xdp_dummy.o test_map_in_map.o test_lwt_ip_encap.o
>
> # Order correspond to 'make run_tests' order
> TEST_PROGS := test_kmod.sh \
> @@ -53,7 +53,8 @@ TEST_PROGS := test_kmod.sh \
> test_lirc_mode2.sh \
> test_skb_cgroup_id.sh \
> test_flow_dissector.sh \
> - test_xdp_vlan.sh
> + test_xdp_vlan.sh \
> + test_lwt_ip_encap.sh
>
> TEST_PROGS_EXTENDED := with_addr.sh \
> tcp_client.py \
> diff --git a/tools/testing/selftests/bpf/test_lwt_ip_encap.c b/tools/testing/selftests/bpf/test_lwt_ip_encap.c
> new file mode 100644
> index 000000000000..a7014277f3fe
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/test_lwt_ip_encap.c
> @@ -0,0 +1,125 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <linux/bpf.h>
> +#include <string.h>
> +#include "bpf_helpers.h"
> +#include "bpf_endian.h"
> +
> +#define BPF_LWT_ENCAP_IP 2
> +#define BPF_LWT_REROUTE 128
if you sync bpf.h header to tools/include/uapi/linux/bpf.h you should
not need to hard code those.
> +
> +struct iphdr {
> +#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> + __u8 ihl:4,
> + version:4;
> +#elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> + __u8 version:4,
> + ihl:4;
> +#else
> +#error "Fix your compiler's __BYTE_ORDER__?!"
> +#endif
> + __u8 tos;
> + __be16 tot_len;
> + __be16 id;
> + __be16 frag_off;
> + __u8 ttl;
> + __u8 protocol;
> + __sum16 check;
> + __be32 saddr;
> + __be32 daddr;
> +};
> +
> +struct ipv6hdr {
> +#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> + __u8 priority:4,
> + version:4;
> +#elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> + __u8 version:4,
> + priority:4;
> +#else
> +#error "Fix your compiler's __BYTE_ORDER__?!"
> +#endif
> + __u8 flow_lbl[3];
> +
> + __be16 payload_len;
> + __u8 nexthdr;
> + __u8 hop_limit;
> +
> + __u8 saddr[16];
> + __u8 daddr[16];
> +};
you should be using proper header files for iphdr and ipv6hdr
> +
> +struct grehdr {
> + __be16 flags;
> + __be16 protocol;
> +};
> +
> +SEC("encap_gre")
> +int bpf_lwt_encap_gre(struct __sk_buff *skb)
> +{
> + struct encap_hdr {
> + struct iphdr iph;
> + struct grehdr greh;
> + } hdr;
> + int err;
> +
> + memset(&hdr, 0, sizeof(struct encap_hdr));
> +
> + hdr.iph.ihl = 5;
> + hdr.iph.version = 4;
> + hdr.iph.ttl = 0x40;
> + hdr.iph.protocol = 47; /* IPPROTO_GRE */
> +#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> + hdr.iph.saddr = 0x640110ac; /* 172.16.1.100 */
> + hdr.iph.daddr = 0x641010ac; /* 172.16.16.100 */
> +#elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> + hdr.iph.saddr = 0xac100164; /* 172.16.1.100 */
> + hdr.iph.daddr = 0xac101064; /* 172.16.16.100 */
> +#else
> +#error "Fix your compiler's __BYTE_ORDER__?!"
> +#endif
> + hdr.iph.tot_len = bpf_htons(skb->len + sizeof(struct encap_hdr));
> +
> + hdr.greh.protocol = skb->protocol;
> +
> + err = bpf_lwt_push_encap(skb, BPF_LWT_ENCAP_IP, &hdr,
> + sizeof(struct encap_hdr));
> + if (err)
> + return BPF_DROP;
> + return BPF_LWT_REROUTE;
> +}
> +
> +SEC("encap_gre6")
> +int bpf_lwt_encap_gre6(struct __sk_buff *skb)
> +{
> + struct encap_hdr {
> + struct ipv6hdr ip6hdr;
> + struct grehdr greh;
> + } hdr;
> + int err;
> +
> + memset(&hdr, 0, sizeof(struct encap_hdr));
> +
> + hdr.ip6hdr.version = 6;
> + hdr.ip6hdr.payload_len = bpf_htons(skb->len + sizeof(struct grehdr));
> + hdr.ip6hdr.nexthdr = 47; /* IPPROTO_GRE */
> + hdr.ip6hdr.hop_limit = 0x40;
> + /* fb01::1 */
> + hdr.ip6hdr.saddr[0] = 0xfb;
> + hdr.ip6hdr.saddr[1] = 1;
> + hdr.ip6hdr.saddr[15] = 1;
> + /* fb10::1 */
> + hdr.ip6hdr.daddr[0] = 0xfb;
> + hdr.ip6hdr.daddr[1] = 0x10;
> + hdr.ip6hdr.daddr[15] = 1;
> +
> + hdr.greh.protocol = skb->protocol;
> +
> + err = bpf_lwt_push_encap(skb, BPF_LWT_ENCAP_IP, &hdr,
> + sizeof(struct encap_hdr));
> + if (err)
> + return BPF_DROP;
> +
> + return BPF_LWT_REROUTE;
> +}
> +
> +char _license[] SEC("license") = "GPL";
> diff --git a/tools/testing/selftests/bpf/test_lwt_ip_encap.sh b/tools/testing/selftests/bpf/test_lwt_ip_encap.sh
> new file mode 100755
> index 000000000000..4f511587bb74
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/test_lwt_ip_encap.sh
> @@ -0,0 +1,316 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Setup/topology:
> +#
> +# NS1 NS2 NS3
> +# veth1 <---> veth2 veth3 <---> veth4 (the top route)
> +# veth5 <---> veth6 veth7 <---> veth8 (the bottom route)
> +#
> +# each vethN gets IPv[4|6]_N address
> +#
> +# IPv*_SRC = IPv*_1
> +# IPv*_DST = IPv*_4
> +#
> +# all tests test pings from IPv*_SRC to IPv*_DST
> +#
> +# by default, routes are configured to allow packets to go
> +# IP*_1 <=> IP*_2 <=> IP*_3 <=> IP*_4 (the top route)
> +#
> +# a GRE device is installed in NS3 with IPv*_GRE, and
> +# NS1/NS2 are configured to route packets to IPv*_GRE via IP*_8
> +# (the bottom route)
> +#
> +# Tests:
> +#
> +# 1. routes NS2->IPv*_DST are brought down, so the only way a ping
> +# from IP*_SRC to IP*_DST can work is via IPv*_GRE
> +#
> +# 2a. in an egress test, a bpf LWT_XMIT program is installed on veth1
> +# that encaps the packets with an IP/GRE header to route to IPv*_GRE
> +#
> +# ping: SRC->[encap at veth1:egress]->GRE:decap->DST
> +# ping replies go DST->SRC directly
> +#
> +# 2b. in an ingress test, a bpf LWT_IN program is installed on veth2
> +# that encaps the packets with an IP/GRE header to route to IPv*_GRE
> +#
> +# ping: SRC->[encap at veth2:ingress]->GRE:decap->DST
> +# ping replies go DST->SRC directly
> +
> +set +x # debug OFF
please don't do that. Being able to run a shell script with bash -x is
really helpful.
> +set -e # exit on error
> +
> +if [[ $EUID -ne 0 ]]; then
> + echo "This script must be run as root"
> + echo "FAIL"
> + exit 1
> +fi
> +
> +readonly NS1="ns1-$(mktemp -u XXXXXX)"
> +readonly NS2="ns2-$(mktemp -u XXXXXX)"
> +readonly NS3="ns3-$(mktemp -u XXXXXX)"
> +
> +readonly IPv4_1="172.16.1.100"
> +readonly IPv4_2="172.16.2.100"
> +readonly IPv4_3="172.16.3.100"
> +readonly IPv4_4="172.16.4.100"
> +readonly IPv4_5="172.16.5.100"
> +readonly IPv4_6="172.16.6.100"
> +readonly IPv4_7="172.16.7.100"
> +readonly IPv4_8="172.16.8.100"
> +readonly IPv4_GRE="172.16.16.100"
> +
> +readonly IPv4_SRC=$IPv4_1
> +readonly IPv4_DST=$IPv4_4
> +
> +readonly IPv6_1="fb01::1"
> +readonly IPv6_2="fb02::1"
> +readonly IPv6_3="fb03::1"
> +readonly IPv6_4="fb04::1"
> +readonly IPv6_5="fb05::1"
> +readonly IPv6_6="fb06::1"
> +readonly IPv6_7="fb07::1"
> +readonly IPv6_8="fb08::1"
> +readonly IPv6_GRE="fb10::1"
> +
> +readonly IPv6_SRC=$IPv6_1
> +readonly IPv6_DST=$IPv6_4
> +
> +setup() {
> +set -e # exit on error
> + # create devices and namespaces
> + ip netns add "${NS1}"
> + ip netns add "${NS2}"
> + ip netns add "${NS3}"
> +
> + ip link add veth1 type veth peer name veth2
> + ip link add veth3 type veth peer name veth4
> + ip link add veth5 type veth peer name veth6
> + ip link add veth7 type veth peer name veth8
> +
> + ip netns exec ${NS2} sysctl -w net.ipv4.ip_forward=1 2>&1 > /dev/null
> + ip netns exec ${NS2} sysctl -w net.ipv6.conf.all.forwarding=1 2>&1 > /dev/null
-q on the sysctl will squash the output. Errors are worth seeing
> +
> + ip link set veth1 netns ${NS1}
> + ip link set veth2 netns ${NS2}
> + ip link set veth3 netns ${NS2}
> + ip link set veth4 netns ${NS3}
> + ip link set veth5 netns ${NS1}
> + ip link set veth6 netns ${NS2}
> + ip link set veth7 netns ${NS2}
> + ip link set veth8 netns ${NS3}
> +
> + # configure addesses: the top route (1-2-3-4)
> + ip -netns ${NS1} addr add ${IPv4_1}/24 dev veth1
> + ip -netns ${NS2} addr add ${IPv4_2}/24 dev veth2
> + ip -netns ${NS2} addr add ${IPv4_3}/24 dev veth3
> + ip -netns ${NS3} addr add ${IPv4_4}/24 dev veth4
> + ip -netns ${NS1} -6 addr add ${IPv6_1}/128 dev veth1
> + ip -netns ${NS2} -6 addr add ${IPv6_2}/128 dev veth2
> + ip -netns ${NS2} -6 addr add ${IPv6_3}/128 dev veth3
> + ip -netns ${NS3} -6 addr add ${IPv6_4}/128 dev veth4
> +
> + # configure addresses: the bottom route (5-6-7-8)
> + ip -netns ${NS1} addr add ${IPv4_5}/24 dev veth5
> + ip -netns ${NS2} addr add ${IPv4_6}/24 dev veth6
> + ip -netns ${NS2} addr add ${IPv4_7}/24 dev veth7
> + ip -netns ${NS3} addr add ${IPv4_8}/24 dev veth8
> + ip -netns ${NS1} -6 addr add ${IPv6_5}/128 dev veth5
> + ip -netns ${NS2} -6 addr add ${IPv6_6}/128 dev veth6
> + ip -netns ${NS2} -6 addr add ${IPv6_7}/128 dev veth7
> + ip -netns ${NS3} -6 addr add ${IPv6_8}/128 dev veth8
> +
> +
> + ip -netns ${NS1} link set dev veth1 up
> + ip -netns ${NS2} link set dev veth2 up
> + ip -netns ${NS2} link set dev veth3 up
> + ip -netns ${NS3} link set dev veth4 up
> + ip -netns ${NS1} link set dev veth5 up
> + ip -netns ${NS2} link set dev veth6 up
> + ip -netns ${NS2} link set dev veth7 up
> + ip -netns ${NS3} link set dev veth8 up
> +
> + # configure routes: IP*_SRC -> veth1/IP*_2 (= top route) default;
> + # the bottom route to specific bottom addresses
> +
> + # NS1
> + # top route
> + ip -netns ${NS1} route add ${IPv4_2}/32 dev veth1
> + ip -netns ${NS1} route add default dev veth1 via ${IPv4_2} # go top by default
> + ip -netns ${NS1} -6 route add ${IPv6_2}/128 dev veth1
> + ip -netns ${NS1} -6 route add default dev veth1 via ${IPv6_2} # go top by default
> + # bottom route
> + ip -netns ${NS1} route add ${IPv4_6}/32 dev veth5
> + ip -netns ${NS1} route add ${IPv4_7}/32 dev veth5 via ${IPv4_6}
> + ip -netns ${NS1} route add ${IPv4_8}/32 dev veth5 via ${IPv4_6}
> + ip -netns ${NS1} -6 route add ${IPv6_6}/128 dev veth5
> + ip -netns ${NS1} -6 route add ${IPv6_7}/128 dev veth5 via ${IPv6_6}
> + ip -netns ${NS1} -6 route add ${IPv6_8}/128 dev veth5 via ${IPv6_6}
> +
> + # NS2
> + # top route
> + ip -netns ${NS2} route add ${IPv4_1}/32 dev veth2
> + ip -netns ${NS2} route add ${IPv4_4}/32 dev veth3
> + ip -netns ${NS2} -6 route add ${IPv6_1}/128 dev veth2
> + ip -netns ${NS2} -6 route add ${IPv6_4}/128 dev veth3
> + # bottom route
> + ip -netns ${NS2} route add ${IPv4_5}/32 dev veth6
> + ip -netns ${NS2} route add ${IPv4_8}/32 dev veth7
> + ip -netns ${NS2} -6 route add ${IPv6_5}/128 dev veth6
> + ip -netns ${NS2} -6 route add ${IPv6_8}/128 dev veth7
> +
> + # NS3
> + # top route
> + ip -netns ${NS3} route add ${IPv4_3}/32 dev veth4
> + ip -netns ${NS3} route add ${IPv4_1}/32 dev veth4 via ${IPv4_3}
> + ip -netns ${NS3} route add ${IPv4_2}/32 dev veth4 via ${IPv4_3}
> + ip -netns ${NS3} -6 route add ${IPv6_3}/128 dev veth4
> + ip -netns ${NS3} -6 route add ${IPv6_1}/128 dev veth4 via ${IPv6_3}
> + ip -netns ${NS3} -6 route add ${IPv6_2}/128 dev veth4 via ${IPv6_3}
> + # bottom route
> + ip -netns ${NS3} route add ${IPv4_7}/32 dev veth8
> + ip -netns ${NS3} route add ${IPv4_5}/32 dev veth8 via ${IPv4_7}
> + ip -netns ${NS3} route add ${IPv4_6}/32 dev veth8 via ${IPv4_7}
> + ip -netns ${NS3} -6 route add ${IPv6_7}/128 dev veth8
> + ip -netns ${NS3} -6 route add ${IPv6_5}/128 dev veth8 via ${IPv6_7}
> + ip -netns ${NS3} -6 route add ${IPv6_6}/128 dev veth8 via ${IPv6_7}
> +
> + # configure IPv4 GRE device in NS3, and a route to it via the "bottom" route
> + ip -netns ${NS3} tunnel add gre_dev mode gre remote ${IPv4_1} local ${IPv4_GRE} ttl 255
> + ip -netns ${NS3} link set gre_dev up
> + ip -netns ${NS3} addr add ${IPv4_GRE} dev gre_dev
> + ip -netns ${NS1} route add ${IPv4_GRE}/32 dev veth5 via ${IPv4_6}
> + ip -netns ${NS2} route add ${IPv4_GRE}/32 dev veth7 via ${IPv4_8}
> +
> +
> + # configure IPv6 GRE device in NS3, and a route to it via the "bottom" route
> + ip -netns ${NS3} -6 tunnel add name gre6_dev mode ip6gre remote ${IPv6_1} local ${IPv6_GRE} ttl 255
> + ip -netns ${NS3} link set gre6_dev up
> + ip -netns ${NS3} -6 addr add ${IPv6_GRE} dev gre6_dev
> + ip -netns ${NS1} -6 route add ${IPv6_GRE}/128 dev veth5 via ${IPv6_6}
> + ip -netns ${NS2} -6 route add ${IPv6_GRE}/128 dev veth7 via ${IPv6_8}
> +
> + # rp_filter gets confused by what these tests are doing, so disable it
> + ip netns exec ${NS1} sysctl -w net.ipv4.conf.all.rp_filter=0 2>&1 > /dev/null
> + ip netns exec ${NS2} sysctl -w net.ipv4.conf.all.rp_filter=0 2>&1 > /dev/null
> + ip netns exec ${NS3} sysctl -w net.ipv4.conf.all.rp_filter=0 2>&1 > /dev/null
> +}
> +
> +cleanup() {
> + ip netns del ${NS1} 2> /dev/null
> + ip netns del ${NS2} 2> /dev/null
> + ip netns del ${NS3} 2> /dev/null
> +}
> +
> +trap cleanup EXIT
> +
> +test_ping() {
> + local readonly PROTO=$1
> + local readonly EXPECTED=$2
> + local RET=0
> +
> + set +e
> + if [ "${PROTO}" == "IPv4" ] ; then
> + ip netns exec ${NS1} ping -c 1 -W 1 -I ${IPv4_SRC} ${IPv4_DST} 2>&1 > /dev/null
> + # ip netns exec ${NS1} ping -c 1 -W 10 -I ${IPv4_SRC} ${IPv4_DST}
> + RET=$?
> + elif [ "${PROTO}" == "IPv6" ] ; then
> + ip netns exec ${NS1} ping6 -c 1 -W 6 -I ${IPv6_SRC} ${IPv6_DST} 2>&1 > /dev/null
> + # ip netns exec ${NS1} ping6 -c 1 -W 6 -I ${IPv6_SRC} ${IPv6_DST}
> + RET=$?
> + else
> + echo "test_ping: unknown PROTO: ${PROTO}"
> + exit 1
> + fi
> + set -e
> +
> + if [ "0" != "${RET}" ]; then
> + RET=1
> + fi
> +
> + if [ "${EXPECTED}" != "${RET}" ] ; then
> + echo "FAIL: test_ping: ${RET}"
> + exit 1
> + fi
> +}
> +
> +test_egress() {
> + local readonly ENCAP=$1
> + echo "starting egress ${ENCAP} encap test"
> + setup
> +
> + # need to wait a bit for IPv6 to autoconf, otherwise
> + # ping6 sometimes fails with "unable to bind to address"
> + sleep 1
My attempts to run this test script fail:
$ ./test_lwt_ip_encap.sh
starting egress IPv4 encap test
PASS
starting egress IPv6 encap test
ping: bind icmp socket: Cannot assign requested address
FAIL: test_ping: 1
Adding 'nodad' for IPv6 addresses avoids the race of DAD completing
before the ping6 is attempted. And it removes the need for the sleep 1.
> +
> + # by default, pings work
> + test_ping IPv4 0
> + test_ping IPv6 0
> +
> + # remove NS2->DST routes, ping fails
> + ip -netns ${NS2} route del ${IPv4_DST}/32 dev veth3
> + ip -netns ${NS2} -6 route del ${IPv6_DST}/128 dev veth3
> + test_ping IPv4 1
> + test_ping IPv6 1
> +
> + # install replacement routes (LWT/eBPF), pings succeed
> + if [ "${ENCAP}" == "IPv4" ] ; then
> + ip -netns ${NS1} route add ${IPv4_DST} encap bpf xmit obj test_lwt_ip_encap.o sec encap_gre dev veth1
> + ip -netns ${NS1} -6 route add ${IPv6_DST} encap bpf xmit obj test_lwt_ip_encap.o sec encap_gre dev veth1
> + elif [ "${ENCAP}" == "IPv6" ] ; then
> + ip -netns ${NS1} route add ${IPv4_DST} encap bpf xmit obj test_lwt_ip_encap.o sec encap_gre6 dev veth1
> + ip -netns ${NS1} -6 route add ${IPv6_DST} encap bpf xmit obj test_lwt_ip_encap.o sec encap_gre6 dev veth1
> + else
> + echo "FAIL: unknown encap ${ENCAP}"
> + fi
> + test_ping IPv4 0
> + test_ping IPv6 0
> +
> + cleanup
> + echo "PASS"
> +}
> +
> +test_ingress() {
> + local readonly ENCAP=$1
> + echo "starting ingress ${ENCAP} encap test"
> + setup
> +
> + # need to wait a bit for IPv6 to autoconf, otherwise
> + # ping6 sometimes fails with "unable to bind to address"
> + sleep 1
> +
> + # by default, pings work
> + test_ping IPv4 0
> + test_ping IPv6 0
> +
> + # remove NS2->DST routes, pings fail
> + ip -netns ${NS2} route del ${IPv4_DST}/32 dev veth3
> + ip -netns ${NS2} -6 route del ${IPv6_DST}/128 dev veth3
> + test_ping IPv4 1
> + test_ping IPv6 1
> +
> + # install replacement routes (LWT/eBPF), pings succeed
> + if [ "${ENCAP}" == "IPv4" ] ; then
> + ip -netns ${NS2} route add ${IPv4_DST} encap bpf in obj test_lwt_ip_encap.o sec encap_gre dev veth2
> + ip -netns ${NS2} -6 route add ${IPv6_DST} encap bpf in obj test_lwt_ip_encap.o sec encap_gre dev veth2
> + elif [ "${ENCAP}" == "IPv6" ] ; then
> + ip -netns ${NS2} route add ${IPv4_DST} encap bpf in obj test_lwt_ip_encap.o sec encap_gre6 dev veth2
> + ip -netns ${NS2} -6 route add ${IPv6_DST} encap bpf in obj test_lwt_ip_encap.o sec encap_gre6 dev veth2
> + else
> + echo "FAIL: unknown encap ${ENCAP}"
> + fi
> + test_ping IPv4 0
> + test_ping IPv6 0
> +
> + cleanup
> + echo "PASS"
> +}
> +
> +test_egress IPv4
> +test_egress IPv6
> +
> +test_ingress IPv4
> +test_ingress IPv6
> +
> +echo "all tests passed"
>
And adding a VRF version of this test fails as I suspected. See comments
on patch 2.
^ permalink raw reply
* Re: [PATCH bpf-next v2 2/3] bpf: implement BPF_LWT_ENCAP_IP mode in bpf_lwt_push_encap
From: David Ahern @ 2019-01-28 20:31 UTC (permalink / raw)
To: Peter Oskolkov, Alexei Starovoitov, Daniel Borkmann, netdev
Cc: Peter Oskolkov, Willem de Bruijn
In-Reply-To: <20190124193418.81674-3-posk@google.com>
On 1/24/19 12:34 PM, Peter Oskolkov wrote:
> This patch implements BPF_LWT_ENCAP_IP mode in bpf_lwt_push_encap
> BPF helper. It enables BPF programs (specifically, BPF_PROG_TYPE_LWT_IN
> and BPF_PROG_TYPE_LWT_XMIT prog types) to add IP encapsulation headers
> to packets (e.g. IP/GRE, GUE, IPIP).
>
> This is useful when thousands of different short-lived flows should be
> encapped, each with different and dynamically determined destination.
> Although lwtunnels can be used in some of these scenarios, the ability
> to dynamically generate encap headers adds more flexibility, e.g.
> when routing depends on the state of the host (reflected in global bpf
> maps).
>
> Signed-off-by: Peter Oskolkov <posk@google.com>
> ---
> include/net/lwtunnel.h | 3 +
> net/core/filter.c | 3 +-
> net/core/lwt_bpf.c | 142 +++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 147 insertions(+), 1 deletion(-)
>
> diff --git a/include/net/lwtunnel.h b/include/net/lwtunnel.h
> index 33fd9ba7e0e5..f0973eca8036 100644
> --- a/include/net/lwtunnel.h
> +++ b/include/net/lwtunnel.h
> @@ -126,6 +126,8 @@ int lwtunnel_cmp_encap(struct lwtunnel_state *a, struct lwtunnel_state *b);
> int lwtunnel_output(struct net *net, struct sock *sk, struct sk_buff *skb);
> int lwtunnel_input(struct sk_buff *skb);
> int lwtunnel_xmit(struct sk_buff *skb);
> +int bpf_lwt_push_ip_encap(struct sk_buff *skb, void *hdr, u32 len,
> + bool ingress);
>
> static inline void lwtunnel_set_redirect(struct dst_entry *dst)
> {
> @@ -138,6 +140,7 @@ static inline void lwtunnel_set_redirect(struct dst_entry *dst)
> dst->input = lwtunnel_input;
> }
> }
> +
> #else
>
> static inline void lwtstate_free(struct lwtunnel_state *lws)
> diff --git a/net/core/filter.c b/net/core/filter.c
> index fd3ae092d3d7..81d18660c38b 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -73,6 +73,7 @@
> #include <linux/seg6_local.h>
> #include <net/seg6.h>
> #include <net/seg6_local.h>
> +#include <net/lwtunnel.h>
>
> /**
> * sk_filter_trim_cap - run a packet through a socket filter
> @@ -4796,7 +4797,7 @@ static int bpf_push_seg6_encap(struct sk_buff *skb, u32 type, void *hdr, u32 len
> static int bpf_push_ip_encap(struct sk_buff *skb, void *hdr, u32 len,
> bool ingress)
> {
> - return -EINVAL; /* Implemented in the next patch. */
> + return bpf_lwt_push_ip_encap(skb, hdr, len, ingress);
> }
>
> BPF_CALL_4(bpf_lwt_in_push_encap, struct sk_buff *, skb, u32, type, void *, hdr,
> diff --git a/net/core/lwt_bpf.c b/net/core/lwt_bpf.c
> index 3e85437f7106..a3f79bff3776 100644
> --- a/net/core/lwt_bpf.c
> +++ b/net/core/lwt_bpf.c
> @@ -16,6 +16,7 @@
> #include <linux/types.h>
> #include <linux/bpf.h>
> #include <net/lwtunnel.h>
> +#include <net/ip6_route.h>
>
> struct bpf_lwt_prog {
> struct bpf_prog *prog;
> @@ -55,6 +56,7 @@ static int run_lwt_bpf(struct sk_buff *skb, struct bpf_lwt_prog *lwt,
>
> switch (ret) {
> case BPF_OK:
> + case BPF_LWT_REROUTE:
> break;
>
> case BPF_REDIRECT:
> @@ -97,6 +99,8 @@ static int bpf_input(struct sk_buff *skb)
> ret = run_lwt_bpf(skb, &bpf->in, dst, NO_REDIRECT);
> if (ret < 0)
> return ret;
> + if (ret == BPF_LWT_REROUTE)
> + return dst_input(skb);
> }
>
> if (unlikely(!dst->lwtstate->orig_input)) {
> @@ -168,6 +172,13 @@ static int bpf_xmit(struct sk_buff *skb)
> return LWTUNNEL_XMIT_CONTINUE;
> case BPF_REDIRECT:
> return LWTUNNEL_XMIT_DONE;
> + case BPF_LWT_REROUTE:
> + ret = dst_output(dev_net(skb_dst(skb)->dev),
> + skb->sk, skb);
> + if (unlikely(ret))
> + return ret;
> + /* ip[6]_finish_output2 understand LWTUNNEL_XMIT_DONE */
> + return LWTUNNEL_XMIT_DONE;
> default:
> return ret;
> }
> @@ -389,6 +400,137 @@ static const struct lwtunnel_encap_ops bpf_encap_ops = {
> .owner = THIS_MODULE,
> };
>
> +int bpf_lwt_push_ip_encap(struct sk_buff *skb, void *hdr, u32 len, bool ingress)
> +{
> + struct dst_entry *dst = NULL;
> + struct iphdr *iph;
> + bool ipv4;
> + int err;
> +
> + if (unlikely(len < sizeof(struct iphdr) || len > LWT_BPF_MAX_HEADROOM))
> + return -EINVAL;
> +
> + /* validate protocol and length */
> + iph = (struct iphdr *)hdr;
> + if (iph->version == 4) {
> + ipv4 = true;
> + if (iph->ihl * 4 > len)
> + return -EINVAL;
> + } else if (iph->version == 6) {
> + ipv4 = false;
> + if (unlikely(len < sizeof(struct ipv6hdr)))
> + return -EINVAL;
> + } else {
> + return -EINVAL;
> + }
> +
> + /* allocate enough space for the encap headers + L2 hdr */
> + if (ingress) {
> + err = skb_cow_head(skb, len + skb->mac_len);
> + if (unlikely(err))
> + return err;
> + } else {
> + /* ip_route_input_noref below does route lookup and dst
> + * drop/set for ingress. There is no similar function for
> + * egress, so we need to do route lookup and replace skb's
> + * dst in this function.
> + */
> + struct sock *sk;
> + struct net *net;
> +
> + sk = sk_to_full_sk(skb->sk);
> + if (sk)
> + net = sock_net(sk);
> + else
> + net = dev_net(skb_dst(skb)->dev);
This delta gets VRF tests to pass too. Also, you should be able to
always get net from the device.
diff --git a/net/core/lwt_bpf.c b/net/core/lwt_bpf.c
index 526b7cfc6d52..79feebd6da34 100644
--- a/net/core/lwt_bpf.c
+++ b/net/core/lwt_bpf.c
@@ -436,20 +436,24 @@ int bpf_lwt_push_ip_encap(struct sk_buff *skb,
void *hdr, u32 len, bool ingress)
* egress, so we need to do route lookup and replace skb's
* dst in this function.
*/
+ struct net_device *l3mdev =
l3mdev_master_dev_rcu(skb_dst(skb)->dev);
+ int oif = l3mdev ? l3mdev->ifindex : 0;
struct sock *sk;
struct net *net;
sk = sk_to_full_sk(skb->sk);
- if (sk)
+ if (sk) {
+ if (sk->sk_bound_dev_if)
+ oif = sk->sk_bound_dev_if;
net = sock_net(sk);
- else
+ } else
net = dev_net(skb_dst(skb)->dev);
if (ipv4) {
struct flowi4 fl4 = {0};
struct rtable *rt;
- fl4.flowi4_oif = sk ? sk->sk_bound_dev_if : 0;
+ fl4.flowi4_oif = oif;
fl4.flowi4_mark = skb->mark;
fl4.flowi4_uid = sock_net_uid(net, sk);
fl4.flowi4_tos = RT_TOS(iph->tos);
@@ -466,7 +470,7 @@ int bpf_lwt_push_ip_encap(struct sk_buff *skb, void
*hdr, u32 len, bool ingress)
struct ipv6hdr *iph6 = (struct ipv6hdr *)hdr;
struct flowi6 fl6 = {0};
- fl6.flowi6_oif = sk ? sk->sk_bound_dev_if : 0;
+ fl6.flowi6_oif = oif;
fl6.flowi6_mark = skb->mark;
fl6.flowi6_uid = sock_net_uid(net, sk);
fl6.flowlabel = ip6_flowinfo(iph6);
> +
> + if (ipv4) {
> + struct flowi4 fl4 = {0};
> + struct rtable *rt;
> +
> + fl4.flowi4_oif = sk ? sk->sk_bound_dev_if : 0;
> + fl4.flowi4_mark = skb->mark;
> + fl4.flowi4_uid = sock_net_uid(net, sk);
> + fl4.flowi4_tos = RT_TOS(iph->tos);
> + fl4.flowi4_flags = FLOWI_FLAG_ANYSRC;
> + fl4.flowi4_proto = iph->protocol;
> + fl4.daddr = iph->daddr;
> + fl4.saddr = iph->saddr;
> +
> + rt = ip_route_output_key(net, &fl4);
> + if (IS_ERR(rt) || rt->dst.error)
> + return -EINVAL;
> + dst = &rt->dst;
> + } else {
> + struct ipv6hdr *iph6 = (struct ipv6hdr *)hdr;
> + struct flowi6 fl6 = {0};
> +
> + fl6.flowi6_oif = sk ? sk->sk_bound_dev_if : 0;
> + fl6.flowi6_mark = skb->mark;
> + fl6.flowi6_uid = sock_net_uid(net, sk);
> + fl6.flowlabel = ip6_flowinfo(iph6);
> + fl6.flowi6_proto = iph6->nexthdr;
> + fl6.daddr = iph6->daddr;
> + fl6.saddr = iph6->saddr;
> +
> + dst = ip6_route_output(net, skb->sk, &fl6);
> + if (IS_ERR(dst) || dst->error)
> + return -EINVAL;
> + }
> +
> + err = skb_cow_head(skb, len + LL_RESERVED_SPACE(dst->dev));
> + if (unlikely(err))
> + return err;
> + }
> +
> + /* push the encap headers and fix pointers */
> + skb_reset_inner_headers(skb);
> + skb->encapsulation = 1;
> + skb_push(skb, len);
> + if (ingress)
> + skb_postpush_rcsum(skb, iph, len);
> + skb_reset_network_header(skb);
> + iph = ip_hdr(skb);
> + memcpy(iph, hdr, len);
Calling it iph and using ip_hdr seems wrong given that hdr can also be
IPv6. Why not just use skb_network_header?
> + bpf_compute_data_pointers(skb);
> +
> + /* final skb touches + routing */
> + if (ipv4) {
> + skb->protocol = htons(ETH_P_IP);
> + if (iph->ihl * 4 < len)
> + skb_set_transport_header(skb, iph->ihl * 4);
> +
> + if (!iph->check)
> + iph->check = ip_fast_csum((unsigned char *)iph,
> + iph->ihl);
> +
> + if (ingress) {
> + err = ip_route_input_noref(skb, iph->daddr, iph->saddr,
> + iph->tos, skb_dst(skb)->dev);
> + if (err)
> + return err;
> + } else {
> + skb_dst_drop(skb);
> + skb_dst_set(skb, dst);
> + }
> + } else {
> + skb->protocol = htons(ETH_P_IPV6);
> + if (sizeof(struct ipv6hdr) < len)
> + skb_set_transport_header(skb, sizeof(struct ipv6hdr));
> +
> + if (ingress) {
> + ip6_route_input(skb);
> + if (skb_dst(skb)->error)
> + return skb_dst(skb)->error;
> + } else {
> + skb_dst_drop(skb);
> + skb_dst_set(skb, dst);
> + }
> + }
> +
> + return 0;
> +}
> +
> static int __init bpf_lwt_init(void)
> {
> return lwtunnel_encap_add_ops(&bpf_encap_ops, LWTUNNEL_ENCAP_BPF);
>
^ permalink raw reply related
* Re: [PATCH net-next v1 1/2] net: dsa: mv88e6xxx: Default CMODE to 1000BaseX only on 6390X
From: Florian Fainelli @ 2019-01-28 20:34 UTC (permalink / raw)
To: Marek Behún, netdev; +Cc: Andrew Lunn, David Miller
In-Reply-To: <20190124154309.24987-1-marek.behun@nic.cz>
On 1/24/19 7:43 AM, Marek Behún wrote:
> Commit 787799a9d555 sets the SERDES interfaces of 6390 and 6390X to
> 1000BaseX, but this is only needed on 6390X, since there are SERDES
> interfaces which can be used on lower ports on 6390.
>
> This commit fixes this by returning to previous behaviour on 6390.
> (Previous behaviour means that CMODE is not set at all if requested mode
> is NA).
>
> This is needed on Turris MOX, where the 88e6190 is connected to CPU in
> 2500BaseX mode.
>
> Fixes: 787799a9d555 ("net: dsa: mv88e6xxx: Default ports 9/10 6390X CMODE to 1000BaseX")
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
I suppose for now, this is the best way to approach that problem given
the shortcomings of the fixed link support in net/dsa/port.c:
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Thanks!
--
Florian
^ permalink raw reply
* Re: [PATCH net] sk_msg: Always cancel strp work before freeing the psock
From: Song Liu @ 2019-01-28 20:35 UTC (permalink / raw)
To: Jakub Sitnicki
Cc: Networking, John Fastabend, Daniel Borkmann, Marek Majkowski
In-Reply-To: <20190128091335.20908-1-jakub@cloudflare.com>
On Mon, Jan 28, 2019 at 1:15 AM Jakub Sitnicki <jakub@cloudflare.com> wrote:
>
> Despite having stopped the parser, we still need to deinitialize it by
> calling strp_done so that it cancels its work. Otherwise the worker
> thread can run after we have freed the parser, and attempt to access its
> workqueue resulting in a use-after-free:
>
> ==================================================================
> BUG: KASAN: use-after-free in pwq_activate_delayed_work+0x1b/0x1d0
> Read of size 8 at addr ffff888069975240 by task kworker/u2:2/93
>
> CPU: 0 PID: 93 Comm: kworker/u2:2 Not tainted 5.0.0-rc2-00335-g28f9d1a3d4fe-dirty #14
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-2.fc27 04/01/2014
> Workqueue: (null) (kstrp)
> Call Trace:
> print_address_description+0x6e/0x2b0
> ? pwq_activate_delayed_work+0x1b/0x1d0
> kasan_report+0xfd/0x177
> ? pwq_activate_delayed_work+0x1b/0x1d0
> ? pwq_activate_delayed_work+0x1b/0x1d0
> pwq_activate_delayed_work+0x1b/0x1d0
> ? process_one_work+0x4aa/0x660
> pwq_dec_nr_in_flight+0x9b/0x100
> worker_thread+0x82/0x680
> ? process_one_work+0x660/0x660
> kthread+0x1b9/0x1e0
> ? __kthread_create_on_node+0x250/0x250
> ret_from_fork+0x1f/0x30
>
> Allocated by task 111:
> sk_psock_init+0x3c/0x1b0
> sock_map_link.isra.2+0x103/0x4b0
> sock_map_update_common+0x94/0x270
> sock_map_update_elem+0x145/0x160
> __se_sys_bpf+0x152e/0x1e10
> do_syscall_64+0xb2/0x3e0
> entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> Freed by task 112:
> kfree+0x7f/0x140
> process_one_work+0x40b/0x660
> worker_thread+0x82/0x680
> kthread+0x1b9/0x1e0
> ret_from_fork+0x1f/0x30
>
> The buggy address belongs to the object at ffff888069975180
> which belongs to the cache kmalloc-512 of size 512
> The buggy address is located 192 bytes inside of
> 512-byte region [ffff888069975180, ffff888069975380)
> The buggy address belongs to the page:
> page:ffffea0001a65d00 count:1 mapcount:0 mapping:ffff88806d401280 index:0x0 compound_mapcount: 0
> flags: 0x4000000000010200(slab|head)
> raw: 4000000000010200 dead000000000100 dead000000000200 ffff88806d401280
> raw: 0000000000000000 00000000800c000c 00000001ffffffff 0000000000000000
> page dumped because: kasan: bad access detected
>
> Memory state around the buggy address:
> ffff888069975100: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> ffff888069975180: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> >ffff888069975200: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ^
> ffff888069975280: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ffff888069975300: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ==================================================================
>
> Reported-by: Marek Majkowski <marek@cloudflare.com>
> Link: https://lore.kernel.org/netdev/CAJPywTLwgXNEZ2dZVoa=udiZmtrWJ0q5SuBW64aYs0Y1khXX3A@mail.gmail.com
> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
Acked-by: Song Liu <songliubraving@fb.com>
> ---
> net/core/skmsg.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/net/core/skmsg.c b/net/core/skmsg.c
> index d6d5c20d7044..8c826603bf36 100644
> --- a/net/core/skmsg.c
> +++ b/net/core/skmsg.c
> @@ -545,8 +545,7 @@ static void sk_psock_destroy_deferred(struct work_struct *gc)
> struct sk_psock *psock = container_of(gc, struct sk_psock, gc);
>
> /* No sk_callback_lock since already detached. */
> - if (psock->parser.enabled)
> - strp_done(&psock->parser.strp);
> + strp_done(&psock->parser.strp);
>
> cancel_work_sync(&psock->work);
>
> --
> 2.17.2
>
^ permalink raw reply
* Re: [PATCH bpf-next v3 2/3] bpf: add BPF_PROG_TEST_RUN support for flow dissector
From: Song Liu @ 2019-01-28 20:37 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Networking, David S . Miller, Alexei Starovoitov, Daniel Borkmann
In-Reply-To: <20190128165355.229403-3-sdf@google.com>
On Mon, Jan 28, 2019 at 8:55 AM Stanislav Fomichev <sdf@google.com> wrote:
>
> The input is packet data, the output is struct bpf_flow_key. This should
> make it easy to test flow dissector programs without elaborate
> setup.
>
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
Acked-by: Song Liu <songliubraving@fb.com>
> ---
> include/linux/bpf.h | 3 ++
> net/bpf/test_run.c | 82 +++++++++++++++++++++++++++++++++++++++++++++
> net/core/filter.c | 1 +
> 3 files changed, 86 insertions(+)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 3851529062ec..0394f1f9213b 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -404,6 +404,9 @@ int bpf_prog_test_run_xdp(struct bpf_prog *prog, const union bpf_attr *kattr,
> union bpf_attr __user *uattr);
> int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
> union bpf_attr __user *uattr);
> +int bpf_prog_test_run_flow_dissector(struct bpf_prog *prog,
> + const union bpf_attr *kattr,
> + union bpf_attr __user *uattr);
>
> /* an array of programs to be executed under rcu_lock.
> *
> diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
> index fa2644d276ef..2c5172b33209 100644
> --- a/net/bpf/test_run.c
> +++ b/net/bpf/test_run.c
> @@ -240,3 +240,85 @@ int bpf_prog_test_run_xdp(struct bpf_prog *prog, const union bpf_attr *kattr,
> kfree(data);
> return ret;
> }
> +
> +int bpf_prog_test_run_flow_dissector(struct bpf_prog *prog,
> + const union bpf_attr *kattr,
> + union bpf_attr __user *uattr)
> +{
> + u32 size = kattr->test.data_size_in;
> + u32 repeat = kattr->test.repeat;
> + struct bpf_flow_keys flow_keys;
> + u64 time_start, time_spent = 0;
> + struct bpf_skb_data_end *cb;
> + u32 retval, duration;
> + struct sk_buff *skb;
> + struct sock *sk;
> + void *data;
> + int ret;
> + u32 i;
> +
> + if (prog->type != BPF_PROG_TYPE_FLOW_DISSECTOR)
> + return -EINVAL;
> +
> + data = bpf_test_init(kattr, size, NET_SKB_PAD + NET_IP_ALIGN,
> + SKB_DATA_ALIGN(sizeof(struct skb_shared_info)));
> + if (IS_ERR(data))
> + return PTR_ERR(data);
> +
> + sk = kzalloc(sizeof(*sk), GFP_USER);
> + if (!sk) {
> + kfree(data);
> + return -ENOMEM;
> + }
> + sock_net_set(sk, current->nsproxy->net_ns);
> + sock_init_data(NULL, sk);
> +
> + skb = build_skb(data, 0);
> + if (!skb) {
> + kfree(data);
> + kfree(sk);
> + return -ENOMEM;
> + }
> + skb->sk = sk;
> +
> + skb_reserve(skb, NET_SKB_PAD + NET_IP_ALIGN);
> + __skb_put(skb, size);
> + skb->protocol = eth_type_trans(skb,
> + current->nsproxy->net_ns->loopback_dev);
> + skb_reset_network_header(skb);
> +
> + cb = (struct bpf_skb_data_end *)skb->cb;
> + cb->qdisc_cb.flow_keys = &flow_keys;
> +
> + if (!repeat)
> + repeat = 1;
> +
> + time_start = ktime_get_ns();
> + for (i = 0; i < repeat; i++) {
> + preempt_disable();
> + rcu_read_lock();
> + retval = __skb_flow_bpf_dissect(prog, skb,
> + &flow_keys_dissector,
> + &flow_keys);
> + rcu_read_unlock();
> + preempt_enable();
> +
> + if (need_resched()) {
> + if (signal_pending(current))
> + break;
> + time_spent += ktime_get_ns() - time_start;
> + cond_resched();
> + time_start = ktime_get_ns();
> + }
> + }
> + time_spent += ktime_get_ns() - time_start;
> + do_div(time_spent, repeat);
> + duration = time_spent > U32_MAX ? U32_MAX : (u32)time_spent;
> +
> + ret = bpf_test_finish(kattr, uattr, &flow_keys, sizeof(flow_keys),
> + retval, duration);
> +
> + kfree_skb(skb);
> + kfree(sk);
> + return ret;
> +}
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 8e587dd1da20..8ce421796ac6 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -7711,6 +7711,7 @@ const struct bpf_verifier_ops flow_dissector_verifier_ops = {
> };
>
> const struct bpf_prog_ops flow_dissector_prog_ops = {
> + .test_run = bpf_prog_test_run_flow_dissector,
> };
>
> int sk_detach_filter(struct sock *sk)
> --
> 2.20.1.495.gaa96b0ce6b-goog
>
^ permalink raw reply
* Re: [PATCH bpf-next v3 3/3] selftests/bpf: add simple BPF_PROG_TEST_RUN examples for flow dissector
From: Song Liu @ 2019-01-28 20:37 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Networking, David S . Miller, Alexei Starovoitov, Daniel Borkmann
In-Reply-To: <20190128165355.229403-4-sdf@google.com>
On Mon, Jan 28, 2019 at 8:55 AM Stanislav Fomichev <sdf@google.com> wrote:
>
> Use existing pkt_v4 and pkt_v6 to make sure flow_keys are what we want.
>
> Also, add new bpf_flow_load routine (and flow_dissector_load.h header)
> that loads bpf_flow.o program and does all required setup.
>
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
Acked-by: Song Liu <songliubraving@fb.com>
> ---
> tools/testing/selftests/bpf/Makefile | 3 +
> .../selftests/bpf/flow_dissector_load.c | 43 ++--------
> .../selftests/bpf/flow_dissector_load.h | 55 +++++++++++++
> tools/testing/selftests/bpf/test_progs.c | 78 ++++++++++++++++++-
> 4 files changed, 139 insertions(+), 40 deletions(-)
> create mode 100644 tools/testing/selftests/bpf/flow_dissector_load.h
>
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index 89b0d1799ff3..c566090e5657 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -148,6 +148,9 @@ $(OUTPUT)/test_xdp_noinline.o: CLANG_FLAGS += -fno-inline
> $(OUTPUT)/test_queue_map.o: test_queue_stack_map.h
> $(OUTPUT)/test_stack_map.o: test_queue_stack_map.h
>
> +$(OUTPUT)/flow_dissector_load.o: flow_dissector_load.h
> +$(OUTPUT)/test_progs.o: flow_dissector_load.h
> +
> BTF_LLC_PROBE := $(shell $(LLC) -march=bpf -mattr=help 2>&1 | grep dwarfris)
> BTF_PAHOLE_PROBE := $(shell $(BTF_PAHOLE) --help 2>&1 | grep BTF)
> BTF_OBJCOPY_PROBE := $(shell $(LLVM_OBJCOPY) --help 2>&1 | grep -i 'usage.*llvm')
> diff --git a/tools/testing/selftests/bpf/flow_dissector_load.c b/tools/testing/selftests/bpf/flow_dissector_load.c
> index ae8180b11d5f..77cafa66d048 100644
> --- a/tools/testing/selftests/bpf/flow_dissector_load.c
> +++ b/tools/testing/selftests/bpf/flow_dissector_load.c
> @@ -12,6 +12,7 @@
> #include <bpf/libbpf.h>
>
> #include "bpf_rlimit.h"
> +#include "flow_dissector_load.h"
>
> const char *cfg_pin_path = "/sys/fs/bpf/flow_dissector";
> const char *cfg_map_name = "jmp_table";
> @@ -21,46 +22,13 @@ char *cfg_path_name;
>
> static void load_and_attach_program(void)
> {
> - struct bpf_program *prog, *main_prog;
> - struct bpf_map *prog_array;
> - int i, fd, prog_fd, ret;
> + int prog_fd, ret;
> struct bpf_object *obj;
> - int prog_array_fd;
>
> - ret = bpf_prog_load(cfg_path_name, BPF_PROG_TYPE_FLOW_DISSECTOR, &obj,
> - &prog_fd);
> + ret = bpf_flow_load(&obj, cfg_path_name, cfg_section_name,
> + cfg_map_name, &prog_fd);
> if (ret)
> - error(1, 0, "bpf_prog_load %s", cfg_path_name);
> -
> - main_prog = bpf_object__find_program_by_title(obj, cfg_section_name);
> - if (!main_prog)
> - error(1, 0, "bpf_object__find_program_by_title %s",
> - cfg_section_name);
> -
> - prog_fd = bpf_program__fd(main_prog);
> - if (prog_fd < 0)
> - error(1, 0, "bpf_program__fd");
> -
> - prog_array = bpf_object__find_map_by_name(obj, cfg_map_name);
> - if (!prog_array)
> - error(1, 0, "bpf_object__find_map_by_name %s", cfg_map_name);
> -
> - prog_array_fd = bpf_map__fd(prog_array);
> - if (prog_array_fd < 0)
> - error(1, 0, "bpf_map__fd %s", cfg_map_name);
> -
> - i = 0;
> - bpf_object__for_each_program(prog, obj) {
> - fd = bpf_program__fd(prog);
> - if (fd < 0)
> - error(1, 0, "bpf_program__fd");
> -
> - if (fd != prog_fd) {
> - printf("%d: %s\n", i, bpf_program__title(prog, false));
> - bpf_map_update_elem(prog_array_fd, &i, &fd, BPF_ANY);
> - ++i;
> - }
> - }
> + error(1, 0, "bpf_flow_load %s", cfg_path_name);
>
> ret = bpf_prog_attach(prog_fd, 0 /* Ignore */, BPF_FLOW_DISSECTOR, 0);
> if (ret)
> @@ -69,7 +37,6 @@ static void load_and_attach_program(void)
> ret = bpf_object__pin(obj, cfg_pin_path);
> if (ret)
> error(1, 0, "bpf_object__pin %s", cfg_pin_path);
> -
> }
>
> static void detach_program(void)
> diff --git a/tools/testing/selftests/bpf/flow_dissector_load.h b/tools/testing/selftests/bpf/flow_dissector_load.h
> new file mode 100644
> index 000000000000..41dd6959feb0
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/flow_dissector_load.h
> @@ -0,0 +1,55 @@
> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> +#ifndef FLOW_DISSECTOR_LOAD
> +#define FLOW_DISSECTOR_LOAD
> +
> +#include <bpf/bpf.h>
> +#include <bpf/libbpf.h>
> +
> +static inline int bpf_flow_load(struct bpf_object **obj,
> + const char *path,
> + const char *section_name,
> + const char *map_name,
> + int *prog_fd)
> +{
> + struct bpf_program *prog, *main_prog;
> + struct bpf_map *prog_array;
> + int prog_array_fd;
> + int ret, fd, i;
> +
> + ret = bpf_prog_load(path, BPF_PROG_TYPE_FLOW_DISSECTOR, obj,
> + prog_fd);
> + if (ret)
> + return ret;
> +
> + main_prog = bpf_object__find_program_by_title(*obj, section_name);
> + if (!main_prog)
> + return ret;
> +
> + *prog_fd = bpf_program__fd(main_prog);
> + if (*prog_fd < 0)
> + return ret;
> +
> + prog_array = bpf_object__find_map_by_name(*obj, map_name);
> + if (!prog_array)
> + return ret;
> +
> + prog_array_fd = bpf_map__fd(prog_array);
> + if (prog_array_fd < 0)
> + return ret;
> +
> + i = 0;
> + bpf_object__for_each_program(prog, *obj) {
> + fd = bpf_program__fd(prog);
> + if (fd < 0)
> + return fd;
> +
> + if (fd != *prog_fd) {
> + bpf_map_update_elem(prog_array_fd, &i, &fd, BPF_ANY);
> + ++i;
> + }
> + }
> +
> + return 0;
> +}
> +
> +#endif /* FLOW_DISSECTOR_LOAD */
> diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> index 126fc624290d..5f46680d4ad4 100644
> --- a/tools/testing/selftests/bpf/test_progs.c
> +++ b/tools/testing/selftests/bpf/test_progs.c
> @@ -39,6 +39,7 @@ typedef __u16 __sum16;
> #include "bpf_endian.h"
> #include "bpf_rlimit.h"
> #include "trace_helpers.h"
> +#include "flow_dissector_load.h"
>
> static int error_cnt, pass_cnt;
> static bool jit_enabled;
> @@ -53,9 +54,10 @@ static struct {
> } __packed pkt_v4 = {
> .eth.h_proto = __bpf_constant_htons(ETH_P_IP),
> .iph.ihl = 5,
> - .iph.protocol = 6,
> + .iph.protocol = IPPROTO_TCP,
> .iph.tot_len = __bpf_constant_htons(MAGIC_BYTES),
> .tcp.urg_ptr = 123,
> + .tcp.doff = 5,
> };
>
> /* ipv6 test vector */
> @@ -65,9 +67,10 @@ static struct {
> struct tcphdr tcp;
> } __packed pkt_v6 = {
> .eth.h_proto = __bpf_constant_htons(ETH_P_IPV6),
> - .iph.nexthdr = 6,
> + .iph.nexthdr = IPPROTO_TCP,
> .iph.payload_len = __bpf_constant_htons(MAGIC_BYTES),
> .tcp.urg_ptr = 123,
> + .tcp.doff = 5,
> };
>
> #define _CHECK(condition, tag, duration, format...) ({ \
> @@ -1882,6 +1885,76 @@ static void test_queue_stack_map(int type)
> bpf_object__close(obj);
> }
>
> +#define CHECK_FLOW_KEYS(desc, got, expected) \
> + CHECK(memcmp(&got, &expected, sizeof(got)) != 0, \
> + desc, \
> + "nhoff=%u/%u " \
> + "thoff=%u/%u " \
> + "addr_proto=0x%x/0x%x " \
> + "is_frag=%u/%u " \
> + "is_first_frag=%u/%u " \
> + "is_encap=%u/%u " \
> + "n_proto=0x%x/0x%x " \
> + "sport=%u/%u " \
> + "dport=%u/%u\n", \
> + got.nhoff, expected.nhoff, \
> + got.thoff, expected.thoff, \
> + got.addr_proto, expected.addr_proto, \
> + got.is_frag, expected.is_frag, \
> + got.is_first_frag, expected.is_first_frag, \
> + got.is_encap, expected.is_encap, \
> + got.n_proto, expected.n_proto, \
> + got.sport, expected.sport, \
> + got.dport, expected.dport)
> +
> +static struct bpf_flow_keys pkt_v4_flow_keys = {
> + .nhoff = 0,
> + .thoff = sizeof(struct iphdr),
> + .addr_proto = ETH_P_IP,
> + .ip_proto = IPPROTO_TCP,
> + .n_proto = bpf_htons(ETH_P_IP),
> +};
> +
> +static struct bpf_flow_keys pkt_v6_flow_keys = {
> + .nhoff = 0,
> + .thoff = sizeof(struct ipv6hdr),
> + .addr_proto = ETH_P_IPV6,
> + .ip_proto = IPPROTO_TCP,
> + .n_proto = bpf_htons(ETH_P_IPV6),
> +};
> +
> +static void test_flow_dissector(void)
> +{
> + struct bpf_flow_keys flow_keys;
> + struct bpf_object *obj;
> + __u32 duration, retval;
> + int err, prog_fd;
> + __u32 size;
> +
> + err = bpf_flow_load(&obj, "./bpf_flow.o", "flow_dissector",
> + "jmp_table", &prog_fd);
> + if (err) {
> + error_cnt++;
> + return;
> + }
> +
> + err = bpf_prog_test_run(prog_fd, 10, &pkt_v4, sizeof(pkt_v4),
> + &flow_keys, &size, &retval, &duration);
> + CHECK(size != sizeof(flow_keys) || err || retval != 1, "ipv4",
> + "err %d errno %d retval %d duration %d size %u/%lu\n",
> + err, errno, retval, duration, size, sizeof(flow_keys));
> + CHECK_FLOW_KEYS("ipv4_flow_keys", flow_keys, pkt_v4_flow_keys);
> +
> + err = bpf_prog_test_run(prog_fd, 10, &pkt_v6, sizeof(pkt_v6),
> + &flow_keys, &size, &retval, &duration);
> + CHECK(size != sizeof(flow_keys) || err || retval != 1, "ipv6",
> + "err %d errno %d retval %d duration %d size %u/%lu\n",
> + err, errno, retval, duration, size, sizeof(flow_keys));
> + CHECK_FLOW_KEYS("ipv6_flow_keys", flow_keys, pkt_v6_flow_keys);
> +
> + bpf_object__close(obj);
> +}
> +
> int main(void)
> {
> srand(time(NULL));
> @@ -1909,6 +1982,7 @@ int main(void)
> test_reference_tracking();
> test_queue_stack_map(QUEUE);
> test_queue_stack_map(STACK);
> + test_flow_dissector();
>
> printf("Summary: %d PASSED, %d FAILED\n", pass_cnt, error_cnt);
> return error_cnt ? EXIT_FAILURE : EXIT_SUCCESS;
> --
> 2.20.1.495.gaa96b0ce6b-goog
>
^ permalink raw reply
* Re: [PATCH bpf] tools: bpftool: fix crash with un-owned prog arrays
From: Song Liu @ 2019-01-28 20:42 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Alexei Starovoitov, Daniel Borkmann, oss-drivers, Networking
In-Reply-To: <20190128180121.31362-1-jakub.kicinski@netronome.com>
On Mon, Jan 28, 2019 at 10:06 AM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> Prog arrays don't have 'owner_prog_type' and 'owner_jited'
> fields in their fdinfo when they are created. Those fields
> are set and reported when first program is checked for
> compatibility by bpf_prog_array_compatible().
>
> This means that bpftool cannot expect the fields to always
> be there. Currently trying to show maps on a system with
> an un-owned prog array leads to a crash:
>
> $ bpftool map show
> 389: prog_array name tail_call_map flags 0x0
> Error: key 'owner_prog_type' not found in fdinfo
> Error: key 'owner_jited' not found in fdinfo
> key 4B value 4B max_entries 4 memlock 4096B
> Segmentation fault (core dumped)
>
> We pass a NULL pointer to atoi().
>
> Remove the assumption that fdinfo keys are always present.
> Add missing validations and remove the p_err() calls which
> may lead to broken JSON output as caller will not propagate
> the failure.
>
> Fixes: 99a44bef5870 ("tools: bpftool: add owner_prog_type and owner_jited to bpftool output")
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
Acked-by: Song Liu <songliubraving@fb.com>
> ---
> tools/bpf/bpftool/common.c | 6 +-----
> tools/bpf/bpftool/map.c | 17 ++++++++---------
> 2 files changed, 9 insertions(+), 14 deletions(-)
>
> diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
> index 897483457bf0..f7261fad45c1 100644
> --- a/tools/bpf/bpftool/common.c
> +++ b/tools/bpf/bpftool/common.c
> @@ -297,10 +297,8 @@ char *get_fdinfo(int fd, const char *key)
> snprintf(path, sizeof(path), "/proc/self/fdinfo/%d", fd);
>
> fdi = fopen(path, "r");
> - if (!fdi) {
> - p_err("can't open fdinfo: %s", strerror(errno));
> + if (!fdi)
> return NULL;
> - }
>
> while ((n = getline(&line, &line_n, fdi)) > 0) {
> char *value;
> @@ -313,7 +311,6 @@ char *get_fdinfo(int fd, const char *key)
>
> value = strchr(line, '\t');
> if (!value || !value[1]) {
> - p_err("malformed fdinfo!?");
> free(line);
> return NULL;
> }
> @@ -326,7 +323,6 @@ char *get_fdinfo(int fd, const char *key)
> return line;
> }
>
> - p_err("key '%s' not found in fdinfo", key);
> free(line);
> fclose(fdi);
> return NULL;
> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> index 29a3468c6cf6..1ef1ee2280a2 100644
> --- a/tools/bpf/bpftool/map.c
> +++ b/tools/bpf/bpftool/map.c
> @@ -513,10 +513,9 @@ static int show_map_close_json(int fd, struct bpf_map_info *info)
> jsonw_uint_field(json_wtr, "owner_prog_type",
> prog_type);
> }
> - if (atoi(owner_jited))
> - jsonw_bool_field(json_wtr, "owner_jited", true);
> - else
> - jsonw_bool_field(json_wtr, "owner_jited", false);
> + if (owner_jited)
> + jsonw_bool_field(json_wtr, "owner_jited",
> + !!atoi(owner_jited));
>
> free(owner_prog_type);
> free(owner_jited);
> @@ -569,7 +568,8 @@ static int show_map_close_plain(int fd, struct bpf_map_info *info)
> char *owner_prog_type = get_fdinfo(fd, "owner_prog_type");
> char *owner_jited = get_fdinfo(fd, "owner_jited");
>
> - printf("\n\t");
> + if (owner_prog_type || owner_jited)
> + printf("\n\t");
> if (owner_prog_type) {
> unsigned int prog_type = atoi(owner_prog_type);
>
> @@ -579,10 +579,9 @@ static int show_map_close_plain(int fd, struct bpf_map_info *info)
> else
> printf("owner_prog_type %d ", prog_type);
> }
> - if (atoi(owner_jited))
> - printf("owner jited");
> - else
> - printf("owner not jited");
> + if (owner_jited)
> + printf("owner%s jited",
> + atoi(owner_jited) ? "" : " not");
>
> free(owner_prog_type);
> free(owner_jited);
> --
> 2.19.2
>
^ permalink raw reply
* Re: [PATCH bpf-next] tools: bpftool: warn about risky prog array updates
From: Song Liu @ 2019-01-28 20:45 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Alexei Starovoitov, Daniel Borkmann, oss-drivers, Networking
In-Reply-To: <20190128182915.434-1-jakub.kicinski@netronome.com>
On Mon, Jan 28, 2019 at 10:30 AM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> When prog array is updated with bpftool users often refer
> to the map via the ID. Unfortunately, that's likely
> to lead to confusion because prog arrays get flushed when
> the last user reference is gone. If there is no other
> reference bpftool will create one, update successfully
> just to close the map again and have it flushed.
>
> Warn about this case in non-JSON mode.
>
> If the problem continues causing confusion we can remove
> the support for referring to a map by ID for prog array
> update completely. For now it seems like the potential
> inconvenience to users who know what they're doing outweighs
> the benefit.
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
Acked-by: Song Liu <songliubraving@fb.com>
> ---
> tools/bpf/bpftool/map.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> index 8cb0e26907ff..6f33818bb6b6 100644
> --- a/tools/bpf/bpftool/map.c
> +++ b/tools/bpf/bpftool/map.c
> @@ -426,6 +426,9 @@ static int parse_elem(char **argv, struct bpf_map_info *info,
> p_err("not enough value arguments for map of progs");
> return -1;
> }
> + if (is_prefix(*argv, "id"))
> + p_info("Warning: updating program array via MAP_ID, make sure this map is kept open\n"
> + " by some process or pinned otherwise update will be lost");
>
> fd = prog_parse_fd(&argc, &argv);
> if (fd < 0)
> --
> 2.19.2
>
^ permalink raw reply
* Re: [PATCH v4 bpf-next 1/9] bpf: introduce bpf_spin_lock
From: Alexei Starovoitov @ 2019-01-28 20:49 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Alexei Starovoitov, davem, daniel, jakub.kicinski, netdev,
kernel-team, mingo, will.deacon, Paul McKenney, jannh
In-Reply-To: <20190128083508.GA28878@hirez.programming.kicks-ass.net>
On Mon, Jan 28, 2019 at 09:35:08AM +0100, Peter Zijlstra wrote:
> On Mon, Jan 28, 2019 at 09:31:23AM +0100, Peter Zijlstra wrote:
> > On Fri, Jan 25, 2019 at 03:42:43PM -0800, Alexei Starovoitov wrote:
> > > On Fri, Jan 25, 2019 at 10:10:57AM +0100, Peter Zijlstra wrote:
> > > > On Thu, Jan 24, 2019 at 03:58:59PM -0800, Alexei Starovoitov wrote:
> >
> > > > > nmi checks for bpf_prog_active==0. See bpf_overflow_handler.
> >
> > > > yuck yuck yuck.. That's horrific :-( That means the whole BPF crud is
> > > > unreliable and events can go randomly missing.
> > >
> > > bpf_prog_active is the mechanism to workaround non-reentrant pieces of the kernel.
> >
> > 'the kernel' or 'bpf' ?
> >
> > perf has a recursion counter per context (task,softirq,hardirq,nmi) and
> > that ensures that perf doesn't recurse in on itself while allowing the
> > nesting of these contexts.
> >
> > But if BPF itself is not able to deal with such nesting that won't work
> > of course.
>
> Ooh, later you say:
>
> > Also we allow tracing progs to nest with networking progs.
>
> Which seems to suggest BPF itself can suppord (limited) nesting.
well I'm not sure where is the boundary between bpf and the kernel :)
By non-reentrant I meant pieces of the kernel that bpf maps
or bpf helpers may use.
For example kmalloc-style bpf map is using rcu and map update/delete
do call_rcu() from inside bpf program.
If kprobe or tracepoint bpf prog is called from inner bits of
__call_rcu_core and the prog is doing map update we get into
recursive __call_rcu_core and things go bad.
Hence we disallow such situations when possible.
If bpf had its own infrastructure for everything then it would be
easy enough to allow proper recursion using mechanism similar
to what perf does with get_recursion_context.
^ permalink raw reply
* Re: [PATCH bpf-next v4 1/7] libbpf: Add a helper for retrieving a map fd for a given name
From: John Fastabend @ 2019-01-28 20:49 UTC (permalink / raw)
To: Maciej Fijalkowski, daniel, ast; +Cc: netdev, jakub.kicinski, brouer
In-Reply-To: <20190128191613.11705-2-maciejromanfijalkowski@gmail.com>
On 1/28/19 11:16 AM, Maciej Fijalkowski wrote:
> From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
>
> XDP samples are mostly cooperating with eBPF maps through their file
> descriptors. In case of a eBPF program that contains multiple maps it
> might be tiresome to iterate through them and call bpf_map__fd for each
> one. Add a helper mostly based on bpf_object__find_map_by_name, but
> instead of returning the struct bpf_map pointer, return map fd.
>
> Suggested-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> ---
I've been carrying something similar around for awhile as well.
Acked-by: John Fastabend <john.fastabend@gmail.com>
^ permalink raw reply
* Re: [PATCH RFC v2 3/3] udp: Support UDP fraglist GRO/GSO.
From: Willem de Bruijn @ 2019-01-28 20:49 UTC (permalink / raw)
To: Steffen Klassert
Cc: Network Development, Willem de Bruijn, Paolo Abeni,
Jason A. Donenfeld
In-Reply-To: <20190128085025.14532-4-steffen.klassert@secunet.com>
On Mon, Jan 28, 2019 at 2:51 AM Steffen Klassert
<steffen.klassert@secunet.com> wrote:
>
> This patch extends UDP GRO to support fraglist GRO/GSO
> by using the previously introduced infrastructure.
> All UDP packets that are not targeted to a GRO capable
> UDP sockets are going to fraglist GRO now (local input
> and forward).
> ---
> net/ipv4/udp_offload.c | 45 ++++++++++++++++++++++++++++++++++++++----
> net/ipv6/udp_offload.c | 9 +++++++++
> 2 files changed, 50 insertions(+), 4 deletions(-)
>
> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> index 584635db9231..c0be33216750 100644
> --- a/net/ipv4/udp_offload.c
> +++ b/net/ipv4/udp_offload.c
> @@ -188,6 +188,22 @@ struct sk_buff *skb_udp_tunnel_segment(struct sk_buff *skb,
> }
> EXPORT_SYMBOL(skb_udp_tunnel_segment);
>
> +static struct sk_buff *__udp_gso_segment_list(struct sk_buff *skb,
> + netdev_features_t features)
> +{
> + unsigned int mss = skb_shinfo(skb)->gso_size;
> +
> + skb = skb_segment_list(skb, features, skb_mac_header_len(skb));
> + if (IS_ERR(skb))
> + return skb;
> +
> + udp_hdr(skb)->len = htons(sizeof(struct udphdr) + mss);
> + skb->ip_summed = CHECKSUM_NONE;
> + skb->csum_valid = 1;
csum_valid is only used on ingress.
Hardcoding CHECKSUM_NONE is probably fine as long as this function is
only used for forwarding, assuming we don't care about verifiying
checksums in the forwarding case.
But this is fragile if we ever add local list segment output. Should
convert the checksum field in skb_forward_csum, instead of at the GSO
layer, just as for forwarding of non list skbs? Though that would
require traversing the list yet another time. Other option is to
already do this conversion when building the list in GRO.
The comment also applies to the same logic in skb_segment_list. As a
matter or fact, even if this belongs in GSO instead of core forwarding
or GRO, then probably both the list head and frag_list skbs should be
set in the same function, so skb_segment_list.
> +
> + return skb;
> +}
> +
> struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
> netdev_features_t features)
> {
> @@ -200,6 +216,9 @@ struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
> __sum16 check;
> __be16 newlen;
>
> + if (skb_shinfo(gso_skb)->gso_type & SKB_GSO_FRAGLIST)
> + return __udp_gso_segment_list(gso_skb, features);
> +
> mss = skb_shinfo(gso_skb)->gso_size;
> if (gso_skb->len <= sizeof(*uh) + mss)
> return ERR_PTR(-EINVAL);
> @@ -352,16 +371,15 @@ static struct sk_buff *udp_gro_receive_segment(struct list_head *head,
> struct sk_buff *pp = NULL;
> struct udphdr *uh2;
> struct sk_buff *p;
> + int ret;
>
> /* requires non zero csum, for symmetry with GSO */
> if (!uh->check) {
> NAPI_GRO_CB(skb)->flush = 1;
> return NULL;
> }
> -
Accidental whitespace removal?
> /* pull encapsulating udp header */
> skb_gro_pull(skb, sizeof(struct udphdr));
> - skb_gro_postpull_rcsum(skb, uh, sizeof(struct udphdr));
>
> list_for_each_entry(p, head, list) {
> if (!NAPI_GRO_CB(p)->same_flow)
> @@ -379,8 +397,17 @@ static struct sk_buff *udp_gro_receive_segment(struct list_head *head,
> * Under small packet flood GRO count could elsewhere grow a lot
> * leading to execessive truesize values
> */
> - if (!skb_gro_receive(p, skb) &&
> - NAPI_GRO_CB(p)->count >= UDP_GRO_CNT_MAX)
> + if (NAPI_GRO_CB(skb)->is_flist) {
> + if (!pskb_may_pull(skb, skb_gro_offset(skb)))
> + return NULL;
> + ret = skb_gro_receive_list(p, skb);
> + } else {
> + skb_gro_postpull_rcsum(skb, uh, sizeof(struct udphdr));
> +
> + ret = skb_gro_receive(p, skb);
> + }
> +
> + if (!ret && NAPI_GRO_CB(p)->count > UDP_GRO_CNT_MAX)
> pp = p;
> else if (uh->len != uh2->len)
> pp = p;
> @@ -402,6 +429,7 @@ struct sk_buff *udp_gro_receive(struct list_head *head, struct sk_buff *skb,
> int flush = 1;
>
> if (!sk || !udp_sk(sk)->gro_receive) {
> + NAPI_GRO_CB(skb)->is_flist = sk ? !udp_sk(sk)->gro_enabled: 1;
This updates the choice of whether to use a list on each received skb.
Which is problematic as a socket can call the setsockopt in between
packets.
Actually, there no longer is a need for a route lookup for each skb at
all. We always apply GRO, which was the previous reason for the
lookup. And if a matching flow is found in the GRO table, we already
the choice to use a list is already stored.
> pp = call_gro_receive(udp_gro_receive_segment, head, skb);
> return pp;
> }
> @@ -530,6 +558,15 @@ INDIRECT_CALLABLE_SCOPE int udp4_gro_complete(struct sk_buff *skb, int nhoff)
> const struct iphdr *iph = ip_hdr(skb);
> struct udphdr *uh = (struct udphdr *)(skb->data + nhoff);
>
> + if (NAPI_GRO_CB(skb)->is_flist) {
> + uh->len = htons(skb->len - nhoff);
> +
> + skb_shinfo(skb)->gso_type |= (SKB_GSO_FRAGLIST|SKB_GSO_UDP_L4);
> + skb_shinfo(skb)->gso_segs = NAPI_GRO_CB(skb)->count;
> +
> + return 0;
> + }
> +
> if (uh->check)
> uh->check = ~udp_v4_check(skb->len - nhoff, iph->saddr,
> iph->daddr, 0);
> diff --git a/net/ipv6/udp_offload.c b/net/ipv6/udp_offload.c
> index 5f7937a4f71a..7c3f28310baa 100644
> --- a/net/ipv6/udp_offload.c
> +++ b/net/ipv6/udp_offload.c
> @@ -154,6 +154,15 @@ INDIRECT_CALLABLE_SCOPE int udp6_gro_complete(struct sk_buff *skb, int nhoff)
> const struct ipv6hdr *ipv6h = ipv6_hdr(skb);
> struct udphdr *uh = (struct udphdr *)(skb->data + nhoff);
>
> + if (NAPI_GRO_CB(skb)->is_flist) {
> + uh->len = htons(skb->len - nhoff);
> +
> + skb_shinfo(skb)->gso_type |= (SKB_GSO_FRAGLIST|SKB_GSO_UDP_L4);
> + skb_shinfo(skb)->gso_segs = NAPI_GRO_CB(skb)->count;
> +
> + return 0;
> + }
> +
> if (uh->check)
> uh->check = ~udp_v6_check(skb->len - nhoff, &ipv6h->saddr,
> &ipv6h->daddr, 0);
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH bpf-next v4 2/7] samples/bpf: xdp_redirect_cpu have not need for read_trace_pipe
From: John Fastabend @ 2019-01-28 20:50 UTC (permalink / raw)
To: Maciej Fijalkowski, daniel, ast; +Cc: netdev, jakub.kicinski, brouer
In-Reply-To: <20190128191613.11705-3-maciejromanfijalkowski@gmail.com>
On 1/28/19 11:16 AM, Maciej Fijalkowski wrote:
> From: Jesper Dangaard Brouer <brouer@redhat.com>
>
> The sample xdp_redirect_cpu is not using helper bpf_trace_printk.
> Thus it makes no sense that the --debug option us reading
> from /sys/kernel/debug/tracing/trace_pipe via read_trace_pipe.
> Simply remove it.
>
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
Acked-by: John Fastabend <john.fastabend@gmail.com>
^ permalink raw reply
* Re: [PATCH RFC v2 2/3] net: Support GRO/GSO fraglist chaining.
From: Willem de Bruijn @ 2019-01-28 20:50 UTC (permalink / raw)
To: Steffen Klassert
Cc: Network Development, Willem de Bruijn, Paolo Abeni,
Jason A. Donenfeld
In-Reply-To: <20190128085025.14532-3-steffen.klassert@secunet.com>
On Mon, Jan 28, 2019 at 2:53 AM Steffen Klassert
<steffen.klassert@secunet.com> wrote:
>
> This patch adds the core functions to chain/unchain
> GSO skbs at the frag_list pointer. This also adds
> a new GSO type SKB_GSO_FRAGLIST and a is_flist
> flag to napi_gro_cb which indicates that this
> flow will be GROed by fraglist chaining.
>
> Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
> +struct sk_buff *skb_segment_list(struct sk_buff *skb,
> + netdev_features_t features,
> + unsigned int offset)
> +{
> + struct sk_buff *list_skb = skb_shinfo(skb)->frag_list;
> + unsigned int tnl_hlen = skb_tnl_header_len(skb);
> + unsigned int delta_truesize = 0;
> + unsigned int delta_len = 0;
> + struct sk_buff *tail = NULL;
> + struct sk_buff *nskb;
> +
> + skb_push(skb, -skb_network_offset(skb) + offset);
> +
> + skb_shinfo(skb)->frag_list = NULL;
> +
> + do {
> + nskb = list_skb;
> + list_skb = list_skb->next;
> +
> + if (!tail)
> + skb->next = nskb;
> + else
> + tail->next = nskb;
> +
> + tail = nskb;
> +
> + delta_len += nskb->len;
> + delta_truesize += nskb->truesize;
> +
> + skb_push(nskb, -skb_network_offset(nskb) + offset);
> +
> + if (!secpath_exists(nskb))
> + __skb_ext_copy(nskb, skb);
> +
> + memcpy(nskb->cb, skb->cb, sizeof(skb->cb));
> +
> + nskb->ip_summed = CHECKSUM_NONE;
> + nskb->csum_valid = 1;
> + nskb->tstamp = skb->tstamp;
> + nskb->dev = skb->dev;
> + nskb->queue_mapping = skb->queue_mapping;
> +
> + nskb->mac_len = skb->mac_len;
> + nskb->mac_header = skb->mac_header;
> + nskb->transport_header = skb->transport_header;
> + nskb->network_header = skb->network_header;
> + skb_dst_copy(nskb, skb);
> +
> + skb_headers_offset_update(nskb, skb_headroom(nskb) - skb_headroom(skb));
> + skb_copy_from_linear_data_offset(skb, -tnl_hlen,
> + nskb->data - tnl_hlen,
> + offset + tnl_hlen);
> +
> + if (skb_needs_linearize(nskb, features) &&
> + __skb_linearize(nskb)) {
> + kfree_skb_list(skb->next);
> + skb->next = NULL;
> + return ERR_PTR(-ENOMEM);
> + }
> + } while (list_skb);
> +
> + skb->truesize = skb->truesize - delta_truesize;
> + skb->data_len = skb->data_len - delta_len;
> + skb->len = skb->len - delta_len;
> +
> + skb_gso_reset(skb);
> +
> + skb->prev = tail;
> +
> + if (skb_needs_linearize(skb, features) &&
> + __skb_linearize(skb)) {
> + skb->next = NULL;
> + kfree_skb_list(skb->next);
inverse order
also, I would probably deduplicate with the same branch above in a new
err_linearize: block
^ permalink raw reply
* Re: [PATCH bpf-next v4 4/7] samples/bpf: Extend RLIMIT_MEMLOCK for xdp_{sample_pkts, router_ipv4}
From: John Fastabend @ 2019-01-28 20:54 UTC (permalink / raw)
To: Maciej Fijalkowski, daniel, ast; +Cc: netdev, jakub.kicinski, brouer
In-Reply-To: <20190128191613.11705-5-maciejromanfijalkowski@gmail.com>
On 1/28/19 11:16 AM, Maciej Fijalkowski wrote:
> From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
>
> There is a common problem with xdp samples that happens when user wants
> to run a particular sample and some bpf program is already loaded. The
> default 64kb RLIMIT_MEMLOCK resource limit will cause a following error
> (assuming that xdp sample that is failing was converted to libbpf
> usage):
>
> libbpf: Error in bpf_object__probe_name():Operation not permitted(1).
> Couldn't load basic 'r0 = 0' BPF program.
> libbpf: failed to load object './xdp_sample_pkts_kern.o'
>
> Fix it in xdp_sample_pkts and xdp_router_ipv4 by setting RLIMIT_MEMLOCK
> to RLIM_INFINITY.
>
> Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
> ---
Seems we do this for most samples so nice to have here as well.
Acked-by: John Fastabend <john.fastabend@gmail.com>
^ permalink raw reply
* Re: [PATCH bpf-next v4 5/7] samples/bpf: Add a "force" flag to XDP samples
From: John Fastabend @ 2019-01-28 20:57 UTC (permalink / raw)
To: Maciej Fijalkowski, daniel, ast; +Cc: netdev, jakub.kicinski, brouer
In-Reply-To: <20190128191613.11705-6-maciejromanfijalkowski@gmail.com>
On 1/28/19 11:16 AM, Maciej Fijalkowski wrote:
> From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
>
> Make xdp samples consistent with iproute2 behavior and set the
> XDP_FLAGS_UPDATE_IF_NOEXIST by default when setting the xdp program on
> interface. Provide an option for user to force the program loading,
> which as a result will not include the mentioned flag in
> bpf_set_link_xdp_fd call.
>
> Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> ---
Patch is fine but I don't really know if being consistent with
iproute2 here gets us much. I guess if users find the "force"
behavior better for a sample that is OK though.
Acked-by: John Fastabend <john.fastabend@gmail.com>
^ permalink raw reply
* Re: WoL broken in r8169.c since kernel 4.19
From: Marc Haber @ 2019-01-28 20:59 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: netdev@vger.kernel.org
In-Reply-To: <d677ecf0-b106-03a0-2c66-d817ff39d20a@gmail.com>
On Mon, Jan 28, 2019 at 08:02:47PM +0100, Heiner Kallweit wrote:
> One more test .. Can you provide the output of the following under 4.18 and under 4.19?
> It may not apply cleanly, but you get the idea. The message is written when suspending.
I booted, suspeneded, sent a magic packet that got ignored. I then woke
the box up with the any key, went through the ethtool motions, suspended
again, sent a magic paket that the machine acted on and woke up
Log says:
1 [1/4994]mh@fan:~ $ grep 'may wakeup' /var/log/syslog/syslog
Jan 28 21:51:44 fan kernel: [ 77.571211] may wakeup? 0
Jan 28 21:54:11 fan kernel: [ 183.994131] may wakeup? 1
[2/4995]mh@fan:~ $
Greetings
Marc
--
-----------------------------------------------------------------------------
Marc Haber | "I don't trust Computers. They | Mailadresse im Header
Leimen, Germany | lose things." Winona Ryder | Fon: *49 6224 1600402
Nordisch by Nature | How to make an American Quilt | Fax: *49 6224 1600421
^ permalink raw reply
* Re: [PATCH bpf-next v4 6/7] libbpf: Add a support for getting xdp prog id on ifindex
From: John Fastabend @ 2019-01-28 21:07 UTC (permalink / raw)
To: Maciej Fijalkowski, daniel, ast; +Cc: netdev, jakub.kicinski, brouer
In-Reply-To: <20190128191613.11705-7-maciejromanfijalkowski@gmail.com>
On 1/28/19 11:16 AM, Maciej Fijalkowski wrote:
> From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
>
> Since we have a dedicated netlink attributes for xdp setup on a
> particular interface, it is now possible to retrieve the program id that
> is currently attached to the interface. The use case is targeted for
> sample xdp programs, which will store the program id just after loading
> bpf program onto iface. On shutdown, the sample will make sure that it
> can unload the program by querying again the iface and verifying that
> both program id's matches.
>
> Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> ---
small nit.
> +
> +int bpf_get_link_xdp_id(int ifindex, __u32 *prog_id, __u32 flags)
> +{
> + struct xdp_id_md xdp_id = {};
> + int sock, ret;
> + __u32 nl_pid;
> + __u32 mask;
> +
> + if (flags & ~XDP_FLAGS_MASK)
> + return -EINVAL;
> +
> + /* Check whether the single {HW,DRV,SKB} mode is set */
> + flags &= XDP_FLAGS_MODES;
> + mask = flags - 1;
> + if (flags && flags & mask)
> + return -EINVAL;
> +
> + sock = libbpf_netlink_open(&nl_pid);
> + if (sock < 0)
> + return sock;
> +
> + xdp_id.ifindex = ifindex;
> + xdp_id.flags = flags;
> +
> + ret = libbpf_nl_get_link(sock, nl_pid, get_xdp_id, &xdp_id);
> + *prog_id = xdp_id.id;
just a nit but should we really set prog_id from user if there is
an error. Probably friendlier not to set caller data in error
case.
> +
> + close(sock);
> + return ret;
> +}
> +
> int libbpf_nl_get_link(int sock, unsigned int nl_pid,
> libbpf_dump_nlmsg_t dump_link_nlmsg, void *cookie)
> {
>
^ permalink raw reply
* Re: [PATCH bpf-next v4 7/7] samples/bpf: Check the prog id before exiting
From: John Fastabend @ 2019-01-28 21:10 UTC (permalink / raw)
To: Maciej Fijalkowski, daniel, ast; +Cc: netdev, jakub.kicinski, brouer
In-Reply-To: <20190128191613.11705-8-maciejromanfijalkowski@gmail.com>
On 1/28/19 11:16 AM, Maciej Fijalkowski wrote:
> From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
>
> Check the program id within the signal handler on polling xdp samples
> that were previously converted to libbpf usage. Avoid the situation of
> unloading the program that was not attached by sample that is exiting.
>
> Reported-by: Michal Papaj <michal.papaj@intel.com>
> Reported-by: Jakub Spizewski <jakub.spizewski@intel.com>
> Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> ---
Acked-by: John Fastabend <john.fastabend@gmail.com>
^ permalink raw reply
* Re: WoL broken in r8169.c since kernel 4.19
From: Heiner Kallweit @ 2019-01-28 21:21 UTC (permalink / raw)
To: Marc Haber; +Cc: netdev@vger.kernel.org
In-Reply-To: <20190128205957.GI27062@torres.zugschlus.de>
On 28.01.2019 21:59, Marc Haber wrote:
> On Mon, Jan 28, 2019 at 08:02:47PM +0100, Heiner Kallweit wrote:
>> One more test .. Can you provide the output of the following under 4.18 and under 4.19?
>> It may not apply cleanly, but you get the idea. The message is written when suspending.
>
> I booted, suspeneded, sent a magic packet that got ignored. I then woke
> the box up with the any key, went through the ethtool motions, suspended
> again, sent a magic paket that the machine acted on and woke up
>
> Log says:
> 1 [1/4994]mh@fan:~ $ grep 'may wakeup' /var/log/syslog/syslog
> Jan 28 21:51:44 fan kernel: [ 77.571211] may wakeup? 0
> Jan 28 21:54:11 fan kernel: [ 183.994131] may wakeup? 1
> [2/4995]mh@fan:~ $
>
> Greetings
> Marc
>
Thanks, this makes clearer what's going on.
This change to r8169
bde135a672bf ("r8169: only enable PCI wakeups when WOL is active")
removed marking the device as wakeup-enabled to work around an
issue on certain notebooks (see commit message for details).
ethtool doesn't care about the current settings and sets the new ones
(wakeup-enabling the device). systemd however checks the current settings
and writes new ones only if there's a change. Therefore in your case
systemd does nothing and device doesn't get wakeup-enabled.
Not totally clear to me is why it works under 4.18.
And I wonder why the following didn't work for you, you said it makes
no difference. Could you try again the following in addition to the
latest debug output statement?
drivers/net/ethernet/realtek/r8169.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 3e650bd9e..bd26d3f2e 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -7442,6 +7442,7 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
}
tp->saved_wolopts = __rtl8169_get_wol(tp);
+ device_set_wakeup_enable(&pdev->dev, tp->saved_wolopts);
mutex_init(&tp->wk.mutex);
INIT_WORK(&tp->wk.work, rtl_task);
--
^ permalink raw reply related
* [PATCH net 0/2] erspan: always reports output key to userspace
From: Lorenzo Bianconi @ 2019-01-28 21:23 UTC (permalink / raw)
To: davem; +Cc: netdev, u9012063
Erspan protocol relies on output key to set session id header field.
However TUNNEL_KEY bit is cleared in order to not add key field to
the external GRE header and so the configured o_key is not reported
to userspace.
Fix the issue adding TUNNEL_KEY bit to the o_flags parameter dumping
device info
Lorenzo Bianconi (2):
net: ip_gre: always reports o_key to userspace
net: ip6_gre: always reports o_key to userspace
net/ipv4/ip_gre.c | 7 ++++++-
net/ipv6/ip6_gre.c | 7 ++++++-
2 files changed, 12 insertions(+), 2 deletions(-)
--
2.20.1
^ permalink raw reply
* [PATCH net 1/2] net: ip_gre: always reports o_key to userspace
From: Lorenzo Bianconi @ 2019-01-28 21:23 UTC (permalink / raw)
To: davem; +Cc: netdev, u9012063
In-Reply-To: <cover.1548709944.git.lorenzo.bianconi@redhat.com>
Erspan protocol (version 1 and 2) relies on o_key to configure
session id header field. However TUNNEL_KEY bit is cleared in
erspan_xmit since ERSPAN protocol does not set the key field
of the external GRE header and so the configured o_key is not reported
to userspace. The issue can be triggered with the following reproducer:
$ip link add erspan1 type erspan local 192.168.0.1 remote 192.168.0.2 \
key 1 seq erspan_ver 1
$ip link set erspan1 up
$ip -d link sh erspan1
erspan1@NONE: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1450 qdisc pfifo_fast state UNKNOWN mode DEFAULT
link/ether 52:aa:99:95:9a:b5 brd ff:ff:ff:ff:ff:ff promiscuity 0 minmtu 68 maxmtu 1500
erspan remote 192.168.0.2 local 192.168.0.1 ttl inherit ikey 0.0.0.1 iseq oseq erspan_index 0
Fix the issue adding TUNNEL_KEY bit to the o_flags parameter in
ipgre_fill_info
Fixes: 84e54fe0a5ea ("gre: introduce native tunnel support for ERSPAN")
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
net/ipv4/ip_gre.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index 20a64fe6254b..3978f807fa8b 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -1455,12 +1455,17 @@ static int ipgre_fill_info(struct sk_buff *skb, const struct net_device *dev)
{
struct ip_tunnel *t = netdev_priv(dev);
struct ip_tunnel_parm *p = &t->parms;
+ __be16 o_flags = p->o_flags;
+
+ if ((t->erspan_ver == 1 || t->erspan_ver == 2) &&
+ !t->collect_md)
+ o_flags |= TUNNEL_KEY;
if (nla_put_u32(skb, IFLA_GRE_LINK, p->link) ||
nla_put_be16(skb, IFLA_GRE_IFLAGS,
gre_tnl_flags_to_gre_flags(p->i_flags)) ||
nla_put_be16(skb, IFLA_GRE_OFLAGS,
- gre_tnl_flags_to_gre_flags(p->o_flags)) ||
+ gre_tnl_flags_to_gre_flags(o_flags)) ||
nla_put_be32(skb, IFLA_GRE_IKEY, p->i_key) ||
nla_put_be32(skb, IFLA_GRE_OKEY, p->o_key) ||
nla_put_in_addr(skb, IFLA_GRE_LOCAL, p->iph.saddr) ||
--
2.20.1
^ permalink raw reply related
* [PATCH net 2/2] net: ip6_gre: always reports o_key to userspace
From: Lorenzo Bianconi @ 2019-01-28 21:23 UTC (permalink / raw)
To: davem; +Cc: netdev, u9012063
In-Reply-To: <cover.1548709944.git.lorenzo.bianconi@redhat.com>
As Erspan_v4, Erspan_v6 protocol relies on o_key to configure
session id header field. However TUNNEL_KEY bit is cleared in
ip6erspan_tunnel_xmit since ERSPAN protocol does not set the key field
of the external GRE header and so the configured o_key is not reported
to userspace. The issue can be triggered with the following reproducer:
$ip link add ip6erspan1 type ip6erspan local 2000::1 remote 2000::2 \
key 1 seq erspan_ver 1
$ip link set ip6erspan1 up
ip -d link sh ip6erspan1
ip6erspan1@NONE: <BROADCAST,MULTICAST> mtu 1422 qdisc noop state DOWN mode DEFAULT
link/ether ba:ff:09:24:c3:0e brd ff:ff:ff:ff:ff:ff promiscuity 0 minmtu 68 maxmtu 1500
ip6erspan remote 2000::2 local 2000::1 encaplimit 4 flowlabel 0x00000 ikey 0.0.0.1 iseq oseq
Fix the issue adding TUNNEL_KEY bit to the o_flags parameter in
ip6gre_fill_info
Fixes: 5a963eb61b7c ("ip6_gre: Add ERSPAN native tunnel support")
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
net/ipv6/ip6_gre.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
index 4416368dbd49..801a9a0c217e 100644
--- a/net/ipv6/ip6_gre.c
+++ b/net/ipv6/ip6_gre.c
@@ -2098,12 +2098,17 @@ static int ip6gre_fill_info(struct sk_buff *skb, const struct net_device *dev)
{
struct ip6_tnl *t = netdev_priv(dev);
struct __ip6_tnl_parm *p = &t->parms;
+ __be16 o_flags = p->o_flags;
+
+ if ((p->erspan_ver == 1 || p->erspan_ver == 2) &&
+ !p->collect_md)
+ o_flags |= TUNNEL_KEY;
if (nla_put_u32(skb, IFLA_GRE_LINK, p->link) ||
nla_put_be16(skb, IFLA_GRE_IFLAGS,
gre_tnl_flags_to_gre_flags(p->i_flags)) ||
nla_put_be16(skb, IFLA_GRE_OFLAGS,
- gre_tnl_flags_to_gre_flags(p->o_flags)) ||
+ gre_tnl_flags_to_gre_flags(o_flags)) ||
nla_put_be32(skb, IFLA_GRE_IKEY, p->i_key) ||
nla_put_be32(skb, IFLA_GRE_OKEY, p->o_key) ||
nla_put_in6_addr(skb, IFLA_GRE_LOCAL, &p->laddr) ||
--
2.20.1
^ permalink raw reply related
* Re: [PATCH] tty: Fix WARNING in tty_set_termios
From: shuah @ 2019-01-28 21:29 UTC (permalink / raw)
To: Al Viro
Cc: marcel, johan.hedberg, w.d.hubbs, chris, kirk, samuel.thibault,
gregkh, robh, jslaby, sameo, davem, arnd, nishka.dasgupta_ug18,
m.maya.nakamura, santhameena13, zhongjiang, linux-bluetooth,
linux-kernel, speakup, devel, linux-serial, linux-wireless,
netdev, shuah
In-Reply-To: <20190126041416.GF2217@ZenIV.linux.org.uk>
On 1/25/19 9:14 PM, Al Viro wrote:
> On Fri, Jan 25, 2019 at 04:29:05PM -0700, Shuah Khan wrote:
>> tty_set_termios() has the following WARMN_ON which can be triggered with a
>> syscall to invoke TIOCGETD __NR_ioctl.
>>
>> WARN_ON(tty->driver->type == TTY_DRIVER_TYPE_PTY &&
>> tty->driver->subtype == PTY_TYPE_MASTER);
>> Reference: https://syzkaller.appspot.com/bug?id=2410d22f1d8e5984217329dd0884b01d99e3e48d
>>
>> A simple change would have been to print error message instead of WARN_ON.
>> However, the callers assume that tty_set_termios() always returns 0 and
>> don't check return value. The complete solution is fixing all the callers
>> to check error and bail out to fix the WARN_ON.
>>
>> This fix changes tty_set_termios() to return error and all the callers
>> to check error and bail out. The reproducer is used to reproduce the
>> problem and verify the fix.
>
>> --- a/drivers/bluetooth/hci_ldisc.c
>> +++ b/drivers/bluetooth/hci_ldisc.c
>> @@ -321,6 +321,8 @@ void hci_uart_set_flow_control(struct hci_uart *hu, bool enable)
>> status = tty_set_termios(tty, &ktermios);
>> BT_DBG("Disabling hardware flow control: %s",
>> status ? "failed" : "success");
>> + if (status)
>> + return;
>
> Can that ldisc end up set on pty master? And does it make any sense there?
The initial objective of the patch is to prevent the WARN_ON by making
the change to return error instead of WARN_ON. However, without changes
to places that don't check the return and keep making progress, there
will be secondary problems.
Without this change to return here, instead of WARN_ON, it will fail
with the following NULL pointer dereference at the next thing
hci_uart_set_flow_control() attempts.
status = tty->driver->ops->tiocmget(tty);
kernel: [10140.649783] BUG: unable to handle kernel NULL pointer
dereference at 0000000000000000
kernel: [10140.649786] #PF error: [INSTR]
kernel: [10140.649787] PGD 0 P4D 0
kernel: [10140.649790] Oops: 0010 [#1] SMP PTI
Jan 24 15:33:35 deneb kernel: [10140.649793] CPU: 2 PID: 55 Comm:
kworker/u33:0 Tainted: G W 5.0.0-rc3+ #5
kernel: [10140.649794] Hardware name: Dell Inc. OptiPlex 790/0HY9JP,
BIOS A18 09/24/2013
Workqueue: hci0 hci_power_on [bluetooth]
kernel: [10140.649805] RIP: 0010: (null)
kernel: [10140.649809] Code: Bad RIP value.
kernel: [10140.649810] RSP: 0018:ffffa01a8153fd28 EFLAGS: 00010282
kernel: [10140.649812] RAX: 0000000000000000 RBX: ffff8958d6bc4800 RCX:
35ad8b0300000000
kernel: [10140.649814] RDX: ffffffff00000001 RSI: 0000000000000000 RDI:
ffff8958d6bc4800
kernel: [10140.649816] RBP: ffffa01a8153fd78 R08: 0000000091773f09 R09:
0000000000000003
kernel: [10140.649817] R10: ffff8958d6bc4a98 R11: 0000000000000720 R12:
ffff895814500c00
kernel: [10140.649819] R13: ffff8958a858e000 R14: 0000000000000000 R15:
ffff8958af1af440
kernel: [10140.649821] FS: 0000000000000000(0000)
GS:ffff895925880000(0000) knlGS:0000000000000000
kernel: [10140.649823] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
kernel: [10140.649824] CR2: ffffffffffffffd6 CR3: 0000000083f46002 CR4:
00000000000606e0
kernel: [10140.649826] Call Trace:
kernel: [10140.649830] ? hci_uart_set_flow_control+0x20e/0x2c0 [hci_uart]
kernel: [10140.649836] mrvl_setup+0x17/0x80 [hci_uart]
kernel: [10140.649840] hci_uart_setup+0x56/0x160 [hci_uart]
kernel: [10140.649850] hci_dev_do_open+0xe6/0x630 [bluetooth]
kernel: [10140.649860] hci_power_on+0x52/0x220 [bluetooth]
>
> IOW, I don't believe that this patch makes any sense. If anything,
> we need to prevent unconditional tty_set_termios() on the path
> that *does* lead to calling it for pty.
>
I don't think preventing unconditional tty_set_termios() is enough to
prevent secondary problems such as the one above.
For example, the following call chain leads to the WARN_ON that was
reported. Even if void hci_uart_set_baudrate() prevents the very first
tty_set_termios() call, its caller hci_uart_setup() continues with
more tty setup. It goes ahead to call driver setup callback. The
driver callback goes on to do more setup calling tty_set_termios().
WARN_ON call path:
hci_uart_set_baudrate+0x1cc/0x250 drivers/bluetooth/hci_ldisc.c:378
hci_uart_setup+0xa2/0x490 drivers/bluetooth/hci_ldisc.c:401
hci_dev_do_open+0x6b1/0x1920 net/bluetooth/hci_core.c:1423
Once this WARN_ON is changed to return error, the following
happens, when hci_uart_setup() does driver setup callback.
kernel: [10140.649836] mrvl_setup+0x17/0x80 [hci_uart]
kernel: [10140.649840] hci_uart_setup+0x56/0x160 [hci_uart]
kernel: [10140.649850] hci_dev_do_open+0xe6/0x630 [bluetooth]
kernel: [10140.649860] hci_power_on+0x52/0x220 [bluetooth]
I think continuing to catch the invalid condition in tty_set_termios()
and preventing progress by checking return value is a straight forward
change to avoid secondary problems, and it might be difficult to catch
all the cases where it could fail. Here is the reproducer for reference:
#define _GNU_SOURCE
#include <endian.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/syscall.h>
#include <sys/types.h>
#include <unistd.h>
uint64_t r[1] = {0xffffffffffffffff};
int main(void)
{
syscall(__NR_mmap, 0x20000000, 0x1000000, 3, 0x32, -1, 0);
long res = 0;
memcpy((void*)0x20000100, "/dev/ptmx\x00", 10);
res = syscall(__NR_openat, 0xffffffffffffff9c, 0x20000100, 0, 0);
if (res != -1)
r[0] = res;
*(uint32_t*)0x200000c0 = 0xf;
syscall(__NR_ioctl, r[0], 0x5423, 0x200000c0);
syscall(__NR_ioctl, r[0], 0x400455c8, 0xb);
return 0;
}
thanks,
-- Shuah
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox