[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <64da880166403_5ea6e29449@dwillia2-xfh.jf.intel.com.notmuch>
Date: Mon, 14 Aug 2023 13:01:05 -0700
From: Dan Williams <dan.j.williams@...el.com>
To: Valentine Sinitsyn <valesini@...dex-team.ru>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Tejun Heo <tj@...nel.org>
CC: Daniel Vetter <daniel.vetter@...ll.ch>,
Bjorn Helgaas <bhelgaas@...gle.com>,
Dan Williams <dan.j.williams@...el.com>,
<linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] kernfs: implement custom llseek method to fix userspace
regression
Valentine Sinitsyn wrote:
> Since commit 636b21b50152 ("PCI: Revoke mappings like devmem"),
> mmapable sysfs binary attributes have started receiving 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.
>
> This resulted in a userspace-visible regression: lseek(fd, 0, SEEK_END)
> now returns zero regardless the real sysfs attribute size which stat()
> reports. The reason is that kernfs files use generic_file_llseek()
> implementation, 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
> attribute or kernfs file representing it. This being said, f_inode
> remains valid, so stat() which uses it works correctly.
Can you say a bit more about what userspace scenario regressed so that
others doing backports can make a judgement call on the severity?
>
> Fixes the regression by implementing a custom llseek fop for kernfs,
> which uses an attribute's file inode to get the file size,
> just as stat() does.
>
> Fixes: 636b21b50152 ("PCI: Revoke mappings like devmem")
> Cc: stable@...r.kernel.org
> Signed-off-by: Valentine Sinitsyn <valesini@...dex-team.ru>
> ---
> fs/kernfs/file.c | 17 ++++++++++++++++-
> 1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/fs/kernfs/file.c b/fs/kernfs/file.c
> index 180906c36f51..6d81e0c981f3 100644
> --- a/fs/kernfs/file.c
> +++ b/fs/kernfs/file.c
> @@ -903,6 +903,21 @@ static __poll_t kernfs_fop_poll(struct file *filp, poll_table *wait)
> return ret;
> }
>
> +static loff_t kernfs_fop_llseek(struct file *file, loff_t offset, int whence)
> +{
> + /*
> + * This is almost identical to generic_file_llseek() except it uses
> + * cached inode value instead of f_mapping->host.
> + * The reason is that, for PCI resources in sysfs the latter points to
> + * iomem_inode whose size has nothing to do with the attribute's size.
> + */
> + struct inode *inode = file_inode(file);
My only concern is whether there are any scenarios where this is not
appropriate. I.e. do a bit more work to define a kernfs_ops instance
specifically for overriding lseek() in this scenario.
> +
> + return generic_file_llseek_size(file, offset, whence,
> + inode->i_sb->s_maxbytes,
> + i_size_read(inode));
> +}
> +
> static void kernfs_notify_workfn(struct work_struct *work)
> {
> struct kernfs_node *kn;
> @@ -1005,7 +1020,7 @@ EXPORT_SYMBOL_GPL(kernfs_notify);
> const struct file_operations kernfs_file_fops = {
> .read_iter = kernfs_fop_read_iter,
> .write_iter = kernfs_fop_write_iter,
> - .llseek = generic_file_llseek,
> + .llseek = kernfs_fop_llseek,
> .mmap = kernfs_fop_mmap,
> .open = kernfs_fop_open,
> .release = kernfs_fop_release,
> --
> 2.34.1
>
Powered by blists - more mailing lists