Re: [PATCH mptcp-next v2 10/21] mptcp: handle local addrs announced by userspace PMs

[Kishen Maloor <kishen.maloor@intel.com>]

On 1/19/22 12:44 PM, Mat Martineau wrote:
> On Wed, 19 Jan 2022, Kishen Maloor wrote:
> 
>> On 1/19/22 11:20 AM, Mat Martineau wrote:
>>> On Tue, 18 Jan 2022, Kishen Maloor wrote:
>>>
>>>> On 1/14/22 9:11 AM, Paolo Abeni wrote:
>>>>> On Wed, 2022-01-12 at 17:15 -0500, Kishen Maloor wrote:
>>>>>> This change adds a new internal function to store/retrieve local
>>>>>> addrs announced by userspace PM implementations from the kernel
>>>>>> context. The function does not stipulate any limitation on the #
>>>>>> of addrs, and handles the requirements of three scenarios:
>>>>>> 1) ADD_ADDR announcements (which require that a local id be
>>>>>> provided), 2) retrieving the local id associated with an address,
>>>>>> also where one may need to be assigned, and 3) reissuance of
>>>>>> ADD_ADDRs when there's a successful match of addr/id.
>>>>>>
>>>>>> The list of all stored local addr entries is held under the
>>>>>> MPTCP sock structure. This list, if not released by the REMOVE_ADDR
>>>>>> flow is freed while the sock is destructed.
>>>>>
>>>>> It feels strange to me that we need to maintain an additional addresses
>>>>> list inside the kernel for the user-space PM - which should take care
>>>>> of all the status information.
>>>>
>>>> The PM daemon will have the complete picture, but the protocol needs to know the
>>>> local ids assigned to addresses so as such the kernel has to store addresses
>>>> (with their ids) regardless of PM.
>>>>
>>>>>
>>>>> Why anno_list is not enough? Why isn't cheapest to extend it?
>>>>
>>>> The anno_list is the list of addresses that were announced via the ADD_ADDR command, to
>>>> be used specifically for doing re-transmissions.
>>>>
>>>> However, the implementation can also accept connections at local addresses that were not
>>>> explicitly announced (hence not in anno_list), and in this case the kernel records the address
>>>> and assigns it a local id unique in the scope of its connection.
>>>>
>>>> So basically msk->local_addr_list is the list of all known local addresses, both announced
>>>> and not so their ids can be later retrieved.
>>>>
>>>> To give you more context, in my last iteration of the code before I posted the series, I was storing local addrs
>>>> in pernet->local_addr_list just as its done for the kernel PM, but later moved it to
>>>> a per-msk list to eliminate contention (in accessing that list) with other userspace or kernel
>>>> PM managed connections.
>>>>
>>>>>
>>>>> Being the list unlimited a malicius (or buggy) user-space could consume
>>>>> all the kernel memory. I think we need some limits, or at least some
>>>>> accounting.
>>>>
>>>> At this point we're taking the PM daemon as a trusted entity which can issue these
>>>> netlink commands for path management. So there is currently no configurable ceiling
>>>> in the kernel on the size of the PM's kernel stored context.
>>>>
>>>
>>> I think it's still worthwhile to have some limits/accounting as Paolo suggests - part of the point of pushing PM code to userspace is so bugs or other vulnerabilities don't take down the whole machine.
>>>
>>
>> Agreed, but do we put circuit breakers inside the kernel or the PM daemon? (for e.g.,
>> if a PM plug-in can see limitations imposed by the daemon?)
>>
> 
> Kernel, I'd say.
> 
>> If we consider userspace path management as providing "more flexibility",
>> then conceivably the kernel would expose hooks to the PM daemon to bump up any
>> kernel enforced limits when necessary. So a malicious or buggy PM could also raise
>> those limits.
>>
> 
> True, an admin (or bad actor) can get themselves into trouble by setting nonsensical limits. But the limits remain useful for stability and managing resources.
> 
>> Since interactions over the netlink interface are to be carried out by a privileged
>> entity, I've been assuming that the PM daemon is to be trusted.
>>
> 
> We're trusting the PM daemon with certain (granular) privileges/capabilities using CAP_NET_ADMIN, not the old "root-controls-all" model.
> 
>> On the other hand, if we're talking about having fixed upper bounds in the kernel wrt
>> # of addrs/subflows (which cannot be changed by the PM daemon), then that could make sense.
>>
> 
> That's pretty much what I'm talking about, maybe tunable (within a range) by a sysctl.

Sounds good, I will look into adding a couple new params, configurable via sysctl for regulating activities by userspace PMs.
Note that these will be unrelated to the equivalent limits that we have in place for the kernel PM. 

> 
> -- 
> Mat Martineau
> Intel