Re: [PATCH mptcp-net v3 2/8] mptcp: pm: userspace: lookup: match port in priority

Hi Geliang,

Thank you for the review and suggestion!

On 19/08/2026 09:23, Geliang Tang wrote:
> Hi Matt,
> 
> On Fri, 2026-08-07 at 10:41 +0200, Matthieu Baerts (NGI0) wrote:
>> In the local address list, there can be entries with the port set to
>> 0
>> -- corresponding to the source port used by the initial subflow --
>> and
>> others with a specific port.
>>
>> When performing a lookup, it is important to compare the ports to
>> pick
>> the right entry: when a specific port is given, then try to match it
>> first. If no match is found, try to find entries with the port set to
>> 0.
>>
>> Fixes: 24430f8bf516 ("mptcp: add address into userspace pm list")
>> Signed-off-by: Matthieu Baerts (NGI0) <[email protected]>
>> ---
>> v3: new (Sashiko)
>> ---
>>  net/mptcp/pm_userspace.c | 14 +++++++++++++-
>>  1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
>> index 663cbeb79548..3f1471ec3fc7 100644
>> --- a/net/mptcp/pm_userspace.c
>> +++ b/net/mptcp/pm_userspace.c
>> @@ -33,10 +33,22 @@ mptcp_userspace_pm_lookup_addr(struct mptcp_sock
>> *msk,
>>  {
>>  	struct mptcp_pm_addr_entry *entry;
>>  
>> +	/* Compare ports when set in addr */
>>  	mptcp_for_each_userspace_pm_addr(msk, entry) {
>> -		if (mptcp_addresses_equal(&entry->addr, addr,
>> false))
>> +		if (mptcp_addresses_equal(&entry->addr, addr, addr-
>>> port != 0))
>>  			return entry;
>>  	}
>> +
>> +	if (addr->port == 0)
>> +		return NULL;
>> +
>> +	/* Check only wildcard ports if no exact match with the port
>> */
>> +	mptcp_for_each_userspace_pm_addr(msk, entry) {
>> +		if (entry->addr.port == 0 &&
>> +		    mptcp_addresses_equal(&entry->addr, addr,
>> false))
>> +			return entry;
>> +	}
>> +
>>  	return NULL;
>>  }
> 
> Personally, I think a single-pass lookup is better than a two-pass one.

Indeed, I initially thought the code wouldn't be very readable, but
maybe worth it.

> I've implemented a version and it passed the tests:
> 
> static struct mptcp_pm_addr_entry *
> mptcp_userspace_pm_lookup_addr(struct mptcp_sock *msk,
>                                const struct mptcp_addr_info *addr)
> {
>     struct mptcp_pm_addr_entry *entry, *wildcard = NULL;
>     struct mptcp_addr_info match;
> 
>     mptcp_for_each_userspace_pm_addr(msk, entry) {
>         match = entry->addr;

Mmh, but now there is a copy for each entry. Not sure what's better.

>         if (match.port == 0 && addr->port != 0)

(Would it not work to add this condition to the last argument of
mptcp_addresses_equal()? → EDIT: no, see below)

>             match.port = addr->port;

It feels wrong: entry->addr.port == 0 should mean "same port as the msk"
(inet_sk((struct sock *)msk)->inet_sport). But this "addr->port" is
possibly yet another port.

In other words, I think doing that here means accepting the first
wildcard one. An example: ID0 is now the first entry, if there is
another IP with another specific port, it should be picked in priority.
With this code here, I don't think that will be the case: the ID0 entry
will be picked instead, no?

(I think we should have a test with the userspace PM announcing the same
address but with another port + doing the listen for this port manually,
and checking the IDs being used: shouldn't be 0)

>         if (mptcp_addresses_equal(&match, addr, addr->port != 0)) {

What if we always call mptcp_addresses_equal without the port check, and
do that "manually" here below?

-> Return if the port is the same, then save it as wildcard if one of
them has no port set.

Even there, it feels like the comparison should be stricter: if one port
is 0, either the other one is 0 (port is the same, already checked then)
or equal to the msk one. I think I quickly tried this at some points,
and it was causing issues, but I didn't investigate. Maybe the tests are
wrong (mixing up IDs) or there is another bug somewhere... Do you mind
looking at this if you have the opportunity, please?

>             if (addr->port == 0 || entry->addr.port != 0)
>                 return entry;
> 
>             if (!wildcard)
>                 wildcard = entry;
>         }
>     }
> 
>     return wildcard;
> }
> 
> What do you think of this approach?
> 
> Also, this series has conflicts with the current export branch and
> needs a rebase.
Yes indeed. But let's focus on this case above, and wait a bit longer to
have a review on the other patches as well.

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