linux-kernel - RE: [PATCH rc 2/2] iommufd/fault: Use a separate spinlock to protect fault->deliver list

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <BN9PR11MB52769D7C88B3DBBB4DF3A0028C192@BN9PR11MB5276.namprd11.prod.outlook.com>
Date: Wed, 15 Jan 2025 05:24:44 +0000
From: "Tian, Kevin" <kevin.tian@...el.com>
To: Nicolin Chen <nicolinc@...dia.com>, "jgg@...dia.com" <jgg@...dia.com>
CC: "baolu.lu@...ux.intel.com" <baolu.lu@...ux.intel.com>,
	"iommu@...ts.linux.dev" <iommu@...ts.linux.dev>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"joro@...tes.org" <joro@...tes.org>, "will@...nel.org" <will@...nel.org>,
	"robin.murphy@....com" <robin.murphy@....com>
Subject: RE: [PATCH rc 2/2] iommufd/fault: Use a separate spinlock to protect
 fault->deliver list

> From: Nicolin Chen <nicolinc@...dia.com>
> Sent: Wednesday, January 15, 2025 7:29 AM
> 
> @@ -445,12 +445,38 @@ struct iommufd_fault {
> 
>  	/* The lists of outstanding faults protected by below mutex. */
>  	struct mutex mutex;
> +	spinlock_t lock; /* protects the deliver list */
>  	struct list_head deliver;
>  	struct xarray response;

Move 'mutex' together with response then?

> 
> +/* Extract the first node out of the fault->deliver list */
> +static inline struct iopf_group *
> +iommufd_fault_deliver_extract(struct iommufd_fault *fault)

Probably simpler be iommufd_fault_fetch()

> @@ -102,17 +102,19 @@ static void iommufd_auto_response_faults(struct
> iommufd_hw_pagetable *hwpt,
>  					 struct iommufd_attach_handle
> *handle)
>  {
>  	struct iommufd_fault *fault = hwpt->fault;
> -	struct iopf_group *group, *next;
> +	struct iopf_group *group;
>  	unsigned long index;
> 
>  	if (!fault)
>  		return;
> 
>  	mutex_lock(&fault->mutex);
> -	list_for_each_entry_safe(group, next, &fault->deliver, node) {
> -		if (group->attach_handle != &handle->handle)
> +	for (group = iommufd_fault_deliver_extract(fault); group;
> +	     group = iommufd_fault_deliver_extract(fault)) {

	while (group = iommufd_fault_fetch(fault)) {
		...
	}

> @@ -266,17 +268,20 @@ static ssize_t iommufd_fault_fops_read(struct file
> *filep, char __user *buf,
>  		return -ESPIPE;
> 
>  	mutex_lock(&fault->mutex);
> -	while (!list_empty(&fault->deliver) && count > done) {
> -		group = list_first_entry(&fault->deliver,
> -					 struct iopf_group, node);
> -
> -		if (group->fault_count * fault_size > count - done)
> +	for (group = iommufd_fault_deliver_extract(fault); group;
> +	     group = iommufd_fault_deliver_extract(fault)) {
> +		if (done >= count ||
> +		    group->fault_count * fault_size > count - done) {
> +			iommufd_fault_deliver_restore(fault, group);
>  			break;
> +		}
> 
>  		rc = xa_alloc(&fault->response, &group->cookie, group,
>  			      xa_limit_32b, GFP_KERNEL);
> -		if (rc)
> +		if (rc) {
> +			iommufd_fault_deliver_restore(fault, group);
>  			break;
> +		}

The scope of mutex can be reduced to just protect the smaller trunk
touching fault->response.

Otherwise looks good:

Reviewed-by: Kevin Tian <kevin.tian@...el.com>