[XEN PATCH] evtchn/fifo: Don't set PENDING bit if guest misbehaves

[XEN PATCH] evtchn/fifo: Don't set PENDING bit if guest misbehaves

[Raphael Ning]

From: Raphael Ning <raphning@amazon.com>

Currently, evtchn_fifo_set_pending() will mark the event as PENDING even
if it fails to lock the FIFO event queue(s), or if the guest has not
initialized the FIFO control block for the target vCPU. A well-behaved
guest should never trigger either of these cases.

There is no good reason to set the PENDING bit (the guest should not
depend on this behaviour anyway) or check for pollers in such corner
cases, so skip that. In fact, both the comment above the for loop and
the commit message for

 41a822c39263 xen/events: rework fifo queue locking

suggest that the bit should be set after the FIFO queue(s) are locked.

Take the opportunity to rename the was_pending variable (flipping its
sense) and switch to the standard bool type.

Suggested-by: David Vrabel <dvrabel@amazon.co.uk>
Signed-off-by: Raphael Ning <raphning@amazon.com>
---
 xen/common/event_fifo.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/common/event_fifo.c b/xen/common/event_fifo.c
index ed4d3beb10f3..6c74ccebebb7 100644
--- a/xen/common/event_fifo.c
+++ b/xen/common/event_fifo.c
@@ -165,7 +165,7 @@ static void cf_check evtchn_fifo_set_pending(
     unsigned int port;
     event_word_t *word;
     unsigned long flags;
-    bool_t was_pending;
+    bool_t check_pollers = false;
     struct evtchn_fifo_queue *q, *old_q;
     unsigned int try;
     bool linked = true;
@@ -226,8 +226,6 @@ static void cf_check evtchn_fifo_set_pending(
         spin_unlock_irqrestore(&q->lock, flags);
     }
 
-    was_pending = guest_test_and_set_bit(d, EVTCHN_FIFO_PENDING, word);
-
     /* If we didn't get the lock bail out. */
     if ( try == 3 )
     {
@@ -249,6 +247,8 @@ static void cf_check evtchn_fifo_set_pending(
         goto unlock;
     }
 
+    check_pollers = !guest_test_and_set_bit(d, EVTCHN_FIFO_PENDING, word);
+
     /*
      * Link the event if it unmasked and not already linked.
      */
@@ -314,7 +314,7 @@ static void cf_check evtchn_fifo_set_pending(
                                  &v->evtchn_fifo->control_block->ready) )
         vcpu_mark_events_pending(v);
 
-    if ( !was_pending )
+    if ( check_pollers )
         evtchn_check_pollers(d, port);
 }
 
-- 
2.32.0

Re: [XEN PATCH] evtchn/fifo: Don't set PENDING bit if guest misbehaves

[Andrew Cooper]

On 16/03/2022 18:38, Raphael Ning wrote:
> From: Raphael Ning <raphning@amazon.com>
>
> Currently, evtchn_fifo_set_pending() will mark the event as PENDING even
> if it fails to lock the FIFO event queue(s), or if the guest has not
> initialized the FIFO control block for the target vCPU. A well-behaved
> guest should never trigger either of these cases.
>
> There is no good reason to set the PENDING bit (the guest should not
> depend on this behaviour anyway) or check for pollers in such corner
> cases, so skip that. In fact, both the comment above the for loop and
> the commit message for
>
>  41a822c39263 xen/events: rework fifo queue locking
>
> suggest that the bit should be set after the FIFO queue(s) are locked.
>
> Take the opportunity to rename the was_pending variable (flipping its
> sense) and switch to the standard bool type.
>
> Suggested-by: David Vrabel <dvrabel@amazon.co.uk>
> Signed-off-by: Raphael Ning <raphning@amazon.com>
> ---
>  xen/common/event_fifo.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/xen/common/event_fifo.c b/xen/common/event_fifo.c
> index ed4d3beb10f3..6c74ccebebb7 100644
> --- a/xen/common/event_fifo.c
> +++ b/xen/common/event_fifo.c
> @@ -165,7 +165,7 @@ static void cf_check evtchn_fifo_set_pending(
>      unsigned int port;
>      event_word_t *word;
>      unsigned long flags;
> -    bool_t was_pending;
> +    bool_t check_pollers = false;

Considering your commit message, did you intend to change this to bool?

Can be fixed on commit.  Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

~Andrew

P.S. David - do you want your maintainership back?  None of this code
has undergone any major changes since you wrote it.

Re: [XEN PATCH] evtchn/fifo: Don't set PENDING bit if guest misbehaves

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 654 bytes --]

