Re: [PATCH v2 2/3] xen/domain: fix UBSAN null pointer dereference of d->shared_info

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.06.2026 11:47, Oleksii Kurochko wrote:
> On 6/3/26 11:22 AM, Jan Beulich wrote:
>> On 03.06.2026 11:01, Oleksii Kurochko wrote:
>>> On 6/3/26 10:18 AM, Jan Beulich wrote:
>>>> On 03.06.2026 10:07, Oleksii Kurochko wrote:
>>>>> On 6/3/26 7:54 AM, Jan Beulich wrote:
>>>>>> On 02.06.2026 18:11, Oleksii Kurochko wrote:
>>>>>>> On 6/2/26 1:19 PM, Jan Beulich wrote:
>>>>>>>> On 25.05.2026 15:20, Oleksii Kurochko wrote:
>>>>>>> --- a/xen/common/event_fifo.c
>>>>>>> +++ b/xen/common/event_fifo.c
>>>>>>> @@ -562,9 +562,10 @@ static void setup_ports(struct domain *d, unsigned
>>>>>>> int prev_evtchns)
>>>>>>>
>>>>>>>              evtchn = evtchn_from_port(d, port);
>>>>>>>
>>>>>>> -        if ( d->shared_info &&
>>>>>>> -             guest_test_bit(d, port, &shared_info(d, evtchn_pending)) )
>>>>>>> +#ifdef CONFIG_HAS_SHARED_INFO
>>>>>>> +        if ( guest_test_bit(d, port, &shared_info(d, evtchn_pending)) )
>>>>>>>                  evtchn->pending = true;
>>>>>>> +#endif
>>>>>>
>>>>>> While as per above shared_info() would best not exist when 
>>>>>> !HAS_SHARED_INFO
>>>>>> (in which case #ifdef may be unavoidable here), an alternative where
>>>>>> IS_ENABLED() could be used here may want at least considering. E.g.
>>>>>> causing a link-time failure when shared_info() is used (and not compiled
>>>>>> out).
>>>>
>>>> ... here. There are downsides to this, so which route to go needs settling
>>>> on.
>>>
>>> For an alternative approach are you okay with the following introduction:
>>>
>>> #ifdef CONFIG_HAS_SHARED_INFO
>>> #define shared_info(d, field)      __shared_info(d, (d)->shared_info, field)
>>> #else
>>> void *__shared_info_unavailable(void);
>>> #define shared_info(d, field) \
>>>       (*(typeof(__shared_info(d, (d)->shared_info, field))
>>> *)__shared_info_unavailable())
>>> #endif
>>>
>>> And then use IS_ENABLED(CONFIG_HAS_SHARED_INFO) everywhere where
>>> shared_info() is used including the case above:
>>>
>>> v->vcpu_info_area.map =
>>>       IS_ENABLED(CONFIG_HAS_SHARED_INFO) && v->vcpu_id < 
>>> XEN_LEGACY_MAX_VCPUS
>>>       ? (vcpu_info_t *)&shared_info(d, vcpu_info[v->vcpu_id])
>>>       : &dummy_vcpu_info;
>>>
>>> Everything that in event_2l.c could go for now without
>>> IS_ENABLED(CONFIG_HAS_SHARED_INFO) where shared_info() is used as that
>>> code isn't expected to be called by arch which doesn't support 2L so no
>>> linkage error will occur.
>>
>> Yes, this roughly is what I was thinking of. I'd like to remind you though
>> of issues with identifiers with two leading underscores.
> 
> For event_2l.c would you be okay to make compilation of it conditional 
> by CONFIG_HAS_SHARED_INFO or it would be better to introduce separate 
> CONFIG_HAS_EVTCHN_2L:
>    -obj-y += event_2l.o
>    +obj-$(CONFIG_HAS_EVTCHN_2L) += event_2l.o

See my earlier reply: Let's stick to just HAS_SHARED_INFO unless there's
a clear need to distinguish that from 2-level event channels support.

Jan