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

[Julien Grall <julien@xxxxxxx>]

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.
If we want to rely on non-generic mechanism, then I think the right way 
to go is to restrict max_evtchn_port for domUs to 4096 (it is -1 at the 
moment). If we want to give more flexibility then it should be a 
per-domain property in the DT.
Cheers,

--
Julien Grall