[PATCH] fuse: callback for invalidating cached children dentries for a directory

[PATCH] fuse: callback for invalidating cached children dentries for a directory

[Ashish Sangwan]

This patch solves a long standing bug.
"([bug #15](https://github.com/libfuse/libfuse/issues/15)): if the
`default_permissions` mount option is not used, the results of the
first permission check performed by the file system for a directory
entry will be re-used for subsequent accesses as long as the inode of
the accessed entry is present in the kernel cache - even if the
permissions have since changed, and even if the subsequent access is
made by a different user.
This bug needs to be fixed in the Linux kernel and has been known
since 2006 but unfortunately no fix has been applied yet. If you
depend on correct permission handling for FUSE file systems, the only
workaround is to use `default_permissions` (which does not currently
support ACLs), or to completely disable caching of directory entry
attributes. Alternatively, the severity of the bug can be somewhat
reduced by not using the `allow_other` mount option."

This patch introduce a callback which the user space implementation can use
to invalidate the cached entries of a parent directory, for example when
the execute permissions are revoked and force real lookup.

Signed-off-by: Ashish Sangwan <ashishsangwan2@gmail.com>
---
 fs/fuse/dev.c             | 28 ++++++++++++++++++++++++++++
 fs/fuse/dir.c             | 32 ++++++++++++++++++++++++++++++++
 fs/fuse/fuse_i.h          |  3 +++
 include/uapi/linux/fuse.h |  5 +++++
 4 files changed, 68 insertions(+)

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index cbece12..56ce470 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -1490,6 +1490,31 @@ err:
 	return err;
 }
 
