linux-kernel - Re: [PATCH v2] iommufd/device: Enforce reserved IOVA also when attached to hwpt

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] [day] [month] [year] [list]

Message-ID: <ZrJ7QwO+HJ6rxL6Y@Asurada-Nvidia>
Date: Tue, 6 Aug 2024 12:36:35 -0700
From: Nicolin Chen <nicolinc@...dia.com>
To: Jason Gunthorpe <jgg@...dia.com>
CC: <kevin.tian@...el.com>, <yi.l.liu@...el.com>, <iommu@...ts.linux.dev>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] iommufd/device: Enforce reserved IOVA also when
 attached to hwpt_nested

On Tue, Aug 06, 2024 at 02:42:42PM -0300, Jason Gunthorpe wrote:
> > -static int iommufd_hwpt_paging_attach(struct iommufd_hwpt_paging *hwpt_paging,
> > -				      struct iommufd_device *idev)
> > +static int
> > +iommufd_device_attach_reserved_iova(struct iommufd_device *idev,
> > +				    struct iommufd_hw_pagetable *hwpt)
> >  {
> > +	struct iommufd_hwpt_paging *hwpt_paging = to_hwpt_paging(hwpt);
> >  	int rc;
> 
> This seems like the wrong place to put these types, a big point point of the
> struct iommufd_hwpt_paging was to mark functions that should only be
> operating on a paging hwpt.
> 
> The caller is expected to figure out what it is doing.
> 
> >  int iommufd_hw_pagetable_attach(struct iommufd_hw_pagetable *hwpt,
> >  				struct iommufd_device *idev)
> >  {
> > @@ -363,11 +380,9 @@ int iommufd_hw_pagetable_attach(struct iommufd_hw_pagetable *hwpt,
> >  		goto err_unlock;
> >  	}
> >  
> > -	if (hwpt_is_paging(hwpt)) {
> > -		rc = iommufd_hwpt_paging_attach(to_hwpt_paging(hwpt), idev);
> > -		if (rc)
> > -			goto err_unlock;
> > -	}
> 
> Like we had here, so it would be a bit nicer to write it as more like:
> 
> hwpt_paging = to_hwpt_paging(hwpt);
> if (hwpt_paging) {
> 	rc = iommufd_hwpt_paging_attach(hwpt_paging, idev);
> }
> 
> Then we can keep the clearer labeling of the function signatures.

Okay. Will keep the hwpt_paging type in the function signatures.

> > @@ -321,7 +321,20 @@ static inline bool hwpt_is_paging(struct iommufd_hw_pagetable *hwpt)
> >  static inline struct iommufd_hwpt_paging *
> >  to_hwpt_paging(struct iommufd_hw_pagetable *hwpt)
> >  {
> > -	return container_of(hwpt, struct iommufd_hwpt_paging, common);
> > +	switch (hwpt->obj.type) {
> > +	case IOMMUFD_OBJ_HWPT_PAGING:
> > +		return container_of(hwpt, struct iommufd_hwpt_paging, common);
> > +	case IOMMUFD_OBJ_HWPT_NESTED:
> > +		return container_of(hwpt, struct iommufd_hwpt_nested, common)->parent;
> > +	default:
> > +		return NULL;
> > +	}
> > +}
> 
> There are alot of existing callers of this, I think it should get a
> new function to do this behavior and it would only be used in a few
> places.
> 
> It is not OK for a NESTING to get into most of the places that are
> already calling this.

OK. Let's add a find_hwpt_paging(struct iommufd_hw_pagetable *);

Thanks
Nicolin