| Message ID | 20230728105649.3978774-1-xukuohai@huaweicloud.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf] bpf, sockmap: Fix map type error in sock_map_del_link | expand |
On 7/28/23 3:56 AM, Xu Kuohai wrote: > sock_map_del_link() operates on both SOCKMAP and SOCKHASH, although > both types have member named "progs", the offset of "progs" member in > these two types is different, so "progs" should be accessed with the > real map type. The patch makes sense to me. Can a test be written to trigger it? John, please review.
On 8/1/2023 9:19 AM, Martin KaFai Lau wrote: > On 7/28/23 3:56 AM, Xu Kuohai wrote: >> sock_map_del_link() operates on both SOCKMAP and SOCKHASH, although >> both types have member named "progs", the offset of "progs" member in >> these two types is different, so "progs" should be accessed with the >> real map type. > > The patch makes sense to me. Can a test be written to trigger it? > Thank you for the review. I have a messy prog that triggers memleak caused by this issue. I'll try to simplify it to a test. > John, please review. > > > .
Xu Kuohai wrote: > On 8/1/2023 9:19 AM, Martin KaFai Lau wrote: > > On 7/28/23 3:56 AM, Xu Kuohai wrote: > >> sock_map_del_link() operates on both SOCKMAP and SOCKHASH, although > >> both types have member named "progs", the offset of "progs" member in > >> these two types is different, so "progs" should be accessed with the > >> real map type. > > > > The patch makes sense to me. Can a test be written to trigger it? > > > > Thank you for the review. I have a messy prog that triggers memleak > caused by this issue. I'll try to simplify it to a test. > > > John, please review. > > > > > > . > > Thanks good catch. One thing I don't see any tests for is deleting a socket from a sockmap and then trying to use it? My guess is almost no one deletes sockets from a map except on sock close. Maybe to reproduce, 1. connect a bunch of sockets to sockhash with verdict prog 2. remove the sockets 3. remove the sockhash 4. that should leak the bpf ref cnt so we could check for the prog still existing? Reviewed-by: John Fastabend <john.fastabend@gmail.com>
On 8/1/2023 11:47 AM, John Fastabend wrote: > Xu Kuohai wrote: >> On 8/1/2023 9:19 AM, Martin KaFai Lau wrote: >>> On 7/28/23 3:56 AM, Xu Kuohai wrote: >>>> sock_map_del_link() operates on both SOCKMAP and SOCKHASH, although >>>> both types have member named "progs", the offset of "progs" member in >>>> these two types is different, so "progs" should be accessed with the >>>> real map type. >>> >>> The patch makes sense to me. Can a test be written to trigger it? >>> >> >> Thank you for the review. I have a messy prog that triggers memleak >> caused by this issue. I'll try to simplify it to a test. >> >>> John, please review. >>> >>> >>> . >> >> > > Thanks good catch. One thing I don't see any tests for is deleting a > socket from a sockmap and then trying to use it? My guess is almost > no one deletes sockets from a map except on sock close. Maybe to > reproduce, > > 1. connect a bunch of sockets to sockhash with verdict prog > 2. remove the sockets > 3. remove the sockhash > 4. that should leak the bpf ref cnt so we could check for the > prog still existing? > I tried this and found no bpf prog leaks. The debugging shows that the stream_parser and stream_verdict progs are released as follows: sock_map_unref sock_map_del_link struct bpf_stab *stab = container_of(map, struct bpf_stab, map); if (psock->saved_data_ready && stab->progs.stream_parser) strp_stop = true; // (1) not executed, since stab->progs.stream_parser // is actually shtab->progs.msg_parser, which is // NULL, so the if condition is false. if (psock->saved_data_ready && stab->progs.stream_verdict) verdict_stop = true; // (2) executed, so verdict_stop is set to true if (strp_stop) // (3) condition is false since strp_stop is false sk_psock_stop_strp(sk, psock) if (verdict_stop) // (4) condition pass, so stream_verdict prog refcnt // is released by sk_psock_stop_verdict sk_psock_stop_verdict(sk, psock) psock_set_prog(&pock->progs.stream_verdict, NULL) bpf_prog_put // (5) release refcnt of stream_verdict prog sk_psock_put sk_psock_drop(sk, psock) sk_psock_stop_strp(sk, psock) sk_psock_stop_strp(&psock->progs.stream_parser, NULL) bpf_prog_put // (6) release refcnt of stream_parser prog However, this issue triggers a WARNING in strp_done: sock_map_unref sock_map_del_link struct bpf_stab *stab = container_of(map, struct bpf_stab, map); if (psock->saved_data_ready && stab->progs.stream_verdict) verdict_stop = true; // (1) verdict_stop is set to true if (verdict_stop) // (2) condition pass sk_psock_stop_verdict(sk, psock) psock_set_prog(&pock->progs.stream_verdict, NULL) bpf_prog_put psock->saved_data_ready = NULL; // (3) psock->saved_data_ready is // set to NULL sk_psock_put sk_psock_drop(sk, psock) sk_psock_stop_strp(sk, psock) if (!psock->saved_data_ready) return; // (4) sk_psock_stop_strp returns strp_stop(&psock->strp) // (5) so strp_stop can not be called strp->stopped = 1; // (6) so strp->stopped is **NOT** set to 1 sk_psock_destroy sk_psock_done_strp strp_done WARN_ON(!strp->stopped); // (7) WARNING triggered Now I'm convinced the memleak I observed was caused by strp_done not being called, I'll send a test for it. > Reviewed-by: John Fastabend <john.fastabend@gmail.com> > > > .
On Fri, Jul 28, 2023 at 06:56 AM -04, Xu Kuohai wrote: > From: Xu Kuohai <xukuohai@huawei.com> > > sock_map_del_link() operates on both SOCKMAP and SOCKHASH, although > both types have member named "progs", the offset of "progs" member in > these two types is different, so "progs" should be accessed with the > real map type. > > Fixes: 604326b41a6f ("bpf, sockmap: convert to generic sk_msg interface") > Signed-off-by: Xu Kuohai <xukuohai@huawei.com> > --- Reviewed-by: Jakub Sitnicki <jakub@cloudflare.com>
diff --git a/net/core/sock_map.c b/net/core/sock_map.c index 19538d628714..936c5cabe9f3 100644 --- a/net/core/sock_map.c +++ b/net/core/sock_map.c @@ -148,13 +148,13 @@ static void sock_map_del_link(struct sock *sk, list_for_each_entry_safe(link, tmp, &psock->link, list) { if (link->link_raw == link_raw) { struct bpf_map *map = link->map; - struct bpf_stab *stab = container_of(map, struct bpf_stab, - map); - if (psock->saved_data_ready && stab->progs.stream_parser) + struct sk_psock_progs *progs = sock_map_progs(map); + + if (psock->saved_data_ready && progs->stream_parser) strp_stop = true; - if (psock->saved_data_ready && stab->progs.stream_verdict) + if (psock->saved_data_ready && progs->stream_verdict) verdict_stop = true; - if (psock->saved_data_ready && stab->progs.skb_verdict) + if (psock->saved_data_ready && progs->skb_verdict) verdict_stop = true; list_del(&link->list); sk_psock_free_link(link);