[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 2/8] xen/evtchn: modify evtchn_alloc_unbound to allocate specified port
Hi Jan,
On 13/07/2022 11:56, Jan Beulich wrote:
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.
You are right. But we need to be cautious in adding new one that
overlaps with existing one.
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.
This is similar to Xen forbidding to close a static port: it is not the
hypervisor business to check that there are enough event channel ports
freed for dynamic allocation.
Cheers,
--
Julien Grall
|
