* [bpf-next v3 0/2] Fix kcm + sockmap by checking psock type
@ 2018-10-18 20:58 John Fastabend
2018-10-18 20:58 ` [bpf-next v3 1/2] bpf: skmsg, fix psock create on existing kcm/tls port John Fastabend
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: John Fastabend @ 2018-10-18 20:58 UTC (permalink / raw)
To: ast, daniel, eric.dumazet; +Cc: john.fastabend, netdev
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
v3: return EBUSY if refcnt is zero
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] 6+ messages in thread
* [bpf-next v3 1/2] bpf: skmsg, fix psock create on existing kcm/tls port
2018-10-18 20:58 [bpf-next v3 0/2] Fix kcm + sockmap by checking psock type John Fastabend
@ 2018-10-18 20:58 ` John Fastabend
2018-10-18 20:58 ` [bpf-next v3 2/2] bpf: test_maps add a test to catch kcm + sockmap John Fastabend
2018-10-19 22:51 ` [bpf-next v3 0/2] Fix kcm + sockmap by checking psock type Daniel Borkmann
2 siblings, 0 replies; 6+ messages in thread
From: John Fastabend @ 2018-10-18 20:58 UTC (permalink / raw)
To: ast, daniel, eric.dumazet; +Cc: john.fastabend, netdev
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 22347b0..84e1886 100644
--- a/include/linux/skmsg.h
+++ b/include/linux/skmsg.h
@@ -270,11 +270,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)
{
@@ -374,6 +369,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 = ERR_PTR(-EBUSY);
+ }
+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] 6+ messages in thread
* [bpf-next v3 2/2] bpf: test_maps add a test to catch kcm + sockmap
2018-10-18 20:58 [bpf-next v3 0/2] Fix kcm + sockmap by checking psock type John Fastabend
2018-10-18 20:58 ` [bpf-next v3 1/2] bpf: skmsg, fix psock create on existing kcm/tls port John Fastabend
@ 2018-10-18 20:58 ` John Fastabend
2018-10-19 22:51 ` [bpf-next v3 0/2] Fix kcm + sockmap by checking psock type Daniel Borkmann
2 siblings, 0 replies; 6+ messages in thread
From: John Fastabend @ 2018-10-18 20:58 UTC (permalink / raw)
To: ast, daniel, eric.dumazet; +Cc: john.fastabend, netdev
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] 6+ messages in thread
* Re: [bpf-next v3 0/2] Fix kcm + sockmap by checking psock type
2018-10-18 20:58 [bpf-next v3 0/2] Fix kcm + sockmap by checking psock type John Fastabend
2018-10-18 20:58 ` [bpf-next v3 1/2] bpf: skmsg, fix psock create on existing kcm/tls port John Fastabend
2018-10-18 20:58 ` [bpf-next v3 2/2] bpf: test_maps add a test to catch kcm + sockmap John Fastabend
@ 2018-10-19 22:51 ` Daniel Borkmann
2018-10-19 22:57 ` Daniel Borkmann
2 siblings, 1 reply; 6+ messages in thread
From: Daniel Borkmann @ 2018-10-19 22:51 UTC (permalink / raw)
To: John Fastabend, ast, eric.dumazet; +Cc: netdev
On 10/18/2018 10:58 PM, John Fastabend wrote:
> 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
> v3: return EBUSY if refcnt is zero
>
> 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
Applied, thanks!
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [bpf-next v3 0/2] Fix kcm + sockmap by checking psock type
2018-10-19 22:51 ` [bpf-next v3 0/2] Fix kcm + sockmap by checking psock type Daniel Borkmann
@ 2018-10-19 22:57 ` Daniel Borkmann
2018-10-19 23:38 ` John Fastabend
0 siblings, 1 reply; 6+ messages in thread
From: Daniel Borkmann @ 2018-10-19 22:57 UTC (permalink / raw)
To: John Fastabend, ast, eric.dumazet; +Cc: netdev
On 10/20/2018 12:51 AM, Daniel Borkmann wrote:
> On 10/18/2018 10:58 PM, John Fastabend wrote:
>> 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
>> v3: return EBUSY if refcnt is zero
>>
>> 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
>
> Applied, thanks!
Fyi, I've only applied patch 1/2 for now to get the bug fixed. The patch 2/2 throws
a bunch of warnings that look like the below. Also, I think we leak kcm socket in
error paths and once we're done with testing, so would be good to close it once
unneeded. Please respin the test as a stand-alone commit, thanks:
[...]
bpf-next/tools/testing/selftests/bpf/libbpf.a -lcap -lelf -lrt -lpthread -o /home/darkstar/trees/bpf-next-ok/tools/testing/selftests/bpf/test_maps
test_maps.c: In function ‘test_sockmap’:
test_maps.c:869:0: warning: "AF_KCM" redefined
#define AF_KCM 41
In file included from /usr/include/sys/socket.h:38:0,
from test_maps.c:21:
/usr/include/bits/socket.h:133:0: note: this is the location of the previous definition
#define AF_KCM PF_KCM
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [bpf-next v3 0/2] Fix kcm + sockmap by checking psock type
2018-10-19 22:57 ` Daniel Borkmann
@ 2018-10-19 23:38 ` John Fastabend
0 siblings, 0 replies; 6+ messages in thread
From: John Fastabend @ 2018-10-19 23:38 UTC (permalink / raw)
To: Daniel Borkmann, ast, eric.dumazet; +Cc: netdev
On 10/19/2018 03:57 PM, Daniel Borkmann wrote:
> On 10/20/2018 12:51 AM, Daniel Borkmann wrote:
>> On 10/18/2018 10:58 PM, John Fastabend wrote:
>>> 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
>>> v3: return EBUSY if refcnt is zero
>>>
>>> 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
>>
>> Applied, thanks!
>
> Fyi, I've only applied patch 1/2 for now to get the bug fixed. The patch 2/2 throws
> a bunch of warnings that look like the below. Also, I think we leak kcm socket in
> error paths and once we're done with testing, so would be good to close it once
> unneeded. Please respin the test as a stand-alone commit, thanks:
>
Thanks, I didn't see the warnings below locally but will look
into spinning a good version tonight with the closing sock fix
as well.
John
> [...]
> bpf-next/tools/testing/selftests/bpf/libbpf.a -lcap -lelf -lrt -lpthread -o /home/darkstar/trees/bpf-next-ok/tools/testing/selftests/bpf/test_maps
> test_maps.c: In function ‘test_sockmap’:
> test_maps.c:869:0: warning: "AF_KCM" redefined
> #define AF_KCM 41
>
> In file included from /usr/include/sys/socket.h:38:0,
> from test_maps.c:21:
> /usr/include/bits/socket.h:133:0: note: this is the location of the previous definition
> #define AF_KCM PF_KCM
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-10-20 7:46 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-18 20:58 [bpf-next v3 0/2] Fix kcm + sockmap by checking psock type John Fastabend
2018-10-18 20:58 ` [bpf-next v3 1/2] bpf: skmsg, fix psock create on existing kcm/tls port John Fastabend
2018-10-18 20:58 ` [bpf-next v3 2/2] bpf: test_maps add a test to catch kcm + sockmap John Fastabend
2018-10-19 22:51 ` [bpf-next v3 0/2] Fix kcm + sockmap by checking psock type Daniel Borkmann
2018-10-19 22:57 ` Daniel Borkmann
2018-10-19 23:38 ` John Fastabend
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.