[prev in list] [next in list] [prev in thread] [next in thread] 

List:       v9fs-developer
Subject:    Re: [V9fs-developer] 9p was broken by 478ba09edc1f2f2ee27180a06150cb2d1a686f9c
From:       Dominique Martinet <asmadeus () codewreck ! org>
Date:       2022-01-27 22:52:36
Message-ID: YfMiNFopSM5EEeFY () codewreck ! org
[Download RAW message or body]

Latchesar Ionkov wrote on Thu, Jan 27, 2022 at 03:17:38PM -0700:
> At some point of v9fs life the open fids were kept in only in the
> filps, while the unopened fids were kept in the inodes where they can
> be easily reached. I don't know why that was changed at all.

The lists didn't change, there is still a list of unopened fids in the
dentry and a list with all fids (including open ones) in the inode

> You can get unopen fid from the file through the inode, but there is
> no way to confuse which fids are open and which are not. What is the use case
> for having opened fids outside the filps?

What I said:
>> open files should have a fid in filp->private_data but when we reach the
>> setattr function (e.g. v9fs_vfs_setattr_dotl) the filp is nowhere to be
>> found, which means we need a new lookup -- and incidentaly we don't know
>> either if the fd the operation was meant for was open as RO or RW.

This is no longer true (the filp is available if ATTR_FILE is valid),
but nobody seemed to have noticed at the time as setattr would always do
a fresh lookup then.

The rationale is in the commit message of the incriminated patch:
------
From 478ba09edc1f2f2ee27180a06150cb2d1a686f9c Mon Sep 17 00:00:00 2001
From: Greg Kurz <groug@kaod.org>
Date: Wed, 23 Sep 2020 22:11:45 +0800
Subject: [PATCH] fs/9p: search open fids first

A previous patch fixed the "create-unlink-getattr" idiom: if getattr is
called on an unlinked file, we try to find an open fid attached to the
corresponding inode.

We have a similar issue with file permissions and setattr:

open("./test.txt", O_RDWR|O_CREAT, 0666) = 4
chmod("./test.txt", 0)                  = 0
truncate("./test.txt", 0)               = -1 EACCES (Permission denied)
ftruncate(4, 0)                         = -1 EACCES (Permission denied)

The failure is expected with truncate() but not with ftruncate().

This happens because the lookup code does find a matching fid in the
dentry list. Unfortunately, this is not an open fid and the server
will be forced to rely on the path name, rather than on an open file
descriptor. This is the case in QEMU: the setattr operation will use
truncate() and fail because of bad write permissions.

This patch changes the logic in the lookup code, so that we consider
open fids first. It gives a chance to the server to match this open
fid to an open file descriptor and use ftruncate() instead of truncate().
This does not change the current behaviour for truncate() and other
path name based syscalls, since file permissions are checked earlier
in the VFS layer.

With this patch, we get:

open("./test.txt", O_RDWR|O_CREAT, 0666) = 4
chmod("./test.txt", 0)                  = 0
truncate("./test.txt", 0)               = -1 EACCES (Permission denied)
ftruncate(4, 0)                         = 0

Link: http://lkml.kernel.org/r/20200923141146.90046-4-jianyong.wu@arm.com
Signed-off-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>
-----

As that is no longer needed, this commit will be reverted and everyone
should be happy. Sending the revert patch one day earlier won't change
much but I don't want to push untested code.
-- 
Dominique


_______________________________________________
V9fs-developer mailing list
V9fs-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/v9fs-developer
[prev in list] [next in list] [prev in thread] [next in thread]