Re: [PATCH] hvc/xen: prevent concurrent accesses to the shared ring

[Jan Beulich]

On 30.11.2022 10:26, Roger Pau Monné wrote:
> On Tue, Nov 29, 2022 at 02:12:10PM -0800, Stefano Stabellini wrote:
>> On Tue, 29 Nov 2022, Roger Pau Monne wrote:
>>> The hvc machinery registers both a console and a tty device based on
>>> the hv ops provided by the specific implementation.  Those two
>>> interfaces however have different locks, and there's no single locks
>>> that's shared between the tty and the console implementations, hence
>>> the driver needs to protect itself against concurrent accesses.
>>> Otherwise concurrent calls using the split interfaces are likely to
>>> corrupt the ring indexes, leaving the console unusable.
>>>
>>> Introduce a lock to xencons_info to serialize accesses to the shared
>>> ring.  This is only required when using the shared memory console,
>>> concurrent accesses to the hypercall based console implementation are
>>> not an issue.
>>>
>>> Note the conditional logic in domU_read_console() is slightly modified
>>> so the notify_daemon() call can be done outside of the locked region:
>>> it's an hypercall and there's no need for it to be done with the lock
>>> held.
>>>
>>> Fixes: b536b4b96230 ('xen: use the hvc console infrastructure for Xen console')
>>> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>>> ---
>>> While the write handler (domU_write_console()) is used by both the
>>> console and the tty ops, that's not the case for the read side
>>> (domU_read_console()).  It's not obvious to me whether we could get
>>> concurrent poll calls from the poll_get_char tty hook, hence stay on
>>> the safe side also serialize read accesses in domU_read_console().
>>
>> I think domU_read_console doesn't need it. struct hv_ops and struct
>> console are both already locked although independently locked.
>>
>> I think we shouldn't add an unrequired lock there.
> 
> Not all accesses are done using the tty lock.  There's a path using
> tty_find_polling_driver() in kgdboc.c that directly calls into the
> ->poll_get_char() hook without any locks apparently taken.

Simply by the name of the file I'm inclined to say that debugger code
not respecting locks may be kind of intentional (but would then need
to be accompanied by certain other precautions there).

Jan