linux-kernel - RE: [PATCH v2 13/14] iommufd: Introduce iommufd_object_alloc

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: <BN9PR11MB52769CFD429C3E8E0918674A8C48A@BN9PR11MB5276.namprd11.prod.outlook.com>
Date: Thu, 10 Jul 2025 05:32:13 +0000
From: "Tian, Kevin" <kevin.tian@...el.com>
To: Xu Yilun <yilun.xu@...ux.intel.com>, Nicolin Chen <nicolinc@...dia.com>
CC: "jgg@...dia.com" <jgg@...dia.com>, "will@...nel.org" <will@...nel.org>,
	"robin.murphy@....com" <robin.murphy@....com>, "joro@...tes.org"
	<joro@...tes.org>, "praan@...gle.com" <praan@...gle.com>, "Liu, Yi L"
	<yi.l.liu@...el.com>, "peterz@...radead.org" <peterz@...radead.org>,
	"jsnitsel@...hat.com" <jsnitsel@...hat.com>,
	"linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "iommu@...ts.linux.dev"
	<iommu@...ts.linux.dev>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "patches@...ts.linux.dev"
	<patches@...ts.linux.dev>, "baolu.lu@...ux.intel.com"
	<baolu.lu@...ux.intel.com>
Subject: RE: [PATCH v2 13/14] iommufd: Introduce iommufd_object_alloc_ucmd
 helper

> From: Xu Yilun <yilun.xu@...ux.intel.com>
> Sent: Wednesday, July 9, 2025 1:32 PM
> 
> > @@ -61,6 +61,24 @@ struct iommufd_object
> *_iommufd_object_alloc(struct iommufd_ctx *ictx,
> >  	return ERR_PTR(rc);
> >  }
> >
> > +struct iommufd_object *_iommufd_object_alloc_ucmd(struct
> iommufd_ucmd *ucmd,
> > +						  size_t size,
> > +						  enum iommufd_object_type
> type)
> > +{
> > +	struct iommufd_object *new_obj;
> > +
> > +	/* Something is coded wrong if this is hit */
> > +	if (WARN_ON(ucmd->new_obj))
> > +		return ERR_PTR(-EBUSY);
> > +
> > +	new_obj = _iommufd_object_alloc(ucmd->ictx, size, type);
> > +	if (IS_ERR(new_obj))
> > +		return new_obj;
> > +
> > +	ucmd->new_obj = new_obj;
> > +	return new_obj;
> > +}
> > +
> >  /*
> >   * Allow concurrent access to the object.
> >   *
> > @@ -448,6 +466,13 @@ static long iommufd_fops_ioctl(struct file *filp,
> unsigned int cmd,
> >  	if (ret)
> >  		return ret;
> >  	ret = op->execute(&ucmd);
> > +
> > +	if (ucmd.new_obj) {
> > +		if (ret)
> > +			iommufd_object_abort_and_destroy(ictx,
> ucmd.new_obj);
> 
> Sorry I didn't follow this thread before and maybe missed something.
> 
> According to 70eadc7fc7ef, abort op is for the object that can assume
> the caller is holding the lock. But here is for no locking, so calling
> iommufd_object_abort_and_destroy() is quite confusing.
> 
> Is it better we change to:
> 
> 	if (ret) {
> 		iommufd_object_ops[obj->type].destroy(obj);
> 		iommufd_object_abort(ictx, obj);
> 	}

I'd keep the original way. The function name describes what to do,
not what to be called exactly inside. Lacking of the abort method
doesn't change the meaning of the function which is about abort
and destroy (just like how it's called before introducing @abort).

> 
> Also explicitely assert iommufd_object_alloc_ucmd() and abort can't be
> used at the same time.
> 
> in _iommufd_object_alloc_ucmd():
> 
> 	if (WARN_ON(iommufd_object_ops[type].abort))
> 		return ERR_PTR(-EFAULT);
> 

but this check sounds necessary.