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: <Z4dKR4ORF/FcfCSD@Asurada-Nvidia>
Date: Tue, 14 Jan 2025 21:40:23 -0800
From: Nicolin Chen <nicolinc@...dia.com>
To: "Tian, Kevin" <kevin.tian@...el.com>
CC: "jgg@...dia.com" <jgg@...dia.com>, "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

On Wed, Jan 15, 2025 at 05:24:44AM +0000, Tian, Kevin wrote:
> > 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?

Ack.

> > 
> > +/* 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()

We have deliver and response two lists. So I think that "deliver"
would be necessary. Yet, I can do "fetch" v.s. "extract".

> > @@ -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)) {
> 		...
> 	}

Ah, right...how didn't I see this lol.

> 
> > @@ -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.

Ack.

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

Thanks!
Nicolin