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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.07.2022 11:35, Julien Grall wrote:
> Hi,
> 
> 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.

Jan