diff mbox series

[bpf,1/3] bpf, sockmap: Fix update element with same

Message ID 20241202-sockmap-replace-v1-1-1e88579e7bd5@rbox.co (mailing list archive)
State Accepted
Commit 75e072a390da9a22e7ae4a4e8434dfca5da499fb
Delegated to: BPF
Headers show
Series bpf, sockmap: Fix the element replace | expand

Checks

Context Check Description
netdev/series_format warning Series does not have a cover letter
netdev/tree_selection success Clearly marked for bpf
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 3 this patch: 3
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 8 of 8 maintainers
netdev/build_clang success Errors and warnings before: 3 this patch: 3
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 4 this patch: 4
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 7 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-PR success PR summary
bpf/vmtest-bpf-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-11 success Logs for aarch64-gcc / veristat-meta
bpf/vmtest-bpf-VM_Test-10 success Logs for aarch64-gcc / veristat-kernel
bpf/vmtest-bpf-VM_Test-12 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-VM_Test-13 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-VM_Test-35 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-37 success Logs for x86_64-llvm-17 / veristat-meta
bpf/vmtest-bpf-VM_Test-36 success Logs for x86_64-llvm-17 / veristat-kernel
bpf/vmtest-bpf-VM_Test-38 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-39 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18-O2
bpf/vmtest-bpf-VM_Test-44 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-45 success Logs for x86_64-llvm-18 / veristat-kernel
bpf/vmtest-bpf-VM_Test-46 success Logs for x86_64-llvm-18 / veristat-meta
bpf/vmtest-bpf-VM_Test-19 success Logs for set-matrix
bpf/vmtest-bpf-VM_Test-20 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-VM_Test-18 success Logs for s390x-gcc / veristat-meta
bpf/vmtest-bpf-VM_Test-16 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-VM_Test-17 success Logs for s390x-gcc / veristat-kernel
bpf/vmtest-bpf-VM_Test-21 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-VM_Test-30 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-27 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-31 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17-O2
bpf/vmtest-bpf-VM_Test-14 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-VM_Test-33 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-34 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-41 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-42 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-43 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-15 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-VM_Test-22 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-25 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-23 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-26 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-28 success Logs for x86_64-gcc / veristat-kernel / x86_64-gcc veristat_kernel
bpf/vmtest-bpf-VM_Test-29 success Logs for x86_64-gcc / veristat-meta / x86_64-gcc veristat_meta
bpf/vmtest-bpf-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-3 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-VM_Test-1 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-VM_Test-4 success Logs for set-matrix
bpf/vmtest-bpf-VM_Test-6 success Logs for x86_64-gcc / build-release

Commit Message

Michal Luczaj Dec. 2, 2024, 11:29 a.m. UTC 
Consider a sockmap entry being updated with the same socket:

	osk = stab->sks[idx];
	sock_map_add_link(psock, link, map, &stab->sks[idx]);
	stab->sks[idx] = sk;
	if (osk)
		sock_map_unref(osk, &stab->sks[idx]);

Due to sock_map_unref(), which invokes sock_map_del_link(), all the psock's
links for stab->sks[idx] are torn:

	list_for_each_entry_safe(link, tmp, &psock->link, list) {
		if (link->link_raw == link_raw) {
			...
			list_del(&link->list);
			sk_psock_free_link(link);
		}
	}

And that includes the new link sock_map_add_link() added just before the
unref.

This results in a sockmap holding a socket, but without the respective
link. This in turn means that close(sock) won't trigger the cleanup, i.e. a
closed socket will not be automatically removed from the sockmap.

Stop tearing the links when a matching link_raw is found.

Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 net/core/sock_map.c | 1 +
 1 file changed, 1 insertion(+)

Comments

