* Re: [PATCH net-next v6 2/5] bpf: Add tests for eBPF file mode
From: Daniel Borkmann @ 2017-10-16 23:00 UTC (permalink / raw)
To: Chenbo Feng, netdev, SELinux, linux-security-module
Cc: Jeffrey Vander Stoep, Alexei Starovoitov, lorenzo,
Stephen Smalley, James Morris, Paul Moore, Chenbo Feng
In-Reply-To: <20171016191135.8046-3-chenbofeng.kernel@gmail.com>
On 10/16/2017 09:11 PM, Chenbo Feng wrote:
> From: Chenbo Feng <fengc@google.com>
>
> Two related tests are added into bpf selftest to test read only map and
> write only map. The tests verified the read only and write only flags
> are working on hash maps.
>
> Signed-off-by: Chenbo Feng <fengc@google.com>
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
^ permalink raw reply
* Re: [PATCH net-next v6 1/5] bpf: Add file mode configuration into bpf maps
From: Daniel Borkmann @ 2017-10-16 22:59 UTC (permalink / raw)
To: Chenbo Feng, netdev, SELinux, linux-security-module
Cc: Jeffrey Vander Stoep, Alexei Starovoitov, lorenzo,
Stephen Smalley, James Morris, Paul Moore, Chenbo Feng
In-Reply-To: <20171016191135.8046-2-chenbofeng.kernel@gmail.com>
On 10/16/2017 09:11 PM, Chenbo Feng wrote:
> From: Chenbo Feng <fengc@google.com>
>
> Introduce the map read/write flags to the eBPF syscalls that returns the
> map fd. The flags is used to set up the file mode when construct a new
> file descriptor for bpf maps. To not break the backward capability, the
> f_flags is set to O_RDWR if the flag passed by syscall is 0. Otherwise
> it should be O_RDONLY or O_WRONLY. When the userspace want to modify or
> read the map content, it will check the file mode to see if it is
> allowed to make the change.
>
> Signed-off-by: Chenbo Feng <fengc@google.com>
> Acked-by: Alexei Starovoitov <ast@kernel.org>
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
^ permalink raw reply
* WARNING in tcp_update_reordering
From: David Ahern @ 2017-10-16 22:38 UTC (permalink / raw)
To: Eric Dumazet, netdev@vger.kernel.org
I need to throw this one over the fence. I triggered the trace below
testing changes to the tcp tracepoint. It is not readily reproducible
and does not appear to be correlated to the perf session.
Triggered by the following:
ssh into VM and run 'top -d1'
in host, drop tcp packets:
$ sudo iptables -I INPUT -i br1 -p tcp -j DROP
and then 5 - 7 seconds later remove the rule:
$ sudo iptables -D INPUT -i br1 -p tcp -j DROP
[ 73.695849] ------------[ cut here ]------------
[ 73.697258] WARNING: CPU: 1 PID: 0 at
/home/dsa/kernel-2.git/net/ipv4/tcp_input.c:889
tcp_update_reordering+0x4/0x72
[ 73.700306] Modules linked in: vrf
[ 73.701316] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.14.0-rc4+ #43
[ 73.703170] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
BIOS 1.7.5-20140531_083030-gandalf 04/01/2014
[ 73.706040] task: ffff88003e132180 task.stack: ffffc90000078000
[ 73.707769] RIP: 0010:tcp_update_reordering+0x4/0x72
[ 73.709209] RSP: 0018:ffff88003fd039c0 EFLAGS: 00010282
[ 73.710702] RAX: 0000000000000004 RBX: ffff88003c7be800 RCX:
0000000000000004
[ 73.711797] RDX: 0000000000000000 RSI: 00000000fffffffb RDI:
ffff88003c7be800
[ 73.712809] RBP: ffff88003fd03a38 R08: 0000000000000000 R09:
0000000000000000
[ 73.713831] R10: ffff88003fd03a90 R11: 0000000000000000 R12:
ffff88003fd03a90
[ 73.714846] R13: 0000000000000000 R14: ffff88003c7bef40 R15:
ffff88003c7bef48
[ 73.715871] FS: 0000000000000000(0000) GS:ffff88003fd00000(0000)
knlGS:0000000000000000
[ 73.717016] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 73.717823] CR2: 000056441f284000 CR3: 000000003d7ba002 CR4:
00000000000606e0
[ 73.718834] Call Trace:
[ 73.719193] <IRQ>
[ 73.719497] ? tcp_sacktag_write_queue+0x533/0x5ab
[ 73.720180] ? tcp_mtu_to_mss+0xd/0x1c
[ 73.720718] tcp_ack+0x320/0xd21
[ 73.721189] tcp_rcv_established+0x251/0x2c2
[ 73.721799] tcp_v4_do_rcv+0x9a/0x169
[ 73.722328] tcp_v4_rcv+0x55c/0x7bb
[ 73.722841] ip_local_deliver_finish+0xc0/0x141
[ 73.723486] ip_local_deliver+0xa5/0xae
[ 73.724038] ? xfrm4_policy_check.constprop.10+0x52/0x52
[ 73.724787] ip_rcv_finish+0x2de/0x35c
[ 73.725326] ip_rcv+0x271/0x2ea
[ 73.725787] ? deliver_ptype_list_skb+0x48/0x69
[ 73.726430] __netif_receive_skb_core+0x326/0x54a
[ 73.727112] ? kvm_clock_read+0x1e/0x20
[ 73.727664] ? kvm_clock_get_cycles+0x9/0xb
[ 73.728257] __netif_receive_skb+0x16/0x6d
[ 73.728837] ? __netif_receive_skb+0x16/0x6d
[ 73.729446] netif_receive_skb_internal+0x4d/0xa3
[ 73.730116] napi_gro_receive+0x75/0xcc
[ 73.730667] receive_buf+0xa05/0xa16
[ 73.731192] ? vring_unmap_one+0x1a/0x68
[ 73.731754] virtnet_poll+0x103/0x1b1
[ 73.732284] net_rx_action+0xdb/0x249
[ 73.732814] __do_softirq+0xfc/0x26b
[ 73.733332] irq_exit+0x51/0x92
[ 73.733788] do_IRQ+0x98/0xb0
[ 73.734225] common_interrupt+0x93/0x93
[ 73.734779] </IRQ>
[ 73.735095] RIP: 0010:native_safe_halt+0x6/0x8
[ 73.735726] RSP: 0018:ffffc9000007bea8 EFLAGS: 00000246 ORIG_RAX:
ffffffffffffff6e
[ 73.736784] RAX: 0000000000000000 RBX: 0000000000000000 RCX:
ffff88003fd10ca0
[ 73.737776] RDX: 00000000000101ba RSI: 0000000000000001 RDI:
0000000000000001
[ 73.738780] RBP: ffffc9000007bea8 R08: ffff88003d48a900 R09:
0000000000000200
[ 73.739777] R10: 0000000000000003 R11: 0000000000000001 R12:
0000000000000000
[ 73.740779] R13: ffff88003e132180 R14: ffff88003e132180 R15:
ffff88003e132180
[ 73.741781] default_idle+0x1a/0x2d
[ 73.742285] arch_cpu_idle+0xa/0xc
[ 73.742784] default_idle_call+0x19/0x1b
[ 73.743352] do_idle+0xd7/0x1a9
[ 73.743807] cpu_startup_entry+0x1a/0x1c
[ 73.744371] start_secondary+0x11e/0x120
[ 73.744930] secondary_startup_64+0xa5/0xa5
[ 73.745526] Code: 02 74 0b 48 c7 87 e0 06 00 00 00 00 00 00 8a 97 2c
06 00 00 83 e0 0d c1 e0 04 5d 83 e2 0f 09 d0 88 87 2c 06 00 00 c3 85 f6
79 03 <0f> ff c3 3b b7 10 06 00 00 55 48 89 e5 41 54 41 89 d4 53 48 89
[ 73.748142] ---[ end trace 730e0fcd13f383bc ]---
^ permalink raw reply
* Re: [PATCH v2 net-next] tcp: Check daddr_cache before use in tracepoint
From: Cong Wang @ 2017-10-16 22:35 UTC (permalink / raw)
To: David Ahern; +Cc: Linux Kernel Network Developers
In-Reply-To: <1508193127-17626-1-git-send-email-dsahern@gmail.com>
On Mon, Oct 16, 2017 at 3:32 PM, David Ahern <dsahern@gmail.com> wrote:
> Running perf in one window to capture tcp_retransmit_skb tracepoint:
> $ perf record -e tcp:tcp_retransmit_skb -a
>
> And causing a retransmission on an active TCP session (e.g., dropping
> packets in the receiver, changing MTU on the interface to 500 and back
> to 1500) triggers a panic:
>
> [ 58.543144] BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
> [ 58.545300] IP: perf_trace_tcp_retransmit_skb+0xd0/0x145
> [ 58.546770] PGD 0 P4D 0
> [ 58.547472] Oops: 0000 [#1] SMP
> [ 58.548328] Modules linked in: vrf
> [ 58.549262] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.14.0-rc4+ #26
> [ 58.551004] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.7.5-20140531_083030-gandalf 04/01/2014
> [ 58.554560] task: ffffffff81a0e540 task.stack: ffffffff81a00000
> [ 58.555817] RIP: 0010:perf_trace_tcp_retransmit_skb+0xd0/0x145
> [ 58.557137] RSP: 0018:ffff88003fc03d68 EFLAGS: 00010282
> [ 58.558292] RAX: 0000000000000000 RBX: ffffe8ffffc0ec80 RCX: ffff880038543098
> [ 58.559850] RDX: 0400000000000000 RSI: ffff88003fc03d70 RDI: ffff88003fc14b68
> [ 58.561099] RBP: ffff88003fc03da8 R08: 0000000000000000 R09: ffffea0000d3224a
> [ 58.562005] R10: ffff88003fc03db8 R11: 0000000000000010 R12: ffff8800385428c0
> [ 58.562930] R13: ffffe8ffffc0e478 R14: ffffffff81a93a40 R15: ffff88003d4f0c00
> [ 58.563845] FS: 0000000000000000(0000) GS:ffff88003fc00000(0000) knlGS:0000000000000000
> [ 58.564873] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 58.565613] CR2: 0000000000000008 CR3: 000000003d68f004 CR4: 00000000000606f0
> [ 58.566538] Call Trace:
> [ 58.566865] <IRQ>
> [ 58.567140] __tcp_retransmit_skb+0x4ab/0x4c6
> [ 58.567704] ? tcp_set_ca_state+0x22/0x3f
> [ 58.568231] tcp_retransmit_skb+0x14/0xa3
> [ 58.568754] tcp_retransmit_timer+0x472/0x5e3
> [ 58.569324] ? tcp_write_timer_handler+0x1e9/0x1e9
> [ 58.569946] tcp_write_timer_handler+0x95/0x1e9
> [ 58.570548] tcp_write_timer+0x2a/0x58
>
> Check that daddr_cache is non-NULL before de-referencing.
>
> Fixes: e086101b150a ("tcp: add a tracepoint for tcp retransmission")
> Signed-off-by: David Ahern <dsahern@gmail.com>
Acked-by: Cong Wang <xiyou.wangcong@gmail.com>
^ permalink raw reply
* Re: [PATCH net-next] virtio_net: implement VIRTIO_CONFIG_S_NEEDS_RESET
From: Willem de Bruijn @ 2017-10-16 22:34 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Network Development, David Miller, Jason Wang, virtualization,
Willem de Bruijn
In-Reply-To: <20171016191806-mutt-send-email-mst@kernel.org>
On Mon, Oct 16, 2017 at 12:38 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Mon, Oct 16, 2017 at 12:04:57PM -0400, Willem de Bruijn wrote:
>> On Mon, Oct 16, 2017 at 11:31 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> > On Mon, Oct 16, 2017 at 11:03:18AM -0400, Willem de Bruijn wrote:
>> >> >> +static int virtnet_reset(struct virtnet_info *vi)
>> >> >> +{
>> >> >> + struct virtio_device *dev = vi->vdev;
>> >> >> + int ret;
>> >> >> +
>> >> >> + virtio_config_disable(dev);
>> >> >> + dev->failed = dev->config->get_status(dev) & VIRTIO_CONFIG_S_FAILED;
>> >> >> + virtnet_freeze_down(dev, true);
>> >> >> + remove_vq_common(vi);
>> >> >> +
>> >> >> + virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
>> >> >> + virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER);
>> >> >> +
>> >> >> + ret = virtio_finalize_features(dev);
>> >> >> + if (ret)
>> >> >> + goto err;
>> >> >> +
>> >> >> + ret = virtnet_restore_up(dev);
>> >> >> + if (ret)
>> >> >> + goto err;
>> >> >> +
>> >> >> + ret = virtnet_set_queues(vi, vi->curr_queue_pairs);
>> >> >> + if (ret)
>> >> >> + goto err;
>> >> >> +
>> >> >> + virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
>> >> >> + virtio_config_enable(dev);
>> >> >> + return 0;
>> >> >> +
>> >> >> +err:
>> >> >> + virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED);
>> >> >> + return ret;
>> >> >> +}
>> >> >> +
>> >> >> static int virtnet_set_guest_offloads(struct virtnet_info *vi, u64 offloads)
>> >> >> {
>> >> >> struct scatterlist sg;
>> >> >
>> >> > I have a question here though. How do things like MAC address
>> >> > get restored?
>> >> >
>> >> > What about the rx mode?
>> >> >
>> >> > vlans?
>> >>
>> >> The function as is releases and reinitializes only ring state.
>> >> Device configuration such as mac and vlan persist across
>> >> the reset.
>> >
>> > What gave you this impression? Take a look at e.g. this
>> > code in qemu:
>> >
>> > static void virtio_net_reset(VirtIODevice *vdev)
>> > {
>> > VirtIONet *n = VIRTIO_NET(vdev);
>> >
>> > /* Reset back to compatibility mode */
>> > n->promisc = 1;
>> > n->allmulti = 0;
>> > n->alluni = 0;
>> > n->nomulti = 0;
>> > n->nouni = 0;
>> > n->nobcast = 0;
>> > /* multiqueue is disabled by default */
>> > n->curr_queues = 1;
>> > timer_del(n->announce_timer);
>> > n->announce_counter = 0;
>> > n->status &= ~VIRTIO_NET_S_ANNOUNCE;
>> >
>> > /* Flush any MAC and VLAN filter table state */
>> > n->mac_table.in_use = 0;
>> > n->mac_table.first_multi = 0;
>> > n->mac_table.multi_overflow = 0;
>> > n->mac_table.uni_overflow = 0;
>> > memset(n->mac_table.macs, 0, MAC_TABLE_ENTRIES * ETH_ALEN);
>> > memcpy(&n->mac[0], &n->nic->conf->macaddr, sizeof(n->mac));
>> > qemu_format_nic_info_str(qemu_get_queue(n->nic), n->mac);
>> > memset(n->vlans, 0, MAX_VLAN >> 3);
>> > }
>> >
>> > So device seems to lose all state, you have to re-program it.
>>
>> Oh, indeed! The guest does not reset its state, so it might
>> be out of sync with the host after the operation. Was this not
>> an issue when previously resetting in the context of xdp?
>
> I suspect it was broken back then, too.
Okay. I guess that in principle this is all programmable through
virtnet_set_rx_mode, virtnet_vlan_rx_add_vid, etc. But it's a
lot more complex than just restoring virtnet_reset. Will need to
be careful about concurrency issues at the least. Similar to the
ones you point out below.
>
>> >> > Also, it seems that LINK_ANNOUNCE requests will get ignored
>> >> > even if they got set before the reset, leading to downtime.
>> >>
>> >> Do you mean act on VIRTIO_NET_F_GUEST_ANNOUNCE
>> >> requests? That flag is tested and netdev_notify_peers
>> >> called before resetting virtio ring state.
>> >
>> > Yes but I wonder if there's a race where announce
>> > is set after it is read but before NEED_RESET is read.
>> >
>> > Re-reading status from the config before reset
>> > might be necessary.
>>
>> Thanks, I'll have a look. Perhaps a host should simply not
>> request a reset while it is waiting for an announce ack.
>
> It's one option though we can't make this change for existing hosts.
> We also have the reverse condition where announce is requested after
> NEED_RESET is set.
^ permalink raw reply
* Re: [PATCH net-next] tcp: Use pI6c in tcp tracepoint
From: Alexei Starovoitov @ 2017-10-16 22:33 UTC (permalink / raw)
To: David Ahern; +Cc: netdev, xiyou.wangcong
In-Reply-To: <1508189042-19591-1-git-send-email-dsahern@gmail.com>
On Mon, Oct 16, 2017 at 02:24:02PM -0700, David Ahern wrote:
> The compact form for IPv6 addresses is more user friendly than the full
> version. For example:
> compact: 2001:db8:1::1
> full: 2001:0db8:0001:0000:0000:0000:0000:0004i
>
> Update the tcp tracepoint to show the compact form.
>
> Signed-off-by: David Ahern <dsahern@gmail.com>
nice improvement.
Acked-by: Alexei Starovoitov <ast@kernel.org>
^ permalink raw reply
* [PATCH v2 net-next] tcp: Check daddr_cache before use in tracepoint
From: David Ahern @ 2017-10-16 22:32 UTC (permalink / raw)
To: netdev; +Cc: xiyou.wangcong, David Ahern
Running perf in one window to capture tcp_retransmit_skb tracepoint:
$ perf record -e tcp:tcp_retransmit_skb -a
And causing a retransmission on an active TCP session (e.g., dropping
packets in the receiver, changing MTU on the interface to 500 and back
to 1500) triggers a panic:
[ 58.543144] BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
[ 58.545300] IP: perf_trace_tcp_retransmit_skb+0xd0/0x145
[ 58.546770] PGD 0 P4D 0
[ 58.547472] Oops: 0000 [#1] SMP
[ 58.548328] Modules linked in: vrf
[ 58.549262] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.14.0-rc4+ #26
[ 58.551004] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.7.5-20140531_083030-gandalf 04/01/2014
[ 58.554560] task: ffffffff81a0e540 task.stack: ffffffff81a00000
[ 58.555817] RIP: 0010:perf_trace_tcp_retransmit_skb+0xd0/0x145
[ 58.557137] RSP: 0018:ffff88003fc03d68 EFLAGS: 00010282
[ 58.558292] RAX: 0000000000000000 RBX: ffffe8ffffc0ec80 RCX: ffff880038543098
[ 58.559850] RDX: 0400000000000000 RSI: ffff88003fc03d70 RDI: ffff88003fc14b68
[ 58.561099] RBP: ffff88003fc03da8 R08: 0000000000000000 R09: ffffea0000d3224a
[ 58.562005] R10: ffff88003fc03db8 R11: 0000000000000010 R12: ffff8800385428c0
[ 58.562930] R13: ffffe8ffffc0e478 R14: ffffffff81a93a40 R15: ffff88003d4f0c00
[ 58.563845] FS: 0000000000000000(0000) GS:ffff88003fc00000(0000) knlGS:0000000000000000
[ 58.564873] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 58.565613] CR2: 0000000000000008 CR3: 000000003d68f004 CR4: 00000000000606f0
[ 58.566538] Call Trace:
[ 58.566865] <IRQ>
[ 58.567140] __tcp_retransmit_skb+0x4ab/0x4c6
[ 58.567704] ? tcp_set_ca_state+0x22/0x3f
[ 58.568231] tcp_retransmit_skb+0x14/0xa3
[ 58.568754] tcp_retransmit_timer+0x472/0x5e3
[ 58.569324] ? tcp_write_timer_handler+0x1e9/0x1e9
[ 58.569946] tcp_write_timer_handler+0x95/0x1e9
[ 58.570548] tcp_write_timer+0x2a/0x58
Check that daddr_cache is non-NULL before de-referencing.
Fixes: e086101b150a ("tcp: add a tracepoint for tcp retransmission")
Signed-off-by: David Ahern <dsahern@gmail.com>
---
v2
- remove np and get addresses from sock_common
include/trace/events/tcp.h | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/include/trace/events/tcp.h b/include/trace/events/tcp.h
index 1ffab6d96e94..f51c130f1e0f 100644
--- a/include/trace/events/tcp.h
+++ b/include/trace/events/tcp.h
@@ -27,7 +27,6 @@ TRACE_EVENT(tcp_retransmit_skb,
),
TP_fast_assign(
- struct ipv6_pinfo *np = inet6_sk(sk);
struct inet_sock *inet = inet_sk(sk);
struct in6_addr *pin6;
__be32 *p32;
@@ -44,11 +43,12 @@ TRACE_EVENT(tcp_retransmit_skb,
p32 = (__be32 *) __entry->daddr;
*p32 = inet->inet_daddr;
- if (np) {
+ /* IPv6 socket ? */
+ if (inet6_sk(sk)) {
pin6 = (struct in6_addr *)__entry->saddr_v6;
- *pin6 = np->saddr;
+ *pin6 = sk->sk_v6_rcv_saddr;
pin6 = (struct in6_addr *)__entry->daddr_v6;
- *pin6 = *(np->daddr_cache);
+ *pin6 = sk->sk_v6_daddr;
} else {
pin6 = (struct in6_addr *)__entry->saddr_v6;
ipv6_addr_set_v4mapped(inet->inet_saddr, pin6);
--
2.1.4
^ permalink raw reply related
* Re: [PATCH 1/3] bpf: Don't check for current being NULL
From: Alexei Starovoitov @ 2017-10-16 22:31 UTC (permalink / raw)
To: Richard Weinberger
Cc: Al Viro, netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
Daniel Borkmann, Alexei Starovoitov, linux-arch
In-Reply-To: <1838884.qlCOuH57mO@blindfold>
On Tue, Oct 17, 2017 at 12:23:13AM +0200, Richard Weinberger wrote:
> Alexei,
>
> Am Dienstag, 17. Oktober 2017, 00:06:08 CEST schrieb Alexei Starovoitov:
> > On Mon, Oct 16, 2017 at 11:18 AM, Richard Weinberger <richard@nod.at> wrote:
> > > current is never NULL.
> > >
> > > Signed-off-by: Richard Weinberger <richard@nod.at>
> > > ---
> > >
> > > kernel/bpf/helpers.c | 12 ------------
> > > 1 file changed, 12 deletions(-)
> > >
> > > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> > > index 3d24e238221e..e8845adcd15e 100644
> > > --- a/kernel/bpf/helpers.c
> > > +++ b/kernel/bpf/helpers.c
> > > @@ -120,9 +120,6 @@ BPF_CALL_0(bpf_get_current_pid_tgid)
> > >
> > > {
> > >
> > > struct task_struct *task = current;
> > >
> > > - if (unlikely(!task))
> > > - return -EINVAL;
> > > -
> >
> > really? in all context? including irq and nmi?
>
> I would be astonished current is NULL in such a context.
>
> To be sure, let's CC linux-arch.
> IIRC I talked also with Al about this and he also assumed that current
> cannot be NULL.
Hmm I probably mistakenly stole the !current check from somewhere.
Happy to delete all these checks then.
^ permalink raw reply
* Re: [PATCH 1/3] bpf: Don't check for current being NULL
From: Richard Weinberger @ 2017-10-16 22:23 UTC (permalink / raw)
To: Alexei Starovoitov, Al Viro
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
Daniel Borkmann, Alexei Starovoitov, linux-arch
In-Reply-To: <CAADnVQJzM3DH_gxAwo2mnGiv7bOsip1TAFF8YGPLx-dqSb1SPQ@mail.gmail.com>
Alexei,
Am Dienstag, 17. Oktober 2017, 00:06:08 CEST schrieb Alexei Starovoitov:
> On Mon, Oct 16, 2017 at 11:18 AM, Richard Weinberger <richard@nod.at> wrote:
> > current is never NULL.
> >
> > Signed-off-by: Richard Weinberger <richard@nod.at>
> > ---
> >
> > kernel/bpf/helpers.c | 12 ------------
> > 1 file changed, 12 deletions(-)
> >
> > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> > index 3d24e238221e..e8845adcd15e 100644
> > --- a/kernel/bpf/helpers.c
> > +++ b/kernel/bpf/helpers.c
> > @@ -120,9 +120,6 @@ BPF_CALL_0(bpf_get_current_pid_tgid)
> >
> > {
> >
> > struct task_struct *task = current;
> >
> > - if (unlikely(!task))
> > - return -EINVAL;
> > -
>
> really? in all context? including irq and nmi?
I would be astonished current is NULL in such a context.
To be sure, let's CC linux-arch.
IIRC I talked also with Al about this and he also assumed that current
cannot be NULL.
Thanks,
//richard
^ permalink raw reply
* Re: [next-queue PATCH v8 0/6] TSN: Add qdisc based config interface for CBS
From: Vinicius Costa Gomes @ 2017-10-16 22:20 UTC (permalink / raw)
To: David Miller
Cc: netdev, intel-wired-lan, jhs, xiyou.wangcong, jiri, andre.guedes,
ivan.briano, jesus.sanchez-palencia, boon.leong.ong,
richardcochran, henrik, levipearson, rodney.cummings
In-Reply-To: <20171016.205146.1053238236578937188.davem@davemloft.net>
Hi David,
David Miller <davem@davemloft.net> writes:
> I'm fine with this patch set. I see it's against Jeff's next-queue, so
> where exactly do you want this to be merged? My net-next tree?
I think it going through Jeff's next-queue makes more sense.
Anyway, I will send a v9 later today fixing the padding issue that David
Laight pointed out.
>
> Thank you.
Cheers,
^ permalink raw reply
* Re: [PATCH 3/3] bpf: Make sure that ->comm does not change under us.
From: Daniel Borkmann @ 2017-10-16 22:19 UTC (permalink / raw)
To: Alexei Starovoitov, Richard Weinberger
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
Alexei Starovoitov
In-Reply-To: <CAADnVQ+0kTYr2azBW6mDSU6JwWtjWRZVaJQ=ZFmSs3JxCFrrRg@mail.gmail.com>
On 10/17/2017 12:10 AM, Alexei Starovoitov wrote:
> On Mon, Oct 16, 2017 at 2:10 PM, Richard Weinberger <richard@nod.at> wrote:
>> Am Montag, 16. Oktober 2017, 23:02:06 CEST schrieb Daniel Borkmann:
>>> On 10/16/2017 10:55 PM, Richard Weinberger wrote:
>>>> Am Montag, 16. Oktober 2017, 22:50:43 CEST schrieb Daniel Borkmann:
>>>>>> struct task_struct *task = current;
>>>>>>
>>>>>> + task_lock(task);
>>>>>>
>>>>>> strncpy(buf, task->comm, size);
>>>>>>
>>>>>> + task_unlock(task);
>>>>>
>>>>> Wouldn't this potentially lead to a deadlock? E.g. you attach yourself
>>>>> to task_lock() / spin_lock() / etc, and then the BPF prog triggers the
>>>>> bpf_get_current_comm() taking the lock again ...
>>>>
>>>> Yes, but doesn't the same apply to the use case when I attach to strncpy()
>>>> and run bpf_get_current_comm()?
>>>
>>> You mean due to recursion? In that case trace_call_bpf() would bail out
>>> due to the bpf_prog_active counter.
>>
>> Ah, that's true.
>> So, when someone wants to use bpf_get_current_comm() while tracing task_lock,
>> we have a problem. I agree.
>> On the other hand, without locking the function may return wrong results.
>
> it will surely race with somebody else setting task comm and it's fine.
> all of bpf tracing is read-only, so locks are only allowed inside bpf core
> bits like maps. Taking core locks like task_lock() is quite scary.
> bpf scripts rely on bpf_probe_read() of all sorts of kernel fields
> so reading comm here w/o lock is fine.
Yeah, and perf_event_comm() -> perf_event_comm_event() out of __set_task_comm()
is having same approach wrt comm read-out.
^ permalink raw reply
* RE: [next-queue PATCH v7 4/6] net/sched: Introduce Credit Based Shaper (CBS) qdisc
From: Vinicius Costa Gomes @ 2017-10-16 22:13 UTC (permalink / raw)
To: David Laight, 'Ivan Khoronzhuk'
Cc: netdev@vger.kernel.org, intel-wired-lan@lists.osuosl.org,
jhs@mojatatu.com, xiyou.wangcong@gmail.com, jiri@resnulli.us,
andre.guedes@intel.com, ivan.briano@intel.com,
jesus.sanchez-palencia@intel.com, boon.leong.ong@intel.com,
richardcochran@gmail.com, henrik@austad.us, levipearson@gmail.com,
rodney.cummings@ni.com
In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6DD00979BC@AcuExch.aculab.com>
Hi David,
David Laight <David.Laight@ACULAB.COM> writes:
[...]
>> > index 099bf5528fed..41e349df4bf4 100644
>> > --- a/include/uapi/linux/pkt_sched.h
>> > +++ b/include/uapi/linux/pkt_sched.h
>> > @@ -871,4 +871,22 @@ struct tc_pie_xstats {
>> > __u32 maxq; /* maximum queue size */
>> > __u32 ecn_mark; /* packets marked with ecn*/
>> > };
>> > +
>> > +/* CBS */
>> > +struct tc_cbs_qopt {
>> > + __u8 offload;
>
> You probably don't want unnamed padding in a uapi structure.
Yeah, this needs to be fixed.
>
>> > + __s32 hicredit;
>> > + __s32 locredit;
>> > + __s32 idleslope;
>> > + __s32 sendslope;
>> > +};
>> > +
>> > +enum {
>> > + TCA_CBS_UNSPEC,
>> > + TCA_CBS_PARMS,
>> > + __TCA_CBS_MAX,
>> > +};
>> > +
>> > +#define TCA_CBS_MAX (__TCA_CBS_MAX - 1)
>
> Why not:
> TCA_CBS_PARMS,
> TCA_CBS_NEXT,
> TCA_CBS_MAX = TCA_CBS_NEXT - 1,
The way it is proposed, at least is consistent with the rest of the
file. So, if you don't have any stronger reasons, I'd like to keep it
this way.
>
> ...
> David
Cheers,
^ permalink raw reply
* Re: [PATCH 3/3] bpf: Make sure that ->comm does not change under us.
From: Alexei Starovoitov @ 2017-10-16 22:10 UTC (permalink / raw)
To: Richard Weinberger
Cc: Daniel Borkmann, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, Alexei Starovoitov
On Mon, Oct 16, 2017 at 2:10 PM, Richard Weinberger <richard@nod.at> wrote:
> Am Montag, 16. Oktober 2017, 23:02:06 CEST schrieb Daniel Borkmann:
>> On 10/16/2017 10:55 PM, Richard Weinberger wrote:
>> > Am Montag, 16. Oktober 2017, 22:50:43 CEST schrieb Daniel Borkmann:
>> >>> struct task_struct *task = current;
>> >>>
>> >>> + task_lock(task);
>> >>>
>> >>> strncpy(buf, task->comm, size);
>> >>>
>> >>> + task_unlock(task);
>> >>
>> >> Wouldn't this potentially lead to a deadlock? E.g. you attach yourself
>> >> to task_lock() / spin_lock() / etc, and then the BPF prog triggers the
>> >> bpf_get_current_comm() taking the lock again ...
>> >
>> > Yes, but doesn't the same apply to the use case when I attach to strncpy()
>> > and run bpf_get_current_comm()?
>>
>> You mean due to recursion? In that case trace_call_bpf() would bail out
>> due to the bpf_prog_active counter.
>
> Ah, that's true.
> So, when someone wants to use bpf_get_current_comm() while tracing task_lock,
> we have a problem. I agree.
> On the other hand, without locking the function may return wrong results.
it will surely race with somebody else setting task comm and it's fine.
all of bpf tracing is read-only, so locks are only allowed inside bpf core
bits like maps. Taking core locks like task_lock() is quite scary.
bpf scripts rely on bpf_probe_read() of all sorts of kernel fields
so reading comm here w/o lock is fine.
^ permalink raw reply
* Re: [PATCH net-next] tcp: Check daddr_cache before use in tracepoint
From: David Ahern @ 2017-10-16 22:08 UTC (permalink / raw)
To: Cong Wang; +Cc: Linux Kernel Network Developers
In-Reply-To: <CAM_iQpVy-YY+e1R17P_9uV22n5DbXxL8+ACpY+E5yJYdp8doAQ@mail.gmail.com>
On 10/16/17 4:05 PM, Cong Wang wrote:
> Or if you mean non-zero, they are set to LOOPBACK4_IPV6
> which is what I expect.
yes, I meant non-0. thanks for the explanation. I'll send a v2
^ permalink raw reply
* [PATCH] mac80211: aggregation: Convert timers to use timer_setup()
From: Kees Cook @ 2017-10-16 22:08 UTC (permalink / raw)
To: Johannes Berg; +Cc: David S. Miller, netdev
In preparation for unconditionally passing the struct timer_list pointer to
all timer callbacks, switch to using the new timer_setup() and from_timer()
to pass the timer pointer explicitly.
This removes the tid mapping array and expands the tid structures to
add a pointer back to the station, along with the tid index itself.
Cc: Johannes Berg <johannes@sipsolutions.net>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: linux-wireless@vger.kernel.org
Cc: netdev@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
This has been the least trivial timer conversion yet. Given the use of
RCU and other things I may not even know about, I'd love to get a close
look at this. I *think* this is correct, as it will re-lookup the tid
entries when firing the timer.
---
net/mac80211/agg-rx.c | 41 +++++++++++++++++------------------------
net/mac80211/agg-tx.c | 42 ++++++++++++++++--------------------------
net/mac80211/sta_info.c | 8 --------
net/mac80211/sta_info.h | 12 ++++++++++--
4 files changed, 43 insertions(+), 60 deletions(-)
diff --git a/net/mac80211/agg-rx.c b/net/mac80211/agg-rx.c
index 88cc1ae935ea..63aba6dbc92a 100644
--- a/net/mac80211/agg-rx.c
+++ b/net/mac80211/agg-rx.c
@@ -151,21 +151,17 @@ EXPORT_SYMBOL(ieee80211_stop_rx_ba_session);
* After accepting the AddBA Request we activated a timer,
* resetting it after each frame that arrives from the originator.
*/
-static void sta_rx_agg_session_timer_expired(unsigned long data)
+static void sta_rx_agg_session_timer_expired(struct timer_list *t)
{
- /* not an elegant detour, but there is no choice as the timer passes
- * only one argument, and various sta_info are needed here, so init
- * flow in sta_info_create gives the TID as data, while the timer_to_id
- * array gives the sta through container_of */
- u8 *ptid = (u8 *)data;
- u8 *timer_to_id = ptid - *ptid;
- struct sta_info *sta = container_of(timer_to_id, struct sta_info,
- timer_to_tid[0]);
+ struct tid_ampdu_rx *tid_rx_timer =
+ from_timer(tid_rx_timer, t, session_timer);
+ struct sta_info *sta = tid_rx_timer->sta;
+ u16 tid = tid_rx_timer->tid;
struct tid_ampdu_rx *tid_rx;
unsigned long timeout;
rcu_read_lock();
- tid_rx = rcu_dereference(sta->ampdu_mlme.tid_rx[*ptid]);
+ tid_rx = rcu_dereference(sta->ampdu_mlme.tid_rx[tid]);
if (!tid_rx) {
rcu_read_unlock();
return;
@@ -180,21 +176,18 @@ static void sta_rx_agg_session_timer_expired(unsigned long data)
rcu_read_unlock();
ht_dbg(sta->sdata, "RX session timer expired on %pM tid %d\n",
- sta->sta.addr, (u16)*ptid);
+ sta->sta.addr, tid);
- set_bit(*ptid, sta->ampdu_mlme.tid_rx_timer_expired);
+ set_bit(tid, sta->ampdu_mlme.tid_rx_timer_expired);
ieee80211_queue_work(&sta->local->hw, &sta->ampdu_mlme.work);
}
-static void sta_rx_agg_reorder_timer_expired(unsigned long data)
+static void sta_rx_agg_reorder_timer_expired(struct timer_list *t)
{
- u8 *ptid = (u8 *)data;
- u8 *timer_to_id = ptid - *ptid;
- struct sta_info *sta = container_of(timer_to_id, struct sta_info,
- timer_to_tid[0]);
+ struct tid_ampdu_rx *tid_rx = from_timer(tid_rx, t, reorder_timer);
rcu_read_lock();
- ieee80211_release_reorder_timeout(sta, *ptid);
+ ieee80211_release_reorder_timeout(tid_rx->sta, tid_rx->tid);
rcu_read_unlock();
}
@@ -356,14 +349,12 @@ void ___ieee80211_start_rx_ba_session(struct sta_info *sta,
spin_lock_init(&tid_agg_rx->reorder_lock);
/* rx timer */
- setup_deferrable_timer(&tid_agg_rx->session_timer,
- sta_rx_agg_session_timer_expired,
- (unsigned long)&sta->timer_to_tid[tid]);
+ timer_setup(&tid_agg_rx->session_timer,
+ sta_rx_agg_session_timer_expired, TIMER_DEFERRABLE);
/* rx reorder timer */
- setup_timer(&tid_agg_rx->reorder_timer,
- sta_rx_agg_reorder_timer_expired,
- (unsigned long)&sta->timer_to_tid[tid]);
+ timer_setup(&tid_agg_rx->reorder_timer,
+ sta_rx_agg_reorder_timer_expired, 0);
/* prepare reordering buffer */
tid_agg_rx->reorder_buf =
@@ -399,6 +390,8 @@ void ___ieee80211_start_rx_ba_session(struct sta_info *sta,
tid_agg_rx->auto_seq = auto_seq;
tid_agg_rx->started = false;
tid_agg_rx->reorder_buf_filtered = 0;
+ tid_agg_rx->tid = tid;
+ tid_agg_rx->sta = sta;
status = WLAN_STATUS_SUCCESS;
/* activate it for RX */
diff --git a/net/mac80211/agg-tx.c b/net/mac80211/agg-tx.c
index bef516ec47f9..dedbb1fb10e7 100644
--- a/net/mac80211/agg-tx.c
+++ b/net/mac80211/agg-tx.c
@@ -422,15 +422,12 @@ int ___ieee80211_stop_tx_ba_session(struct sta_info *sta, u16 tid,
* add Block Ack response will arrive from the recipient.
* If this timer expires sta_addba_resp_timer_expired will be executed.
*/
-static void sta_addba_resp_timer_expired(unsigned long data)
+static void sta_addba_resp_timer_expired(struct timer_list *t)
{
- /* not an elegant detour, but there is no choice as the timer passes
- * only one argument, and both sta_info and TID are needed, so init
- * flow in sta_info_create gives the TID as data, while the timer_to_id
- * array gives the sta through container_of */
- u16 tid = *(u8 *)data;
- struct sta_info *sta = container_of((void *)data,
- struct sta_info, timer_to_tid[tid]);
+ struct tid_ampdu_tx *tid_tx_timer =
+ from_timer(tid_tx_timer, t, addba_resp_timer);
+ struct sta_info *sta = tid_tx_timer->sta;
+ u16 tid = tid_tx_timer->tid;
struct tid_ampdu_tx *tid_tx;
/* check if the TID waits for addBA response */
@@ -525,21 +522,17 @@ void ieee80211_tx_ba_session_handle_start(struct sta_info *sta, int tid)
* After accepting the AddBA Response we activated a timer,
* resetting it after each frame that we send.
*/
-static void sta_tx_agg_session_timer_expired(unsigned long data)
+static void sta_tx_agg_session_timer_expired(struct timer_list *t)
{
- /* not an elegant detour, but there is no choice as the timer passes
- * only one argument, and various sta_info are needed here, so init
- * flow in sta_info_create gives the TID as data, while the timer_to_id
- * array gives the sta through container_of */
- u8 *ptid = (u8 *)data;
- u8 *timer_to_id = ptid - *ptid;
- struct sta_info *sta = container_of(timer_to_id, struct sta_info,
- timer_to_tid[0]);
+ struct tid_ampdu_tx *tid_tx_timer =
+ from_timer(tid_tx_timer, t, session_timer);
+ struct sta_info *sta = tid_tx_timer->sta;
+ u16 tid = tid_tx_timer->tid;
struct tid_ampdu_tx *tid_tx;
unsigned long timeout;
rcu_read_lock();
- tid_tx = rcu_dereference(sta->ampdu_mlme.tid_tx[*ptid]);
+ tid_tx = rcu_dereference(sta->ampdu_mlme.tid_tx[tid]);
if (!tid_tx || test_bit(HT_AGG_STATE_STOPPING, &tid_tx->state)) {
rcu_read_unlock();
return;
@@ -555,9 +548,9 @@ static void sta_tx_agg_session_timer_expired(unsigned long data)
rcu_read_unlock();
ht_dbg(sta->sdata, "tx session timer expired on %pM tid %d\n",
- sta->sta.addr, (u16)*ptid);
+ sta->sta.addr, tid);
- ieee80211_stop_tx_ba_session(&sta->sta, *ptid);
+ ieee80211_stop_tx_ba_session(&sta->sta, tid);
}
int ieee80211_start_tx_ba_session(struct ieee80211_sta *pubsta, u16 tid,
@@ -672,14 +665,11 @@ int ieee80211_start_tx_ba_session(struct ieee80211_sta *pubsta, u16 tid,
tid_tx->timeout = timeout;
/* response timer */
- setup_timer(&tid_tx->addba_resp_timer,
- sta_addba_resp_timer_expired,
- (unsigned long)&sta->timer_to_tid[tid]);
+ timer_setup(&tid_tx->addba_resp_timer, sta_addba_resp_timer_expired, 0);
/* tx timer */
- setup_deferrable_timer(&tid_tx->session_timer,
- sta_tx_agg_session_timer_expired,
- (unsigned long)&sta->timer_to_tid[tid]);
+ timer_setup(&tid_tx->session_timer,
+ sta_tx_agg_session_timer_expired, TIMER_DEFERRABLE);
/* assign a dialog token */
sta->ampdu_mlme.dialog_token_allocator++;
diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index 877d35796776..b5add1464aeb 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -379,14 +379,6 @@ struct sta_info *sta_info_alloc(struct ieee80211_sub_if_data *sdata,
if (sta_prepare_rate_control(local, sta, gfp))
goto free_txq;
- for (i = 0; i < IEEE80211_NUM_TIDS; i++) {
- /*
- * timer_to_tid must be initialized with identity mapping
- * to enable session_timer's data differentiation. See
- * sta_rx_agg_session_timer_expired for usage.
- */
- sta->timer_to_tid[i] = i;
- }
for (i = 0; i < IEEE80211_NUM_ACS; i++) {
skb_queue_head_init(&sta->ps_tx_buf[i]);
skb_queue_head_init(&sta->tx_filtered[i]);
diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
index 5c54acd10562..1b9c1e81495d 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -126,6 +126,8 @@ enum ieee80211_agg_stop_reason {
AGG_STOP_DESTROY_STA,
};
+struct sta_info;
+
/**
* struct tid_ampdu_tx - TID aggregation information (Tx).
*
@@ -133,8 +135,10 @@ enum ieee80211_agg_stop_reason {
* @session_timer: check if we keep Tx-ing on the TID (by timeout value)
* @addba_resp_timer: timer for peer's response to addba request
* @pending: pending frames queue -- use sta's spinlock to protect
+ * @sta: station we are attached to
* @dialog_token: dialog token for aggregation session
* @timeout: session timeout value to be filled in ADDBA requests
+ * @tid: index in station tid list
* @state: session state (see above)
* @last_tx: jiffies of last tx activity
* @stop_initiator: initiator of a session stop
@@ -158,9 +162,11 @@ struct tid_ampdu_tx {
struct timer_list session_timer;
struct timer_list addba_resp_timer;
struct sk_buff_head pending;
+ struct sta_info *sta;
unsigned long state;
unsigned long last_tx;
u16 timeout;
+ u16 tid;
u8 dialog_token;
u8 stop_initiator;
bool tx_stop;
@@ -181,12 +187,14 @@ struct tid_ampdu_tx {
* @reorder_time: jiffies when skb was added
* @session_timer: check if peer keeps Tx-ing on the TID (by timeout value)
* @reorder_timer: releases expired frames from the reorder buffer.
+ * @sta: station we are attached to
* @last_rx: jiffies of last rx activity
* @head_seq_num: head sequence number in reordering buffer.
* @stored_mpdu_num: number of MPDUs in reordering buffer
* @ssn: Starting Sequence Number expected to be aggregated.
* @buf_size: buffer size for incoming A-MPDUs
* @timeout: reset timer value (in TUs).
+ * @tid: index in station tid list
* @rcu_head: RCU head used for freeing this struct
* @reorder_lock: serializes access to reorder buffer, see below.
* @auto_seq: used for offloaded BA sessions to automatically pick head_seq_and
@@ -208,6 +216,7 @@ struct tid_ampdu_rx {
u64 reorder_buf_filtered;
struct sk_buff_head *reorder_buf;
unsigned long *reorder_time;
+ struct sta_info *sta;
struct timer_list session_timer;
struct timer_list reorder_timer;
unsigned long last_rx;
@@ -216,6 +225,7 @@ struct tid_ampdu_rx {
u16 ssn;
u16 buf_size;
u16 timeout;
+ u16 tid;
u8 auto_seq:1,
removed:1,
started:1;
@@ -447,7 +457,6 @@ struct ieee80211_sta_rx_stats {
* plus one for non-QoS frames)
* @tid_seq: per-TID sequence numbers for sending to this STA
* @ampdu_mlme: A-MPDU state machine state
- * @timer_to_tid: identity mapping to ID timers
* @mesh: mesh STA information
* @debugfs_dir: debug filesystem directory dentry
* @dead: set to true when sta is unlinked
@@ -554,7 +563,6 @@ struct sta_info {
* Aggregation information, locked with lock.
*/
struct sta_ampdu_mlme ampdu_mlme;
- u8 timer_to_tid[IEEE80211_NUM_TIDS];
#ifdef CONFIG_MAC80211_DEBUGFS
struct dentry *debugfs_dir;
--
2.7.4
--
Kees Cook
Pixel Security
^ permalink raw reply related
* Re: [PATCH 1/3] bpf: Don't check for current being NULL
From: Alexei Starovoitov @ 2017-10-16 22:06 UTC (permalink / raw)
To: Richard Weinberger
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
Daniel Borkmann, Alexei Starovoitov
On Mon, Oct 16, 2017 at 11:18 AM, Richard Weinberger <richard@nod.at> wrote:
> current is never NULL.
>
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
> kernel/bpf/helpers.c | 12 ------------
> 1 file changed, 12 deletions(-)
>
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 3d24e238221e..e8845adcd15e 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -120,9 +120,6 @@ BPF_CALL_0(bpf_get_current_pid_tgid)
> {
> struct task_struct *task = current;
>
> - if (unlikely(!task))
> - return -EINVAL;
> -
really? in all context? including irq and nmi?
^ permalink raw reply
* Re: [PATCH net-next] tcp: Check daddr_cache before use in tracepoint
From: Cong Wang @ 2017-10-16 22:05 UTC (permalink / raw)
To: David Ahern; +Cc: Linux Kernel Network Developers
In-Reply-To: <cfd6b39a-41a6-ccf6-80a7-07089a2d6fea@gmail.com>
On Mon, Oct 16, 2017 at 2:55 PM, David Ahern <dsahern@gmail.com> wrote:
> On 10/16/17 3:46 PM, Cong Wang wrote:
>> Well, for NULL case, the entry->saddr_v6 will not be filled with
>> your patch.
>
> I think you meant daddr_v6 and yes it will not get filled in, but 0 is
> better than a panic ;-)
>
Sure, but we can do even better with sk->sk_v6_daddr.
>>
>> How about using sk->sk_v6_daddr?
>>
>
> That works, but then both addresses should come from sk and not np:
Yes.
>
> diff --git a/include/trace/events/tcp.h b/include/trace/events/tcp.h
> index 1ffab6d96e94..7989c2dcedf1 100644
> --- a/include/trace/events/tcp.h
> +++ b/include/trace/events/tcp.h
> @@ -46,9 +46,9 @@ TRACE_EVENT(tcp_retransmit_skb,
>
> if (np) {
> pin6 = (struct in6_addr *)__entry->saddr_v6;
> - *pin6 = np->saddr;
> + *pin6 = sk->sk_v6_rcv_saddr;
> pin6 = (struct in6_addr *)__entry->daddr_v6;
> - *pin6 = *(np->daddr_cache);
> + *pin6 = sk->sk_v6_daddr;
> } else {
> pin6 = (struct in6_addr *)__entry->saddr_v6;
> ipv6_addr_set_v4mapped(inet->inet_saddr, pin6);
>
>
> So np is just a trigger for IPv6 versus v4 mapped.
>
> I do have a question about why ipv4 addresses are non-NULL:
If you really mean non-NULL, they are addresses, not pointers to
addresses.
Or if you mean non-zero, they are set to LOOPBACK4_IPV6
which is what I expect.
^ permalink raw reply
* Re: [PATCH net v2] bpf: disallow arithmetic operations on context pointer
From: Alexei Starovoitov @ 2017-10-16 22:02 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: netdev, oss-drivers, daniel, ecree
In-Reply-To: <20171016181655.16366-1-jakub.kicinski@netronome.com>
On Mon, Oct 16, 2017 at 11:16:55AM -0700, Jakub Kicinski wrote:
> Commit f1174f77b50c ("bpf/verifier: rework value tracking")
> removed the crafty selection of which pointer types are
> allowed to be modified. This is OK for most pointer types
> since adjust_ptr_min_max_vals() will catch operations on
> immutable pointers. One exception is PTR_TO_CTX which is
> now allowed to be offseted freely.
>
> The intent of aforementioned commit was to allow context
> access via modified registers. The offset passed to
> ->is_valid_access() verifier callback has been adjusted
> by the value of the variable offset.
>
> What is missing, however, is taking the variable offset
> into account when the context register is used. Or in terms
> of the code adding the offset to the value passed to the
> ->convert_ctx_access() callback. This leads to the following
> eBPF user code:
>
> r1 += 68
> r0 = *(u32 *)(r1 + 8)
> exit
>
> being translated to this in kernel space:
>
> 0: (07) r1 += 68
> 1: (61) r0 = *(u32 *)(r1 +180)
> 2: (95) exit
>
> Offset 8 is corresponding to 180 in the kernel, but offset
> 76 is valid too. Verifier will "accept" access to offset
> 68+8=76 but then "convert" access to offset 8 as 180.
> Effective access to offset 248 is beyond the kernel context.
> (This is a __sk_buff example on a debug-heavy kernel -
> packet mark is 8 -> 180, 76 would be data.)
>
> Dereferencing the modified context pointer is not as easy
> as dereferencing other types, because we have to translate
> the access to reading a field in kernel structures which is
> usually at a different offset and often of a different size.
> To allow modifying the pointer we would have to make sure
> that given eBPF instruction will always access the same
> field or the fields accessed are "compatible" in terms of
> offset and size...
>
> Disallow dereferencing modified context pointers and add
> to selftests the test case described here.
>
> Fixes: f1174f77b50c ("bpf/verifier: rework value tracking")
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Acked-by: Alexei Starovoitov <ast@kernel.org>
that's a required fix for net and for net-next I like Ed's idea
to teach convert_ctx_access to recognize such complex pattern
and do fancier rewrite.
iirc we were hitting similar ctx rewrite issue from time to time
and managed to change .c and/or llvm codegen in the past
when compiler was producing code that technically was valid, but
didn't satisfy 'ctx+const' requirement.
Back then verifier wasn't that smart. Now we can do better.
^ permalink raw reply
* [PATCH] ipv6: mark expected switch fall-throughs
From: Gustavo A. R. Silva @ 2017-10-16 21:36 UTC (permalink / raw)
To: Steffen Klassert, Herbert Xu, David S. Miller, Alexey Kuznetsov,
Hideaki YOSHIFUJI, Pablo Neira Ayuso, Jozsef Kadlecsik,
Florian Westphal
Cc: netdev, linux-kernel, netfilter-devel, coreteam,
Gustavo A. R. Silva
In preparation to enabling -Wimplicit-fallthrough, mark switch cases
where we are expecting to fall through.
Notice that in some cases I placed the "fall through" comment
on its own line, which is what GCC is expecting to find.
Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
---
This code was tested by compilation only (GCC 7.2.0 was used).
Please, verify if the actual intention of the code is to fall through.
net/ipv6/ah6.c | 1 +
net/ipv6/exthdrs.c | 1 +
net/ipv6/icmp.c | 6 ++----
net/ipv6/ip6_fib.c | 4 ++++
net/ipv6/ip6_tunnel.c | 1 +
net/ipv6/ip6mr.c | 1 +
net/ipv6/netfilter/nf_nat_l3proto_ipv6.c | 3 ++-
net/ipv6/raw.c | 4 ++++
net/ipv6/tcp_ipv6.c | 3 ++-
net/ipv6/xfrm6_policy.c | 1 +
10 files changed, 19 insertions(+), 6 deletions(-)
diff --git a/net/ipv6/ah6.c b/net/ipv6/ah6.c
index 7802b72..37bb33f 100644
--- a/net/ipv6/ah6.c
+++ b/net/ipv6/ah6.c
@@ -271,6 +271,7 @@ static int ipv6_clear_mutable_options(struct ipv6hdr *iph, int len, int dir)
case NEXTHDR_DEST:
if (dir == XFRM_POLICY_OUT)
ipv6_rearrange_destopt(iph, exthdr.opth);
+ /* fall through */
case NEXTHDR_HOP:
if (!zero_out_mutable_opts(exthdr.opth)) {
net_dbg_ratelimited("overrun %sopts\n",
diff --git a/net/ipv6/exthdrs.c b/net/ipv6/exthdrs.c
index 9551613..7835dea 100644
--- a/net/ipv6/exthdrs.c
+++ b/net/ipv6/exthdrs.c
@@ -89,6 +89,7 @@ static bool ip6_tlvopt_unknown(struct sk_buff *skb, int optoff)
*/
if (ipv6_addr_is_multicast(&ipv6_hdr(skb)->daddr))
break;
+ /* fall through */
case 2: /* send ICMP PARM PROB regardless and drop packet */
icmpv6_param_prob(skb, ICMPV6_UNK_OPTION, optoff);
return false;
diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c
index 4e52d52..6ae5dd3 100644
--- a/net/ipv6/icmp.c
+++ b/net/ipv6/icmp.c
@@ -864,10 +864,8 @@ static int icmpv6_rcv(struct sk_buff *skb)
goto discard_it;
hdr = icmp6_hdr(skb);
- /*
- * Drop through to notify
- */
-
+ /* to notify */
+ /* fall through */
case ICMPV6_DEST_UNREACH:
case ICMPV6_TIME_EXCEED:
case ICMPV6_PARAMPROB:
diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
index c2ecd5e..dcb0649 100644
--- a/net/ipv6/ip6_fib.c
+++ b/net/ipv6/ip6_fib.c
@@ -1777,6 +1777,7 @@ static int fib6_walk_continue(struct fib6_walker *w)
}
w->state = FWS_L;
#endif
+ /* fall through */
case FWS_L:
left = rcu_dereference_protected(fn->left, 1);
if (left) {
@@ -1785,6 +1786,7 @@ static int fib6_walk_continue(struct fib6_walker *w)
continue;
}
w->state = FWS_R;
+ /* fall through */
case FWS_R:
right = rcu_dereference_protected(fn->right, 1);
if (right) {
@@ -1794,6 +1796,7 @@ static int fib6_walk_continue(struct fib6_walker *w)
}
w->state = FWS_C;
w->leaf = rcu_dereference_protected(fn->leaf, 1);
+ /* fall through */
case FWS_C:
if (w->leaf && fn->fn_flags & RTN_RTINFO) {
int err;
@@ -1812,6 +1815,7 @@ static int fib6_walk_continue(struct fib6_walker *w)
}
skip:
w->state = FWS_U;
+ /* fall through */
case FWS_U:
if (fn == w->root)
return 0;
diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index 8255486..4212879 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -593,6 +593,7 @@ ip4ip6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
case NDISC_REDIRECT:
rel_type = ICMP_REDIRECT;
rel_code = ICMP_REDIR_HOST;
+ /* fall through */
default:
return 0;
}
diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index f5500f5..59fad81 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -1722,6 +1722,7 @@ int ip6_mroute_setsockopt(struct sock *sk, int optname, char __user *optval, uns
case MRT6_ADD_MFC:
case MRT6_DEL_MFC:
parent = -1;
+ /* fall through */
case MRT6_ADD_MFC_PROXY:
case MRT6_DEL_MFC_PROXY:
if (optlen < sizeof(mfc))
diff --git a/net/ipv6/netfilter/nf_nat_l3proto_ipv6.c b/net/ipv6/netfilter/nf_nat_l3proto_ipv6.c
index 46d6dba..1d2fb92 100644
--- a/net/ipv6/netfilter/nf_nat_l3proto_ipv6.c
+++ b/net/ipv6/netfilter/nf_nat_l3proto_ipv6.c
@@ -290,7 +290,8 @@ nf_nat_ipv6_fn(void *priv, struct sk_buff *skb,
else
return NF_ACCEPT;
}
- /* Fall thru... (Only ICMPs can be IP_CT_IS_REPLY) */
+ /* Only ICMPs can be IP_CT_IS_REPLY: */
+ /* fall through */
case IP_CT_NEW:
/* Seen it before? This can happen for loopback, retrans,
* or local packets.
diff --git a/net/ipv6/raw.c b/net/ipv6/raw.c
index 041d1cd..08a85fa 100644
--- a/net/ipv6/raw.c
+++ b/net/ipv6/raw.c
@@ -1055,6 +1055,7 @@ static int rawv6_setsockopt(struct sock *sk, int level, int optname,
if (optname == IPV6_CHECKSUM ||
optname == IPV6_HDRINCL)
break;
+ /* fall through */
default:
return ipv6_setsockopt(sk, level, optname, optval, optlen);
}
@@ -1077,6 +1078,7 @@ static int compat_rawv6_setsockopt(struct sock *sk, int level, int optname,
if (optname == IPV6_CHECKSUM ||
optname == IPV6_HDRINCL)
break;
+ /* fall through */
default:
return compat_ipv6_setsockopt(sk, level, optname,
optval, optlen);
@@ -1138,6 +1140,7 @@ static int rawv6_getsockopt(struct sock *sk, int level, int optname,
if (optname == IPV6_CHECKSUM ||
optname == IPV6_HDRINCL)
break;
+ /* fall through */
default:
return ipv6_getsockopt(sk, level, optname, optval, optlen);
}
@@ -1160,6 +1163,7 @@ static int compat_rawv6_getsockopt(struct sock *sk, int level, int optname,
if (optname == IPV6_CHECKSUM ||
optname == IPV6_HDRINCL)
break;
+ /* fall through */
default:
return compat_ipv6_getsockopt(sk, level, optname,
optval, optlen);
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 64d94af..ae83615 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1577,8 +1577,9 @@ static int tcp_v6_rcv(struct sk_buff *skb)
refcounted = false;
goto process;
}
- /* Fall through to ACK */
}
+ /* to ACK */
+ /* fall through */
case TCP_TW_ACK:
tcp_v6_timewait_ack(sk, skb);
break;
diff --git a/net/ipv6/xfrm6_policy.c b/net/ipv6/xfrm6_policy.c
index 11d1314..4ed9f8c 100644
--- a/net/ipv6/xfrm6_policy.c
+++ b/net/ipv6/xfrm6_policy.c
@@ -152,6 +152,7 @@ _decode_session6(struct sk_buff *skb, struct flowi *fl, int reverse)
switch (nexthdr) {
case NEXTHDR_FRAGMENT:
onlyproto = 1;
+ /* fall through */
case NEXTHDR_ROUTING:
case NEXTHDR_HOP:
case NEXTHDR_DEST:
--
2.7.4
^ permalink raw reply related
* Re: [PATCH net v2] bpf: disallow arithmetic operations on context pointer
From: Daniel Borkmann @ 2017-10-16 21:56 UTC (permalink / raw)
To: Jakub Kicinski, netdev; +Cc: oss-drivers, alexei.starovoitov, ecree
In-Reply-To: <20171016181655.16366-1-jakub.kicinski@netronome.com>
On 10/16/2017 08:16 PM, Jakub Kicinski wrote:
> Commit f1174f77b50c ("bpf/verifier: rework value tracking")
> removed the crafty selection of which pointer types are
> allowed to be modified. This is OK for most pointer types
> since adjust_ptr_min_max_vals() will catch operations on
> immutable pointers. One exception is PTR_TO_CTX which is
> now allowed to be offseted freely.
>
> The intent of aforementioned commit was to allow context
> access via modified registers. The offset passed to
> ->is_valid_access() verifier callback has been adjusted
> by the value of the variable offset.
>
> What is missing, however, is taking the variable offset
> into account when the context register is used. Or in terms
> of the code adding the offset to the value passed to the
> ->convert_ctx_access() callback. This leads to the following
> eBPF user code:
>
> r1 += 68
> r0 = *(u32 *)(r1 + 8)
> exit
>
> being translated to this in kernel space:
>
> 0: (07) r1 += 68
> 1: (61) r0 = *(u32 *)(r1 +180)
> 2: (95) exit
>
> Offset 8 is corresponding to 180 in the kernel, but offset
> 76 is valid too. Verifier will "accept" access to offset
> 68+8=76 but then "convert" access to offset 8 as 180.
> Effective access to offset 248 is beyond the kernel context.
> (This is a __sk_buff example on a debug-heavy kernel -
> packet mark is 8 -> 180, 76 would be data.)
>
> Dereferencing the modified context pointer is not as easy
> as dereferencing other types, because we have to translate
> the access to reading a field in kernel structures which is
> usually at a different offset and often of a different size.
> To allow modifying the pointer we would have to make sure
> that given eBPF instruction will always access the same
> field or the fields accessed are "compatible" in terms of
> offset and size...
>
> Disallow dereferencing modified context pointers and add
> to selftests the test case described here.
>
> Fixes: f1174f77b50c ("bpf/verifier: rework value tracking")
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Thanks for the fix!
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
^ permalink raw reply
* Re: [PATCH net-next] tcp: Check daddr_cache before use in tracepoint
From: David Ahern @ 2017-10-16 21:55 UTC (permalink / raw)
To: Cong Wang; +Cc: Linux Kernel Network Developers
In-Reply-To: <CAM_iQpUgWJwjaRYw3vX4t20foheOrebU3U2ACZqrAbTYqYc-5A@mail.gmail.com>
On 10/16/17 3:46 PM, Cong Wang wrote:
> Well, for NULL case, the entry->saddr_v6 will not be filled with
> your patch.
I think you meant daddr_v6 and yes it will not get filled in, but 0 is
better than a panic ;-)
>
> How about using sk->sk_v6_daddr?
>
That works, but then both addresses should come from sk and not np:
diff --git a/include/trace/events/tcp.h b/include/trace/events/tcp.h
index 1ffab6d96e94..7989c2dcedf1 100644
--- a/include/trace/events/tcp.h
+++ b/include/trace/events/tcp.h
@@ -46,9 +46,9 @@ TRACE_EVENT(tcp_retransmit_skb,
if (np) {
pin6 = (struct in6_addr *)__entry->saddr_v6;
- *pin6 = np->saddr;
+ *pin6 = sk->sk_v6_rcv_saddr;
pin6 = (struct in6_addr *)__entry->daddr_v6;
- *pin6 = *(np->daddr_cache);
+ *pin6 = sk->sk_v6_daddr;
} else {
pin6 = (struct in6_addr *)__entry->saddr_v6;
ipv6_addr_set_v4mapped(inet->inet_saddr, pin6);
So np is just a trigger for IPv6 versus v4 mapped.
I do have a question about why ipv4 addresses are non-NULL:
$ perf script
swapper 0 [000] 25.025102: tcp:tcp_retransmit_skb:
sport=22 dport=48076 saddr=127.0.0.6 daddr=127.0.0.6
saddrv6=2001:db8:1::4 daddrv6=2001:db8:1::64
swapper 0 [000] 25.231125: tcp:tcp_retransmit_skb:
sport=22 dport=48076 saddr=127.0.0.6 daddr=127.0.0.6
saddrv6=2001:db8:1::4 daddrv6=2001:db8:1::64
^ permalink raw reply related
* [PATCH] net: ipx: mark expected switch fall-through
From: Gustavo A. R. Silva @ 2017-10-16 21:53 UTC (permalink / raw)
To: David S. Miller; +Cc: netdev, linux-kernel, Gustavo A. R. Silva
In preparation to enabling -Wimplicit-fallthrough, mark switch cases
where we are expecting to fall through.
Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
---
This code was tested by compilation only (GCC 7.2.0 was used).
Please, verify if the actual intention of the code is to fall through.
net/ipx/af_ipx.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/ipx/af_ipx.c b/net/ipx/af_ipx.c
index ac598ec..d21a9d1 100644
--- a/net/ipx/af_ipx.c
+++ b/net/ipx/af_ipx.c
@@ -1867,6 +1867,7 @@ static int ipx_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg)
rc = -EPERM;
if (!capable(CAP_NET_ADMIN))
break;
+ /* fall through */
case SIOCGIFADDR:
rc = ipxitf_ioctl(cmd, argp);
break;
--
2.7.4
^ permalink raw reply related
* Re: [PATCH net-next] tools: bpftool: use more common tag format
From: Alexei Starovoitov @ 2017-10-16 21:52 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: netdev, oss-drivers, daniel
In-Reply-To: <20171016171254.26225-1-jakub.kicinski@netronome.com>
On Mon, Oct 16, 2017 at 10:12:54AM -0700, Jakub Kicinski wrote:
> Program tag is usually displayed as string of bytes without
> any separators (e.g. as "aa5520b1090cfeb6" vs MAC addr-like
> format bpftool uses currently: "aa:55:20:b1:09:0c:fe:b6").
> Make bptfool use the more common format both for displaying
> the tag and selecting the program by tag.
>
> This was pointed out in review but I misunderstood the comment.
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Reviewed-by: Simon Horman <simon.horman@netronome.com>
thanks!
Acked-by: Alexei Starovoitov <ast@kernel.org>
^ permalink raw reply
* Re: [net-next V8 PATCH 1/5] bpf: introduce new bpf cpu map type BPF_MAP_TYPE_CPUMAP
From: Alexei Starovoitov @ 2017-10-16 21:49 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: netdev, jakub.kicinski, Michael S. Tsirkin, pavel.odintsov,
Jason Wang, mchan, John Fastabend, peter.waskiewicz.jr, ast,
Daniel Borkmann, Andy Gospodarek
In-Reply-To: <150814916887.1806.4443991765779135803.stgit@firesoul>
On Mon, Oct 16, 2017 at 12:19:28PM +0200, Jesper Dangaard Brouer wrote:
> The 'cpumap' is primarily used as a backend map for XDP BPF helper
> call bpf_redirect_map() and XDP_REDIRECT action, like 'devmap'.
>
> This patch implement the main part of the map. It is not connected to
> the XDP redirect system yet, and no SKB allocation are done yet.
>
> The main concern in this patch is to ensure the datapath can run
> without any locking. This adds complexity to the setup and tear-down
> procedure, which assumptions are extra carefully documented in the
> code comments.
>
> V2:
> - make sure array isn't larger than NR_CPUS
> - make sure CPUs added is a valid possible CPU
>
> V3: fix nitpicks from Jakub Kicinski <kubakici@wp.pl>
>
> V5:
> - Restrict map allocation to root / CAP_SYS_ADMIN
> - WARN_ON_ONCE if queue is not empty on tear-down
> - Return -EPERM on memlock limit instead of -ENOMEM
> - Error code in __cpu_map_entry_alloc() also handle ptr_ring_cleanup()
> - Moved cpu_map_enqueue() to next patch
>
> V6: all notice by Daniel Borkmann
> - Fix err return code in cpu_map_alloc() introduced in V5
> - Move cpu_possible() check after max_entries boundary check
> - Forbid usage initially in check_map_func_compatibility()
>
> V7:
> - Fix alloc error path spotted by Daniel Borkmann
> - Did stress test adding+removing CPUs from the map concurrently
> - Fixed refcnt issue on cpu_map_entry, kthread started too soon
> - Make sure packets are flushed during tear-down, involved use of
> rcu_barrier() and kthread_run only exit after queue is empty
> - Fix alloc error path in __cpu_map_entry_alloc() for ptr_ring
>
> V8:
> - Nitpicking comments and gramma by Edward Cree
> - Fix missing semi-colon introduced in V7 due to rebasing
> - Move struct bpf_cpu_map_entry members cpu+map_id to tracepoint patch
>
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> ---
> include/linux/bpf_types.h | 1
> include/uapi/linux/bpf.h | 1
> kernel/bpf/Makefile | 1
> kernel/bpf/cpumap.c | 560 ++++++++++++++++++++++++++++++++++++++++
> kernel/bpf/syscall.c | 8 +
> kernel/bpf/verifier.c | 5
> tools/include/uapi/linux/bpf.h | 1
> 7 files changed, 576 insertions(+), 1 deletion(-)
> create mode 100644 kernel/bpf/cpumap.c
Looks good to me
I like the idea of running networking stack from kthread
and hope adding GRO won't change the api.
Acked-by: Alexei Starovoitov <ast@kernel.org>
^ permalink raw reply
* Re: [PATCH net-next] tcp: Check daddr_cache before use in tracepoint
From: Cong Wang @ 2017-10-16 21:46 UTC (permalink / raw)
To: David Ahern; +Cc: Linux Kernel Network Developers
In-Reply-To: <1508189387-27931-1-git-send-email-dsahern@gmail.com>
On Mon, Oct 16, 2017 at 2:29 PM, David Ahern <dsahern@gmail.com> wrote:
> Running perf in one window to capture tcp_retransmit_skb tracepoint:
> $ perf record -e tcp:tcp_retransmit_skb -a
>
> And causing a retransmission on an active TCP session (e.g., dropping
> packets in the receiver, changing MTU on the interface to 500 and back
> to 1500) triggers a panic:
>
> [ 58.543144] BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
> [ 58.545300] IP: perf_trace_tcp_retransmit_skb+0xd0/0x145
> [ 58.546770] PGD 0 P4D 0
> [ 58.547472] Oops: 0000 [#1] SMP
> [ 58.548328] Modules linked in: vrf
> [ 58.549262] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.14.0-rc4+ #26
> [ 58.551004] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.7.5-20140531_083030-gandalf 04/01/2014
> [ 58.554560] task: ffffffff81a0e540 task.stack: ffffffff81a00000
> [ 58.555817] RIP: 0010:perf_trace_tcp_retransmit_skb+0xd0/0x145
> [ 58.557137] RSP: 0018:ffff88003fc03d68 EFLAGS: 00010282
> [ 58.558292] RAX: 0000000000000000 RBX: ffffe8ffffc0ec80 RCX: ffff880038543098
> [ 58.559850] RDX: 0400000000000000 RSI: ffff88003fc03d70 RDI: ffff88003fc14b68
> [ 58.561099] RBP: ffff88003fc03da8 R08: 0000000000000000 R09: ffffea0000d3224a
> [ 58.562005] R10: ffff88003fc03db8 R11: 0000000000000010 R12: ffff8800385428c0
> [ 58.562930] R13: ffffe8ffffc0e478 R14: ffffffff81a93a40 R15: ffff88003d4f0c00
> [ 58.563845] FS: 0000000000000000(0000) GS:ffff88003fc00000(0000) knlGS:0000000000000000
> [ 58.564873] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 58.565613] CR2: 0000000000000008 CR3: 000000003d68f004 CR4: 00000000000606f0
> [ 58.566538] Call Trace:
> [ 58.566865] <IRQ>
> [ 58.567140] __tcp_retransmit_skb+0x4ab/0x4c6
> [ 58.567704] ? tcp_set_ca_state+0x22/0x3f
> [ 58.568231] tcp_retransmit_skb+0x14/0xa3
> [ 58.568754] tcp_retransmit_timer+0x472/0x5e3
> [ 58.569324] ? tcp_write_timer_handler+0x1e9/0x1e9
> [ 58.569946] tcp_write_timer_handler+0x95/0x1e9
> [ 58.570548] tcp_write_timer+0x2a/0x58
>
> Check that daddr_cache is non-NULL before de-referencing.
Well, for NULL case, the entry->saddr_v6 will not be filled with
your patch.
How about using sk->sk_v6_daddr?
^ 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