dmaengine: idxd: Change wmb() to smp_wmb() when copying completion record to user space

  wmb() is used to ensure status in the completion record is written
after the rest of the completion record, making it visible to the user.
However, on SMP systems, this may not guarantee visibility across
different CPUs.

Considering this scenario that event log handler is running on CPU1 while
user app is polling completion record (cr) status on CPU2:

	CPU1				CPU2
event log handler			user app

					1. cr = 0 (status = 0)
2. copy X to user cr except "status"
3. wmb()
4. copy Y to user cr "status"
					5. poll status value Y
				 	6. read rest cr which is still 0.
					   cr handling fails
					7. cr value X visible now

Although wmb() ensure value Y is written and visible after X is written
on CPU1, the order is not guaranteed on CPU2. So user app may see status
value Y while cr value X is still not visible yet on CPU2. This will
cause reading 0 from the rest of cr and cr handling fails.

Changing wmb() to smp_wmb() ensures Y is written after X on both CPU1
and CPU2. This guarantees that user app can consume cr in right order.

Fixes: b022f59725f0 ("dmaengine: idxd: add idxd_copy_cr() to copy user completion record during page fault handling")
Suggested-by: Nikhil Rao <nikhil.rao@intel.com>
Tested-by: Tony Zhu <tony.zhu@intel.com>
Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
---
 drivers/dma/idxd/cdev.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
  