John Fastabend Dec. 9, 2024, 5:47 a.m. UTC | #1 
Michal Luczaj wrote:
> Consider a sockmap entry being updated with the same socket:
> 
> 	osk = stab->sks[idx];
> 	sock_map_add_link(psock, link, map, &stab->sks[idx]);
> 	stab->sks[idx] = sk;
> 	if (osk)
> 		sock_map_unref(osk, &stab->sks[idx]);
> 
> Due to sock_map_unref(), which invokes sock_map_del_link(), all the psock's
> links for stab->sks[idx] are torn:
> 
> 	list_for_each_entry_safe(link, tmp, &psock->link, list) {
> 		if (link->link_raw == link_raw) {
> 			...
> 			list_del(&link->list);
> 			sk_psock_free_link(link);
> 		}
> 	}
> 
> And that includes the new link sock_map_add_link() added just before the
> unref.
> 
> This results in a sockmap holding a socket, but without the respective
> link. This in turn means that close(sock) won't trigger the cleanup, i.e. a
> closed socket will not be automatically removed from the sockmap.
> 
> Stop tearing the links when a matching link_raw is found.
> 
> Signed-off-by: Michal Luczaj <mhal@rbox.co>
> ---

Thanks. LGTM.

Reviewed-by: John Fastabend <john.fastabend@gmail.com>

>  net/core/sock_map.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/net/core/sock_map.c b/net/core/sock_map.c
> index 78347d7d25ef31525f8ec0a755a18e5793ad92c0..20b348b1964a10a1b0bfbe1a90a4a4cd99715b81 100644
> --- a/net/core/sock_map.c
> +++ b/net/core/sock_map.c
> @@ -159,6 +159,7 @@ static void sock_map_del_link(struct sock *sk,
>  				verdict_stop = true;
>  			list_del(&link->list);
>  			sk_psock_free_link(link);
> +			break;
>  		}
>  	}
>  	spin_unlock_bh(&psock->link_lock);
> 
> -- 
> 2.46.2
>
Michal Luczaj Dec. 9, 2024, 9:54 a.m. UTC | #2 
On 12/9/24 06:47, John Fastabend wrote:
> Michal Luczaj wrote:
>> Consider a sockmap entry being updated with the same socket:
>>
>> 	osk = stab->sks[idx];
>> 	sock_map_add_link(psock, link, map, &stab->sks[idx]);
>> 	stab->sks[idx] = sk;
>> 	if (osk)
>> 		sock_map_unref(osk, &stab->sks[idx]);
>>
>> Due to sock_map_unref(), which invokes sock_map_del_link(), all the psock's
>> links for stab->sks[idx] are torn:
>>
>> 	list_for_each_entry_safe(link, tmp, &psock->link, list) {
>> 		if (link->link_raw == link_raw) {
>> 			...
>> 			list_del(&link->list);
>> 			sk_psock_free_link(link);
>> 		}
>> 	}
>>
>> And that includes the new link sock_map_add_link() added just before the
>> unref.
>>
>> This results in a sockmap holding a socket, but without the respective
>> link. This in turn means that close(sock) won't trigger the cleanup, i.e. a
>> closed socket will not be automatically removed from the sockmap.
>>
>> Stop tearing the links when a matching link_raw is found.
>>
>> Signed-off-by: Michal Luczaj <mhal@rbox.co>
>> ---
> 
> Thanks. LGTM.
> 
> Reviewed-by: John Fastabend <john.fastabend@gmail.com>

Thanks, and sorry for a missing tag:

Fixes: 604326b41a6f ("bpf, sockmap: convert to generic sk_msg interface")
diff mbox series

Patch

diff --git a/net/core/sock_map.c b/net/core/sock_map.c
index 78347d7d25ef31525f8ec0a755a18e5793ad92c0..20b348b1964a10a1b0bfbe1a90a4a4cd99715b81 100644
--- a/net/core/sock_map.c
+++ b/net/core/sock_map.c
@@ -159,6 +159,7 @@  static void sock_map_del_link(struct sock *sk,
 				verdict_stop = true;
 			list_del(&link->list);
 			sk_psock_free_link(link);
+			break;
 		}
 	}
 	spin_unlock_bh(&psock->link_lock);