* [bpf-next v2 0/2] Fix kcm + sockmap by checking psock type @ 2018-10-18 5:20 John Fastabend 2018-10-18 5:20 ` [bpf-next v2 1/2] bpf: skmsg, fix psock create on existing kcm/tls port John Fastabend 2018-10-18 5:20 ` [bpf-next v2 2/2] bpf: test_maps add a test to catch kcm + sockmap John Fastabend 0 siblings, 2 replies; 5+ messages in thread From: John Fastabend @ 2018-10-18 5:20 UTC (permalink / raw) To: ast, daniel; +Cc: john.fastabend, netdev, eric.dumazet We check if the sk_user_data (the psock in skmsg) is in fact a sockmap type to late, after we read the refcnt which is an error. This series moves the check up before reading refcnt and also adds a test to test_maps to test trying to add a KCM socket into a sockmap. While reviewig this code I also found an issue with KCM and kTLS where each uses sk_data_ready hooks and associated stream parser breaking expectations in kcm, ktls or both. But that fix will need to go to net. Thanks to Eric for reporting. v2: Fix up file +/- my scripts lost track of them John Fastabend (2): bpf: skmsg, fix psock create on existing kcm/tls port bpf: test_maps add a test to catch kcm + sockmap include/linux/skmsg.h | 25 +++++++++--- net/core/sock_map.c | 11 +++--- tools/testing/selftests/bpf/Makefile | 2 +- tools/testing/selftests/bpf/sockmap_kcm.c | 14 +++++++ tools/testing/selftests/bpf/test_maps.c | 64 ++++++++++++++++++++++++++++++- 5 files changed, 103 insertions(+), 13 deletions(-) create mode 100644 tools/testing/selftests/bpf/sockmap_kcm.c -- 1.9.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [bpf-next v2 1/2] bpf: skmsg, fix psock create on existing kcm/tls port 2018-10-18 5:20 [bpf-next v2 0/2] Fix kcm + sockmap by checking psock type John Fastabend @ 2018-10-18 5:20 ` John Fastabend 2018-10-18 17:34 ` Eric Dumazet 2018-10-18 5:20 ` [bpf-next v2 2/2] bpf: test_maps add a test to catch kcm + sockmap John Fastabend 1 sibling, 1 reply; 5+ messages in thread From: John Fastabend @ 2018-10-18 5:20 UTC (permalink / raw) To: ast, daniel; +Cc: john.fastabend, netdev, eric.dumazet Before using the psock returned by sk_psock_get() when adding it to a sockmap we need to ensure it is actually a sockmap based psock. Previously we were only checking this after incrementing the reference counter which was an error. This resulted in a slab-out-of-bounds error when the psock was not actually a sockmap type. This moves the check up so the reference counter is only used if it is a sockmap psock. Eric reported the following KASAN BUG, BUG: KASAN: slab-out-of-bounds in atomic_read include/asm-generic/atomic-instrumented.h:21 [inline] BUG: KASAN: slab-out-of-bounds in refcount_inc_not_zero_checked+0x97/0x2f0 lib/refcount.c:120 Read of size 4 at addr ffff88019548be58 by task syz-executor4/22387 CPU: 1 PID: 22387 Comm: syz-executor4 Not tainted 4.19.0-rc7+ #264 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 Call Trace: __dump_stack lib/dump_stack.c:77 [inline] dump_stack+0x1c4/0x2b4 lib/dump_stack.c:113 print_address_description.cold.8+0x9/0x1ff mm/kasan/report.c:256 kasan_report_error mm/kasan/report.c:354 [inline] kasan_report.cold.9+0x242/0x309 mm/kasan/report.c:412 check_memory_region_inline mm/kasan/kasan.c:260 [inline] check_memory_region+0x13e/0x1b0 mm/kasan/kasan.c:267 kasan_check_read+0x11/0x20 mm/kasan/kasan.c:272 atomic_read include/asm-generic/atomic-instrumented.h:21 [inline] refcount_inc_not_zero_checked+0x97/0x2f0 lib/refcount.c:120 sk_psock_get include/linux/skmsg.h:379 [inline] sock_map_link.isra.6+0x41f/0xe30 net/core/sock_map.c:178 sock_hash_update_common+0x19b/0x11e0 net/core/sock_map.c:669 sock_hash_update_elem+0x306/0x470 net/core/sock_map.c:738 map_update_elem+0x819/0xdf0 kernel/bpf/syscall.c:818 Signed-off-by: John Fastabend <john.fastabend@gmail.com> Reported-by: Eric Dumazet <eric.dumazet@gmail.com> Fixes: 604326b41a6f ("bpf, sockmap: convert to generic sk_msg interface") --- include/linux/skmsg.h | 25 ++++++++++++++++++++----- net/core/sock_map.c | 11 ++++++----- 2 files changed, 26 insertions(+), 10 deletions(-) diff --git a/include/linux/skmsg.h b/include/linux/skmsg.h index 677b673..f44ca6b 100644 --- a/include/linux/skmsg.h +++ b/include/linux/skmsg.h @@ -275,11 +275,6 @@ static inline struct sk_psock *sk_psock(const struct sock *sk) return rcu_dereference_sk_user_data(sk); } -static inline bool sk_has_psock(struct sock *sk) -{ - return sk_psock(sk) != NULL && sk->sk_prot->recvmsg == tcp_bpf_recvmsg; -} - static inline void sk_psock_queue_msg(struct sk_psock *psock, struct sk_msg *msg) { @@ -379,6 +374,26 @@ static inline bool sk_psock_test_state(const struct sk_psock *psock, return test_bit(bit, &psock->state); } +static inline struct sk_psock *sk_psock_get_checked(struct sock *sk) +{ + struct sk_psock *psock; + + rcu_read_lock(); + psock = sk_psock(sk); + if (psock) { + if (sk->sk_prot->recvmsg != tcp_bpf_recvmsg) { + psock = ERR_PTR(-EBUSY); + goto out; + } + + if (!refcount_inc_not_zero(&psock->refcnt)) + psock = NULL; + } +out: + rcu_read_unlock(); + return psock; +} + static inline struct sk_psock *sk_psock_get(struct sock *sk) { struct sk_psock *psock; diff --git a/net/core/sock_map.c b/net/core/sock_map.c index 3c0e44c..be6092a 100644 --- a/net/core/sock_map.c +++ b/net/core/sock_map.c @@ -175,12 +175,13 @@ static int sock_map_link(struct bpf_map *map, struct sk_psock_progs *progs, } } - psock = sk_psock_get(sk); + psock = sk_psock_get_checked(sk); + if (IS_ERR(psock)) { + ret = PTR_ERR(psock); + goto out_progs; + } + if (psock) { - if (!sk_has_psock(sk)) { - ret = -EBUSY; - goto out_progs; - } if ((msg_parser && READ_ONCE(psock->progs.msg_parser)) || (skb_progs && READ_ONCE(psock->progs.skb_parser))) { sk_psock_put(sk, psock); -- 1.9.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [bpf-next v2 1/2] bpf: skmsg, fix psock create on existing kcm/tls port 2018-10-18 5:20 ` [bpf-next v2 1/2] bpf: skmsg, fix psock create on existing kcm/tls port John Fastabend @ 2018-10-18 17:34 ` Eric Dumazet 2018-10-18 20:24 ` John Fastabend 0 siblings, 1 reply; 5+ messages in thread From: Eric Dumazet @ 2018-10-18 17:34 UTC (permalink / raw) To: John Fastabend, ast, daniel; +Cc: netdev On 10/17/2018 10:20 PM, John Fastabend wrote: > Before using the psock returned by sk_psock_get() when adding it to a > sockmap we need to ensure it is actually a sockmap based psock. > Previously we were only checking this after incrementing the reference > counter which was an error. This resulted in a slab-out-of-bounds > error when the psock was not actually a sockmap type. > > This moves the check up so the reference counter is only used > if it is a sockmap psock. > > Eric reported the following KASAN BUG, > > BUG: KASAN: slab-out-of-bounds in atomic_read include/asm-generic/atomic-instrumented.h:21 [inline] > BUG: KASAN: slab-out-of-bounds in refcount_inc_not_zero_checked+0x97/0x2f0 lib/refcount.c:120 > Read of size 4 at addr ffff88019548be58 by task syz-executor4/22387 > > CPU: 1 PID: 22387 Comm: syz-executor4 Not tainted 4.19.0-rc7+ #264 > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 > Call Trace: > __dump_stack lib/dump_stack.c:77 [inline] > dump_stack+0x1c4/0x2b4 lib/dump_stack.c:113 > print_address_description.cold.8+0x9/0x1ff mm/kasan/report.c:256 > kasan_report_error mm/kasan/report.c:354 [inline] > kasan_report.cold.9+0x242/0x309 mm/kasan/report.c:412 > check_memory_region_inline mm/kasan/kasan.c:260 [inline] > check_memory_region+0x13e/0x1b0 mm/kasan/kasan.c:267 > kasan_check_read+0x11/0x20 mm/kasan/kasan.c:272 > atomic_read include/asm-generic/atomic-instrumented.h:21 [inline] > refcount_inc_not_zero_checked+0x97/0x2f0 lib/refcount.c:120 > sk_psock_get include/linux/skmsg.h:379 [inline] > sock_map_link.isra.6+0x41f/0xe30 net/core/sock_map.c:178 > sock_hash_update_common+0x19b/0x11e0 net/core/sock_map.c:669 > sock_hash_update_elem+0x306/0x470 net/core/sock_map.c:738 > map_update_elem+0x819/0xdf0 kernel/bpf/syscall.c:818 > > Signed-off-by: John Fastabend <john.fastabend@gmail.com> > Reported-by: Eric Dumazet <eric.dumazet@gmail.com> > Fixes: 604326b41a6f ("bpf, sockmap: convert to generic sk_msg interface") > --- > include/linux/skmsg.h | 25 ++++++++++++++++++++----- > net/core/sock_map.c | 11 ++++++----- > 2 files changed, 26 insertions(+), 10 deletions(-) > > diff --git a/include/linux/skmsg.h b/include/linux/skmsg.h > index 677b673..f44ca6b 100644 > --- a/include/linux/skmsg.h > +++ b/include/linux/skmsg.h > @@ -275,11 +275,6 @@ static inline struct sk_psock *sk_psock(const struct sock *sk) > return rcu_dereference_sk_user_data(sk); > } > > -static inline bool sk_has_psock(struct sock *sk) > -{ > - return sk_psock(sk) != NULL && sk->sk_prot->recvmsg == tcp_bpf_recvmsg; > -} > - > static inline void sk_psock_queue_msg(struct sk_psock *psock, > struct sk_msg *msg) > { > @@ -379,6 +374,26 @@ static inline bool sk_psock_test_state(const struct sk_psock *psock, > return test_bit(bit, &psock->state); > } > > +static inline struct sk_psock *sk_psock_get_checked(struct sock *sk) > +{ > + struct sk_psock *psock; > + > + rcu_read_lock(); > + psock = sk_psock(sk); > + if (psock) { > + if (sk->sk_prot->recvmsg != tcp_bpf_recvmsg) { > + psock = ERR_PTR(-EBUSY); > + goto out; > + } > + > + if (!refcount_inc_not_zero(&psock->refcnt)) > + psock = NULL; Caller is using IS_ERR(), so you probably want to : psock = ERR_PTR(-E<something>); > + } > +out: > + rcu_read_unlock(); > + return psock; > +} > + > static inline struct sk_psock *sk_psock_get(struct sock *sk) > { > struct sk_psock *psock; > diff --git a/net/core/sock_map.c b/net/core/sock_map.c > index 3c0e44c..be6092a 100644 > --- a/net/core/sock_map.c > +++ b/net/core/sock_map.c > @@ -175,12 +175,13 @@ static int sock_map_link(struct bpf_map *map, struct sk_psock_progs *progs, > } > } > > - psock = sk_psock_get(sk); > + psock = sk_psock_get_checked(sk); > + if (IS_ERR(psock)) { > + ret = PTR_ERR(psock); > + goto out_progs; > + } > + > if (psock) { > - if (!sk_has_psock(sk)) { > - ret = -EBUSY; > - goto out_progs; > - } > if ((msg_parser && READ_ONCE(psock->progs.msg_parser)) || > (skb_progs && READ_ONCE(psock->progs.skb_parser))) { > sk_psock_put(sk, psock); > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bpf-next v2 1/2] bpf: skmsg, fix psock create on existing kcm/tls port 2018-10-18 17:34 ` Eric Dumazet @ 2018-10-18 20:24 ` John Fastabend 0 siblings, 0 replies; 5+ messages in thread From: John Fastabend @ 2018-10-18 20:24 UTC (permalink / raw) To: Eric Dumazet, ast, daniel; +Cc: netdev On 10/18/2018 10:34 AM, Eric Dumazet wrote: > > > On 10/17/2018 10:20 PM, John Fastabend wrote: >> Before using the psock returned by sk_psock_get() when adding it to a >> sockmap we need to ensure it is actually a sockmap based psock. >> Previously we were only checking this after incrementing the reference >> counter which was an error. This resulted in a slab-out-of-bounds >> error when the psock was not actually a sockmap type. >> >> This moves the check up so the reference counter is only used >> if it is a sockmap psock. >> >> Eric reported the following KASAN BUG, >> >> BUG: KASAN: slab-out-of-bounds in atomic_read include/asm-generic/atomic-instrumented.h:21 [inline] >> BUG: KASAN: slab-out-of-bounds in refcount_inc_not_zero_checked+0x97/0x2f0 lib/refcount.c:120 >> Read of size 4 at addr ffff88019548be58 by task syz-executor4/22387 >> >> CPU: 1 PID: 22387 Comm: syz-executor4 Not tainted 4.19.0-rc7+ #264 >> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 >> Call Trace: >> __dump_stack lib/dump_stack.c:77 [inline] >> dump_stack+0x1c4/0x2b4 lib/dump_stack.c:113 >> print_address_description.cold.8+0x9/0x1ff mm/kasan/report.c:256 >> kasan_report_error mm/kasan/report.c:354 [inline] >> kasan_report.cold.9+0x242/0x309 mm/kasan/report.c:412 >> check_memory_region_inline mm/kasan/kasan.c:260 [inline] >> check_memory_region+0x13e/0x1b0 mm/kasan/kasan.c:267 >> kasan_check_read+0x11/0x20 mm/kasan/kasan.c:272 >> atomic_read include/asm-generic/atomic-instrumented.h:21 [inline] >> refcount_inc_not_zero_checked+0x97/0x2f0 lib/refcount.c:120 >> sk_psock_get include/linux/skmsg.h:379 [inline] >> sock_map_link.isra.6+0x41f/0xe30 net/core/sock_map.c:178 >> sock_hash_update_common+0x19b/0x11e0 net/core/sock_map.c:669 >> sock_hash_update_elem+0x306/0x470 net/core/sock_map.c:738 >> map_update_elem+0x819/0xdf0 kernel/bpf/syscall.c:818 >> >> Signed-off-by: John Fastabend <john.fastabend@gmail.com> >> Reported-by: Eric Dumazet <eric.dumazet@gmail.com> >> Fixes: 604326b41a6f ("bpf, sockmap: convert to generic sk_msg interface") >> --- [...] >> +static inline struct sk_psock *sk_psock_get_checked(struct sock *sk) >> +{ >> + struct sk_psock *psock; >> + >> + rcu_read_lock(); >> + psock = sk_psock(sk); >> + if (psock) { >> + if (sk->sk_prot->recvmsg != tcp_bpf_recvmsg) { >> + psock = ERR_PTR(-EBUSY); >> + goto out; >> + } >> + >> + if (!refcount_inc_not_zero(&psock->refcnt)) >> + psock = NULL; > > Caller is using IS_ERR(), so you probably want to : > > psock = ERR_PTR(-E<something>); > > Yeah we can make this EBUSY as well. Originally I was thinking that we could create the psock and attach it in this case but it would be racy and require an rcu sync most likely. To hit this case users would need to have multiple maps and be adding/deleting socks from those maps at the same time. Seems pretty rare and not worth punishing the normal case with synchronization. Nice catch. .John ^ permalink raw reply [flat|nested] 5+ messages in thread
* [bpf-next v2 2/2] bpf: test_maps add a test to catch kcm + sockmap 2018-10-18 5:20 [bpf-next v2 0/2] Fix kcm + sockmap by checking psock type John Fastabend 2018-10-18 5:20 ` [bpf-next v2 1/2] bpf: skmsg, fix psock create on existing kcm/tls port John Fastabend @ 2018-10-18 5:20 ` John Fastabend 1 sibling, 0 replies; 5+ messages in thread From: John Fastabend @ 2018-10-18 5:20 UTC (permalink / raw) To: ast, daniel; +Cc: john.fastabend, netdev, eric.dumazet Adding a socket to both sockmap and kcm is not supported due to collision on sk_user_data usage. If selftests is run without KCM support we will issue a warning and continue with the tests. Signed-off-by: John Fastabend <john.fastabend@gmail.com> --- tools/testing/selftests/bpf/Makefile | 2 +- tools/testing/selftests/bpf/sockmap_kcm.c | 14 +++++++ tools/testing/selftests/bpf/test_maps.c | 64 ++++++++++++++++++++++++++++++- 3 files changed, 77 insertions(+), 3 deletions(-) create mode 100644 tools/testing/selftests/bpf/sockmap_kcm.c diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile index d99dd6f..f290554 100644 --- a/tools/testing/selftests/bpf/Makefile +++ b/tools/testing/selftests/bpf/Makefile @@ -28,7 +28,7 @@ TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_lpm_map test TEST_GEN_FILES = test_pkt_access.o test_xdp.o test_l4lb.o test_tcp_estats.o test_obj_id.o \ test_pkt_md_access.o test_xdp_redirect.o test_xdp_meta.o sockmap_parse_prog.o \ - sockmap_verdict_prog.o dev_cgroup.o sample_ret0.o test_tracepoint.o \ + sockmap_verdict_prog.o sockmap_kcm.o dev_cgroup.o sample_ret0.o test_tracepoint.o \ test_l4lb_noinline.o test_xdp_noinline.o test_stacktrace_map.o \ sample_map_ret0.o test_tcpbpf_kern.o test_stacktrace_build_id.o \ sockmap_tcp_msg_prog.o connect4_prog.o connect6_prog.o test_adjust_tail.o \ diff --git a/tools/testing/selftests/bpf/sockmap_kcm.c b/tools/testing/selftests/bpf/sockmap_kcm.c new file mode 100644 index 0000000..4377adc --- /dev/null +++ b/tools/testing/selftests/bpf/sockmap_kcm.c @@ -0,0 +1,14 @@ +#include <linux/bpf.h> +#include "bpf_helpers.h" +#include "bpf_util.h" +#include "bpf_endian.h" + +int _version SEC("version") = 1; + +SEC("socket_kcm") +int bpf_prog1(struct __sk_buff *skb) +{ + return skb->len; +} + +char _license[] SEC("license") = "GPL"; diff --git a/tools/testing/selftests/bpf/test_maps.c b/tools/testing/selftests/bpf/test_maps.c index 9b552c0..be20f1d 100644 --- a/tools/testing/selftests/bpf/test_maps.c +++ b/tools/testing/selftests/bpf/test_maps.c @@ -20,6 +20,7 @@ #include <sys/socket.h> #include <netinet/in.h> #include <linux/bpf.h> +#include <linux/kcm.h> #include <bpf/bpf.h> #include <bpf/libbpf.h> @@ -479,14 +480,16 @@ static void test_devmap(int task, void *data) #define SOCKMAP_PARSE_PROG "./sockmap_parse_prog.o" #define SOCKMAP_VERDICT_PROG "./sockmap_verdict_prog.o" #define SOCKMAP_TCP_MSG_PROG "./sockmap_tcp_msg_prog.o" +#define KCM_PROG "./sockmap_kcm.o" static void test_sockmap(int tasks, void *data) { struct bpf_map *bpf_map_rx, *bpf_map_tx, *bpf_map_msg, *bpf_map_break; - int map_fd_msg = 0, map_fd_rx = 0, map_fd_tx = 0, map_fd_break; + int map_fd_msg = 0, map_fd_rx = 0, map_fd_tx = 0, map_fd_break, kcm; int ports[] = {50200, 50201, 50202, 50204}; int err, i, fd, udp, sfd[6] = {0xdeadbeef}; u8 buf[20] = {0x0, 0x5, 0x3, 0x2, 0x1, 0x0}; - int parse_prog, verdict_prog, msg_prog; + int parse_prog, verdict_prog, msg_prog, kcm_prog; + struct kcm_attach attach_info; struct sockaddr_in addr; int one = 1, s, sc, rc; struct bpf_object *obj; @@ -744,6 +747,62 @@ static void test_sockmap(int tasks, void *data) goto out_sockmap; } + /* Test adding a KCM socket into map */ +#define AF_KCM 41 + kcm = socket(AF_KCM, SOCK_DGRAM, KCMPROTO_CONNECTED); + if (kcm == -1) { + printf("Warning, KCM+Sockmap could not be tested.\n"); + goto skip_kcm; + } + + err = bpf_prog_load(KCM_PROG, + BPF_PROG_TYPE_SOCKET_FILTER, + &obj, &kcm_prog); + if (err) { + printf("Failed to load SK_SKB parse prog\n"); + goto out_sockmap; + } + + i = 2; + memset(&attach_info, 0, sizeof(attach_info)); + attach_info.fd = sfd[i]; + attach_info.bpf_fd = kcm_prog; + err = ioctl(kcm, SIOCKCMATTACH, &attach_info); + if (!err) { + perror("Failed KCM attached to sockmap fd: "); + goto out_sockmap; + } + + err = bpf_map_delete_elem(fd, &i); + if (err) { + printf("Failed delete sockmap from empty map %i %i\n", err, errno); + goto out_sockmap; + } + + err = ioctl(kcm, SIOCKCMATTACH, &attach_info); + if (err) { + perror("Failed KCM attach"); + goto out_sockmap; + } + + err = bpf_map_update_elem(fd, &i, &sfd[i], BPF_ANY); + if (!err) { + printf("Failed sockmap attached KCM sock!\n"); + goto out_sockmap; + } + err = ioctl(kcm, SIOCKCMUNATTACH, &attach_info); + if (err) { + printf("Failed detach KCM sock!\n"); + goto out_sockmap; + } + + err = bpf_map_update_elem(fd, &i, &sfd[i], BPF_ANY); + if (err) { + printf("Failed post-kcm update sockmap '%i:%i'\n", + i, sfd[i]); + goto out_sockmap; + } + /* Test map update elem afterwards fd lives in fd and map_fd */ for (i = 2; i < 6; i++) { err = bpf_map_update_elem(map_fd_rx, &i, &sfd[i], BPF_ANY); @@ -776,6 +835,7 @@ static void test_sockmap(int tasks, void *data) } } +skip_kcm: /* Put sfd[2] (sending fd below) into msg map to test sendmsg bpf */ i = 0; err = bpf_map_update_elem(map_fd_msg, &i, &sfd[2], BPF_ANY); -- 1.9.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-10-19 4:27 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-10-18 5:20 [bpf-next v2 0/2] Fix kcm + sockmap by checking psock type John Fastabend 2018-10-18 5:20 ` [bpf-next v2 1/2] bpf: skmsg, fix psock create on existing kcm/tls port John Fastabend 2018-10-18 17:34 ` Eric Dumazet 2018-10-18 20:24 ` John Fastabend 2018-10-18 5:20 ` [bpf-next v2 2/2] bpf: test_maps add a test to catch kcm + sockmap John Fastabend
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).