linux-kernel - Re: [PATCH RFCv1 08/14] iommufd: Add IOMMU_VIOMMU_SET_DEV

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 for Android: free password hash cracker in your pocket

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

Date: Tue, 21 May 2024 15:30:11 -0300
From: Jason Gunthorpe <jgg@...dia.com>
To: Nicolin Chen <nicolinc@...dia.com>
Cc: will@...nel.org, robin.murphy@....com, kevin.tian@...el.com,
	suravee.suthikulpanit@....com, joro@...tes.org,
	linux-kernel@...r.kernel.org, iommu@...ts.linux.dev,
	linux-arm-kernel@...ts.infradead.org, linux-tegra@...r.kernel.org,
	yi.l.liu@...el.com, eric.auger@...hat.com, vasant.hegde@....com,
	jon.grimm@....com, santosh.shukla@....com, Dhaval.Giani@....com,
	shameerali.kolothum.thodi@...wei.com
Subject: Re: [PATCH RFCv1 08/14] iommufd: Add IOMMU_VIOMMU_SET_DEV_ID ioctl

On Thu, May 16, 2024 at 10:14:38PM -0700, Nicolin Chen wrote:
> I implemented it with a small tweak, to align with viommu_alloc
> and vqueue_alloc:
> 
> 	// core
> 	struct iommufd_vdev_id {
> 		struct iommufd_viommu *viommu;
> 		struct device *dev;
> 		u64 vdev_id;
> 		struct list_head idev_item;
> 	};
> 
> 	// driver
> 	struct my_driver_vdev_id {
> 	    struct iommufd_vdev_id core;
> 	    unsigned int private_attrs;
> 	};
> 
> 	static struct iommufd_vdev_id *
> 	my_driver_set_vdev_id(struct iommufd_viommu *viommu,
> 			      struct device *dev, u64 id)
> 	{
> 	    struct my_driver_vdev_id *my_vdev_id;
> 
> 	    my_vdev_id = kzalloc(sizeof(*my_vdev_id), GFP_KERNEL);
> 	    .... /* set private_attrs */
> 	    return &my_driver_vdev_id->core;
> 	}
> 
> 	static void my_driver_unset_vdev_id(struct iommufd_vdev_id *vdev_id)
> 	{
> 	    struct my_driver_vdev_id *my_vdev_id =
> 		    container_of(vdev_id, struct my_driver_vdev_id, core);
> 	    .... /* unset private_attrs */
> 	}
> 
> Please let me know if you like it inverted as you wrote above.

Seems like a reasonable direction

> I also moved xa_cmpxchg to the driver, as I feel that the vdev_id
> here is very driver/iommu specific. There can be some complication
> if iommufd core handles this u64 vdev_id: most likely we will use
> this u64 vdev_id to index the xarray that takes an unsigned-long
> xa_index for a fast vdev_id-to-pdev_id lookup, while only a driver
> knows whether u64 vdev_id is compatible with unsigned long or not.

This seems like the most general part.. The core code should just have
a check like:

  if (vdevid >= ULONG_MAX) return -EINVAL;

And if someone wants to make 32 bit kernels support a 64bit vdevid
then they can sort it out someday :) I think this is not a big issue
as all iommus seem to have some kind of radix lookup for vdevid and
want it to be small.

Matthew has been talking about support for 64bit indexes in
maple/xarray or something for a bit so it might sort itself out.

> And, we have a list_head in the structure idev, so a device unbind
> will for-each the list and unset all the vdev_ids in it, meanwhile
> the viommu stays. I wonder if we need to add another list_head in
> the structure viommu, so a viommu tear down will for-each its list
> and unset all the vdev_ids on its side while a device (idev) stays.
> I don't see a use case of that though..any thought?

I think you need to support viommu teardown, at least from a
correctness perspective. The API permits it. But isn't it just
list_del everything in the xarray and that will clean it up enough?

Jason