| Message ID | c98deccb13e527c3fc120e9f6f6627db3b3cf6ab.1732181419.git.tanggeliang@kylinos.cn (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Matthieu Baerts |
| Headers | show |
| Series | Squash to "Add mptcp_subflow bpf_iter support" | expand |
| Context | Check | Description |
|---|---|---|
| matttbe/build | success | Build and static analysis OK |
| matttbe/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 11 lines checked |
| matttbe/shellcheck | success | MPTCP selftests files have not been modified |
| matttbe/KVM_Validation__normal | success | Success! ✅ |
| matttbe/KVM_Validation__debug | success | Success! ✅ |
| matttbe/KVM_Validation__btf-normal__only_bpftest_all_ | success | Success! ✅ |
| matttbe/KVM_Validation__btf-debug__only_bpftest_all_ | success | Success! ✅ |
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
Hi Matt, On Fri, 2024-11-22 at 11:02 +0100, Matthieu Baerts wrote: > 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, Other bpf_iter have added "sizeof" and "alignof" checks, I think mptcp_subflow bpf_iter also needs to add them too: BUILD_BUG_ON(sizeof(struct bpf_iter_mptcp_subflow_kern) != sizeof(struct bpf_iter_mptcp_subflow)); BUILD_BUG_ON(__alignof__(struct bpf_iter_mptcp_subflow_kern) != __alignof__(struct bpf_iter_mptcp_subflow)); > > 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 spin_is_locked() doesn't work, since this msk socket lock isn't a spinlock, right? > 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.) I think the answer is no, sometimes locking is not necessary when iterating over the different subflows. Does this mean we can simply drop msk_owned_by_me() here? > > 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? Not found in BPF TCP CA. Thanks, -Geliang > > Cheers, > Matt
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;