[v5] mptcp: fix endpoints with 'signal' and 'subflow' flags

[PATCH mptcp-net v5 07/13] mptcp: pm: only in-kernel cannot have entries with ID 0

Posted by Matthieu Baerts (NGI0) 1 month, 3 weeks ago

The ID 0 is specific per MPTCP connections. The per netns entries cannot
have this special ID 0 then.

But that's different for the userspace PM where the entries are per
connection, they can then use this special ID 0.

Fixes: f40be0db0b76 ("mptcp: unify pm get_flags_and_ifindex_by_id")
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/pm.c         | 3 ---
 net/mptcp/pm_netlink.c | 4 ++++
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 925123e99889..3e6e0f5510bb 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -434,9 +434,6 @@ int mptcp_pm_get_flags_and_ifindex_by_id(struct mptcp_sock *msk, unsigned int id
 	*flags = 0;
 	*ifindex = 0;
 
-	if (!id)
-		return 0;
-
 	if (mptcp_pm_is_userspace(msk))
 		return mptcp_userspace_pm_get_flags_and_ifindex_by_id(msk, id, flags, ifindex);
 	return mptcp_pm_nl_get_flags_and_ifindex_by_id(msk, id, flags, ifindex);
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 3cb02fe359c0..6a1495fec7ae 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1395,6 +1395,10 @@ int mptcp_pm_nl_get_flags_and_ifindex_by_id(struct mptcp_sock *msk, unsigned int
 	struct sock *sk = (struct sock *)msk;
 	struct net *net = sock_net(sk);
 
+	/* No entries with ID 0 */
+	if (id == 0)
+		return 0;
+
 	rcu_read_lock();
 	entry = __lookup_addr_by_id(pm_nl_get_pernet(net), id);
 	if (entry) {

-- 
2.45.2

Re: [PATCH mptcp-net v5 07/13] mptcp: pm: only in-kernel cannot have entries with ID 0

Posted by Geliang Tang 1 month, 2 weeks ago

On Fri, 2024-07-26 at 16:28 +0200, Matthieu Baerts (NGI0) wrote:
> The ID 0 is specific per MPTCP connections. The per netns entries
> cannot
> have this special ID 0 then.
> 
> But that's different for the userspace PM where the entries are per
> connection, they can then use this special ID 0.
> 
> Fixes: f40be0db0b76 ("mptcp: unify pm get_flags_and_ifindex_by_id")
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
>  net/mptcp/pm.c         | 3 ---
>  net/mptcp/pm_netlink.c | 4 ++++
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 925123e99889..3e6e0f5510bb 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -434,9 +434,6 @@ int mptcp_pm_get_flags_and_ifindex_by_id(struct
> mptcp_sock *msk, unsigned int id
>  	*flags = 0;
>  	*ifindex = 0;
>  
> -	if (!id)
> -		return 0;
> -
>  	if (mptcp_pm_is_userspace(msk))
>  		return
> mptcp_userspace_pm_get_flags_and_ifindex_by_id(msk, id, flags,
> ifindex);
>  	return mptcp_pm_nl_get_flags_and_ifindex_by_id(msk, id,
> flags, ifindex);
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index 3cb02fe359c0..6a1495fec7ae 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -1395,6 +1395,10 @@ int
> mptcp_pm_nl_get_flags_and_ifindex_by_id(struct mptcp_sock *msk,
> unsigned int
>  	struct sock *sk = (struct sock *)msk;
>  	struct net *net = sock_net(sk);
>  
> +	/* No entries with ID 0 */
> +	if (id == 0)
> +		return 0;
> +

I think this check needs to be added in
mptcp_userspace_pm_get_flags_and_ifindex_by_id() too.

>  	rcu_read_lock();
>  	entry = __lookup_addr_by_id(pm_nl_get_pernet(net), id);
>  	if (entry) {
>

Re: [PATCH mptcp-net v5 07/13] mptcp: pm: only in-kernel cannot have entries with ID 0

Posted by Matthieu Baerts 1 month, 2 weeks ago

Hi Geliang,

On 29/07/2024 03:12, Geliang Tang wrote:
> On Fri, 2024-07-26 at 16:28 +0200, Matthieu Baerts (NGI0) wrote:
>> The ID 0 is specific per MPTCP connections. The per netns entries
>> cannot
>> have this special ID 0 then.
>>
>> But that's different for the userspace PM where the entries are per
>> connection, they can then use this special ID 0.
>>
>> Fixes: f40be0db0b76 ("mptcp: unify pm get_flags_and_ifindex_by_id")
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>> ---
>>  net/mptcp/pm.c         | 3 ---
>>  net/mptcp/pm_netlink.c | 4 ++++
>>  2 files changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
>> index 925123e99889..3e6e0f5510bb 100644
>> --- a/net/mptcp/pm.c
>> +++ b/net/mptcp/pm.c
>> @@ -434,9 +434,6 @@ int mptcp_pm_get_flags_and_ifindex_by_id(struct
>> mptcp_sock *msk, unsigned int id
>>  	*flags = 0;
>>  	*ifindex = 0;
>>  
>> -	if (!id)
>> -		return 0;
>> -
>>  	if (mptcp_pm_is_userspace(msk))
>>  		return
>> mptcp_userspace_pm_get_flags_and_ifindex_by_id(msk, id, flags,
>> ifindex);
>>  	return mptcp_pm_nl_get_flags_and_ifindex_by_id(msk, id,
>> flags, ifindex);
>> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
>> index 3cb02fe359c0..6a1495fec7ae 100644
>> --- a/net/mptcp/pm_netlink.c
>> +++ b/net/mptcp/pm_netlink.c
>> @@ -1395,6 +1395,10 @@ int
>> mptcp_pm_nl_get_flags_and_ifindex_by_id(struct mptcp_sock *msk,
>> unsigned int
>>  	struct sock *sk = (struct sock *)msk;
>>  	struct net *net = sock_net(sk);
>>  
>> +	/* No entries with ID 0 */
>> +	if (id == 0)
>> +		return 0;
>> +
> 
> I think this check needs to be added in
> mptcp_userspace_pm_get_flags_and_ifindex_by_id() too.

We don't need it for the userspace PM, because the endpoints are managed
per MPTCP connection, they can have an ID 0. On the other hand, the
in-kernel PM manages the endpoints globally, they cannot have the ID 0
because it is reserved to the address used by the initial subflow, which
can be different per MPTCP connection.

Please see the commit message for more details.

> 
>>  	rcu_read_lock();
>>  	entry = __lookup_addr_by_id(pm_nl_get_pernet(net), id);
>>  	if (entry) {
>>
> 

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

Re: [PATCH mptcp-net v5 07/13] mptcp: pm: only in-kernel cannot have entries with ID 0

Posted by Geliang Tang 1 month, 2 weeks ago

On Mon, 2024-07-29 at 11:29 +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 29/07/2024 03:12, Geliang Tang wrote:
> > On Fri, 2024-07-26 at 16:28 +0200, Matthieu Baerts (NGI0) wrote:
> > > The ID 0 is specific per MPTCP connections. The per netns entries
> > > cannot
> > > have this special ID 0 then.
> > > 
> > > But that's different for the userspace PM where the entries are
> > > per
> > > connection, they can then use this special ID 0.
> > > 
> > > Fixes: f40be0db0b76 ("mptcp: unify pm
> > > get_flags_and_ifindex_by_id")
> > > Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> > > ---
> > >  net/mptcp/pm.c         | 3 ---
> > >  net/mptcp/pm_netlink.c | 4 ++++
> > >  2 files changed, 4 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> > > index 925123e99889..3e6e0f5510bb 100644
> > > --- a/net/mptcp/pm.c
> > > +++ b/net/mptcp/pm.c
> > > @@ -434,9 +434,6 @@ int
> > > mptcp_pm_get_flags_and_ifindex_by_id(struct
> > > mptcp_sock *msk, unsigned int id
> > >  	*flags = 0;
> > >  	*ifindex = 0;
> > >  
> > > -	if (!id)
> > > -		return 0;
> > > -
> > >  	if (mptcp_pm_is_userspace(msk))
> > >  		return
> > > mptcp_userspace_pm_get_flags_and_ifindex_by_id(msk, id, flags,
> > > ifindex);
> > >  	return mptcp_pm_nl_get_flags_and_ifindex_by_id(msk, id,
> > > flags, ifindex);
> > > diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> > > index 3cb02fe359c0..6a1495fec7ae 100644
> > > --- a/net/mptcp/pm_netlink.c
> > > +++ b/net/mptcp/pm_netlink.c
> > > @@ -1395,6 +1395,10 @@ int
> > > mptcp_pm_nl_get_flags_and_ifindex_by_id(struct mptcp_sock *msk,
> > > unsigned int
> > >  	struct sock *sk = (struct sock *)msk;
> > >  	struct net *net = sock_net(sk);
> > >  
> > > +	/* No entries with ID 0 */
> > > +	if (id == 0)
> > > +		return 0;
> > > +
> > 
> > I think this check needs to be added in
> > mptcp_userspace_pm_get_flags_and_ifindex_by_id() too.
> 
> We don't need it for the userspace PM, because the endpoints are
> managed
> per MPTCP connection, they can have an ID 0. On the other hand, the
> in-kernel PM manages the endpoints globally, they cannot have the ID
> 0
> because it is reserved to the address used by the initial subflow,
> which
> can be different per MPTCP connection.
> 
> Please see the commit message for more details.

Thanks for your explanation.

Acked-by: Geliang Tang <geliang@kernel.org>

> 
> > 
> > >  	rcu_read_lock();
> > >  	entry = __lookup_addr_by_id(pm_nl_get_pernet(net), id);
> > >  	if (entry) {
> > > 
> > 
> 
> Cheers,
> Matt