Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 20 Jul 2020 16:53:40 +0200
From:   Dominique Martinet <asmadeus@codewreck.org>
To:     Jianyong Wu <jianyong.wu@arm.com>
Cc:     ericvh@gmail.com, hch@lst.de, dhowells@redhat.com,
        lucho@ionkov.net, v9fs-developer@lists.sourceforge.net,
        linux-kernel@vger.kernel.org, Kaly.Xin@arm.com, justin.he@arm.com,
        wei.chen@arm.com
Subject: Re: [RFC PATCH 1/2] vfs: pass file down when getattr to avoid losing
 info.
Message-ID: <20200720145340.GA13275@nautica>
References: <20200720014622.37364-1-jianyong.wu@arm.com>
 <20200720014622.37364-2-jianyong.wu@arm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20200720014622.37364-2-jianyong.wu@arm.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Jianyong Wu wrote on Mon, Jul 20, 2020:
> Currently, getting attribute for a file represented by fd is always
> by inode or path which may lead to bug for a certain network file system.
> Adding file struct into struct kstat and assigning file for it in
> vfs_statx_fd can avoid this issue. This change refers to struct istat.
> 
> Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
> ---
>  fs/stat.c            | 1 +
>  include/linux/stat.h | 6 ++++++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/fs/stat.c b/fs/stat.c
> index 44f8ad346db4..0dee5487f6d6 100644
> --- a/fs/stat.c
> +++ b/fs/stat.c
> @@ -147,6 +147,7 @@ int vfs_statx_fd(unsigned int fd, struct kstat *stat,
>  		return -EINVAL;
>  
>  	f = fdget_raw(fd);
> +	stat->filp = f.file;
>  	if (f.file) {
>  		error = vfs_getattr(&f.file->f_path, stat,
>  				    request_mask, query_flags);
> diff --git a/include/linux/stat.h b/include/linux/stat.h
> index 56614af83d4a..4755c528d49a 100644
> --- a/include/linux/stat.h
> +++ b/include/linux/stat.h
> @@ -48,6 +48,12 @@ struct kstat {
>  	struct timespec64 btime;			/* File creation time */
>  	u64		blocks;
>  	u64		mnt_id;
> +
> +	/*
> +	 * Not an attribute, but an auxiliary info for filesystems wanting to
> +	 * implement an fstat() like method.
> +	 */
> +	struct file	*filp;

Just, no ; don't touch this, it isn't likely to get accepted ever.
file operations should all have a filp around already, we need to fix
this in our code.

(also missing quite a few Cc if you ever want to touch these bits, at
least linux-fsdevel@)


FWIW the problem has been discussed a few times previously.

I'd appreciate if you could take over from Eric and Greg's old series
that intended to fix that:
https://lore.kernel.org/lkml/146659832556.15781.17414806975641516683.stgit@bahia.lan/

it introduced a race somewhere, but the idea looked good at the time so
it's worth looking into.

-- 
Dominique