Re: [PATCH v2] hvc/xen: prevent concurrent accesses to the shared ring
On Fri, Mar 10, 2023 at 10:18:32AM +0100, Roger Pau Monné wrote: > On Fri, Mar 10, 2023 at 10:01:39AM +1100, Michael Ellerman wrote: > > Roger Pau Monné writes: > > > On Mon, Dec 12, 2022 at 01:36:48PM +0100, Roger Pau Monné wrote: > > >> On Fri, Dec 02, 2022 at 12:40:05PM +0100, Roger Pau Monné 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 > > >> > > > 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. > > >> > > > > >> > > 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 > > >> > > expected > > >> > > 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 > > >> > > think > > >> > > 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(). > > Yes, that was one of the motivation for adding the lock also here, and > it has already been mentioned. The other is the usage of the hooks by > callers of hvc_poll_get_char(). Ping? Thanks, Roger.
Re: [PATCH v2] hvc/xen: prevent concurrent accesses to the shared ring
On Fri, Mar 10, 2023 at 10:01:39AM +1100, Michael Ellerman wrote: > Roger Pau Monné writes: > > On Mon, Dec 12, 2022 at 01:36:48PM +0100, Roger Pau Monné wrote: > >> On Fri, Dec 02, 2022 at 12:40:05PM +0100, Roger Pau Monné 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 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. > >> > > > >> > > 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 expected > >> > > 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 think > >> > > 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(). Yes, that was one of the motivation for adding the lock also here, and it has already been mentioned. The other is the usage of the hooks by callers of hvc_poll_get_char(). Thanks, Roger.
Re: [PATCH v2] hvc/xen: prevent concurrent accesses to the shared ring
Hello, It's been 3 months and no reply. On Mon, Dec 12, 2022 at 01:36:48PM +0100, Roger Pau Monné wrote: > Hello, > > Gentle ping regarding the locking question below. > > Thanks, Roger. > > On Fri, Dec 02, 2022 at 12:40:05PM +0100, Roger Pau Monné 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 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. > > > > > > 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 expected > > > 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 think > > > 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. Roger.
Re: [PATCH v2] hvc/xen: prevent concurrent accesses to the shared ring
Hello, Gentle ping regarding the locking question below. Thanks, Roger. On Fri, Dec 02, 2022 at 12:40:05PM +0100, Roger Pau Monné 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 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. > > > > 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 expected > > 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 think > > 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. > > Thanks for the review, Roger. >
Re: [PATCH v2] hvc/xen: prevent concurrent accesses to the shared ring
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 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. > > 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 expected > 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 think > 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. Thanks for the review, Roger.
Re: [PATCH] hvc/xen: prevent concurrent accesses to the shared ring
On Wed, Nov 30, 2022 at 10:34:41AM +0100, Jan Beulich wrote:
> 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é
> >>> ---
> >>> 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).
I'm also confused because hvc_poll() which calls get_chars() does so
while holding an hvc lock, while hvc_poll_get_char() calls get_chars()
without holding any lock. The call to get_chars() being done with a
lock held in hvc_poll() might just be a side-effect of the lock
being hold to keep consistency in the hvc_struct struct.
I also wonder whether new users of tty_find_polling_driver() and
->poll_get_char() could start appearing and assuming that the
underlying implementation would already take the necessary locks for
consistency. Just looking at hvc_vio.c it does take a lock in
its get_chars() implementation to serialize accesses to the buffer.
Thanks, Roger.
Re: [PATCH] hvc/xen: prevent concurrent accesses to the shared ring
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é
> > ---
> > 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.
>
> > ---
> > drivers/tty/hvc/hvc_xen.c | 16 ++--
> > 1 file changed, 14 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c
> > index 7c23112dc923..d65741983837 100644
> > --- a/drivers/tty/hvc/hvc_xen.c
> > +++ b/drivers/tty/hvc/hvc_xen.c
> > @@ -43,6 +43,7 @@ struct xencons_info {
> > int irq;
> > int vtermno;
> > grant_ref_t gntref;
> > + spinlock_t ring_lock;
> > };
> >
> > static LIST_HEAD(xenconsoles);
> > @@ -84,12 +85,15 @@ static int __write_console(struct xencons_info *xencons,
> > XENCONS_RING_IDX cons, prod;
> > struct xencons_interface *intf = xencons->intf;
> > int sent = 0;
> > + unsigned long flags;
> >
> > + spin_lock_irqsave(>ring_lock, flags);
> > cons = intf->out_cons;
> > prod = intf->out_prod;
> > mb(); /* update queue values before going on */
> >
> > if ((prod - cons) > sizeof(intf->out)) {
> > + spin_unlock_irqrestore(>ring_lock, flags);
> > pr_err_once("xencons: Illegal ring page indices");
> > return -EINVAL;
> > }
> > @@ -99,6 +103,7 @@ static int __write_console(struct xencons_info *xencons,
> >
> > wmb(); /* write ring before updating pointer */
> > intf->out_prod = prod;
> > + spin_unlock_irqrestore(>ring_lock, flags);
> >
> > if (sent)
> > notify_daemon(xencons);
> > @@ -141,16 +146,19 @@ static int domU_read_console(uint32_t vtermno, char
> > *buf, int len)
> > int recv = 0;
> > struct xencons_info *xencons = vtermno_to_xencons(vtermno);
> > unsigned int eoiflag = 0;
> > + unsigned long flags;
> >
> > if (xencons == NULL)
> > return -EINVAL;
> > intf = xencons->intf;
> >
> > + spin_lock_irqsave(>ring_lock, flags);
> > cons = intf->in_cons;
> > prod = intf->in_prod;
> > mb(); /* get pointers before reading ring */
> >
> > if ((prod - cons) > sizeof(intf->in)) {
> > + spin_unlock_irqrestore(>ring_lock, flags);
> > pr_err_once("xencons: Illegal ring page indices");
> > return -EINVAL;
> > }
> > @@ -174,10 +182,13 @@ static int domU_read_console(uint32_t vtermno, char
> > *buf, int len)
> > xencons->out_cons = intf->out_cons;
> > xencons->out_cons_same =
