| Message ID | 20231221232327.43678-1-john.fastabend@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | fix sockmap + stream af_unix memleak | expand |
On Thu, Dec 21, 2023 at 03:23 PM -08, John Fastabend wrote: > There was a memleak when streaming af_unix sockets were inserted into > multiple sockmap slots and/or maps. This is because each insert would > call a proto update operatino and these must be allowed to be called > multiple times. The streaming af_unix implementation recently added > a refcnt to handle a use after free issue, however it introduced a > memleak when inserted into multiple maps. > > This series fixes the memleak, adds a note in the code so we remember > that proto updates need to support this. And then we add three tests > for each of the slightly different iterations of adding sockets into > multiple maps. I kept them as 3 independent test cases here. I have > some slight preference for this they could however be a single test, > but then you don't get to run them independently which was sort of > useful while debugging. > > John Fastabend (5): > bpf: sockmap, fix proto update hook to avoid dup calls > bpf: sockmap, added comments describing update proto rules > bpf: sockmap, add tests for proto updates many to single map > bpf: sockmap, add tests for proto updates single socket to many map > bpf: sockmap, add tests for proto updates replace socket > > include/linux/skmsg.h | 5 + > net/unix/unix_bpf.c | 21 +- > .../selftests/bpf/prog_tests/sockmap_basic.c | 199 +++++++++++++++++- > 3 files changed, 221 insertions(+), 4 deletions(-) Sorry for the delay. I was out. This LGTM with some room for improvement in tests. You repeat the code to create different kind of sockets in each test. That could be refactored to use some kind of a factory helper. Reviewed-by: Jakub Sitnicki <jakub@cloudflare.com>
Jakub Sitnicki wrote: > On Thu, Dec 21, 2023 at 03:23 PM -08, John Fastabend wrote: > > There was a memleak when streaming af_unix sockets were inserted into > > multiple sockmap slots and/or maps. This is because each insert would > > call a proto update operatino and these must be allowed to be called > > multiple times. The streaming af_unix implementation recently added > > a refcnt to handle a use after free issue, however it introduced a > > memleak when inserted into multiple maps. > > > > This series fixes the memleak, adds a note in the code so we remember > > that proto updates need to support this. And then we add three tests > > for each of the slightly different iterations of adding sockets into > > multiple maps. I kept them as 3 independent test cases here. I have > > some slight preference for this they could however be a single test, > > but then you don't get to run them independently which was sort of > > useful while debugging. > > > > John Fastabend (5): > > bpf: sockmap, fix proto update hook to avoid dup calls > > bpf: sockmap, added comments describing update proto rules > > bpf: sockmap, add tests for proto updates many to single map > > bpf: sockmap, add tests for proto updates single socket to many map > > bpf: sockmap, add tests for proto updates replace socket > > > > include/linux/skmsg.h | 5 + > > net/unix/unix_bpf.c | 21 +- > > .../selftests/bpf/prog_tests/sockmap_basic.c | 199 +++++++++++++++++- > > 3 files changed, 221 insertions(+), 4 deletions(-) > > Sorry for the delay. I was out. Thanks for the review. > > This LGTM with some room for improvement in tests. > You repeat the code to create different kind of sockets in each test. > That could be refactored to use some kind of a factory helper. Yeah, my first attempt was uglier than the repeated setup in my opinion. So figured I would get this out and think a bit more about it. Lets see if BPF maintainers want me to fix the typo on Reported-by or if it can be fixed on merged. > > Reviewed-by: Jakub Sitnicki <jakub@cloudflare.com>
Hello: This series was applied to bpf/bpf-next.git (master) by Martin KaFai Lau <martin.lau@kernel.org>: On Thu, 21 Dec 2023 15:23:22 -0800 you wrote: > There was a memleak when streaming af_unix sockets were inserted into > multiple sockmap slots and/or maps. This is because each insert would > call a proto update operatino and these must be allowed to be called > multiple times. The streaming af_unix implementation recently added > a refcnt to handle a use after free issue, however it introduced a > memleak when inserted into multiple maps. > > [...] Here is the summary with links: - [bpf,1/5] bpf: sockmap, fix proto update hook to avoid dup calls https://git.kernel.org/bpf/bpf-next/c/16b2f264983d - [bpf,2/5] bpf: sockmap, added comments describing update proto rules https://git.kernel.org/bpf/bpf-next/c/7865dfb1eb94 - [bpf,3/5] bpf: sockmap, add tests for proto updates many to single map https://git.kernel.org/bpf/bpf-next/c/8c1b382a555a - [bpf,4/5] bpf: sockmap, add tests for proto updates single socket to many map https://git.kernel.org/bpf/bpf-next/c/f1300467dd9f - [bpf,5/5] bpf: sockmap, add tests for proto updates replace socket https://git.kernel.org/bpf/bpf-next/c/bdbca46d3f84 You are awesome, thank you!