From: Geliang Tang <tanggeliang@kylinos.cn>

Address Martin's comments in v1:

- bpf_iter_mptcp_subflow_new returns -EINVAL when msk socket lock isn't
  held.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/bpf.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
index 02038db59956..89a7a482368d 100644
--- a/net/mptcp/bpf.c
+++ b/net/mptcp/bpf.c
@@ -252,7 +252,10 @@ bpf_iter_mptcp_subflow_new(struct bpf_iter_mptcp_subflow *it,
 	if (!msk)
 		return -EINVAL;
 
-	msk_owned_by_me(msk);
+#ifdef CONFIG_LOCKDEP
+	if (!lockdep_sock_is_held((const struct sock *)msk))
+		return -EINVAL;
+#endif
 
 	kit->pos = &msk->conn_list;
 	return 0;
-- 
2.45.2

Hi Geliang,

On 21/11/2024 10:36, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> Address Martin's comments in v1:
> 
> - bpf_iter_mptcp_subflow_new returns -EINVAL when msk socket lock isn't
>   held.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  net/mptcp/bpf.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
> index 02038db59956..89a7a482368d 100644
> --- a/net/mptcp/bpf.c
> +++ b/net/mptcp/bpf.c
> @@ -252,7 +252,10 @@ bpf_iter_mptcp_subflow_new(struct bpf_iter_mptcp_subflow *it,
>  	if (!msk)
>  		return -EINVAL;
>  
> -	msk_owned_by_me(msk);
> +#ifdef CONFIG_LOCKDEP
> +	if (!lockdep_sock_is_held((const struct sock *)msk))
> +		return -EINVAL;

This feels wrong: lockdep is a debugging tool, the goal is usually to
WARN in case of issue, not to take a different path.

Maybe you want to use spin_is_locked(), but still, I'm not sure whether
that's the direction to take: it means the msk always needs to be locked
when iterating over the different subflow → is it always the case? Maybe
yes if it is used only to modify update it?

(If not, maybe you just need to check the sk_refcnt as a safety measure?
But that's maybe not enough for all cases.)

Did you check what was being done for the BPF TCP CA? I guess they need
a way to prevent calling some kfunc without the required locks or
refcount, no?

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.