From:   Michael Ellerman <mpe@ellerman.id.au>
To:     Roger Pau =?utf-8?Q?Monn=C3=A9?= <roger.pau@citrix.com>,
        Stefano Stabellini <sstabellini@kernel.org>,
        Juergen Gross <jgross@suse.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jiri Slaby <jirislaby@kernel.org>,
        linux-kernel@vger.kernel.org, xen-devel@lists.xenproject.org,
        linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH v2] hvc/xen: prevent concurrent accesses to the shared ring
In-Reply-To: <ZAm8BDGTMaI0XmMI@Air-de-Roger>
References: <20221130150919.13935-1-roger.pau@citrix.com>
 <alpine.DEB.2.22.394.2211301657200.4039@ubuntu-linux-20-04-desktop>
 <Y4nkFZal7oy+aICa@Air-de-Roger> <Y5cgYLNwtPbmP1JL@Air-de-Roger>
 <ZAm8BDGTMaI0XmMI@Air-de-Roger>
Date:   Fri, 10 Mar 2023 10:01:39 +1100
Message-ID: <87zg8lo7fw.fsf@mpe.ellerman.id.au>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Roger Pau Monn=C3=A9 <roger.pau@citrix.com> writes:
> On Mon, Dec 12, 2022 at 01:36:48PM +0100, Roger Pau Monn=C3=A9 wrote:
>> On Fri, Dec 02, 2022 at 12:40:05PM +0100, Roger Pau Monn=C3=A9 wrote:
>> > On Wed, Nov 30, 2022 at 05:08:06PM -0800, Stefano Stabellini wrote:
>> > > On Wed, 30 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 loc=
ks
>> > > > that's shared between the tty and the console implementations, hen=
ce
>> > > > 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 shar=
ed
>> > > > 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 modi=
fied
>> > > > so the notify_daemon() call can be done outside of the locked regi=
on:
>> > > > it's an hypercall and there's no need for it to be done with the l=
ock
>> > > > held.
>> > >
>> > > For domU_read_console: I don't mean to block this patch but we need =
to
>> > > be sure about the semantics of hv_ops.get_chars. Either it is expect=
ed
>> > > to be already locked, then we definitely shouldn't add another lock =
to
>> > > domU_read_console. Or it is not expected to be already locked, then =
we
>> > > should add the lock.
>> > >
>> > > My impression is that it is expected to be already locked, but I thi=
nk
>> > > we need Greg or Jiri to confirm one way or the other.
>> >
>> > Let me move both to the 'To:' field then.
>> >
>> > My main concern is the usage of hv_ops.get_chars hook in
>> > hvc_poll_get_char(), as it's not obvious to me that callers of
>> > tty->poll_get_char hook as returned by tty_find_polling_driver() will
>> > always do so with the tty lock held (in fact the only user right now
>> > doesn't seem to hold the tty lock).
>> >
>> > > Aside from that the rest looks fine.
>
> I guess I could reluctantly remove the lock in the get_chars hook,
> albeit I'm not convinced at all the lock is not needed there.

I don't know the xen driver, but other HVC backends have a lock around
their private state in their get_chars() implementations.

See eg. hvterm_raw_get_chars().

cheers