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@xxxxxxxxx>
.
