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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

> On 13 Jul 2022, at 10:53, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> 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.

I agree with this. Right now all static port must be defined on boot so this is 
fully
ok to have a maximum compilation value or something that can be customised
on command line.
In this case the admin must be fully aware of what he does from the start.

Also whatever value we define as default will probably be far bigger than any
real use case I think.

Bertrand

> 
> Jan