| Message ID | 20210713074401.475209-1-jakub@cloudflare.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf] bpf, sockmap, udp: sk_prot needs inuse_idx set for proc stats | expand |
Jakub Sitnicki wrote: > Proc socket stats use sk_prot->inuse_idx value to record inuse sock stats. > We currently do not set this correctly from sockmap side. The result is > reading sock stats '/proc/net/sockstat' gives incorrect values. The > socket counter is incremented correctly, but because we don't set the > counter correctly when we replace sk_prot we may omit the decrement. > > To get the correct inuse_idx value move the core_initcall that initializes > the udp proto handlers to late_initcall. This way it is initialized after > UDP has the chance to assign the inuse_idx value from the register protocol > handler. > > Fixes: 5e21bb4e8125 ("bpf, test: fix NULL pointer dereference on invalid expected_attach_type") > Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com> > --- > > Missing bit from John's fix [1]. > > [1] https://lore.kernel.org/bpf/20210712195546.423990-1-john.fastabend@gmail.com/T/#mba9e0b6aa8dd0c01d7421a084c62ec93c9eea764 > Yep. Thanks. Acked-by: John Fastabend <john.fastabend@gmail.com>
On Tue, Jul 13, 2021 at 12:44 AM Jakub Sitnicki <jakub@cloudflare.com> wrote: > > Proc socket stats use sk_prot->inuse_idx value to record inuse sock stats. > We currently do not set this correctly from sockmap side. The result is > reading sock stats '/proc/net/sockstat' gives incorrect values. The > socket counter is incremented correctly, but because we don't set the > counter correctly when we replace sk_prot we may omit the decrement. > > To get the correct inuse_idx value move the core_initcall that initializes > the udp proto handlers to late_initcall. This way it is initialized after > UDP has the chance to assign the inuse_idx value from the register protocol > handler. Interesting. What about IPv6 module? Based on my understanding, it should always be loaded before we can trigger udp_bpf_check_v6_needs_rebuild(). If so, your patch is complete. > > Fixes: 5e21bb4e8125 ("bpf, test: fix NULL pointer dereference on invalid expected_attach_type") Should be commit edc6741cc66059532ba621928e3f1b02a53a2f39 (bpf: Add sockmap hooks for UDP sockets), right? Thanks.
On Wed, Jul 14, 2021 at 02:51 AM CEST, Cong Wang wrote: > On Tue, Jul 13, 2021 at 12:44 AM Jakub Sitnicki <jakub@cloudflare.com> wrote: >> >> Proc socket stats use sk_prot->inuse_idx value to record inuse sock stats. >> We currently do not set this correctly from sockmap side. The result is >> reading sock stats '/proc/net/sockstat' gives incorrect values. The >> socket counter is incremented correctly, but because we don't set the >> counter correctly when we replace sk_prot we may omit the decrement. >> >> To get the correct inuse_idx value move the core_initcall that initializes >> the udp proto handlers to late_initcall. This way it is initialized after >> UDP has the chance to assign the inuse_idx value from the register protocol >> handler. > > Interesting. What about IPv6 module? Based on my understanding, it should > always be loaded before we can trigger udp_bpf_check_v6_needs_rebuild(). > If so, your patch is complete. That's my understanding as well. The lazy update_proto call chain is: sock_map_update_common sock_map_link sock_map_init_proto psock->psock_update_sk_prot udp_bpf_update_proto udp_bpf_check_v6_needs_rebuild If that happens we are being passed an AF_INET6 socket. Socket has been created so IPv6 module must have been loaded. >> >> Fixes: 5e21bb4e8125 ("bpf, test: fix NULL pointer dereference on invalid expected_attach_type") > > Should be commit edc6741cc66059532ba621928e3f1b02a53a2f39 > (bpf: Add sockmap hooks for UDP sockets), right? Thanks. Fixed in v2.
diff --git a/net/ipv4/udp_bpf.c b/net/ipv4/udp_bpf.c index 45b8782aec0c..9f5a5cdc38e6 100644 --- a/net/ipv4/udp_bpf.c +++ b/net/ipv4/udp_bpf.c @@ -134,7 +134,7 @@ static int __init udp_bpf_v4_build_proto(void) udp_bpf_rebuild_protos(&udp_bpf_prots[UDP_BPF_IPV4], &udp_prot); return 0; } -core_initcall(udp_bpf_v4_build_proto); +late_initcall(udp_bpf_v4_build_proto); int udp_bpf_update_proto(struct sock *sk, struct sk_psock *psock, bool restore) {
Proc socket stats use sk_prot->inuse_idx value to record inuse sock stats. We currently do not set this correctly from sockmap side. The result is reading sock stats '/proc/net/sockstat' gives incorrect values. The socket counter is incremented correctly, but because we don't set the counter correctly when we replace sk_prot we may omit the decrement. To get the correct inuse_idx value move the core_initcall that initializes the udp proto handlers to late_initcall. This way it is initialized after UDP has the chance to assign the inuse_idx value from the register protocol handler. Fixes: 5e21bb4e8125 ("bpf, test: fix NULL pointer dereference on invalid expected_attach_type") Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com> --- Missing bit from John's fix [1]. [1] https://lore.kernel.org/bpf/20210712195546.423990-1-john.fastabend@gmail.com/T/#mba9e0b6aa8dd0c01d7421a084c62ec93c9eea764 net/ipv4/udp_bpf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)