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

From: Julien Grall <julien@xen.org>
To: Jan Beulich <jbeulich@suse.com>
Cc: xen-devel <xen-devel@lists.xenproject.org>,
	Bertrand Marquis <Bertrand.Marquis@arm.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	George Dunlap <george.dunlap@citrix.com>, Wei Liu <wl@xen.org>,
	Rahul Singh <Rahul.Singh@arm.com>
Subject: Re: [PATCH 2/8] xen/evtchn: modify evtchn_alloc_unbound to allocate specified port
Date: Wed, 13 Jul 2022 11:18:36 +0100	[thread overview]
Message-ID: <758779b3-ef39-aa95-15c9-9b84b952e80b@xen.org> (raw)
In-Reply-To: <5f200481-ed3c-a463-90aa-3718c0ab57a3@suse.com>

Hi Jan,

On 13/07/2022 10:53, Jan Beulich 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.

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).

Instead, I think we want to have a limit that apply for both statically 
and dynamically allocated even channel. This is what d->max_evtchn_port 
is for.

Cheers,

-- 
Julien Grall