linux-kernel - Re: [PATCH v4 05/17] iommufd: Separate kernel-managed HWPT alloc/destroy/abort functions

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: <20231012190931.GO3952@nvidia.com>
Date:   Thu, 12 Oct 2023 16:09:31 -0300
From:   Jason Gunthorpe <jgg@...dia.com>
To:     Yi Liu <yi.l.liu@...el.com>
Cc:     joro@...tes.org, alex.williamson@...hat.com, kevin.tian@...el.com,
        robin.murphy@....com, baolu.lu@...ux.intel.com, cohuck@...hat.com,
        eric.auger@...hat.com, nicolinc@...dia.com, kvm@...r.kernel.org,
        mjrosato@...ux.ibm.com, chao.p.peng@...ux.intel.com,
        yi.y.sun@...ux.intel.com, peterx@...hat.com, jasowang@...hat.com,
        shameerali.kolothum.thodi@...wei.com, lulu@...hat.com,
        suravee.suthikulpanit@....com, iommu@...ts.linux.dev,
        linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
        zhenzhong.duan@...el.com, joao.m.martins@...cle.com
Subject: Re: [PATCH v4 05/17] iommufd: Separate kernel-managed HWPT
 alloc/destroy/abort functions

On Tue, Oct 10, 2023 at 03:49:32PM -0300, Jason Gunthorpe wrote:
> > diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
> > index 1d3b1a74e854..3e89c3d530f3 100644
> > --- a/drivers/iommu/iommufd/iommufd_private.h
> > +++ b/drivers/iommu/iommufd/iommufd_private.h
> > @@ -234,6 +234,9 @@ struct iommufd_hw_pagetable {
> >  	struct iommufd_object obj;
> >  	struct iommu_domain *domain;
> >  
> > +	void (*abort)(struct iommufd_object *obj);
> > +	void (*destroy)(struct iommufd_object *obj);
> > +
> >  	union {
> >  		struct { /* kernel-managed */
> >  			struct iommufd_ioas *ioas;
> 
> I think if you are doing this you are trying too hard to share the
> struct.. Defaintely want to avoid function pointers in general, and
> function pointers in a writable struct in particular.

I looked at this for a while and I do still have the feeling that
probably two structs and even two type IDs is probably a cleaner
design.

Like this:

// Or maybe use obj.type ?
enum iommufd_hw_pagetable_type {
	IOMMUFD_HWPT_PAGING,
	IOMMUFD_HWPT_NESTED,
};

struct iommufd_hw_pagetable_common {
	struct iommufd_object obj;
	struct iommu_domain *domain;
	enum iommufd_hw_pagetable_type type;
};

struct iommufd_hw_pagetable {
	struct iommufd_hw_pagetable_common common;
	struct iommufd_ioas *ioas;
	bool auto_domain : 1;
	bool enforce_cache_coherency : 1;
	bool msi_cookie : 1;
	/* Head at iommufd_ioas::hwpt_list */
	struct list_head hwpt_item;
};

struct iommufd_hw_pagetable_nested {
	struct iommufd_hw_pagetable_common common;
	// ??
};

I poked at it in an editor for a bit and it was looking OK but
requires breaking up a bunch of functions then I ran out of time

Also, we probably should feed enforce_cache_coherency through the
alloc_hwpt uapi and not try to autodetect it..

Jason