[PATCH mptcp-next v1] mptcp: fix cast type of is_fully_established

[PATCH mptcp-next v1] mptcp: fix cast type of is_fully_established

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

The parameter type of mptcp_is_fully_established() is 'struct sock *',
not 'void *', so this patch casts 'msk' as 'struct sock *' instead of
'void *' in mptcp_can_accept_new_subflow().

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/subflow.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 409bd415ef1d..da452a1d5433 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -60,7 +60,7 @@ static void subflow_generate_hmac(u64 key1, u64 key2, u32 nonce1, u32 nonce2,
 
 static bool mptcp_can_accept_new_subflow(const struct mptcp_sock *msk)
 {
-	return mptcp_is_fully_established((void *)msk) &&
+	return mptcp_is_fully_established((struct sock *)msk) &&
 		((mptcp_pm_is_userspace(msk) &&
 		  mptcp_userspace_pm_active(msk)) ||
 		 READ_ONCE(msk->pm.accept_subflow));
-- 
2.43.0

Re: [PATCH mptcp-next v1] mptcp: fix cast type of is_fully_established

[Matthieu Baerts]

Hi Geliang,

On 25/03/2025 11:36, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> The parameter type of mptcp_is_fully_established() is 'struct sock *',
> not 'void *', so this patch casts 'msk' as 'struct sock *' instead of
> 'void *' in mptcp_can_accept_new_subflow().

Indeed, but it is not really wrong to use 'void *'. Also, it looks like
we are doing that in most other files from the net/mptcp directory.

If you have to modify this line for another commit, feel free to include
the modification in this commit if it bothers you ("While at it, use the
proper cast instead of the generic 'void *'"), but it can also be left
unchanged I think.

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

Re: [PATCH mptcp-next v1] mptcp: fix cast type of is_fully_established

[Geliang Tang]

Hi Matt,

Thanks for the review.

On Tue, 2025-03-25 at 11:49 +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 25/03/2025 11:36, Geliang Tang wrote:
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> > 
> > The parameter type of mptcp_is_fully_established() is 'struct sock
> > *',
> > not 'void *', so this patch casts 'msk' as 'struct sock *' instead
> > of
> > 'void *' in mptcp_can_accept_new_subflow().
> 
> Indeed, but it is not really wrong to use 'void *'. Also, it looks
> like
> we are doing that in most other files from the net/mptcp directory.

I just checked, and there're indeed 6 other places that use '(void
*)msk' too, but more places (98) use '(struct sock *)msk'.

$ grep -r "(void \*)msk" net/mptcp/ | wc -l
7
$ grep -r "(struct sock \*)msk" net/mptcp/ | wc -l
98

I'm wondering if we should change all 7 '(void *)msk' to '(struct sock
*)msk' in v2 to be consistent.

> 
> If you have to modify this line for another commit, feel free to
> include
> the modification in this commit if it bothers you ("While at it, use
> the
> proper cast instead of the generic 'void *'"), but it can also be
> left
> unchanged I think.

Or simply drop this patch. Both are OK to me.

Thanks,
-Geliang

> 
> Cheers,
> Matt

Re: [PATCH mptcp-next v1] mptcp: fix cast type of is_fully_established

[Matthieu Baerts]

Hi Geliang,

On 26/03/2025 02:08, Geliang Tang wrote:
> Hi Matt,
> 
> Thanks for the review.
> 
> On Tue, 2025-03-25 at 11:49 +0100, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 25/03/2025 11:36, Geliang Tang wrote:
>>> From: Geliang Tang <tanggeliang@kylinos.cn>
>>>
>>> The parameter type of mptcp_is_fully_established() is 'struct sock
>>> *',
>>> not 'void *', so this patch casts 'msk' as 'struct sock *' instead
>>> of
>>> 'void *' in mptcp_can_accept_new_subflow().
>>
>> Indeed, but it is not really wrong to use 'void *'. Also, it looks
>> like
>> we are doing that in most other files from the net/mptcp directory.
> 
> I just checked, and there're indeed 6 other places that use '(void
> *)msk' too, but more places (98) use '(struct sock *)msk'.
> 
> $ grep -r "(void \*)msk" net/mptcp/ | wc -l
> 7
> $ grep -r "(struct sock \*)msk" net/mptcp/ | wc -l
> 98
> 
> I'm wondering if we should change all 7 '(void *)msk' to '(struct sock
> *)msk' in v2 to be consistent.

Maybe, I don't know to be honest. There are other (void *) usages, and
they are "fine" for the moment. Changing them might be good for the
consistency, but annoying in case of backports :-/

>> If you have to modify this line for another commit, feel free to
>> include
>> the modification in this commit if it bothers you ("While at it, use
>> the
>> proper cast instead of the generic 'void *'"), but it can also be
>> left
>> unchanged I think.
> 
> Or simply drop this patch. Both are OK to me.

Yes, maybe. I will ask Mat what he thinks about that.

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

Re: [PATCH mptcp-next v1] mptcp: fix cast type of is_fully_established

[Geliang Tang]

On Wed, 2025-03-26 at 11:17 +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 26/03/2025 02:08, Geliang Tang wrote:
> > Hi Matt,
> > 
> > Thanks for the review.
> > 
> > On Tue, 2025-03-25 at 11:49 +0100, Matthieu Baerts wrote:
> > > Hi Geliang,
> > > 
> > > On 25/03/2025 11:36, Geliang Tang wrote:
> > > > From: Geliang Tang <tanggeliang@kylinos.cn>
> > > > 
> > > > The parameter type of mptcp_is_fully_established() is 'struct
> > > > sock
> > > > *',
> > > > not 'void *', so this patch casts 'msk' as 'struct sock *'
> > > > instead
> > > > of
> > > > 'void *' in mptcp_can_accept_new_subflow().
> > > 
> > > Indeed, but it is not really wrong to use 'void *'. Also, it
> > > looks
> > > like
> > > we are doing that in most other files from the net/mptcp
> > > directory.
> > 
> > I just checked, and there're indeed 6 other places that use '(void
> > *)msk' too, but more places (98) use '(struct sock *)msk'.
> > 
> > $ grep -r "(void \*)msk" net/mptcp/ | wc -l
> > 7
> > $ grep -r "(struct sock \*)msk" net/mptcp/ | wc -l
> > 98
> > 
> > I'm wondering if we should change all 7 '(void *)msk' to '(struct
> > sock
> > *)msk' in v2 to be consistent.
> 
> Maybe, I don't know to be honest. There are other (void *) usages,
> and
> they are "fine" for the moment. Changing them might be good for the
> consistency, but annoying in case of backports :-/
> 
> > > If you have to modify this line for another commit, feel free to
> > > include
> > > the modification in this commit if it bothers you ("While at it,
> > > use
> > > the
> > > proper cast instead of the generic 'void *'"), but it can also be
> > > left
> > > unchanged I think.
> > 
> > Or simply drop this patch. Both are OK to me.
> 
> Yes, maybe. I will ask Mat what he thinks about that.

No need, let's drop this patch :)

Thanks,
-Geliang

> 
> Cheers,
> Matt

Re: [PATCH mptcp-next v1] mptcp: fix cast type of is_fully_established

[MPTCP CI]

Hi Geliang,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal: Success! ✅
- KVM Validation: debug: Success! ✅
- KVM Validation: btf-normal (only bpftest_all): Success! ✅
- KVM Validation: btf-debug (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/14057787415

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/8313b14b74fa
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=947109


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-normal

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)