Re: [PATCH mptcp-next] mptcp: pm: reduce entries iterations on connect

[Paolo Abeni <pabeni@redhat.com>]

On 7/22/24 17:55, Matthieu Baerts wrote:
> On 22/07/2024 17:14, Paolo Abeni wrote:
>> On 7/19/24 16:26, Matthieu Baerts (NGI0) wrote:
>>> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
>>> index 1b0e1617e90a..9fed7c92e52b 100644
>>> --- a/net/mptcp/pm_netlink.c
>>> +++ b/net/mptcp/pm_netlink.c
>>> @@ -633,8 +633,9 @@ static void mptcp_pm_nl_subflow_established(struct
>>> mptcp_sock *msk)
>>>     */
>>>    static unsigned int fill_local_addresses_vec(struct mptcp_sock *msk,
>>>                             struct mptcp_addr_info *remote,
>>> -                         struct mptcp_addr_info *addrs)
>>> +                         struct mptcp_pm_addr_entry *entries)
>>>    {
>>> +    struct mptcp_pm_addr_entry new_entry;
>>>        struct sock *sk = (struct sock *)msk;
>>>        struct mptcp_pm_addr_entry *entry;
>>>        struct mptcp_addr_info mpc_addr;
>>> @@ -655,14 +656,14 @@ static unsigned int
>>> fill_local_addresses_vec(struct mptcp_sock *msk,
>>>                continue;
>>>              if (msk->pm.subflows < subflows_max) {
>>> -            msk->pm.subflows++;
>>> -            addrs[i] = entry->addr;
>>> +            memcpy(&new_entry, entry, sizeof(new_entry));
>>>                  /* Special case for ID0: set the correct endpoint */
>>>                if (mptcp_addresses_equal(&entry->addr, &mpc_addr,
>>> entry->addr.port))
>>> -                addrs[i].id = 0;
>>> +                new_entry.addr.id = 0;
>>>    -            i++;
>>> +            msk->pm.subflows++;
>>> +            entries[i++] = new_entry;
>>
>> 'new_entry' is escaping the rcu protected section, dereferencing
>> 'entries' after the rcu unlock below could cause UaF.
>>
>> Note, AFAICS we already have a similar problem in select_local_address().
> 
> Good catch!
> 
> And with select_signal_address() since the beginning as well, no?

Yep.

>> One possibility would be to do a deep copy of mptcp_pm_addr_entry, but
>> that will waste a lot of memory on the stack. What about to copy the id
>> separately?
> 
> Is it not what is already done here? Before, only 'entry->addr' was
> copied, now the whole 'entry' is duplicated with the 'memcpy()' call
> above. Is it not enough?

Yes, it is. I got lost into the incremental diffs.

> I'm doing that because we also need the 'flags' and 'ifindex' info from
> this structure -- which has 3 additional pointers we don't need, but I
> thought it was OK to re-use it -- on top of the address we had before.
> 
> BTW, I just realised the copy is done twice (entry -> new_entry ->
> entries[i]). Same below. I can fix that.

Yes, would be better to avoid multiple copies.

The additional memory user is a bit concerning for the 
fill_local_addresses_vec() case, where the total amount of data on the 
stack grows from 192 (already quite big) to 384 (quite concerning)...

Paolo