RE: [RFC PATCH] vfio/pci: add PCIe TPH to device feature ioctl

[Wathsala Wathawana Vithanage]

> > +static ssize_t vfio_pci_tph_uinfo_dup(struct vfio_pci_tph *tph,
> > +				      void __user *arg, size_t
> > argsz,
> > +				      struct vfio_pci_tph_info
> > **info)
> > +{
> > +	size_t minsz;
> > +
> > +	if (tph->count > VFIO_TPH_INFO_MAX)
> > +		return -EINVAL;
> > +	if (!tph->count)
> > +		return 0;
> > +
> > +	minsz = tph->count * sizeof(struct vfio_pci_tph_info);
> > +	if (minsz < argsz)
> > +		return -EINVAL;
> > +
> > +	*info = memdup_user(arg, minsz);
> 
> You can use memdup_array_user() instead of the lines above. It does the
> multiplication plus overflow check for you and will make your code more
> compact.
> 

Thank you, wasn't aware of it.

> > +	if (IS_ERR(info))
> > +		return PTR_ERR(info);
> > +
> > +	return minsz;
> 
> see below…
> 
> > +}
> > +
> > +static int vfio_pci_feature_tph_st_op(struct vfio_pci_core_device
> > *vdev,
> > +				      struct vfio_pci_tph *tph,
> > +				      void __user *arg, size_t
> > argsz)
> > +{
> > +	int i, mtype, err = 0;
> > +	u32 cpu_uid;
> > +	struct vfio_pci_tph_info *info = NULL;
> > +	ssize_t data_size = vfio_pci_tph_uinfo_dup(tph, arg, argsz,
> > &info);
> > +
> > +	if (data_size <= 0)
> > +		return data_size;
> 
> So it seems you return here in case of an error. However, that would result in a
> length of 0 being an error?
> 
> I would try to avoid to return 0 for an error whenever possible. That breaks
> convention.
> 
> How about you return the result value of memdup_array_user() in …uinfo_dup()?
> 
> The only thing I can't tell is whether tph->count == 0 should be treated as an
> error. Maybe map it to -EINVAL?
> 
> 

I wasn't sure before, but I lean towards -EINVAL now. I will amend this to -EINVAL.
I saw this like reading 0 bytes from a file descriptor, which returns 0.
But there are also differences, read return the number of bytes read, whereas this
doesn't return the number of records.