Received-SPF: pass (google.com: domain of linux-kernel+bounces-44851-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <0de32f03-5f6e-4bb2-aae1-93fd9d9ae49c@intel.com>
Date: Tue, 30 Jan 2024 08:35:22 -0700
User-Agent: Betterbird (Linux)
Subject: Re: [PATCH] dmaengine: idxd: Change wmb() to smp_wmb() when copying
 completion record to user space
To: Fenghua Yu <fenghua.yu@intel.com>, Vinod Koul <vkoul@kernel.org>
CC: <dmaengine@vger.kernel.org>, linux-kernel <linux-kernel@vger.kernel.org>,
	Nikhil Rao <nikhil.rao@intel.com>, Tony Zhu <tony.zhu@intel.com>
References: <20240130025806.2027284-1-fenghua.yu@intel.com>
Content-Language: en-US
From: Dave Jiang <dave.jiang@intel.com>
In-Reply-To: <20240130025806.2027284-1-fenghua.yu@intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?TGptMGtYcDNUdzdIVUJmdEplcUUxRGlDNE1vWFNNWmg3SmpwWWNNNmt4Rk9E?=
 =?utf-8?B?OE10a3ZwWk5XWmV4Y1VnWVR4ZWE2cHYrQ0JYczhkWHhjVXFNazlDaXdCbU90?=
 =?utf-8?B?RXZySDdVSHN3OTE2UElyMmI3RVJrVkJTVlNGUDVGanZMRHNEUERsZ3JLakFS?=
 =?utf-8?B?OFdPQ3hmQWE5S1BJRHNBTEtPQ0s3TGFrQkkvZHc4alk2Y2YvN1lvSnRGZ1hk?=
 =?utf-8?B?QnBsWjkxTjRvTlk4Yk1ocjRRTWt1bVF3WnJyOFZhMzZMQVNxUVNGaHFGdGc5?=
 =?utf-8?B?bU1WSzVHVC9Hc3FNM0JHVHlDbkErSEJEOEZqYUJPYzF5ZExKUlV3V2FaRUs0?=
 =?utf-8?B?d0NISGovdHFQT0RjaGVFUVhCMHVXV0U3M2xkVTlZMTVXUEVLaWREZlpFc2k5?=
 =?utf-8?B?a042Q0RiczVmWTcyUXRhSDlodzJSbHh4czdkSXkyeEdVaVlsVXFnVmhINkh1?=
 =?utf-8?B?YkptenFvalBUSWxoK3p2SEhxUzJ6ZHhjRzJvbmFZcGlaUmhkd2JQWkJXM1hs?=
 =?utf-8?B?Ky9ROUk4T3JYV0Fsd09lV1B0enE4RUIwSXVUZWtaSXdiQktrK2FYWGprVXBT?=
 =?utf-8?B?dEo1M3J3cjM2ZUMxbGNrUTFPRHRURlU1YWY2QndUNlhlei9YVnppMUlZWVMv?=
 =?utf-8?B?d0ZPaERnNU43M3FnWTlVZmQvMGVma2p3RUtXejFoRkJjQUw3WW43aHJPNVNS?=
 =?utf-8?B?c1RKR1EvZGxoMlY0ejZyOW5adSs1dWlrbmdYZlhHejl4VjNwQk5DVW9NUWhQ?=
 =?utf-8?B?aXpMcXQxYXErMENXbjNlaDdvU0RNNnVLeU5MQ3hnS3V6dG45NGd5YVZkQmVS?=
 =?utf-8?B?UitYd0Y5eGk1bUZacnhubzVYRE5WZFZLVEVSVU5zWkRPbUhweG5LenNRMC82?=
 =?utf-8?B?SXkzN1kvZHkwYVByYmJYdS9TS0g5aWk2UUFXRnpsUXBRVkxNTEtCaU42M0VN?=
 =?utf-8?B?bC9XdGRwMUYxRUx6V094azJNKzZ6dy9NTGhXRE0vSndKVEhHQ3dIYnNHZ3dI?=
 =?utf-8?B?TWZuOHVVQWpHSUlVenpkRjdiS09kMlRVV1VWTlNQUEpSUThITkFWV3hxbGM5?=
 =?utf-8?B?RUI2RWdzdC9ZZFVHRnFUNjArejl1RXNmYUtiNWkyVGxSSW5aWEpPOGNaS0F2?=
 =?utf-8?B?ZE5WWTBpOUhIanBkOG8zTmNSZFZjVDVRQ2szelRBREVIM0RQOGZvbVBRS0Zt?=
 =?utf-8?B?SmxvbDQzZ1hmazdEZDl1NEo4NjV5Q2ppQlNBdDZXK2JUYW9iL0xVLzQvNnhk?=
 =?utf-8?B?cTB6L2NlTjFxcU9yemU0OU1tOE5uM3ZoYjJycjRWRXhEdzJtNW55T0x3K2Zo?=
 =?utf-8?B?MndvNTdVMkFWZDNYVEpramVVNHZ3R1RZRU1jZ2owZUxwU2Irb3hHNDFIYUxD?=
 =?utf-8?B?d3BrVEZRSk9nUFNjNm1zN2UxMGl5Yit4c2JCUmZDVjFlb2s3Y3RFT2Rzakl4?=
 =?utf-8?B?RE5tQ0Z4WGNHT20vWTQ5V0pXZzd4M3RtazB5MWZGU0RJT0NyOU5UNVc5RFhB?=
 =?utf-8?B?QnJwemtML3V4WndhOTk4cmMybTBLZjJxZnpXdUkzbEhXRkFPWFdjMmdIMkZ1?=
 =?utf-8?B?OC9SS2t3VnlhYWYxQyt2VXczNjIxUEhDMWVMeGhjZlZncjlSVE9HaEdydDdH?=
 =?utf-8?B?cmsrZHhMWFYyWnlZU0kvOGZqa09KVy8wcDV2L3h4OFB5OUl4OHR5SUxvRkFW?=
 =?utf-8?B?TWRwczc3Um9MYjJ3SUJidTh0RnZKL3gwSnIzU1Fmc0Erb2o1RjEybDJBaE9M?=
 =?utf-8?B?TW5ST2xJSUtYb2FTWGFyTnlsNUl5VytaMEhFeDZESDQra29aQlBlZUIrZFhK?=
 =?utf-8?B?QnZ3QlN1c05wYXovQVFNZ0JjR3cyTzhPWmJrUndHNFNERndxV1VrYmlPWllD?=
 =?utf-8?B?WndwOG0zamhQOU9tVUhoczdJaFltM1AvUmtXU1U5ajJrandVUzRHSUR1ZDRh?=
 =?utf-8?B?VEFyUkU0UGJXc0VDK3AxTXVsTXBkQVJOV2NzWGhCdWhKRWd1V0dNY0xoM2NT?=
 =?utf-8?B?ZWh3MytMdDFzYVZNSGVsV0ozYm5IVGNJc25FUVJSNEY2UVg4SkcxbDFhVHRw?=
 =?utf-8?B?dzQxbFdQbVNMME54UEdKQTgxNmpYSGVFTGtQeXZqOU54TG11VUovWThFSFZS?=
 =?utf-8?Q?qRN1SrFtTE1F3m1TWPCzJlafw?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 055d1391-1f19-4db1-dec6-08dc21a91323
X-MS-Exchange-CrossTenant-AuthSource: PH7PR11MB5984.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 Jan 2024 15:35:23.6839
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: joTFEGNGTavPJBDjl1E0y25SpPTyzJUrfOXO45BVOr7gpFa0bNsljq47TUliyiJJb0TzzBd/Drkt7dT8TvXFFA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB7451
X-OriginatorOrg: intel.com


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;