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

2022-03-21 Thread 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 


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

42sh> b4 am 
6b84a20b2753130cc62406a0fd14d2708f6f504b.1647455219.git.raphn...@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 
+ Tested-by: Luca Fancellu  (✓ 
DKIM/armh.onmicrosoft.com)

  ---
  ✓ Signed: DKIM/gmail.com
---
Total patches: 1
---
 Link: 
https://lore.kernel.org/r/6b84a20b2753130cc62406a0fd14d2708f6f504b.1647455219.git.raphn...@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



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

2022-03-21 Thread 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 

David



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

2022-03-17 Thread 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 


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

2022-03-17 Thread Luca Fancellu


> On 16 Mar 2022, at 18:58, Andrew Cooper  wrote:
> 
> On 16/03/2022 18:38, Raphael Ning wrote:
>> From: Raphael Ning 
>> 
>> 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 
>> Signed-off-by: Raphael Ning 
>> ---
>> 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 
> 

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 

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

2022-03-17 Thread Raphael Ning


On 16/03/2022 18:58, Andrew Cooper wrote:
> On 16/03/2022 18:38, Raphael Ning wrote:
>> From: Raphael Ning 
>>
>> 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 
>> Signed-off-by: Raphael Ning 
>> ---
>>  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 


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

2022-03-17 Thread David Vrabel




On 17/03/2022 06:28, Juergen Gross wrote:

On 16.03.22 19:38, Raphael Ning wrote:

From: Raphael Ning 

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

2022-03-17 Thread Juergen Gross

On 16.03.22 19:38, Raphael Ning wrote:

From: Raphael Ning 

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


OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature


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

2022-03-16 Thread Andrew Cooper
On 16/03/2022 18:38, Raphael Ning wrote:
> From: Raphael Ning 
>
> 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 
> Signed-off-by: Raphael Ning 
> ---
>  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

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