On 16.03.22 19:38, Raphael Ning wrote:
> From: Raphael Ning <raphning@amazon.com>
> 
> Currently, evtchn_fifo_set_pending() will mark the event as PENDING even
> if it fails to lock the FIFO event queue(s), or if the guest has not
> initialized the FIFO control block for the target vCPU. A well-behaved
> guest should never trigger either of these cases.

Is this true even for the resume case e.g. after a migration?

The guests starts on the new host with no FIFO control block for any
vcpu registered, so couldn't an event get lost with your patch in case
it was sent before the target vcpu's control block gets registered?


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [XEN PATCH] evtchn/fifo: Don't set PENDING bit if guest misbehaves

[David Vrabel]

On 17/03/2022 06:28, Juergen Gross wrote:
> On 16.03.22 19:38, Raphael Ning wrote:
>> From: Raphael Ning <raphning@amazon.com>
>>
>> Currently, evtchn_fifo_set_pending() will mark the event as PENDING even
>> if it fails to lock the FIFO event queue(s), or if the guest has not
>> initialized the FIFO control block for the target vCPU. A well-behaved
>> guest should never trigger either of these cases.
> 
> Is this true even for the resume case e.g. after a migration?
> 
> The guests starts on the new host with no FIFO control block for any
> vcpu registered, so couldn't an event get lost with your patch in case
> it was sent before the target vcpu's control block gets registered?

An event that is PENDING but not LINKED is not reachable by the guest so 
it won't ever see such an event, so the event is lost whether the P bit 
is set or not.

Guests ensure that event channels are not bound to VCPUs that don't 
(yet) have FIFO control blocks.

For example, in Linux xen_irq_resume() reinitializes the control blocks 
(in xen_evtchn_resume()) before restoring any of the event channels.

David

Re: [XEN PATCH] evtchn/fifo: Don't set PENDING bit if guest misbehaves

[Raphael Ning]

