| Message ID | 20200623003636.3074473-1-yhs@fb.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | implement bpf iterator for tcp and udp sockets | expand |
On 6/22/20 5:36 PM, Yonghong Song wrote: > The helper is used in tracing programs to cast a socket > pointer to a udp6_sock pointer. > The return value could be NULL if the casting is illegal. > > Acked-by: Martin KaFai Lau <kafai@fb.com> > Signed-off-by: Yonghong Song <yhs@fb.com> > --- > include/linux/bpf.h | 1 + > include/uapi/linux/bpf.h | 9 ++++++++- > kernel/trace/bpf_trace.c | 2 ++ > net/core/filter.c | 22 ++++++++++++++++++++++ > scripts/bpf_helpers_doc.py | 2 ++ > tools/include/uapi/linux/bpf.h | 9 ++++++++- > 6 files changed, 43 insertions(+), 2 deletions(-) > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > index cc3f89827b89..3f5c6bb5e3a7 100644 > --- a/include/linux/bpf.h > +++ b/include/linux/bpf.h > @@ -1649,6 +1649,7 @@ extern const struct bpf_func_proto bpf_skc_to_tcp6_sock_proto; > extern const struct bpf_func_proto bpf_skc_to_tcp_sock_proto; > extern const struct bpf_func_proto bpf_skc_to_tcp_timewait_sock_proto; > extern const struct bpf_func_proto bpf_skc_to_tcp_request_sock_proto; > +extern const struct bpf_func_proto bpf_skc_to_udp6_sock_proto; > > const struct bpf_func_proto *bpf_tracing_func_proto( > enum bpf_func_id func_id, const struct bpf_prog *prog); > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > index e256417d94c2..3f4b12c5c563 100644 > --- a/include/uapi/linux/bpf.h > +++ b/include/uapi/linux/bpf.h > @@ -3276,6 +3276,12 @@ union bpf_attr { > * Dynamically cast a *sk* pointer to a *tcp_request_sock* pointer. > * Return > * *sk* if casting is valid, or NULL otherwise. > + * > + * struct udp6_sock *bpf_skc_to_udp6_sock(void *sk) > + * Description > + * Dynamically cast a *sk* pointer to a *udp6_sock* pointer. > + * Return > + * *sk* if casting is valid, or NULL otherwise. > */ > #define __BPF_FUNC_MAPPER(FN) \ > FN(unspec), \ > @@ -3417,7 +3423,8 @@ union bpf_attr { > FN(skc_to_tcp6_sock), \ > FN(skc_to_tcp_sock), \ > FN(skc_to_tcp_timewait_sock), \ > - FN(skc_to_tcp_request_sock), > + FN(skc_to_tcp_request_sock), \ > + FN(skc_to_udp6_sock), > > /* integer value in 'imm' field of BPF_CALL instruction selects which helper > * function eBPF program intends to call > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > index de5fbe66e1ca..d10ab16c4a2f 100644 > --- a/kernel/trace/bpf_trace.c > +++ b/kernel/trace/bpf_trace.c > @@ -1523,6 +1523,8 @@ tracing_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) > return &bpf_skc_to_tcp_timewait_sock_proto; > case BPF_FUNC_skc_to_tcp_request_sock: > return &bpf_skc_to_tcp_request_sock_proto; > + case BPF_FUNC_skc_to_udp6_sock: > + return &bpf_skc_to_udp6_sock_proto; > #endif > case BPF_FUNC_seq_printf: > return prog->expected_attach_type == BPF_TRACE_ITER ? > diff --git a/net/core/filter.c b/net/core/filter.c > index 140fc0fdf3e1..9a98f3616273 100644 > --- a/net/core/filter.c > +++ b/net/core/filter.c > @@ -9325,3 +9325,25 @@ const struct bpf_func_proto bpf_skc_to_tcp_request_sock_proto = { > .check_btf_id = check_arg_btf_id, > .ret_btf_id = &btf_sock_ids[BTF_SOCK_TYPE_TCP_REQ], > }; > + > +BPF_CALL_1(bpf_skc_to_udp6_sock, struct sock *, sk) > +{ > + /* udp6_sock type is not generated in dwarf and hence btf, > + * trigger an explicit type generation here. > + */ > + BTF_TYPE_EMIT(struct udp6_sock); > + if (sk_fullsock(sk) && sk->sk_protocol == IPPROTO_UDP && Why is the sk_fullsock(sk) needed ? > + sk->sk_family == AF_INET6) > + return (unsigned long)sk; > + > + return (unsigned long)NULL; > +} > + > +const struct bpf_func_proto bpf_skc_to_udp6_sock_proto = { > + .func = bpf_skc_to_udp6_sock, > + .gpl_only = false, > + .ret_type = RET_PTR_TO_BTF_ID_OR_NULL, > + .arg1_type = ARG_PTR_TO_BTF_ID, > + .check_btf_id = check_arg_btf_id, > + .ret_btf_id = &btf_sock_ids[BTF_SOCK_TYPE_UDP6], > +}; > diff --git a/scripts/bpf_helpers_doc.py b/scripts/bpf_helpers_doc.py > index d886657c6aaa..6bab40ff442e 100755 > --- a/scripts/bpf_helpers_doc.py > +++ b/scripts/bpf_helpers_doc.py > @@ -425,6 +425,7 @@ class PrinterHelpers(Printer): > 'struct tcp_sock', > 'struct tcp_timewait_sock', > 'struct tcp_request_sock', > + 'struct udp6_sock', > > 'struct __sk_buff', > 'struct sk_msg_md', > @@ -466,6 +467,7 @@ class PrinterHelpers(Printer): > 'struct tcp_sock', > 'struct tcp_timewait_sock', > 'struct tcp_request_sock', > + 'struct udp6_sock', > } > mapped_types = { > 'u8': '__u8', > diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h > index e256417d94c2..3f4b12c5c563 100644 > --- a/tools/include/uapi/linux/bpf.h > +++ b/tools/include/uapi/linux/bpf.h > @@ -3276,6 +3276,12 @@ union bpf_attr { > * Dynamically cast a *sk* pointer to a *tcp_request_sock* pointer. > * Return > * *sk* if casting is valid, or NULL otherwise. > + * > + * struct udp6_sock *bpf_skc_to_udp6_sock(void *sk) > + * Description > + * Dynamically cast a *sk* pointer to a *udp6_sock* pointer. > + * Return > + * *sk* if casting is valid, or NULL otherwise. > */ > #define __BPF_FUNC_MAPPER(FN) \ > FN(unspec), \ > @@ -3417,7 +3423,8 @@ union bpf_attr { > FN(skc_to_tcp6_sock), \ > FN(skc_to_tcp_sock), \ > FN(skc_to_tcp_timewait_sock), \ > - FN(skc_to_tcp_request_sock), > + FN(skc_to_tcp_request_sock), \ > + FN(skc_to_udp6_sock), > > /* integer value in 'imm' field of BPF_CALL instruction selects which helper > * function eBPF program intends to call >
On 6/22/20 6:47 PM, Eric Dumazet wrote: > > > On 6/22/20 5:36 PM, Yonghong Song wrote: >> The helper is used in tracing programs to cast a socket >> pointer to a udp6_sock pointer. >> The return value could be NULL if the casting is illegal. >> >> Acked-by: Martin KaFai Lau <kafai@fb.com> >> Signed-off-by: Yonghong Song <yhs@fb.com> >> --- >> include/linux/bpf.h | 1 + >> include/uapi/linux/bpf.h | 9 ++++++++- >> kernel/trace/bpf_trace.c | 2 ++ >> net/core/filter.c | 22 ++++++++++++++++++++++ >> scripts/bpf_helpers_doc.py | 2 ++ >> tools/include/uapi/linux/bpf.h | 9 ++++++++- >> 6 files changed, 43 insertions(+), 2 deletions(-) >> >> diff --git a/include/linux/bpf.h b/include/linux/bpf.h >> index cc3f89827b89..3f5c6bb5e3a7 100644 >> --- a/include/linux/bpf.h >> +++ b/include/linux/bpf.h >> @@ -1649,6 +1649,7 @@ extern const struct bpf_func_proto bpf_skc_to_tcp6_sock_proto; >> extern const struct bpf_func_proto bpf_skc_to_tcp_sock_proto; >> extern const struct bpf_func_proto bpf_skc_to_tcp_timewait_sock_proto; >> extern const struct bpf_func_proto bpf_skc_to_tcp_request_sock_proto; >> +extern const struct bpf_func_proto bpf_skc_to_udp6_sock_proto; >> >> const struct bpf_func_proto *bpf_tracing_func_proto( >> enum bpf_func_id func_id, const struct bpf_prog *prog); >> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h >> index e256417d94c2..3f4b12c5c563 100644 >> --- a/include/uapi/linux/bpf.h >> +++ b/include/uapi/linux/bpf.h >> @@ -3276,6 +3276,12 @@ union bpf_attr { >> * Dynamically cast a *sk* pointer to a *tcp_request_sock* pointer. >> * Return >> * *sk* if casting is valid, or NULL otherwise. >> + * >> + * struct udp6_sock *bpf_skc_to_udp6_sock(void *sk) >> + * Description >> + * Dynamically cast a *sk* pointer to a *udp6_sock* pointer. >> + * Return >> + * *sk* if casting is valid, or NULL otherwise. >> */ >> #define __BPF_FUNC_MAPPER(FN) \ >> FN(unspec), \ >> @@ -3417,7 +3423,8 @@ union bpf_attr { >> FN(skc_to_tcp6_sock), \ >> FN(skc_to_tcp_sock), \ >> FN(skc_to_tcp_timewait_sock), \ >> - FN(skc_to_tcp_request_sock), >> + FN(skc_to_tcp_request_sock), \ >> + FN(skc_to_udp6_sock), >> >> /* integer value in 'imm' field of BPF_CALL instruction selects which helper >> * function eBPF program intends to call >> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c >> index de5fbe66e1ca..d10ab16c4a2f 100644 >> --- a/kernel/trace/bpf_trace.c >> +++ b/kernel/trace/bpf_trace.c >> @@ -1523,6 +1523,8 @@ tracing_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) >> return &bpf_skc_to_tcp_timewait_sock_proto; >> case BPF_FUNC_skc_to_tcp_request_sock: >> return &bpf_skc_to_tcp_request_sock_proto; >> + case BPF_FUNC_skc_to_udp6_sock: >> + return &bpf_skc_to_udp6_sock_proto; >> #endif >> case BPF_FUNC_seq_printf: >> return prog->expected_attach_type == BPF_TRACE_ITER ? >> diff --git a/net/core/filter.c b/net/core/filter.c >> index 140fc0fdf3e1..9a98f3616273 100644 >> --- a/net/core/filter.c >> +++ b/net/core/filter.c >> @@ -9325,3 +9325,25 @@ const struct bpf_func_proto bpf_skc_to_tcp_request_sock_proto = { >> .check_btf_id = check_arg_btf_id, >> .ret_btf_id = &btf_sock_ids[BTF_SOCK_TYPE_TCP_REQ], >> }; >> + >> +BPF_CALL_1(bpf_skc_to_udp6_sock, struct sock *, sk) >> +{ >> + /* udp6_sock type is not generated in dwarf and hence btf, >> + * trigger an explicit type generation here. >> + */ >> + BTF_TYPE_EMIT(struct udp6_sock); >> + if (sk_fullsock(sk) && sk->sk_protocol == IPPROTO_UDP && > > Why is the sk_fullsock(sk) needed ? The parameter 'sk' could be a sock_common. That is why the helper name bpf_skc_to_udp6_sock implies. The sock_common cannot access sk_protocol, hence we requires sk_fullsock(sk) here. Did I miss anything? > >> + sk->sk_family == AF_INET6) >> + return (unsigned long)sk; >> + >> + return (unsigned long)NULL; >> +} >> + >> +const struct bpf_func_proto bpf_skc_to_udp6_sock_proto = { >> + .func = bpf_skc_to_udp6_sock, >> + .gpl_only = false, >> + .ret_type = RET_PTR_TO_BTF_ID_OR_NULL, >> + .arg1_type = ARG_PTR_TO_BTF_ID, >> + .check_btf_id = check_arg_btf_id, >> + .ret_btf_id = &btf_sock_ids[BTF_SOCK_TYPE_UDP6], >> +}; [...]
On 6/22/20 7:22 PM, Yonghong Song wrote: > > > On 6/22/20 6:47 PM, Eric Dumazet wrote: & >> >> Why is the sk_fullsock(sk) needed ? > > The parameter 'sk' could be a sock_common. That is why the > helper name bpf_skc_to_udp6_sock implies. The sock_common cannot > access sk_protocol, hence we requires sk_fullsock(sk) here. > Did I miss anything? OK, if arbitrary sockets can land here, you need also to check sk_type
On 6/23/20 9:27 AM, Eric Dumazet wrote: > > > On 6/22/20 7:22 PM, Yonghong Song wrote: >> >> >> On 6/22/20 6:47 PM, Eric Dumazet wrote: > & >>> >>> Why is the sk_fullsock(sk) needed ? >> >> The parameter 'sk' could be a sock_common. That is why the >> helper name bpf_skc_to_udp6_sock implies. The sock_common cannot >> access sk_protocol, hence we requires sk_fullsock(sk) here. >> Did I miss anything? > > OK, if arbitrary sockets can land here, you need also to check sk_type The current check is: if (sk_fullsock(sk) && sk->sk_protocol == IPPROTO_UDP && sk->sk_family == AF_INET6) return (unsigned long)sk; it checks to ensure it is full socket, it is a ipv6 socket and then check protocol. Are you suggesting to add the following check? sk->sk_type == SOCK_DGRAM Not a networking expert. Maybe you can explain when we could have protocol is IPPROTO_UDP and sk_type not SOCK_DGRAM?
On 6/23/20 10:03 AM, Yonghong Song wrote: > > > On 6/23/20 9:27 AM, Eric Dumazet wrote: >> >> >> On 6/22/20 7:22 PM, Yonghong Song wrote: >>> >>> >>> On 6/22/20 6:47 PM, Eric Dumazet wrote: >> & >>>> >>>> Why is the sk_fullsock(sk) needed ? >>> >>> The parameter 'sk' could be a sock_common. That is why the >>> helper name bpf_skc_to_udp6_sock implies. The sock_common cannot >>> access sk_protocol, hence we requires sk_fullsock(sk) here. >>> Did I miss anything? >> >> OK, if arbitrary sockets can land here, you need also to check sk_type > > The current check is: > if (sk_fullsock(sk) && sk->sk_protocol == IPPROTO_UDP && > sk->sk_family == AF_INET6) > return (unsigned long)sk; > it checks to ensure it is full socket, it is a ipv6 socket and then check protocol. > > Are you suggesting to add the following check? > sk->sk_type == SOCK_DGRAM > > Not a networking expert. Maybe you can explain when we could have > protocol is IPPROTO_UDP and sk_type not SOCK_DGRAM? RAW sockets for instance. Look at : commit 940ba14986657a50c15f694efca1beba31fa568f Author: Eric Dumazet <edumazet@google.com> Date: Tue Jan 21 23:17:14 2020 -0800 gtp: make sure only SOCK_DGRAM UDP sockets are accepted A malicious user could use RAW sockets and fool GTP using them as standard SOCK_DGRAM UDP sockets.
On 6/23/20 3:11 PM, Eric Dumazet wrote: > > > On 6/23/20 10:03 AM, Yonghong Song wrote: >> >> >> On 6/23/20 9:27 AM, Eric Dumazet wrote: >>> >>> >>> On 6/22/20 7:22 PM, Yonghong Song wrote: >>>> >>>> >>>> On 6/22/20 6:47 PM, Eric Dumazet wrote: >>> & >>>>> >>>>> Why is the sk_fullsock(sk) needed ? >>>> >>>> The parameter 'sk' could be a sock_common. That is why the >>>> helper name bpf_skc_to_udp6_sock implies. The sock_common cannot >>>> access sk_protocol, hence we requires sk_fullsock(sk) here. >>>> Did I miss anything? >>> >>> OK, if arbitrary sockets can land here, you need also to check sk_type >> >> The current check is: >> if (sk_fullsock(sk) && sk->sk_protocol == IPPROTO_UDP && >> sk->sk_family == AF_INET6) >> return (unsigned long)sk; >> it checks to ensure it is full socket, it is a ipv6 socket and then check protocol. >> >> Are you suggesting to add the following check? >> sk->sk_type == SOCK_DGRAM >> >> Not a networking expert. Maybe you can explain when we could have >> protocol is IPPROTO_UDP and sk_type not SOCK_DGRAM? > > > RAW sockets for instance. > > Look at : > > commit 940ba14986657a50c15f694efca1beba31fa568f > Author: Eric Dumazet <edumazet@google.com> > Date: Tue Jan 21 23:17:14 2020 -0800 > > gtp: make sure only SOCK_DGRAM UDP sockets are accepted > > A malicious user could use RAW sockets and fool > GTP using them as standard SOCK_DGRAM UDP sockets. Thanks for the pointer! Will fix it in the next revision.
diff --git a/include/linux/bpf.h b/include/linux/bpf.h index cc3f89827b89..3f5c6bb5e3a7 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -1649,6 +1649,7 @@ extern const struct bpf_func_proto bpf_skc_to_tcp6_sock_proto; extern const struct bpf_func_proto bpf_skc_to_tcp_sock_proto; extern const struct bpf_func_proto bpf_skc_to_tcp_timewait_sock_proto; extern const struct bpf_func_proto bpf_skc_to_tcp_request_sock_proto; +extern const struct bpf_func_proto bpf_skc_to_udp6_sock_proto; const struct bpf_func_proto *bpf_tracing_func_proto( enum bpf_func_id func_id, const struct bpf_prog *prog); diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index e256417d94c2..3f4b12c5c563 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h @@ -3276,6 +3276,12 @@ union bpf_attr { * Dynamically cast a *sk* pointer to a *tcp_request_sock* pointer. * Return * *sk* if casting is valid, or NULL otherwise. + * + * struct udp6_sock *bpf_skc_to_udp6_sock(void *sk) + * Description + * Dynamically cast a *sk* pointer to a *udp6_sock* pointer. + * Return + * *sk* if casting is valid, or NULL otherwise. */ #define __BPF_FUNC_MAPPER(FN) \ FN(unspec), \ @@ -3417,7 +3423,8 @@ union bpf_attr { FN(skc_to_tcp6_sock), \ FN(skc_to_tcp_sock), \ FN(skc_to_tcp_timewait_sock), \ - FN(skc_to_tcp_request_sock), + FN(skc_to_tcp_request_sock), \ + FN(skc_to_udp6_sock), /* integer value in 'imm' field of BPF_CALL instruction selects which helper * function eBPF program intends to call diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c index de5fbe66e1ca..d10ab16c4a2f 100644 --- a/kernel/trace/bpf_trace.c +++ b/kernel/trace/bpf_trace.c @@ -1523,6 +1523,8 @@ tracing_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) return &bpf_skc_to_tcp_timewait_sock_proto; case BPF_FUNC_skc_to_tcp_request_sock: return &bpf_skc_to_tcp_request_sock_proto; + case BPF_FUNC_skc_to_udp6_sock: + return &bpf_skc_to_udp6_sock_proto; #endif case BPF_FUNC_seq_printf: return prog->expected_attach_type == BPF_TRACE_ITER ? diff --git a/net/core/filter.c b/net/core/filter.c index 140fc0fdf3e1..9a98f3616273 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -9325,3 +9325,25 @@ const struct bpf_func_proto bpf_skc_to_tcp_request_sock_proto = { .check_btf_id = check_arg_btf_id, .ret_btf_id = &btf_sock_ids[BTF_SOCK_TYPE_TCP_REQ], }; + +BPF_CALL_1(bpf_skc_to_udp6_sock, struct sock *, sk) +{ + /* udp6_sock type is not generated in dwarf and hence btf, + * trigger an explicit type generation here. + */ + BTF_TYPE_EMIT(struct udp6_sock); + if (sk_fullsock(sk) && sk->sk_protocol == IPPROTO_UDP && + sk->sk_family == AF_INET6) + return (unsigned long)sk; + + return (unsigned long)NULL; +} + +const struct bpf_func_proto bpf_skc_to_udp6_sock_proto = { + .func = bpf_skc_to_udp6_sock, + .gpl_only = false, + .ret_type = RET_PTR_TO_BTF_ID_OR_NULL, + .arg1_type = ARG_PTR_TO_BTF_ID, + .check_btf_id = check_arg_btf_id, + .ret_btf_id = &btf_sock_ids[BTF_SOCK_TYPE_UDP6], +}; diff --git a/scripts/bpf_helpers_doc.py b/scripts/bpf_helpers_doc.py index d886657c6aaa..6bab40ff442e 100755 --- a/scripts/bpf_helpers_doc.py +++ b/scripts/bpf_helpers_doc.py @@ -425,6 +425,7 @@ class PrinterHelpers(Printer): 'struct tcp_sock', 'struct tcp_timewait_sock', 'struct tcp_request_sock', + 'struct udp6_sock', 'struct __sk_buff', 'struct sk_msg_md', @@ -466,6 +467,7 @@ class PrinterHelpers(Printer): 'struct tcp_sock', 'struct tcp_timewait_sock', 'struct tcp_request_sock', + 'struct udp6_sock', } mapped_types = { 'u8': '__u8', diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h index e256417d94c2..3f4b12c5c563 100644 --- a/tools/include/uapi/linux/bpf.h +++ b/tools/include/uapi/linux/bpf.h @@ -3276,6 +3276,12 @@ union bpf_attr { * Dynamically cast a *sk* pointer to a *tcp_request_sock* pointer. * Return * *sk* if casting is valid, or NULL otherwise. + * + * struct udp6_sock *bpf_skc_to_udp6_sock(void *sk) + * Description + * Dynamically cast a *sk* pointer to a *udp6_sock* pointer. + * Return + * *sk* if casting is valid, or NULL otherwise. */ #define __BPF_FUNC_MAPPER(FN) \ FN(unspec), \ @@ -3417,7 +3423,8 @@ union bpf_attr { FN(skc_to_tcp6_sock), \ FN(skc_to_tcp_sock), \ FN(skc_to_tcp_timewait_sock), \ - FN(skc_to_tcp_request_sock), + FN(skc_to_tcp_request_sock), \ + FN(skc_to_udp6_sock), /* integer value in 'imm' field of BPF_CALL instruction selects which helper * function eBPF program intends to call