Re: [RESEND PATCH v2 2/2] PCI: implement custom llseek method for PCI resource entries in sysfs

[Bjorn Helgaas]

In subject, to match history:

  PCI: Implement custom llseek for sysfs resource entries

On Mon, Aug 21, 2023 at 12:29:56PM +0500, Valentine Sinitsyn wrote:
> Since commit 636b21b50152 ("PCI: Revoke mappings like devmem"), mmapable
> sysfs entries have started to receive their f_mapping from the iomem
> pseudo filesystem, so that CONFIG_IO_STRICT_DEVMEM is honored in sysfs
> (and procfs) as well as in /dev/[k]mem.

s/mmapable/mmappable/ (there's precedent for both, but by analogy with
"mappable", I think "mmappable" makes more sense)

> This resulted in a userspace-visible regression:
> 
> 1. Open a sysfs PCI resource file (eg. /sys/bus/pci/devices/*/resource0)
> 2. Use lseek(fd, 0, SEEK_END) to determine its size
> 
> Expected result: a PCI region size is returned.
> Actual result: 0 is returned.
> 
> The reason is that PCI resource files residing in sysfs use
> generic_file_llseek(), which relies on f_mapping->host inode to get the
> file size. As f_mapping is now redefined, f_mapping->host points to an
> anonymous zero-sized iomem_inode which has nothing to do with sysfs file
> in question.
> 
> Implement a custom llseek method for sysfs PCI resources, which is
> almost the same as proc_bus_pci_lseek() used for procfs entries.
> 
> This makes sysfs and procfs entries consistent with regards to seeking,
> but also introduces userspace-visible changes to seeking PCI resources
> in sysfs:
> 
> - SEEK_DATA and SEEK_HOLE are no longer supported;
> - Seeking past the end of the file is prohibited while previously
>   offsets up to MAX_NON_LFS were accepted (reading from these offsets
>   was always invalid).
> 
> Fixes: 636b21b50152 ("PCI: Revoke mappings like devmem")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Valentine Sinitsyn <valesini@xxxxxxxxxxxxxx>

It'd be nice but not essential to tweak commit log as above.

Acked-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>

> ---
> Looks like the patch didn't get to lkml, resending
> 
>  drivers/pci/pci-sysfs.c | 21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index ab32a91f287b..42eaeb8d4a4f 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -967,6 +967,8 @@ void pci_create_legacy_files(struct pci_bus *b)
>  	b->legacy_io->attr.mode = 0600;
>  	b->legacy_io->read = pci_read_legacy_io;
>  	b->legacy_io->write = pci_write_legacy_io;
> +	/* See pci_create_attr() for motivation */
> +	b->legacy_io->llseek = pci_llseek_resource;
>  	b->legacy_io->mmap = pci_mmap_legacy_io;
>  	b->legacy_io->f_mapping = iomem_get_mapping;
>  	pci_adjust_legacy_attr(b, pci_mmap_io);
> @@ -981,6 +983,8 @@ void pci_create_legacy_files(struct pci_bus *b)
>  	b->legacy_mem->size = 1024*1024;
>  	b->legacy_mem->attr.mode = 0600;
>  	b->legacy_mem->mmap = pci_mmap_legacy_mem;
> +	/* See pci_create_attr() for motivation */
> +	b->legacy_io->llseek = pci_llseek_resource;
>  	b->legacy_mem->f_mapping = iomem_get_mapping;
>  	pci_adjust_legacy_attr(b, pci_mmap_mem);
>  	error = device_create_bin_file(&b->dev, b->legacy_mem);
> @@ -1138,6 +1142,14 @@ static ssize_t pci_write_resource_io(struct file *filp, struct kobject *kobj,
>  	return pci_resource_io(filp, kobj, attr, buf, off, count, true);
>  }
>  
> +static loff_t pci_llseek_resource(struct file *filep,
> +				  struct kobject *kobj __always_unused,
> +				  struct bin_attribute *attr,
> +				  loff_t offset, int whence)
> +{
> +	return fixed_size_llseek(filep, offset, whence, attr->size);
> +}
> +
>  /**
>   * pci_remove_resource_files - cleanup resource files
>   * @pdev: dev to cleanup
> @@ -1195,8 +1207,15 @@ static int pci_create_attr(struct pci_dev *pdev, int num, int write_combine)
>  			res_attr->mmap = pci_mmap_resource_uc;
>  		}
>  	}
> -	if (res_attr->mmap)
> +	if (res_attr->mmap) {
>  		res_attr->f_mapping = iomem_get_mapping;
> +		/*
> +		 * generic_file_llseek() consults f_mapping->host to determine
> +		 * the file size. As iomem_inode knows nothing about the
> +		 * attribute, it's not going to work, so override it as well.
> +		 */
> +		res_attr->llseek = pci_llseek_resource;
> +	}
>  	res_attr->attr.name = res_attr_name;
>  	res_attr->attr.mode = 0600;
>  	res_attr->size = pci_resource_len(pdev, num);
> -- 
> 2.34.1
>