On 16/03/2022 18:58, Andrew Cooper wrote:
> On 16/03/2022 18:38, Raphael Ning wrote:
>> From: Raphael Ning <raphning@amazon.com>
>>
>> Currently, evtchn_fifo_set_pending() will mark the event as PENDING even
>> if it fails to lock the FIFO event queue(s), or if the guest has not
>> initialized the FIFO control block for the target vCPU. A well-behaved
>> guest should never trigger either of these cases.
>>
>> There is no good reason to set the PENDING bit (the guest should not
>> depend on this behaviour anyway) or check for pollers in such corner
>> cases, so skip that. In fact, both the comment above the for loop and
>> the commit message for
>>
>>  41a822c39263 xen/events: rework fifo queue locking
>>
>> suggest that the bit should be set after the FIFO queue(s) are locked.
>>
>> Take the opportunity to rename the was_pending variable (flipping its
>> sense) and switch to the standard bool type.
>>
>> Suggested-by: David Vrabel <dvrabel@amazon.co.uk>
>> Signed-off-by: Raphael Ning <raphning@amazon.com>
>> ---
>>  xen/common/event_fifo.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/xen/common/event_fifo.c b/xen/common/event_fifo.c
>> index ed4d3beb10f3..6c74ccebebb7 100644
>> --- a/xen/common/event_fifo.c
>> +++ b/xen/common/event_fifo.c
>> @@ -165,7 +165,7 @@ static void cf_check evtchn_fifo_set_pending(
>>      unsigned int port;
>>      event_word_t *word;
>>      unsigned long flags;
>> -    bool_t was_pending;
>> +    bool_t check_pollers = false;
> Considering your commit message, did you intend to change this to bool?
>
> Can be fixed on commit.  Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>


My mistake, I missed that when rebasing the patch.


>
> ~Andrew
>
> P.S. David - do you want your maintainership back?  None of this code
> has undergone any major changes since you wrote it.

Re: [XEN PATCH] evtchn/fifo: Don't set PENDING bit if guest misbehaves

[Luca Fancellu]

> On 16 Mar 2022, at 18:58, Andrew Cooper <amc96@srcf.net> wrote:
> 
> On 16/03/2022 18:38, Raphael Ning wrote:
>> From: Raphael Ning <raphning@amazon.com>
>> 
>> Currently, evtchn_fifo_set_pending() will mark the event as PENDING even
>> if it fails to lock the FIFO event queue(s), or if the guest has not
>> initialized the FIFO control block for the target vCPU. A well-behaved
>> guest should never trigger either of these cases.
>> 
>> There is no good reason to set the PENDING bit (the guest should not
>> depend on this behaviour anyway) or check for pollers in such corner
>> cases, so skip that. In fact, both the comment above the for loop and
>> the commit message for
>> 
>> 41a822c39263 xen/events: rework fifo queue locking
>> 
>> suggest that the bit should be set after the FIFO queue(s) are locked.
>> 
>> Take the opportunity to rename the was_pending variable (flipping its
>> sense) and switch to the standard bool type.
>> 
>> Suggested-by: David Vrabel <dvrabel@amazon.co.uk>
>> Signed-off-by: Raphael Ning <raphning@amazon.com>
>> ---
>> xen/common/event_fifo.c | 8 ++++----
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>> 
>> diff --git a/xen/common/event_fifo.c b/xen/common/event_fifo.c
>> index ed4d3beb10f3..6c74ccebebb7 100644
>> --- a/xen/common/event_fifo.c
>> +++ b/xen/common/event_fifo.c
>> @@ -165,7 +165,7 @@ static void cf_check evtchn_fifo_set_pending(
>>     unsigned int port;
>>     event_word_t *word;
>>     unsigned long flags;
>> -    bool_t was_pending;
>> +    bool_t check_pollers = false;
> 
> Considering your commit message, did you intend to change this to bool?
> 
> Can be fixed on commit.  Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 

I’ve tested on the ARM side, I’ve started/destroyed few guests from Dom0, connect to the console, run
some network communications between guest and Dom0, everything works:

Tested-by: Luca Fancellu <luca.fancellu@arm.com>

Cheers,
Luca

> ~Andrew
> 
> P.S. David - do you want your maintainership back?  None of this code
> has undergone any major changes since you wrote it.
> 

Re: [XEN PATCH] evtchn/fifo: Don't set PENDING bit if guest misbehaves

[Raphael Ning]

On 17/03/2022 14:26, Luca Fancellu wrote:
> I’ve tested on the ARM side, I’ve started/destroyed few guests from Dom0, connect to the console, run
> some network communications between guest and Dom0, everything works:
>
> Tested-by: Luca Fancellu <luca.fancellu@arm.com>


Thanks!  I tested on x86 (in a QEMU VM) by launching and destroying an HVM guest; both dom0 and the guest use FIFO event channels.


>
> Cheers,
> Luca
>

Re: [XEN PATCH] evtchn/fifo: Don't set PENDING bit if guest misbehaves

[David Vrabel]

On 16/03/2022 18:38, Raphael Ning wrote:
> Currently, evtchn_fifo_set_pending() will mark the event as PENDING even
> if it fails to lock the FIFO event queue(s), or if the guest has not
> initialized the FIFO control block for the target vCPU. A well-behaved
> guest should never trigger either of these cases.

Reviewed-by: David Vrabel <dvrabel@amazon.co.uk>

David

Re: [XEN PATCH] evtchn/fifo: Don't set PENDING bit if guest misbehaves

[Julien Grall]

Hi Andrew,

On 16/03/2022 18:58, Andrew Cooper wrote:
>> diff --git a/xen/common/event_fifo.c b/xen/common/event_fifo.c
>> index ed4d3beb10f3..6c74ccebebb7 100644
>> --- a/xen/common/event_fifo.c
>> +++ b/xen/common/event_fifo.c
>> @@ -165,7 +165,7 @@ static void cf_check evtchn_fifo_set_pending(
>>       unsigned int port;
>>       event_word_t *word;
>>       unsigned long flags;
>> -    bool_t was_pending;
>> +    bool_t check_pollers = false;
> 
> Considering your commit message, did you intend to change this to bool?
> 
> Can be fixed on commit.  Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

So far, tools like b4 [1] are not able to find your tag on lore.kernel.org:

42sh> b4 am 
6b84a20b2753130cc62406a0fd14d2708f6f504b.1647455219.git.raphning@amazon.com

Looking up 
https://lore.kernel.org/r/6b84a20b2753130cc62406a0fd14d2708f6f504b.1647455219.git.raphning%40amazon.com
Analyzing 8 messages in the thread
Checking attestation on all messages, may take a moment...
---
   ✓ [PATCH] evtchn/fifo: Don't set PENDING bit if guest misbehaves
     + Reviewed-by: David Vrabel <dvrabel@amazon.co.uk>
     + Tested-by: Luca Fancellu <luca.fancellu@arm.com> (✓ 
DKIM/armh.onmicrosoft.com)
   ---
   ✓ Signed: DKIM/gmail.com
---
Total patches: 1
---
  Link: 
https://lore.kernel.org/r/6b84a20b2753130cc62406a0fd14d2708f6f504b.1647455219.git.raphning@amazon.com
  Base: not specified
        git am 
./20220316_raphning_evtchn_fifo_don_t_set_pending_bit_if_guest_misbehaves.mbx

This is because they are not at the start of the line. In the future, 
would you mind write the tag on a separate line?

Cheers,

[1] 
https://people.kernel.org/monsieuricon/introducing-b4-and-patch-attestation

-- 
Julien Grall