Re: [PATCH 2/8] xen/evtchn: modify evtchn_alloc_unbound to allocate specified port

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.07.2022 12:18, Julien Grall wrote:
> On 13/07/2022 10:53, Jan Beulich wrote:
>> On 13.07.2022 11:35, Julien Grall wrote:
>>> On 13/07/2022 07:21, Jan Beulich wrote:
>>>>>> For the FIFO issue, we can introduce the new config option to restrict 
>>>>>> the maximum number of static
>>>>>> port supported in Xen. We can check the user-defined static port when we 
>>>>>> parse the device tree and if
>>>>>> a user-defined static port is greater than the maximum allowed static 
>>>>>> port will return an error to the user.
>>>>>> In this way, we can avoid allocating a lot of memory to fill the hole.
>>>>>>
>>>>>> Let me know your view on this.
>>>>>>
>>>>>> config MAX_STATIC_PORT
>>>>>>        int "Maximum number of static ports”
>>>>>>        range 1 4095
>>>>>>        help
>>>>>>           Controls the build-time maximum number of static port supported
>>>>>
>>>>> The problem is not exclusive to the static event channel. So I don't
>>>>> think this is right to introduce MAX_STATIC_PORT to mitigate the issue
>>>>> (even though this is the only user today).
>>>>>
>>>>> A few of alternative solutions:
>>>>>      1) Handle preemption in alloc_evtchn_bucket()
>>>>>      2) Allocate all the buckets when the domain is created (the max
>>>>> numbers event channel is known). We may need to think about preemption
>>>>>      3) Tweak is_port_valid() to check if the bucket is valid. This would
>>>>> introduce a couple of extra memory access (might be OK as the bucket
>>>>> would be accessed afterwards) and we would need to update some users.
>>>>>
>>>>> At the moment, 3) is appealing me the most. I would be interested to
>>>>> have an opionions from the other maintainers.
>>>>
>>>> Fwiw of the named alternatives I would also prefer 3. Whether things
>>>> really need generalizing at this point I'm not sure, though.
>>> I am worry that we may end up to forget that we had non-generaic way
>>> (e.g. MAX_STATIC_PORT) to prevent trigger the issue. So we could end up
>>> to mistakenly introduce a security issue.
>>>
>>> However, my point was less about generalization but more about
>>> introducing CONFIG_MAX_STATIC_PORT.
>>>
>>> It seems strange to let the admin to decide the maximum number of static
>>> port supported.
>>
>> Why (assuming s/admin/build admin/)? I view both a build time upper bound
>> as well as (alternatively) a command line driven upper bound as reasonable
>> (in the latter case there of course still would want/need to be an upper
>> bound on what is legitimate to specify from the command line). Using
>> static ports after all means there's a static largest port number.
>> Determined across all intended uses of a build such an upper bound can be
>> a feasible mechanism.
> 
> AFAICT, the limit is only here to mitigate the risk with the patch I 
> proposed. With a proper fix, the limit would be articial and therefore 
> it is not clear why the admin should be able to configure it (even 
> temporarily).

The limit would be as artificial as other limits we enforce. I can't
see why it would be wrong to have a more tight limit on static ports
than on traditional ("dynamic") ones. Even if only to make sure so
many dynamic ones are left. That said, ...

> Instead, I think we want to have a limit that apply for both statically 
> and dynamically allocated even channel. This is what d->max_evtchn_port 
> is for.

... I also have no issue with following your way of thinking. I view
both perspectives as valid ones to take.

Jan