On 1/29/24 19:58, Fenghua Yu wrote:
> wmb() is used to ensure status in the completion record is written
> after the rest of the completion record, making it visible to the user.
> However, on SMP systems, this may not guarantee visibility across
> different CPUs.
> 
> Considering this scenario that event log handler is running on CPU1 while
> user app is polling completion record (cr) status on CPU2:
> 
> 	CPU1				CPU2
> event log handler			user app
> 
> 					1. cr = 0 (status = 0)
> 2. copy X to user cr except "status"
> 3. wmb()
> 4. copy Y to user cr "status"
> 					5. poll status value Y
> 				 	6. read rest cr which is still 0.
> 					   cr handling fails
> 					7. cr value X visible now
> 
> Although wmb() ensure value Y is written and visible after X is written
> on CPU1, the order is not guaranteed on CPU2. So user app may see status
> value Y while cr value X is still not visible yet on CPU2. This will
> cause reading 0 from the rest of cr and cr handling fails.
> 
> Changing wmb() to smp_wmb() ensures Y is written after X on both CPU1
> and CPU2. This guarantees that user app can consume cr in right order.
> 
> Fixes: b022f59725f0 ("dmaengine: idxd: add idxd_copy_cr() to copy user completion record during page fault handling")
> Suggested-by: Nikhil Rao <nikhil.rao@intel.com>
> Tested-by: Tony Zhu <tony.zhu@intel.com>
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/dma/idxd/cdev.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dma/idxd/cdev.c b/drivers/dma/idxd/cdev.c
> index 77f8885cf407..9b7388a23cbe 100644
> --- a/drivers/dma/idxd/cdev.c
> +++ b/drivers/dma/idxd/cdev.c
> @@ -681,9 +681,10 @@ int idxd_copy_cr(struct idxd_wq *wq, ioasid_t pasid, unsigned long addr,
>  		 * Ensure that the completion record's status field is written
>  		 * after the rest of the completion record has been written.
>  		 * This ensures that the user receives the correct completion
> -		 * record information once polling for a non-zero status.
> +		 * record information on any CPU once polling for a non-zero
> +		 * status.
>  		 */
> -		wmb();
> +		smp_wmb();
>  		status = *(u8 *)cr;
>  		if (put_user(status, (u8 __user *)addr))
>  			left += status_size;
  

On 1/29/2024 8:58 PM, Fenghua Yu wrote:
> wmb() is used to ensure status in the completion record is written
> after the rest of the completion record, making it visible to the user.
> However, on SMP systems, this may not guarantee visibility across
> different CPUs.
> 
> Considering this scenario that event log handler is running on CPU1 while
> user app is polling completion record (cr) status on CPU2:
> 
> 	CPU1				CPU2
> event log handler			user app
> 
> 					1. cr = 0 (status = 0)
> 2. copy X to user cr except "status"
> 3. wmb()
> 4. copy Y to user cr "status"
> 					5. poll status value Y
> 				 	6. read rest cr which is still 0.
> 					   cr handling fails
> 					7. cr value X visible now
> 
> Although wmb() ensure value Y is written and visible after X is written
> on CPU1, the order is not guaranteed on CPU2. So user app may see status
> value Y while cr value X is still not visible yet on CPU2. This will
> cause reading 0 from the rest of cr and cr handling fails.
> 
> Changing wmb() to smp_wmb() ensures Y is written after X on both CPU1
> and CPU2. This guarantees that user app can consume cr in right order.
> 
> Fixes: b022f59725f0 ("dmaengine: idxd: add idxd_copy_cr() to copy user completion record during page fault handling")
> Suggested-by: Nikhil Rao <nikhil.rao@intel.com>
> Tested-by: Tony Zhu <tony.zhu@intel.com>
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>

Acked-by: Lijun Pan <lijun.pan@intel.com>

some minor comments below.

> ---
>   drivers/dma/idxd/cdev.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dma/idxd/cdev.c b/drivers/dma/idxd/cdev.c
> index 77f8885cf407..9b7388a23cbe 100644
> --- a/drivers/dma/idxd/cdev.c
> +++ b/drivers/dma/idxd/cdev.c
> @@ -681,9 +681,10 @@ int idxd_copy_cr(struct idxd_wq *wq, ioasid_t pasid, unsigned long addr,
>   		 * Ensure that the completion record's status field is written
>   		 * after the rest of the completion record has been written.
>   		 * This ensures that the user receives the correct completion
> -		 * record information once polling for a non-zero status.
> +		 * record information on any CPU once polling for a non-zero

Maybe add "smp system" to the sentence to be more explicit since people 
usually only read above comments instead of the commit message later on.
say,
"record information on any CPUs of a SMP system once polling for a 
non-zero"

> +		 * status.
>   		 */
> -		wmb();
> +		smp_wmb();
>   		status = *(u8 *)cr;
>   		if (put_user(status, (u8 __user *)addr))
>   			left += status_size;
  

This patch might be ok (it looks reasonable as an optimization), but I think
the description of wmb() and smp_wmb() is incorrect. I also think that you're
missing an rmb()/smp_rmb()eor equivalent on the reader side.

On Mon, Jan 29, 2024 at 06:58:06PM -0800, Fenghua Yu wrote:
> wmb() is used to ensure status in the completion record is written
> after the rest of the completion record, making it visible to the user.
> However, on SMP systems, this may not guarantee visibility across
> different CPUs.
> 
> Considering this scenario that event log handler is running on CPU1 while
> user app is polling completion record (cr) status on CPU2:
> 
> 	CPU1				CPU2
> event log handler			user app
> 
> 					1. cr = 0 (status = 0)
> 2. copy X to user cr except "status"
> 3. wmb()
> 4. copy Y to user cr "status"
> 					5. poll status value Y
> 				 	6. read rest cr which is still 0.
> 					   cr handling fails
> 					7. cr value X visible now
> 
> Although wmb() ensure value Y is written and visible after X is written
> on CPU1, the order is not guaranteed on CPU2. So user app may see status
> value Y while cr value X is still not visible yet on CPU2. This will
> cause reading 0 from the rest of cr and cr handling fails.

The wmb() on CPU1 ensures the order of the reads, but you need an rmb() on CPU2
between reading the 'status' and 'rest' parts; otherwise CPU2 (or the
compiler!) is permitted to hoist the read of 'rest' early, before reading from
'status', and hence you can end up with a sequence that is effectively:

	CPU1				CPU2
  event log handler			user app
					
  					1. cr = 0 (status = 0)
  				 	6a. read rest cr which is still 0.
  2. copy X to user cr except "status"
  3. wmb()
  4. copy Y to user cr "status"
  					5. poll status value Y
  					6b. cr handling fails
  					7. cr value X visible now

Since this is all to regular cacheable memory, it's *sufficient* to use
smp_wmb() and smp_rmb(), but that's an optimization rather than an ordering
fix.

Note that on x86_64, TSO means that the stores are in-order (and so smp_wmb()
is just a compiler barrier), and IIUC loads are not reordered w.r.t. other
loads (and so smp_rmb() is also just a compiler barrier).

> Changing wmb() to smp_wmb() ensures Y is written after X on both CPU1
> and CPU2. This guarantees that user app can consume cr in right order.

This implies that smp_wmb() is *stronger* than wmb(), whereas smp_wmb() is
actually *weaker* (e.g. on x86_64 wmb() is an sfence, whereas smp_wmb() is a
barrier()).

Thanks,
Mark.

> 
> Fixes: b022f59725f0 ("dmaengine: idxd: add idxd_copy_cr() to copy user completion record during page fault handling")
> Suggested-by: Nikhil Rao <nikhil.rao@intel.com>
> Tested-by: Tony Zhu <tony.zhu@intel.com>
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> ---
>  drivers/dma/idxd/cdev.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dma/idxd/cdev.c b/drivers/dma/idxd/cdev.c
> index 77f8885cf407..9b7388a23cbe 100644
> --- a/drivers/dma/idxd/cdev.c
> +++ b/drivers/dma/idxd/cdev.c
> @@ -681,9 +681,10 @@ int idxd_copy_cr(struct idxd_wq *wq, ioasid_t pasid, unsigned long addr,
>  		 * Ensure that the completion record's status field is written
>  		 * after the rest of the completion record has been written.
>  		 * This ensures that the user receives the correct completion
> -		 * record information once polling for a non-zero status.
> +		 * record information on any CPU once polling for a non-zero
> +		 * status.
>  		 */
> -		wmb();
> +		smp_wmb();
>  		status = *(u8 *)cr;
>  		if (put_user(status, (u8 __user *)addr))
>  			left += status_size;
> -- 
> 2.37.1
> 
>
  

On Tue, Jan 30, 2024 at 05:58:24PM +0000, Mark Rutland wrote:
> This patch might be ok (it looks reasonable as an optimization), but I think
> the description of wmb() and smp_wmb() is incorrect. I also think that you're
> missing an rmb()/smp_rmb()eor equivalent on the reader side.

Sorry, the above should have said:
	
	an rmb()/smp_rmb() *or* equivalent

> 
> On Mon, Jan 29, 2024 at 06:58:06PM -0800, Fenghua Yu wrote:
> > wmb() is used to ensure status in the completion record is written
> > after the rest of the completion record, making it visible to the user.
> > However, on SMP systems, this may not guarantee visibility across
> > different CPUs.
> > 
> > Considering this scenario that event log handler is running on CPU1 while
> > user app is polling completion record (cr) status on CPU2:
> > 
> > 	CPU1				CPU2
> > event log handler			user app
> > 
> > 					1. cr = 0 (status = 0)
> > 2. copy X to user cr except "status"
> > 3. wmb()
> > 4. copy Y to user cr "status"
> > 					5. poll status value Y
> > 				 	6. read rest cr which is still 0.
> > 					   cr handling fails
> > 					7. cr value X visible now
> > 
> > Although wmb() ensure value Y is written and visible after X is written
> > on CPU1, the order is not guaranteed on CPU2. So user app may see status
> > value Y while cr value X is still not visible yet on CPU2. This will
> > cause reading 0 from the rest of cr and cr handling fails.
> 
> The wmb() on CPU1 ensures the order of the reads, but you need an rmb() on CPU2

Sorry again, the above should have said:

	The wmb() on CPU1 ensures the order of the *writes*

Apologies for any confusion resulting from those mistakes.

Mark.

> between reading the 'status' and 'rest' parts; otherwise CPU2 (or the
> compiler!) is permitted to hoist the read of 'rest' early, before reading from
> 'status', and hence you can end up with a sequence that is effectively:
> 
> 	CPU1				CPU2
>   event log handler			user app
> 					
>   					1. cr = 0 (status = 0)
>   				 	6a. read rest cr which is still 0.
>   2. copy X to user cr except "status"
>   3. wmb()
>   4. copy Y to user cr "status"
>   					5. poll status value Y
>   					6b. cr handling fails
>   					7. cr value X visible now
> 
> Since this is all to regular cacheable memory, it's *sufficient* to use
> smp_wmb() and smp_rmb(), but that's an optimization rather than an ordering
> fix.
> 
> Note that on x86_64, TSO means that the stores are in-order (and so smp_wmb()
> is just a compiler barrier), and IIUC loads are not reordered w.r.t. other
> loads (and so smp_rmb() is also just a compiler barrier).
> 
> > Changing wmb() to smp_wmb() ensures Y is written after X on both CPU1
> > and CPU2. This guarantees that user app can consume cr in right order.
> 
> This implies that smp_wmb() is *stronger* than wmb(), whereas smp_wmb() is
> actually *weaker* (e.g. on x86_64 wmb() is an sfence, whereas smp_wmb() is a
> barrier()).
> 
> Thanks,
> Mark.
> 
> > 
> > Fixes: b022f59725f0 ("dmaengine: idxd: add idxd_copy_cr() to copy user completion record during page fault handling")
> > Suggested-by: Nikhil Rao <nikhil.rao@intel.com>
> > Tested-by: Tony Zhu <tony.zhu@intel.com>
> > Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> > ---
> >  drivers/dma/idxd/cdev.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/dma/idxd/cdev.c b/drivers/dma/idxd/cdev.c
> > index 77f8885cf407..9b7388a23cbe 100644
> > --- a/drivers/dma/idxd/cdev.c
> > +++ b/drivers/dma/idxd/cdev.c
> > @@ -681,9 +681,10 @@ int idxd_copy_cr(struct idxd_wq *wq, ioasid_t pasid, unsigned long addr,
> >  		 * Ensure that the completion record's status field is written
> >  		 * after the rest of the completion record has been written.
> >  		 * This ensures that the user receives the correct completion
> > -		 * record information once polling for a non-zero status.
> > +		 * record information on any CPU once polling for a non-zero
> > +		 * status.
> >  		 */
> > -		wmb();
> > +		smp_wmb();
> >  		status = *(u8 *)cr;
> >  		if (put_user(status, (u8 __user *)addr))
> >  			left += status_size;
> > -- 
> > 2.37.1
> > 
> > 
>
  

On Tue, Jan 30, 2024 at 05:58:24PM +0000, Mark Rutland wrote:
> This patch might be ok (it looks reasonable as an optimization), but I think
> the description of wmb() and smp_wmb() is incorrect. I also think that you're

Agreed. A wmb() -> smp_wmb() change can only be an optimization rather
than a fix.

> missing an rmb()/smp_rmb()eor equivalent on the reader side.
> 
> On Mon, Jan 29, 2024 at 06:58:06PM -0800, Fenghua Yu wrote:
> > wmb() is used to ensure status in the completion record is written
> > after the rest of the completion record, making it visible to the user.
> > However, on SMP systems, this may not guarantee visibility across
> > different CPUs.
> > 
> > Considering this scenario that event log handler is running on CPU1 while
> > user app is polling completion record (cr) status on CPU2:
> > 
> > 	CPU1				CPU2
> > event log handler			user app
> > 
> > 					1. cr = 0 (status = 0)
> > 2. copy X to user cr except "status"
> > 3. wmb()
> > 4. copy Y to user cr "status"
> > 					5. poll status value Y
> > 				 	6. read rest cr which is still 0.
> > 					   cr handling fails
> > 					7. cr value X visible now
> > 
> > Although wmb() ensure value Y is written and visible after X is written
> > on CPU1, the order is not guaranteed on CPU2. So user app may see status
> > value Y while cr value X is still not visible yet on CPU2. This will
> > cause reading 0 from the rest of cr and cr handling fails.
> 
> The wmb() on CPU1 ensures the order of the reads, but you need an rmb() on CPU2
> between reading the 'status' and 'rest' parts; otherwise CPU2 (or the
> compiler!) is permitted to hoist the read of 'rest' early, before reading from
> 'status', and hence you can end up with a sequence that is effectively:
> 
> 	CPU1				CPU2
>   event log handler			user app
> 					
>   					1. cr = 0 (status = 0)
>   				 	6a. read rest cr which is still 0.
>   2. copy X to user cr except "status"
>   3. wmb()
>   4. copy Y to user cr "status"
>   					5. poll status value Y
>   					6b. cr handling fails
>   					7. cr value X visible now
> 
> Since this is all to regular cacheable memory, it's *sufficient* to use
> smp_wmb() and smp_rmb(), but that's an optimization rather than an ordering
> fix.
> 
> Note that on x86_64, TSO means that the stores are in-order (and so smp_wmb()
> is just a compiler barrier), and IIUC loads are not reordered w.r.t. other
> loads (and so smp_rmb() is also just a compiler barrier).
> 
> > Changing wmb() to smp_wmb() ensures Y is written after X on both CPU1
> > and CPU2. This guarantees that user app can consume cr in right order.

A barrier can only provide ordering for memory accesses on the same CPU,
so this doesn't make any sense.

> 
> This implies that smp_wmb() is *stronger* than wmb(), whereas smp_wmb() is
> actually *weaker* (e.g. on x86_64 wmb() is an sfence, whereas smp_wmb() is a
> barrier()).
> 
> Thanks,
> Mark.
> 
> > 
> > Fixes: b022f59725f0 ("dmaengine: idxd: add idxd_copy_cr() to copy user completion record during page fault handling")
> > Suggested-by: Nikhil Rao <nikhil.rao@intel.com>
> > Tested-by: Tony Zhu <tony.zhu@intel.com>

Since it has a "Fixes" tag and a "Tested-by" tag, I'd assume there has
been a test w/ and w/o this patch showing it can resolve a real issue
*constantly*? If so, I think x86 might be broken somewhere.

[Cc x86 maintainers]

Regards,
Boqun

> > Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> > ---
> >  drivers/dma/idxd/cdev.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/dma/idxd/cdev.c b/drivers/dma/idxd/cdev.c
> > index 77f8885cf407..9b7388a23cbe 100644
> > --- a/drivers/dma/idxd/cdev.c
> > +++ b/drivers/dma/idxd/cdev.c
> > @@ -681,9 +681,10 @@ int idxd_copy_cr(struct idxd_wq *wq, ioasid_t pasid, unsigned long addr,
> >  		 * Ensure that the completion record's status field is written
> >  		 * after the rest of the completion record has been written.
> >  		 * This ensures that the user receives the correct completion
> > -		 * record information once polling for a non-zero status.
> > +		 * record information on any CPU once polling for a non-zero
> > +		 * status.
> >  		 */
> > -		wmb();
> > +		smp_wmb();
> >  		status = *(u8 *)cr;
> >  		if (put_user(status, (u8 __user *)addr))
> >  			left += status_size;
> > -- 
> > 2.37.1
> > 
> >
  

On 1/30/24 11:53, Boqun Feng wrote:
>>> Fixes: b022f59725f0 ("dmaengine: idxd: add idxd_copy_cr() to copy user completion record during page fault handling")
>>> Suggested-by: Nikhil Rao <nikhil.rao@intel.com>
>>> Tested-by: Tony Zhu <tony.zhu@intel.com>
> Since it has a "Fixes" tag and a "Tested-by" tag, I'd assume there has
> been a test w/ and w/o this patch showing it can resolve a real issue
> *constantly*? If so, I think x86 might be broken somewhere.
> 
> [Cc x86 maintainers]

Fenghua, could you perhaps explain how this problem affects end users?
What symptom was observed that made it obvious something was broken and
what changes with this patch?
  

Hi, Dave, Boqun, and Mark,

On 1/30/24 12:30, Dave Hansen wrote:
> On 1/30/24 11:53, Boqun Feng wrote:
>>>> Fixes: b022f59725f0 ("dmaengine: idxd: add idxd_copy_cr() to copy user completion record during page fault handling")
>>>> Suggested-by: Nikhil Rao <nikhil.rao@intel.com>
>>>> Tested-by: Tony Zhu <tony.zhu@intel.com>
>> Since it has a "Fixes" tag and a "Tested-by" tag, I'd assume there has
>> been a test w/ and w/o this patch showing it can resolve a real issue
>> *constantly*? If so, I think x86 might be broken somewhere.
>>
>> [Cc x86 maintainers]
> 
> Fenghua, could you perhaps explain how this problem affects end users?
> What symptom was observed that made it obvious something was broken and
> what changes with this patch?

There is no issue found by any test. This wmb() code was reviewed and 
was "thought" that it may have a potential issue. The patch was tested 
without breaking any existing tests.

 From the discussions with Boqun and Mark, this patch might just be an 
optimization rather than a fix. Let me think about it further and may 
update commit message in v2 or withdraw this patch since it won't really 
fix an issue.

Thank you very much for review!

-Fenghua
  

> -----Original Message-----
> From: Yu, Fenghua <fenghua.yu@intel.com>
> Sent: Wednesday, January 31, 2024 2:12 AM
> To: Hansen, Dave <dave.hansen@intel.com>; Boqun Feng
> <boqun.feng@gmail.com>; Mark Rutland <mark.rutland@arm.com>
> Cc: Vinod Koul <vkoul@kernel.org>; Jiang, Dave <dave.jiang@intel.com>;
> dmaengine@vger.kernel.org; linux-kernel <linux-kernel@vger.kernel.org>; Rao,
> Nikhil <nikhil.rao@intel.com>; Zhu, Tony <tony.zhu@intel.com>; Mathieu
> Desnoyers <mathieu.desnoyers@efficios.com>; Thomas Gleixner
> <tglx@linutronix.de>; Ingo Molnar <mingo@redhat.com>; Borislav Petkov
> <bp@alien8.de>; Dave Hansen <dave.hansen@linux.intel.com>;
> x86@kernel.org
> Subject: Re: [PATCH] dmaengine: idxd: Change wmb() to smp_wmb() when
> copying completion record to user space
> 
> Hi, Dave, Boqun, and Mark,
> 
> On 1/30/24 12:30, Dave Hansen wrote:
> > On 1/30/24 11:53, Boqun Feng wrote:
> >>>> Fixes: b022f59725f0 ("dmaengine: idxd: add idxd_copy_cr() to copy
> >>>> user completion record during page fault handling")
> >>>> Suggested-by: Nikhil Rao <nikhil.rao@intel.com>
> >>>> Tested-by: Tony Zhu <tony.zhu@intel.com>
> >> Since it has a "Fixes" tag and a "Tested-by" tag, I'd assume there
> >> has been a test w/ and w/o this patch showing it can resolve a real
> >> issue *constantly*? If so, I think x86 might be broken somewhere.
> >>
> >> [Cc x86 maintainers]
> >
> > Fenghua, could you perhaps explain how this problem affects end users?
> > What symptom was observed that made it obvious something was broken
> > and what changes with this patch?
> 
> There is no issue found by any test. This wmb() code was reviewed and was
> "thought" that it may have a potential issue. 

I had made this suggestion since the code only needed a smp_wmb(), If the review refers to my suggestion, sorry if my message indicated a potential issue, that certainly wasn't my intention.
memory-barriers.txt does say that mandatory barriers (of which wmb() is one) should not be used to control SMP effects.

Nikhil