Re: [PATCH RFC 3/4] mptcp: allow the in kernel PM to set MPC subflow priority

[Paolo Abeni <pabeni@redhat.com>]

On Tue, 2022-06-28 at 17:11 -0700, Mat Martineau wrote:
> On Fri, 24 Jun 2022, Paolo Abeni wrote:
> 
> > Any local endpoints configured on the address matching the
> > MPC subflow are currently ignored.
> > 
> > Specifically, setting a backup flag on them has no effect
> > on the first subflow, as the MPC handshake can't carry such
> > info.
> > 
> > This change refactors the MPC endpoint id accounting to
> > additionally fetch the priority info from the relevant endpoint
> > and eventually trigger the MP_PRIO handshake as needed.
> > 
> > As a result, the MPC subflow now switches to backup priority
> > after that the MPTCP socket is fully established, according
> > to the local endpoint configuration.
> > 
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > ---
> > net/mptcp/pm_netlink.c | 37 +++++++++++++++----------------------
> > 1 file changed, 15 insertions(+), 22 deletions(-)
> > 
> > diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> > index 91f6ed2a076a..a6eb501e5031 100644
> > --- a/net/mptcp/pm_netlink.c
> > +++ b/net/mptcp/pm_netlink.c
> > @@ -505,30 +505,14 @@ __lookup_addr(struct pm_nl_pernet *pernet, const struct mptcp_addr_info *info,
> > 	struct mptcp_pm_addr_entry *entry;
> > 
> > 	list_for_each_entry(entry, &pernet->local_addr_list, list) {
> > -		if ((!lookup_by_id && mptcp_addresses_equal(&entry->addr, info, true)) ||
> > +		if ((!lookup_by_id &&
> > +		     mptcp_addresses_equal(&entry->addr, info, entry->addr.port)) ||
> 
> It seems like we could have multiple entries in the local_addr_list with 
> the same address, but with different entries having port==0 or nonzero 
> ports. If mptcp_nl_cmd_set_flags() is called with a nonzero port, but this 
> lookup function encounters the port==0 entry first, this will match an 
> unexpected entry in local_addr_list.
> 
> Is there some other constraint preventing this?

Uhmmm.. I updated the mptcp_addresses_equal() statement to match the
existing one in lookup_id_by_addr() below.

You are right, this check is uncorrect, and generally speaking the
mptcp_addresses_equal() invocations are a bit fuzzy. I'll try to
clarify that with an additional patches:

- mptcp_addresses_equal() on subflow list use port matching if the
looked-up address has  non 0 port (the current code looks correct)

- mptcp_addresses_equal on local_addr_list should use port matching if
address_use_port(entry), and entry with port matching should be
preferred over entries without port-matching. The latter can be
enforces adding entries with address_use_port(entry) at the list head
and entries with !address_use_port(entry) at tail

WDYT?

Thanks!

Paolo