linux-kernel - RE: [PATCH v6 06/10] iommufd: Add iommufd fault object

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: <BN9PR11MB52766278644CB92F7E195E988CCD2@BN9PR11MB5276.namprd11.prod.outlook.com>
Date: Mon, 17 Jun 2024 07:32:44 +0000
From: "Tian, Kevin" <kevin.tian@...el.com>
To: Jason Gunthorpe <jgg@...pe.ca>
CC: Lu Baolu <baolu.lu@...ux.intel.com>, Joerg Roedel <joro@...tes.org>, "Will
 Deacon" <will@...nel.org>, Robin Murphy <robin.murphy@....com>, Jean-Philippe
 Brucker <jean-philippe@...aro.org>, Nicolin Chen <nicolinc@...dia.com>, "Liu,
 Yi L" <yi.l.liu@...el.com>, Jacob Pan <jacob.jun.pan@...ux.intel.com>, Joel
 Granados <j.granados@...sung.com>, "iommu@...ts.linux.dev"
	<iommu@...ts.linux.dev>, "virtualization@...ts.linux-foundation.org"
	<virtualization@...ts.linux-foundation.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v6 06/10] iommufd: Add iommufd fault object

> From: Jason Gunthorpe <jgg@...pe.ca>
> Sent: Wednesday, June 12, 2024 9:24 PM
> 
> On Fri, Jun 07, 2024 at 09:17:28AM +0000, Tian, Kevin wrote:
> > > From: Lu Baolu <baolu.lu@...ux.intel.com>
> > > Sent: Monday, May 27, 2024 12:05 PM
> > >
> > > +static ssize_t iommufd_fault_fops_read(struct file *filep, char __user
> *buf,
> > > +				       size_t count, loff_t *ppos)
> > > +{
> > > +	size_t fault_size = sizeof(struct iommu_hwpt_pgfault);
> > > +	struct iommufd_fault *fault = filep->private_data;
> > > +	struct iommu_hwpt_pgfault data;
> > > +	struct iommufd_device *idev;
> > > +	struct iopf_group *group;
> > > +	struct iopf_fault *iopf;
> > > +	size_t done = 0;
> > > +	int rc = 0;
> > > +
> > > +	if (*ppos || count % fault_size)
> > > +		return -ESPIPE;
> >
> > the man page says:
> >
> > "If count is zero, read() returns zero and has no  other  results."
> 
> The above does that? 0 % X == 0

but if *ppos is non-zero then an error will be returned even if count==0.

but seems I looked at an old man page on an old system. A newer one
says:

       If count is zero, read() may detect the errors described below.
       In the absence of any errors, or if read() does not check for
       errors, a read() with a count of 0 returns zero and has no other
       effects.

so above should be fine. 😊

> > > +				xa_erase(&fault->response, group->cookie);
> > > +				break;
> > > +			}
> > > +			done += fault_size;
> > > +		}
> > > +
> > > +		list_del(&group->node);
> > > +	}
> > > +	mutex_unlock(&fault->mutex);
> > > +
> > > +	return done == 0 ? rc : done;
> >
> > again this doesn't match the manual:
> >
> > "On error, -1 is returned, and errno is set appropriately. "
> >
> > it doesn't matter whether 'done' is 0.
> 
> It is setup so that once the list_del() below happens it is guarenteed
> that the system call will return a positive result so that the
> list_del'd items are always returned to userspace.
> 
> If we hit any fault here on the Nth item we should still return the
> prior items and ignore the fault.
> 
> If we hit a fault on the first item then we should return the fault.
> 

yes that does make sense. I just didn't find such statement in the
man page which simply said to return -1 on error. The number of
bytes  is returned only on success.

RETURN VALUE
       On success, the number of bytes read is returned (zero indicates
       end of file), and the file position is advanced by this number.
       It is not an error if this number is smaller than the number of
       bytes requested; this may happen for example because fewer bytes
       are actually available right now (maybe because we were close to
       end-of-file, or because we are reading from a pipe, or from a
       terminal), or because read() was interrupted by a signal.  See
       also NOTES.

       On error, -1 is returned, and errno is set to indicate the error.
       In this case, it is left unspecified whether the file position
       (if any) changes.