+static int
+fuse_notify_inval_dircache_entries(struct fuse_conn *fc, unsigned int size,
+				   struct fuse_copy_state *cs)
+{
+	struct fuse_notify_inval_dircache_entries_out outarg;
+	int err = -EINVAL;
+
+	if (size < sizeof(outarg))
+		goto err;
+	err = fuse_copy_one(cs, &outarg, sizeof(outarg));
+	if (err)
+		goto err;
+	fuse_copy_finish(cs);
+	down_read(&fc->killsb);
+	err = -ENOENT;
+	if (fc->sb)
+		err = fuse_reverse_inval_dircache_entries(fc->sb,
+							  outarg.parent);
+	up_read(&fc->killsb);
+	return err;
+err:
+	fuse_copy_finish(cs);
+	return err;
+}
+
 static int fuse_notify_inval_entry(struct fuse_conn *fc, unsigned int size,
 				   struct fuse_copy_state *cs)
 {
@@ -1815,6 +1840,9 @@ static int fuse_notify(struct fuse_conn *fc, enum fuse_notify_code code,
 	case FUSE_NOTIFY_DELETE:
 		return fuse_notify_delete(fc, size, cs);
 
+	case FUSE_NOTIFY_INVAL_DIRCACHE_ENTRIES:
+		return fuse_notify_inval_dircache_entries(fc, size, cs);
+
 	default:
 		fuse_copy_finish(cs);
 		return -EINVAL;
diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index ccd4971..8bc5f32 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -932,6 +932,38 @@ int fuse_update_attributes(struct inode *inode, struct kstat *stat,
 	return err;
 }
 
+int fuse_reverse_inval_dircache_entries(struct super_block *sb,
+					u64 parent_nodeid)
+{
+	int err = -ENOTDIR;
+	struct inode *parent;
+	struct dentry *dir;
+	struct dentry *child;
+
+	parent = ilookup5(sb, parent_nodeid, fuse_inode_eq, &parent_nodeid);
+	if (!parent)
+		return -ENOENT;
+
+	inode_lock(parent);
+	if (!S_ISDIR(parent->i_mode))
+		goto unlock;
+
+	err = -ENOENT;
+	dir = d_find_alias(parent);
+	if (!dir)
+		goto unlock;
+	err = 0;
+	spin_lock(&dir->d_lock);
+	list_for_each_entry(child, &dir->d_subdirs, d_child)
+		fuse_invalidate_entry_cache(child);
+	spin_unlock(&dir->d_lock);
+	dput(dir);
+ unlock:
+	inode_unlock(parent);
+	iput(parent);
+	return err;
+}
+
 int fuse_reverse_inval_entry(struct super_block *sb, u64 parent_nodeid,
 			     u64 child_nodeid, struct qstr *name)
 {
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index eddbe02..7cf0a3f 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -924,6 +924,9 @@ int fuse_reverse_inval_inode(struct super_block *sb, u64 nodeid,
 int fuse_reverse_inval_entry(struct super_block *sb, u64 parent_nodeid,
 			     u64 child_nodeid, struct qstr *name);
 
+int fuse_reverse_inval_dircache_entries(struct super_block *sb,
+					u64 parent_nodeid);
+
 int fuse_do_open(struct fuse_conn *fc, u64 nodeid, struct file *file,
 		 bool isdir);
 
diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
index 5974fae..c74fc3c 100644
--- a/include/uapi/linux/fuse.h
+++ b/include/uapi/linux/fuse.h
@@ -374,6 +374,7 @@ enum fuse_notify_code {
 	FUSE_NOTIFY_STORE = 4,
 	FUSE_NOTIFY_RETRIEVE = 5,
 	FUSE_NOTIFY_DELETE = 6,
+	FUSE_NOTIFY_INVAL_DIRCACHE_ENTRIES = 7,
 	FUSE_NOTIFY_CODE_MAX,
 };
 
@@ -715,6 +716,10 @@ struct fuse_direntplus {
 #define FUSE_DIRENTPLUS_SIZE(d) \
 	FUSE_DIRENT_ALIGN(FUSE_NAME_OFFSET_DIRENTPLUS + (d)->dirent.namelen)
 
+struct fuse_notify_inval_dircache_entries_out {
+	uint64_t	parent;
+};
+
 struct fuse_notify_inval_inode_out {
 	uint64_t	ino;
 	int64_t		off;
-- 
1.9.1

Re: [PATCH] fuse: callback for invalidating cached children dentries for a directory

[Miklos Szeredi]

On Tue, May 31, 2016 at 11:04 AM, Ashish Sangwan
<ashishsangwan2@gmail.com> wrote:
> This patch solves a long standing bug.
> "([bug #15](https://github.com/libfuse/libfuse/issues/15)): if the
> `default_permissions` mount option is not used, the results of the
> first permission check performed by the file system for a directory
> entry will be re-used for subsequent accesses as long as the inode of
> the accessed entry is present in the kernel cache - even if the
> permissions have since changed, and even if the subsequent access is
> made by a different user.
> This bug needs to be fixed in the Linux kernel and has been known
> since 2006 but unfortunately no fix has been applied yet. If you
> depend on correct permission handling for FUSE file systems, the only
> workaround is to use `default_permissions` (which does not currently
> support ACLs), or to completely disable caching of directory entry
> attributes. Alternatively, the severity of the bug can be somewhat
> reduced by not using the `allow_other` mount option."
>
> This patch introduce a callback which the user space implementation can use
> to invalidate the cached entries of a parent directory, for example when
> the execute permissions are revoked and force real lookup.

This doesn't solve the real problem.

As I just mentioned, the bigger problem is that when doing cached
lookups we ignore the credentials of the current process.  Without
that (and without default_permission) we just can't make a good
decision whether to allow the cached lookup or not.

Thanks,
Miklos

Re: [PATCH] fuse: callback for invalidating cached children dentries for a directory

[Ashish Sangwan]

On Tue, May 31, 2016 at 2:40 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Tue, May 31, 2016 at 11:04 AM, Ashish Sangwan
> <ashishsangwan2@gmail.com> wrote:
>> This patch solves a long standing bug.
>> "([bug #15](https://github.com/libfuse/libfuse/issues/15)): if the
>> `default_permissions` mount option is not used, the results of the
>> first permission check performed by the file system for a directory
>> entry will be re-used for subsequent accesses as long as the inode of
>> the accessed entry is present in the kernel cache - even if the
>> permissions have since changed, and even if the subsequent access is
>> made by a different user.
>> This bug needs to be fixed in the Linux kernel and has been known
>> since 2006 but unfortunately no fix has been applied yet. If you
>> depend on correct permission handling for FUSE file systems, the only
>> workaround is to use `default_permissions` (which does not currently
>> support ACLs), or to completely disable caching of directory entry
>> attributes. Alternatively, the severity of the bug can be somewhat
>> reduced by not using the `allow_other` mount option."
>>
>> This patch introduce a callback which the user space implementation can use
>> to invalidate the cached entries of a parent directory, for example when
>> the execute permissions are revoked and force real lookup.
>
> This doesn't solve the real problem.

Agree.
>
> As I just mentioned, the bigger problem is that when doing cached
> lookups we ignore the credentials of the current process.  Without
> that (and without default_permission) we just can't make a good
> decision whether to allow the cached lookup or not.
IMHO even that won't be enough. For the file system, like ours, which
has its own
security policies and does not use default_permission, there is no
straight forward
match of the user credentials to the standard mode bits/uid/gids etc.
Checking for these at the time of cached lookup will break the semantics of not
using default_permission.
I think the best bet for now is to set entry timeout to 0 seconds and
let every lookup
reach the library.

Thanks,
Ashish
>
> Thanks,
> Miklos

Re: [PATCH] fuse: callback for invalidating cached children dentries for a directory

[Miklos Szeredi]

On Tue, May 31, 2016 at 11:44 AM, Ashish Sangwan
<ashishsangwan2@gmail.com> wrote:
> On Tue, May 31, 2016 at 2:40 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>> On Tue, May 31, 2016 at 11:04 AM, Ashish Sangwan
>> <ashishsangwan2@gmail.com> wrote:
>>> This patch solves a long standing bug.
>>> "([bug #15](https://github.com/libfuse/libfuse/issues/15)): if the
>>> `default_permissions` mount option is not used, the results of the
>>> first permission check performed by the file system for a directory
>>> entry will be re-used for subsequent accesses as long as the inode of
>>> the accessed entry is present in the kernel cache - even if the
>>> permissions have since changed, and even if the subsequent access is
>>> made by a different user.
>>> This bug needs to be fixed in the Linux kernel and has been known
>>> since 2006 but unfortunately no fix has been applied yet. If you
>>> depend on correct permission handling for FUSE file systems, the only
>>> workaround is to use `default_permissions` (which does not currently
>>> support ACLs), or to completely disable caching of directory entry
>>> attributes. Alternatively, the severity of the bug can be somewhat
>>> reduced by not using the `allow_other` mount option."
>>>
>>> This patch introduce a callback which the user space implementation can use
>>> to invalidate the cached entries of a parent directory, for example when
>>> the execute permissions are revoked and force real lookup.
>>
>> This doesn't solve the real problem.
>
> Agree.
>>
>> As I just mentioned, the bigger problem is that when doing cached
>> lookups we ignore the credentials of the current process.  Without
>> that (and without default_permission) we just can't make a good
>> decision whether to allow the cached lookup or not.
> IMHO even that won't be enough. For the file system, like ours, which
> has its own
> security policies and does not use default_permission, there is no
> straight forward
> match of the user credentials to the standard mode bits/uid/gids etc.

You don't need that.  The only thing you need is that access is based
on user/group(s).  If that's not the case (e.g. some other attribute
of the process doing the access is used) then this will indeed not
work.

Thanks,
Miklos

Re: [PATCH] fuse: callback for invalidating cached children dentries for a directory

[Nikolaus Rath]

On May 31 2016, Ashish Sangwan <ashishsangwan2@gmail.com> wrote:
> On Tue, May 31, 2016 at 2:40 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>> On Tue, May 31, 2016 at 11:04 AM, Ashish Sangwan
>> <ashishsangwan2@gmail.com> wrote:
>>> This patch solves a long standing bug.
>>> "([bug #15](https://github.com/libfuse/libfuse/issues/15)): if the
>>> `default_permissions` mount option is not used, the results of the
>>> first permission check performed by the file system for a directory
>>> entry will be re-used for subsequent accesses as long as the inode of
>>> the accessed entry is present in the kernel cache - even if the
>>> permissions have since changed, and even if the subsequent access is
>>> made by a different user.
>>> This bug needs to be fixed in the Linux kernel and has been known
>>> since 2006 but unfortunately no fix has been applied yet. If you
>>> depend on correct permission handling for FUSE file systems, the only
>>> workaround is to use `default_permissions` (which does not currently
>>> support ACLs), or to completely disable caching of directory entry
>>> attributes. Alternatively, the severity of the bug can be somewhat
>>> reduced by not using the `allow_other` mount option."
>>>
>>> This patch introduce a callback which the user space implementation can use
>>> to invalidate the cached entries of a parent directory, for example when
>>> the execute permissions are revoked and force real lookup.
>>
>> This doesn't solve the real problem.
>
> Agree.
>>
>> As I just mentioned, the bigger problem is that when doing cached
>> lookups we ignore the credentials of the current process.  Without
>> that (and without default_permission) we just can't make a good
>> decision whether to allow the cached lookup or not.
> IMHO even that won't be enough. For the file system, like ours, which
> has its own
> security policies and does not use default_permission, there is no
> straight forward
> match of the user credentials to the standard mode bits/uid/gids etc.
> Checking for these at the time of cached lookup will break the semantics of not
> using default_permission.
> I think the best bet for now is to set entry timeout to 0 seconds and
> let every lookup
> reach the library.

I think that's the only solution for such access policies in general
(not just for now). Any attempt to tell the kernel when to invalidate
will be prone to race conditions. If the kernel does not understand your
permission model, you cannot use caching. Period.

Best,
-Nikolaus


-- 
GPG encrypted emails preferred. Key id: 0xD113FCAC3C4E599F
Fingerprint: ED31 791B 2C5C 1613 AF38 8B8A D113 FCAC 3C4E 599F

             »Time flies like an arrow, fruit flies like a Banana.«