linux-kernel - Re: [PATCH v5] fs/9p: remove writeback fid and fix per-file modes

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: <7686c810-4ed6-9e3a-3714-8b803e2d3c46@wanadoo.fr>
Date:   Tue, 25 Apr 2023 09:11:01 +0200
From:   Christophe JAILLET <christophe.jaillet@...adoo.fr>
To:     Eric Van Hensbergen <ericvh@...nel.org>,
        v9fs-developer@...ts.sourceforge.net
Cc:     asmadeus@...ewreck.org, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux_oss@...debyte.com
Subject: Re: [PATCH v5] fs/9p: remove writeback fid and fix per-file modes

Le 27/03/2023 à 04:59, Eric Van Hensbergen a écrit :
> This patch removes the creating of an additional writeback_fid
> for opened files.  The patch addresses problems when files
> were opened write-only or getattr on files with dirty caches.
> 
> This patch also incorporates information about cache behavior
> in the fid for every file.  This allows us to reflect cache
> behavior from mount flags, open mode, and information from
> the server to inform readahead and writeback behavior.
> 
> This includes adding support for a 9p semantic that qid.version==0
> is used to mark a file as non-cachable which is important for
> synthetic files.  This may have a side-effect of not supporting
> caching on certain legacy file servers that do not properly set
> qid.version.  There is also now a mount flag which can disable
> the qid.version behavior.
> 
> Signed-off-by: Eric Van Hensbergen <ericvh@...nel.org>
> ---
>   fs/9p/fid.c            | 48 +++++++++-------------
>   fs/9p/fid.h            | 33 ++++++++++++++-
>   fs/9p/v9fs.h           |  1 -
>   fs/9p/vfs_addr.c       | 22 +++++-----
>   fs/9p/vfs_file.c       | 91 ++++++++++++++----------------------------
>   fs/9p/vfs_inode.c      | 45 +++++++--------------
>   fs/9p/vfs_inode_dotl.c | 48 +++++++++-------------
>   fs/9p/vfs_super.c      | 33 ++++-----------
>   8 files changed, 135 insertions(+), 186 deletions(-)
> 

Hi,

this patch has already reached -next, but there is some spurious code.

As, I'm not sure what the real intent is, I prefer to reply here instead 
of sending a patch.


[...]

> @@ -817,9 +814,14 @@ v9fs_vfs_atomic_open(struct inode *dir, struct dentry *dentry,
>   
>   	v9ses = v9fs_inode2v9ses(dir);
>   	perm = unixmode2p9mode(v9ses, mode);
> -	fid = v9fs_create(v9ses, dir, dentry, NULL, perm,
> -				v9fs_uflags2omode(flags,
> -						v9fs_proto_dotu(v9ses)));
> +	p9_omode = v9fs_uflags2omode(flags, v9fs_proto_dotu(v9ses));
> +
> +	if ((v9ses->cache >= CACHE_WRITEBACK) && (p9_omode & P9_OWRITE)) {
> +		p9_omode = (p9_omode & !P9_OWRITE) | P9_ORDWR;

This code looks strange.
P9_OWRITE is 0x01, so !P9_OWRITE is 0.
So the code is equivalent to "p9_omode = P9_ORDWR;"

Is it what is expexted?

Maybe
	p9_omode = (p9_omode & ~P9_OWRITE) | P9_ORDWR;
?

> +		p9_debug(P9_DEBUG_CACHE,
> +			"write-only file with writeback enabled, creating w/ O_RDWR\n");
> +	}
> +	fid = v9fs_create(v9ses, dir, dentry, NULL, perm, p9_omode);
>   	if (IS_ERR(fid)) {
>   		err = PTR_ERR(fid);
>   		goto error;

[...]

> diff --git a/fs/9p/vfs_inode_dotl.c b/fs/9p/vfs_inode_dotl.c
> index a28eb3aeab29..4b9488cb7a56 100644
> --- a/fs/9p/vfs_inode_dotl.c
> +++ b/fs/9p/vfs_inode_dotl.c
> @@ -232,12 +232,12 @@ v9fs_vfs_atomic_open_dotl(struct inode *dir, struct dentry *dentry,
>   	int err = 0;
>   	kgid_t gid;
>   	umode_t mode;
> +	int p9_omode = v9fs_open_to_dotl_flags(flags);
>   	const unsigned char *name = NULL;
>   	struct p9_qid qid;
>   	struct inode *inode;
>   	struct p9_fid *fid = NULL;
> -	struct v9fs_inode *v9inode;
> -	struct p9_fid *dfid = NULL, *ofid = NULL, *inode_fid = NULL;
> +	struct p9_fid *dfid = NULL, *ofid = NULL;
>   	struct v9fs_session_info *v9ses;
>   	struct posix_acl *pacl = NULL, *dacl = NULL;
>   	struct dentry *res = NULL;
> @@ -282,14 +282,19 @@ v9fs_vfs_atomic_open_dotl(struct inode *dir, struct dentry *dentry,
>   	/* Update mode based on ACL value */
>   	err = v9fs_acl_mode(dir, &mode, &dacl, &pacl);
>   	if (err) {
> -		p9_debug(P9_DEBUG_VFS, "Failed to get acl values in creat %d\n",
> +		p9_debug(P9_DEBUG_VFS, "Failed to get acl values in create %d\n",
>   			 err);
>   		goto out;
>   	}
> -	err = p9_client_create_dotl(ofid, name, v9fs_open_to_dotl_flags(flags),
> -				    mode, gid, &qid);
> +
> +	if ((v9ses->cache >= CACHE_WRITEBACK) && (p9_omode & P9_OWRITE)) {
> +		p9_omode = (p9_omode & !P9_OWRITE) | P9_ORDWR;

Same here.

CJ

> +		p9_debug(P9_DEBUG_CACHE,
> +			"write-only file with writeback enabled, creating w/ O_RDWR\n");
> +	}
> +	err = p9_client_create_dotl(ofid, name, p9_omode, mode, gid, &qid);
>   	if (err < 0) {
> -		p9_debug(P9_DEBUG_VFS, "p9_client_open_dotl failed in creat %d\n",
> +		p9_debug(P9_DEBUG_VFS, "p9_client_open_dotl failed in create %d\n",
>   			 err);
>   		goto out;
>   	}