[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,

On 13/07/2022 11:03, Bertrand Marquis wrote:


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.

Let me start that I think it would be a mistake to introduce a command line option (or Kconfig) that have high chance to disappear or would become moot.

Today we already have a per-domain limit for the number of event channels (see d->max_evtchn_port). If we leave aside the issue in the event channel code, it is not clear to me why we need a separate limit for statically allocated event channel.

Cheers,

--
Julien Grall



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.