| Message ID | 164021541207.640689.564689725898537127.stgit@warthog.procyon.org.uk |
|---|---|
| State | New |
| Headers | show |
| Series | fscache, cachefiles: Rewrite | expand |
On Wed, 2021-12-22 at 23:23 +0000, David Howells wrote: > Use an inode flag, S_KERNEL_FILE, to mark that a backing file is in use by > the kernel to prevent cachefiles or other kernel services from interfering > with that file. > > Alter rmdir to reject attempts to remove a directory marked with this flag. > This is used by cachefiles to prevent cachefilesd from removing them. > > Using S_SWAPFILE instead isn't really viable as that has other effects in > the I/O paths. > > Changes > ======= > ver #3: > - Check for the object pointer being NULL in the tracepoints rather than > the caller. > > Signed-off-by: David Howells <dhowells@redhat.com> > cc: linux-cachefs@redhat.com > Link: https://lore.kernel.org/r/163819630256.215744.4815885535039369574.stgit@warthog.procyon.org.uk/ # v1 > Link: https://lore.kernel.org/r/163906931596.143852.8642051223094013028.stgit@warthog.procyon.org.uk/ # v2 > Link: https://lore.kernel.org/r/163967141000.1823006.12920680657559677789.stgit@warthog.procyon.org.uk/ # v3 > --- > > fs/cachefiles/Makefile | 1 + > fs/cachefiles/namei.c | 43 +++++++++++++++++++++++++++++++++++++ > fs/namei.c | 3 ++- > include/linux/fs.h | 1 + > include/trace/events/cachefiles.h | 42 ++++++++++++++++++++++++++++++++++++ > 5 files changed, 89 insertions(+), 1 deletion(-) > create mode 100644 fs/cachefiles/namei.c > > diff --git a/fs/cachefiles/Makefile b/fs/cachefiles/Makefile > index 463e3d608b75..e0b092ca077f 100644 > --- a/fs/cachefiles/Makefile > +++ b/fs/cachefiles/Makefile > @@ -7,6 +7,7 @@ cachefiles-y := \ > cache.o \ > daemon.o \ > main.o \ > + namei.o \ > security.o > > cachefiles-$(CONFIG_CACHEFILES_ERROR_INJECTION) += error_inject.o > diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c > new file mode 100644 > index 000000000000..913f83f1c900 > --- /dev/null > +++ b/fs/cachefiles/namei.c > @@ -0,0 +1,43 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* CacheFiles path walking and related routines > + * > + * Copyright (C) 2021 Red Hat, Inc. All Rights Reserved. > + * Written by David Howells (dhowells@redhat.com) > + */ > + > +#include <linux/fs.h> > +#include "internal.h" > + > +/* > + * Mark the backing file as being a cache file if it's not already in use. The > + * mark tells the culling request command that it's not allowed to cull the > + * file or directory. The caller must hold the inode lock. > + */ > +static bool __cachefiles_mark_inode_in_use(struct cachefiles_object *object, > + struct dentry *dentry) > +{ > + struct inode *inode = d_backing_inode(dentry); > + bool can_use = false; > + > + if (!(inode->i_flags & S_KERNEL_FILE)) { nit: most of the other S_* flags have a corresponding IS_* macro. Should this be: IS_KERNEL_FILE(inode) ? > + inode->i_flags |= S_KERNEL_FILE; > + trace_cachefiles_mark_active(object, inode); > + can_use = true; > + } else { > + pr_notice("cachefiles: Inode already in use: %pd\n", dentry); > + } > + > + return can_use; > +} > + > +/* > + * Unmark a backing inode. The caller must hold the inode lock. > + */ > +static void __cachefiles_unmark_inode_in_use(struct cachefiles_object *object, > + struct dentry *dentry) > +{ > + struct inode *inode = d_backing_inode(dentry); > + > + inode->i_flags &= ~S_KERNEL_FILE; > + trace_cachefiles_mark_inactive(object, inode); > +} > diff --git a/fs/namei.c b/fs/namei.c > index 1f9d2187c765..d81f04f8d818 100644 > --- a/fs/namei.c > +++ b/fs/namei.c > @@ -3958,7 +3958,8 @@ int vfs_rmdir(struct user_namespace *mnt_userns, struct inode *dir, > inode_lock(dentry->d_inode); > > error = -EBUSY; > - if (is_local_mountpoint(dentry)) > + if (is_local_mountpoint(dentry) || > + (dentry->d_inode->i_flags & S_KERNEL_FILE)) > goto out; > > error = security_inode_rmdir(dir, dentry); > diff --git a/include/linux/fs.h b/include/linux/fs.h > index 2c0b8e77d9ab..bcf1ca430139 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -2249,6 +2249,7 @@ struct super_operations { > #define S_ENCRYPTED (1 << 14) /* Encrypted file (using fs/crypto/) */ > #define S_CASEFOLD (1 << 15) /* Casefolded file */ > #define S_VERITY (1 << 16) /* Verity file (using fs/verity/) */ > +#define S_KERNEL_FILE (1 << 17) /* File is in use by the kernel (eg. fs/cachefiles) */ > > /* > * Note that nosuid etc flags are inode-specific: setting some file-system > diff --git a/include/trace/events/cachefiles.h b/include/trace/events/cachefiles.h > index 9bd5a8a60801..6331cd29880d 100644 > --- a/include/trace/events/cachefiles.h > +++ b/include/trace/events/cachefiles.h > @@ -83,6 +83,48 @@ cachefiles_error_traces; > #define E_(a, b) { a, b } > > > +TRACE_EVENT(cachefiles_mark_active, > + TP_PROTO(struct cachefiles_object *obj, > + struct inode *inode), > + > + TP_ARGS(obj, inode), > + > + /* Note that obj may be NULL */ > + TP_STRUCT__entry( > + __field(unsigned int, obj ) > + __field(ino_t, inode ) > + ), > + > + TP_fast_assign( > + __entry->obj = obj ? obj->debug_id : 0; > + __entry->inode = inode->i_ino; > + ), > + > + TP_printk("o=%08x i=%lx", > + __entry->obj, __entry->inode) > + ); > + > +TRACE_EVENT(cachefiles_mark_inactive, > + TP_PROTO(struct cachefiles_object *obj, > + struct inode *inode), > + > + TP_ARGS(obj, inode), > + > + /* Note that obj may be NULL */ > + TP_STRUCT__entry( > + __field(unsigned int, obj ) > + __field(ino_t, inode ) > + ), > + > + TP_fast_assign( > + __entry->obj = obj ? obj->debug_id : 0; > + __entry->inode = inode->i_ino; > + ), > + > + TP_printk("o=%08x i=%lx", > + __entry->obj, __entry->inode) > + ); > + > TRACE_EVENT(cachefiles_vfs_error, > TP_PROTO(struct cachefiles_object *obj, struct inode *backer, > int error, enum cachefiles_error_trace where), > >
On Sat, Dec 25, 2021 at 1:32 AM David Howells <dhowells@redhat.com> wrote: > > Use an inode flag, S_KERNEL_FILE, to mark that a backing file is in use by > the kernel to prevent cachefiles or other kernel services from interfering > with that file. > > Alter rmdir to reject attempts to remove a directory marked with this flag. > This is used by cachefiles to prevent cachefilesd from removing them. > > Using S_SWAPFILE instead isn't really viable as that has other effects in > the I/O paths. > > Changes > ======= > ver #3: > - Check for the object pointer being NULL in the tracepoints rather than > the caller. > > Signed-off-by: David Howells <dhowells@redhat.com> > cc: linux-cachefs@redhat.com > Link: https://lore.kernel.org/r/163819630256.215744.4815885535039369574.stgit@warthog.procyon.org.uk/ # v1 > Link: https://lore.kernel.org/r/163906931596.143852.8642051223094013028.stgit@warthog.procyon.org.uk/ # v2 > Link: https://lore.kernel.org/r/163967141000.1823006.12920680657559677789.stgit@warthog.procyon.org.uk/ # v3 > --- > > fs/cachefiles/Makefile | 1 + > fs/cachefiles/namei.c | 43 +++++++++++++++++++++++++++++++++++++ > fs/namei.c | 3 ++- > include/linux/fs.h | 1 + > include/trace/events/cachefiles.h | 42 ++++++++++++++++++++++++++++++++++++ > 5 files changed, 89 insertions(+), 1 deletion(-) > create mode 100644 fs/cachefiles/namei.c > > diff --git a/fs/cachefiles/Makefile b/fs/cachefiles/Makefile > index 463e3d608b75..e0b092ca077f 100644 > --- a/fs/cachefiles/Makefile > +++ b/fs/cachefiles/Makefile > @@ -7,6 +7,7 @@ cachefiles-y := \ > cache.o \ > daemon.o \ > main.o \ > + namei.o \ > security.o > > cachefiles-$(CONFIG_CACHEFILES_ERROR_INJECTION) += error_inject.o > diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c > new file mode 100644 > index 000000000000..913f83f1c900 > --- /dev/null > +++ b/fs/cachefiles/namei.c > @@ -0,0 +1,43 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* CacheFiles path walking and related routines > + * > + * Copyright (C) 2021 Red Hat, Inc. All Rights Reserved. > + * Written by David Howells (dhowells@redhat.com) > + */ > + > +#include <linux/fs.h> > +#include "internal.h" > + > +/* > + * Mark the backing file as being a cache file if it's not already in use. The > + * mark tells the culling request command that it's not allowed to cull the > + * file or directory. The caller must hold the inode lock. > + */ > +static bool __cachefiles_mark_inode_in_use(struct cachefiles_object *object, > + struct dentry *dentry) > +{ > + struct inode *inode = d_backing_inode(dentry); > + bool can_use = false; > + > + if (!(inode->i_flags & S_KERNEL_FILE)) { > + inode->i_flags |= S_KERNEL_FILE; > + trace_cachefiles_mark_active(object, inode); > + can_use = true; > + } else { > + pr_notice("cachefiles: Inode already in use: %pd\n", dentry); > + } > + > + return can_use; > +} > + > +/* > + * Unmark a backing inode. The caller must hold the inode lock. > + */ > +static void __cachefiles_unmark_inode_in_use(struct cachefiles_object *object, > + struct dentry *dentry) > +{ > + struct inode *inode = d_backing_inode(dentry); > + > + inode->i_flags &= ~S_KERNEL_FILE; > + trace_cachefiles_mark_inactive(object, inode); > +} > diff --git a/fs/namei.c b/fs/namei.c > index 1f9d2187c765..d81f04f8d818 100644 > --- a/fs/namei.c > +++ b/fs/namei.c > @@ -3958,7 +3958,8 @@ int vfs_rmdir(struct user_namespace *mnt_userns, struct inode *dir, > inode_lock(dentry->d_inode); > > error = -EBUSY; > - if (is_local_mountpoint(dentry)) > + if (is_local_mountpoint(dentry) || > + (dentry->d_inode->i_flags & S_KERNEL_FILE)) Better as this check to the many other checks in may_delete() > goto out; > > error = security_inode_rmdir(dir, dentry); > diff --git a/include/linux/fs.h b/include/linux/fs.h > index 2c0b8e77d9ab..bcf1ca430139 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -2249,6 +2249,7 @@ struct super_operations { > #define S_ENCRYPTED (1 << 14) /* Encrypted file (using fs/crypto/) */ > #define S_CASEFOLD (1 << 15) /* Casefolded file */ > #define S_VERITY (1 << 16) /* Verity file (using fs/verity/) */ > +#define S_KERNEL_FILE (1 << 17) /* File is in use by the kernel (eg. fs/cachefiles) */ > Trying to brand this flag as a generic "in use by kernel" is misleading. Modules other than cachefiles cannot set/clear this flag, because then cachefiles won't know that it is allowed to set/clear the flag. So I think it would be better to call it for what it really is - an inode flag that is controlled by cachefiles. Also, the name KERNEL_FILE for a directory is a bit confusing IMO. Perhaps S_CACHEFILES? Thanks, Amir.
On Sat, Jan 08, 2022 at 09:08:57AM +0200, Amir Goldstein wrote: > > +#define S_KERNEL_FILE (1 << 17) /* File is in use by the kernel (eg. fs/cachefiles) */ > > Trying to brand this flag as a generic "in use by kernel" is misleading. > Modules other than cachefiles cannot set/clear this flag, because then > cachefiles won't know that it is allowed to set/clear the flag. Huh? If some other kernel module sets it, cachefiles will try to set it, see it's already set, and fail. Presumably cachefiles does not go round randomly "unusing" files that it has not previously started using. I mean, yes, obviously, it's a kernel module, it can set and clear whatever flags it likes on any inode in the system, but conceptually, it's an advisory whole-file lock.
Amir Goldstein <amir73il@gmail.com> wrote: > > - if (is_local_mountpoint(dentry)) > > + if (is_local_mountpoint(dentry) || > > + (dentry->d_inode->i_flags & S_KERNEL_FILE)) > > Better as this check to the many other checks in may_delete() Okay. It will make things a bit more complicated, so I'll do it in a follow up patch. The problem is that it will prevent the cachefiles driver in the kernel from renaming directories and unlinking files as it's currently removing the mark *after* moving/deleting them. > > +#define S_KERNEL_FILE (1 << 17) /* File is in use by the kernel (eg. fs/cachefiles) */ > > > > Trying to brand this flag as a generic "in use by kernel" is misleading. > Modules other than cachefiles cannot set/clear this flag, because then > cachefiles won't know that it is allowed to set/clear the flag. If the flag is set, then cachefiles thinks some other kernel driver is using the file and it shouldn't try to use it. It doesn't matter who has it open. It should never happen as other kernel drivers shouldn't be poking around inside cachefiles's cache, but possibly someone could misconfigure something. David
Matthew Wilcox <willy@infradead.org> wrote: > > Huh? If some other kernel module sets it, cachefiles will try to set it, > see it's already set, and fail. Presumably cachefiles does not go round > randomly "unusing" files that it has not previously started using. Correct. It only unuses a file that it marked in-use in the first place. David
diff --git a/fs/cachefiles/Makefile b/fs/cachefiles/Makefile index 463e3d608b75..e0b092ca077f 100644 --- a/fs/cachefiles/Makefile +++ b/fs/cachefiles/Makefile @@ -7,6 +7,7 @@ cachefiles-y := \ cache.o \ daemon.o \ main.o \ + namei.o \ security.o cachefiles-$(CONFIG_CACHEFILES_ERROR_INJECTION) += error_inject.o diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c new file mode 100644 index 000000000000..913f83f1c900 --- /dev/null +++ b/fs/cachefiles/namei.c @@ -0,0 +1,43 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* CacheFiles path walking and related routines + * + * Copyright (C) 2021 Red Hat, Inc. All Rights Reserved. + * Written by David Howells (dhowells@redhat.com) + */ + +#include <linux/fs.h> +#include "internal.h" + +/* + * Mark the backing file as being a cache file if it's not already in use. The + * mark tells the culling request command that it's not allowed to cull the + * file or directory. The caller must hold the inode lock. + */ +static bool __cachefiles_mark_inode_in_use(struct cachefiles_object *object, + struct dentry *dentry) +{ + struct inode *inode = d_backing_inode(dentry); + bool can_use = false; + + if (!(inode->i_flags & S_KERNEL_FILE)) { + inode->i_flags |= S_KERNEL_FILE; + trace_cachefiles_mark_active(object, inode); + can_use = true; + } else { + pr_notice("cachefiles: Inode already in use: %pd\n", dentry); + } + + return can_use; +} + +/* + * Unmark a backing inode. The caller must hold the inode lock. + */ +static void __cachefiles_unmark_inode_in_use(struct cachefiles_object *object, + struct dentry *dentry) +{ + struct inode *inode = d_backing_inode(dentry); + + inode->i_flags &= ~S_KERNEL_FILE; + trace_cachefiles_mark_inactive(object, inode); +} diff --git a/fs/namei.c b/fs/namei.c index 1f9d2187c765..d81f04f8d818 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -3958,7 +3958,8 @@ int vfs_rmdir(struct user_namespace *mnt_userns, struct inode *dir, inode_lock(dentry->d_inode); error = -EBUSY; - if (is_local_mountpoint(dentry)) + if (is_local_mountpoint(dentry) || + (dentry->d_inode->i_flags & S_KERNEL_FILE)) goto out; error = security_inode_rmdir(dir, dentry); diff --git a/include/linux/fs.h b/include/linux/fs.h index 2c0b8e77d9ab..bcf1ca430139 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -2249,6 +2249,7 @@ struct super_operations { #define S_ENCRYPTED (1 << 14) /* Encrypted file (using fs/crypto/) */ #define S_CASEFOLD (1 << 15) /* Casefolded file */ #define S_VERITY (1 << 16) /* Verity file (using fs/verity/) */ +#define S_KERNEL_FILE (1 << 17) /* File is in use by the kernel (eg. fs/cachefiles) */ /* * Note that nosuid etc flags are inode-specific: setting some file-system diff --git a/include/trace/events/cachefiles.h b/include/trace/events/cachefiles.h index 9bd5a8a60801..6331cd29880d 100644 --- a/include/trace/events/cachefiles.h +++ b/include/trace/events/cachefiles.h @@ -83,6 +83,48 @@ cachefiles_error_traces; #define E_(a, b) { a, b } +TRACE_EVENT(cachefiles_mark_active, + TP_PROTO(struct cachefiles_object *obj, + struct inode *inode), + + TP_ARGS(obj, inode), + + /* Note that obj may be NULL */ + TP_STRUCT__entry( + __field(unsigned int, obj ) + __field(ino_t, inode ) + ), + + TP_fast_assign( + __entry->obj = obj ? obj->debug_id : 0; + __entry->inode = inode->i_ino; + ), + + TP_printk("o=%08x i=%lx", + __entry->obj, __entry->inode) + ); + +TRACE_EVENT(cachefiles_mark_inactive, + TP_PROTO(struct cachefiles_object *obj, + struct inode *inode), + + TP_ARGS(obj, inode), + + /* Note that obj may be NULL */ + TP_STRUCT__entry( + __field(unsigned int, obj ) + __field(ino_t, inode ) + ), + + TP_fast_assign( + __entry->obj = obj ? obj->debug_id : 0; + __entry->inode = inode->i_ino; + ), + + TP_printk("o=%08x i=%lx", + __entry->obj, __entry->inode) + ); + TRACE_EVENT(cachefiles_vfs_error, TP_PROTO(struct cachefiles_object *obj, struct inode *backer, int error, enum cachefiles_error_trace where),
Use an inode flag, S_KERNEL_FILE, to mark that a backing file is in use by the kernel to prevent cachefiles or other kernel services from interfering with that file. Alter rmdir to reject attempts to remove a directory marked with this flag. This is used by cachefiles to prevent cachefilesd from removing them. Using S_SWAPFILE instead isn't really viable as that has other effects in the I/O paths. Changes ======= ver #3: - Check for the object pointer being NULL in the tracepoints rather than the caller. Signed-off-by: David Howells <dhowells@redhat.com> cc: linux-cachefs@redhat.com Link: https://lore.kernel.org/r/163819630256.215744.4815885535039369574.stgit@warthog.procyon.org.uk/ # v1 Link: https://lore.kernel.org/r/163906931596.143852.8642051223094013028.stgit@warthog.procyon.org.uk/ # v2 Link: https://lore.kernel.org/r/163967141000.1823006.12920680657559677789.stgit@warthog.procyon.org.uk/ # v3 --- fs/cachefiles/Makefile | 1 + fs/cachefiles/namei.c | 43 +++++++++++++++++++++++++++++++++++++ fs/namei.c | 3 ++- include/linux/fs.h | 1 + include/trace/events/cachefiles.h | 42 ++++++++++++++++++++++++++++++++++++ 5 files changed, 89 insertions(+), 1 deletion(-) create mode 100644 fs/cachefiles/namei.c