| Message ID | 72fc22ebeaf50fabf9d14f90f6f694f88b5fc359.1717015144.git.josef@toxicpanda.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] fs: add levels to inode write access | expand |
On Wed, 2024-05-29 at 16:41 -0400, Josef Bacik wrote: > NOTE: > This is compile tested only. It's also based on an out of tree > feature branch > from Amir that I'm extending to add page fault content events to > allow us to > have on-demand binary loading at exec time. If you need more context > please let > me know, I'll push my current branch somewhere and wire up how I plan > to use > this patch so you can see it in better context, but hopefully I've > described > what I'm trying to accomplish enough that this leads to useful > discussion. > > > Currently we have ->i_writecount to control write access to an inode. > Callers may deny write access by calling deny_write_access(), which > will > cause ->i_writecount to go negative, and allow_write_access() to push > it > back up to 0. > > This is used in a few ways, the biggest user being exec. > Historically > we've blocked write access to files that are opened for executing. > fsverity is the other notable user. > > With the upcoming fanotify feature that allows for on-demand > population > of files, this blanket policy of denying writes to files opened for > executing creates a problem. We have to issue events right before > denying access, and the entire file must be populated before we can > continue with the exec. > > This creates a problem for users who have large binaries they want to > populate on demand. Inside of Meta we often have multi-gigabyte > binaries, even on the order of tens of gigabytes. Pre-loading these > large files is costly, especially when large portions of the binary > may > never be read (think debuginfo). > > In order to facilitate on-demand loading of binaries we need to have > a > way to punch a hole in this exec related write denial. > > This patch accomplishes this by doing something similar to the freeze > levels on the super block. We have two levels, one for all write > denial, and one for exec. People wishing to deny writes will specify > the level they're denying. Users wishing to get write access must go > through all of the levels and increment each levels counter until it > increments them all, or encounters a level that is currently denied, > at > which point they undo their increments and return an error. > > Future patches will use the get_write_access_level() helper in order > to > skip the level they wish to be allowed to access. Any inode being > populated via the pre-content fanotify mechanism will be marked with > a > special flag, and the open path will use get_write_access_level() in > order to bypass the exec restriction. > > This is a significant behavior change, as it allows us to write to a > file that is currently being exec'ed. However this will be limited > to a > very narrow use case. > > Given that this is a change in behavior, should this be behind Kconfig option or a sysctl or something? > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > --- > drivers/md/md.c | 2 +- > fs/binfmt_elf.c | 4 +- > fs/exec.c | 6 +-- > fs/ext4/file.c | 4 +- > fs/inode.c | 3 +- > fs/kernel_read_file.c | 4 +- > fs/locks.c | 2 +- > fs/quota/dquot.c | 2 +- > fs/verity/enable.c | 4 +- > include/linux/fs.h | 90 +++++++++++++++++++++++++++-- > -- > include/trace/events/filelock.h | 2 +- > kernel/fork.c | 11 ++-- > security/integrity/evm/evm_main.c | 2 +- > security/integrity/ima/ima_main.c | 2 +- > 14 files changed, 104 insertions(+), 34 deletions(-) > > diff --git a/drivers/md/md.c b/drivers/md/md.c > index aff9118ff697..134cefbd7bef 100644 > --- a/drivers/md/md.c > +++ b/drivers/md/md.c > @@ -7231,7 +7231,7 @@ static int set_bitmap_file(struct mddev *mddev, > int fd) > pr_warn("%s: error: bitmap file must open > for write\n", > mdname(mddev)); > err = -EBADF; > - } else if (atomic_read(&inode->i_writecount) != 1) { > + } else if (atomic_read(&inode- > >i_writecount[INODE_DENY_WRITE_ALL]) != 1) { > pr_warn("%s: error: bitmap file is already > in use\n", > mdname(mddev)); > err = -EBUSY; > diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c > index a43897b03ce9..9a6fcf8ba17c 100644 > --- a/fs/binfmt_elf.c > +++ b/fs/binfmt_elf.c > @@ -1216,7 +1216,7 @@ static int load_elf_binary(struct linux_binprm > *bprm) > } > reloc_func_desc = interp_load_addr; > > - allow_write_access(interpreter); > + allow_write_access(interpreter, > INODE_DENY_WRITE_EXEC); > fput(interpreter); > > kfree(interp_elf_ex); > @@ -1308,7 +1308,7 @@ static int load_elf_binary(struct linux_binprm > *bprm) > kfree(interp_elf_ex); > kfree(interp_elf_phdata); > out_free_file: > - allow_write_access(interpreter); > + allow_write_access(interpreter, INODE_DENY_WRITE_EXEC); > if (interpreter) > fput(interpreter); > out_free_ph: > diff --git a/fs/exec.c b/fs/exec.c > index 18f057ba64a5..6b2900ee448d 100644 > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -971,7 +971,7 @@ static struct file *do_open_execat(int fd, struct > filename *name, int flags) > if (err) > goto exit; > > - err = deny_write_access(file); > + err = deny_write_access(file, INODE_DENY_WRITE_EXEC); > if (err) > goto exit; > > @@ -1545,7 +1545,7 @@ static void do_close_execat(struct file *file) > { > if (!file) > return; > - allow_write_access(file); > + allow_write_access(file, INODE_DENY_WRITE_EXEC); > fput(file); > } > > @@ -1865,7 +1865,7 @@ static int exec_binprm(struct linux_binprm > *bprm) > bprm->file = bprm->interpreter; > bprm->interpreter = NULL; > > - allow_write_access(exec); > + allow_write_access(exec, INODE_DENY_WRITE_EXEC); > if (unlikely(bprm->have_execfd)) { > if (bprm->executable) { > fput(exec); > diff --git a/fs/ext4/file.c b/fs/ext4/file.c > index c89e434db6b7..6196f449649c 100644 > --- a/fs/ext4/file.c > +++ b/fs/ext4/file.c > @@ -171,8 +171,8 @@ static int ext4_release_file(struct inode *inode, > struct file *filp) > } > /* if we are the last writer on the inode, drop the block > reservation */ > if ((filp->f_mode & FMODE_WRITE) && > - (atomic_read(&inode->i_writecount) == 1) && > - !EXT4_I(inode)->i_reserved_data_blocks) { > + (atomic_read(&inode->i_writecount[INODE_DENY_WRITE_ALL]) > == 1) && > + !EXT4_I(inode)->i_reserved_data_blocks) { > down_write(&EXT4_I(inode)->i_data_sem); > ext4_discard_preallocations(inode); > up_write(&EXT4_I(inode)->i_data_sem); > diff --git a/fs/inode.c b/fs/inode.c > index 3a41f83a4ba5..fb6155412252 100644 > --- a/fs/inode.c > +++ b/fs/inode.c > @@ -173,7 +173,8 @@ int inode_init_always(struct super_block *sb, > struct inode *inode) > inode->i_opflags |= IOP_XATTR; > i_uid_write(inode, 0); > i_gid_write(inode, 0); > - atomic_set(&inode->i_writecount, 0); > + for (int i = 0; i < INODE_DENY_WRITE_LEVEL; i++) > + atomic_set(&inode->i_writecount[i], 0); > inode->i_size = 0; > inode->i_write_hint = WRITE_LIFE_NOT_SET; > inode->i_blocks = 0; > diff --git a/fs/kernel_read_file.c b/fs/kernel_read_file.c > index c429c42a6867..9af82d20aa1f 100644 > --- a/fs/kernel_read_file.c > +++ b/fs/kernel_read_file.c > @@ -48,7 +48,7 @@ ssize_t kernel_read_file(struct file *file, loff_t > offset, void **buf, > if (!S_ISREG(file_inode(file)->i_mode)) > return -EINVAL; > > - ret = deny_write_access(file); > + ret = deny_write_access(file, INODE_DENY_WRITE_ALL); > if (ret) > return ret; > > @@ -119,7 +119,7 @@ ssize_t kernel_read_file(struct file *file, > loff_t offset, void **buf, > } > > out: > - allow_write_access(file); > + allow_write_access(file, INODE_DENY_WRITE_ALL); > return ret == 0 ? copied : ret; > } > EXPORT_SYMBOL_GPL(kernel_read_file); > diff --git a/fs/locks.c b/fs/locks.c > index 90c8746874de..3e6a62f56528 100644 > --- a/fs/locks.c > +++ b/fs/locks.c > @@ -1763,7 +1763,7 @@ check_conflicting_open(struct file *filp, const > int arg, int flags) > else if (filp->f_mode & FMODE_READ) > self_rcount = 1; > > - if (atomic_read(&inode->i_writecount) != self_wcount || > + if (atomic_read(&inode->i_writecount[INODE_DENY_WRITE_ALL]) > != self_wcount || > atomic_read(&inode->i_readcount) != self_rcount) > return -EAGAIN; > > diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c > index 627eb2f72ef3..fa470d76f0b3 100644 > --- a/fs/quota/dquot.c > +++ b/fs/quota/dquot.c > @@ -1031,7 +1031,7 @@ static int add_dquot_ref(struct super_block > *sb, int type) > list_for_each_entry(inode, &sb->s_inodes, i_sb_list) { > spin_lock(&inode->i_lock); > if ((inode->i_state & (I_FREEING|I_WILL_FREE|I_NEW)) > || > - !atomic_read(&inode->i_writecount) || > + !inode_is_open_for_write(inode) || > !dqinit_needed(inode, type)) { > spin_unlock(&inode->i_lock); > continue; > diff --git a/fs/verity/enable.c b/fs/verity/enable.c > index c284f46d1b53..a0e0d49c6ccc 100644 > --- a/fs/verity/enable.c > +++ b/fs/verity/enable.c > @@ -371,7 +371,7 @@ int fsverity_ioctl_enable(struct file *filp, > const void __user *uarg) > if (err) /* -EROFS */ > return err; > > - err = deny_write_access(filp); > + err = deny_write_access(filp, INODE_DENY_WRITE_ALL); > if (err) /* -ETXTBSY */ > goto out_drop_write; > > @@ -397,7 +397,7 @@ int fsverity_ioctl_enable(struct file *filp, > const void __user *uarg) > * allow_write_access() is needed to pair with > deny_write_access(). > * Regardless, the filesystem won't allow writing to verity > files. > */ > - allow_write_access(filp); > + allow_write_access(filp, INODE_DENY_WRITE_ALL); > out_drop_write: > mnt_drop_write_file(filp); > return err; > diff --git a/include/linux/fs.h b/include/linux/fs.h > index 0283cf366c2a..f0720cd0ab45 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -620,6 +620,22 @@ is_uncached_acl(struct posix_acl *acl) > #define IOP_XATTR 0x0008 > #define IOP_DEFAULT_READLINK 0x0010 > > +/* > + * These are used with the *write_access helpers. > + * > + * INODE_DENY_WRITE_ALL - Do not allow writes > at all. > + * INODE_DENY_WRITE_EXEC - Do not allow writes because > we are > + * exec'ing the file. This can > be bypassed > + * in cases where the file > needs to be > + * filled in via the fanotify > pre-content > + * hooks. > + */ > +enum inode_write_level { > + INODE_DENY_WRITE_ALL, > + INODE_DENY_WRITE_EXEC, > + INODE_DENY_WRITE_LEVEL, > +}; > + > /* > * Keep mostly read-only and often accessed (especially for > * the RCU path lookup and 'stat' data) fields at the beginning > @@ -701,7 +717,7 @@ struct inode { > atomic64_t i_sequence; /* see futex */ > atomic_t i_count; > atomic_t i_dio_count; > - atomic_t i_writecount; > + atomic_t i_writecount[INODE_DENY_WRITE_LEVEL] > ; Rather than turning this into an array, I think this would be easier to reason about as i_execcount. Then you could add a new allow/deny_exec_access(), and leave most of the existing write_access helpers alone, etc. > #if defined(CONFIG_IMA) || defined(CONFIG_FILE_LOCKING) > atomic_t i_readcount; /* struct files open RO > */ > #endif > @@ -2920,38 +2936,90 @@ static inline void kiocb_end_write(struct > kiocb *iocb) > * put_write_access() releases this write permission. > * deny_write_access() denies write access to a file. > * allow_write_access() re-enables write access to a file. > + * __get_write_access_level() gets write permission for a file for > the given > + * level. > + * put_write_access_level() releases the level write permission. > * > - * The i_writecount field of an inode can have the following values: > + * The level indicates which level we're denying writes for. Some > levels are > + * allowed to be bypassed in special circumstances. In the cases > that the write > + * level needs to be bypassed the user must use the > + * get_write_access_level()/put_write_access_level() pairs of the > helpers, which > + * will allow the user to bypass the given level, but none of the > others. > + * > + * The i_writecount[level] field of an inode can have the following > values: > * 0: no write access, no denied write access > - * < 0: (-i_writecount) users that denied write access to the file. > - * > 0: (i_writecount) users that have write access to the file. > + * < 0: (-i_writecount[level]) users that denied write access to the > file. > + * > 0: (i_writecount[level]) users that have write access to the > file. > * > * Normally we operate on that counter with atomic_{inc,dec} and > it's safe > * except for the cases where we don't hold i_writecount yet. Then > we need to > * use {get,deny}_write_access() - these functions check the sign > and refuse > * to do the change if sign is wrong. > */ > +static inline int __get_write_access(struct inode *inode, > + enum inode_write_level > skip_level) > +{ > + int i; > + > + /* We are not allowed to skip INODE_DENY_WRITE_ALL. */ > + WARN_ON_ONCE(skip_level == INODE_DENY_WRITE_ALL); > + > + for (i = 0; i < INODE_DENY_WRITE_LEVEL; i++) { > + if (i == skip_level) > + continue; > + if (!atomic_inc_unless_negative(&inode- > >i_writecount[i])) > + goto fail; > + } > + return 0; > +fail: > + while (--i >= 0) { > + if (i == skip_level) > + continue; > + atomic_dec(&inode->i_writecount[i]); > + } > + return -ETXTBSY; > +} > static inline int get_write_access(struct inode *inode) > { > - return atomic_inc_unless_negative(&inode->i_writecount) ? 0 > : -ETXTBSY; > + return __get_write_access(inode, INODE_DENY_WRITE_LEVEL); > } > -static inline int deny_write_access(struct file *file) > +static inline int get_write_access_level(struct inode *inode, > + enum inode_write_level > skip_level) > +{ > + return __get_write_access(inode, skip_level); > +} > +static inline int deny_write_access(struct file *file, > + enum inode_write_level level) > { > struct inode *inode = file_inode(file); > - return atomic_dec_unless_positive(&inode->i_writecount) ? 0 > : -ETXTBSY; > + return atomic_dec_unless_positive(&inode- > >i_writecount[level]) ? 0 : -ETXTBSY; > +} > +static inline void __put_write_access(struct inode *inode, > + enum inode_write_level > skip_level) > +{ > + for (int i = 0; i < INODE_DENY_WRITE_LEVEL; i++) { > + if (i == skip_level) > + continue; > + atomic_dec(&inode->i_writecount[i]); > + } > } > static inline void put_write_access(struct inode * inode) > { > - atomic_dec(&inode->i_writecount); > + __put_write_access(inode, INODE_DENY_WRITE_LEVEL); > } > -static inline void allow_write_access(struct file *file) > +static inline void put_write_access_level(struct inode *inode, > + enum inode_write_level > skip_level) > +{ > + __put_write_access(inode, skip_level); > +} > +static inline void allow_write_access(struct file *file, enum > inode_write_level level) > { > if (file) > - atomic_inc(&file_inode(file)->i_writecount); > + atomic_inc(&file_inode(file)->i_writecount[level]); > } > static inline bool inode_is_open_for_write(const struct inode > *inode) > { > - return atomic_read(&inode->i_writecount) > 0; > + return atomic_read(&inode- > >i_writecount[INODE_DENY_WRITE_ALL]) > 0; > } > > #if defined(CONFIG_IMA) || defined(CONFIG_FILE_LOCKING) > diff --git a/include/trace/events/filelock.h > b/include/trace/events/filelock.h > index b8d1e00a7982..ad076e87a956 100644 > --- a/include/trace/events/filelock.h > +++ b/include/trace/events/filelock.h > @@ -187,7 +187,7 @@ TRACE_EVENT(generic_add_lease, > TP_fast_assign( > __entry->s_dev = inode->i_sb->s_dev; > __entry->i_ino = inode->i_ino; > - __entry->wcount = atomic_read(&inode->i_writecount); > + __entry->wcount = atomic_read(&inode- > >i_writecount[INODE_DENY_WRITE_ALL]); > __entry->rcount = atomic_read(&inode->i_readcount); > __entry->icount = atomic_read(&inode->i_count); > __entry->owner = fl->c.flc_owner; > diff --git a/kernel/fork.c b/kernel/fork.c > index 99076dbe27d8..b6dc7aed2ebf 100644 > --- a/kernel/fork.c > +++ b/kernel/fork.c > @@ -620,7 +620,7 @@ static void dup_mm_exe_file(struct mm_struct *mm, > struct mm_struct *oldmm) > * We depend on the oldmm having properly denied write > access to the > * exe_file already. > */ > - if (exe_file && deny_write_access(exe_file)) > + if (exe_file && deny_write_access(exe_file, > INODE_DENY_WRITE_EXEC)) > pr_warn_once("deny_write_access() failed in %s\n", > __func__); > } > > @@ -1417,13 +1417,14 @@ int set_mm_exe_file(struct mm_struct *mm, > struct file *new_exe_file) > * We expect the caller (i.e., sys_execve) to > already denied > * write access, so this is unlikely to fail. > */ > - if (unlikely(deny_write_access(new_exe_file))) > + if (unlikely(deny_write_access(new_exe_file, > + > INODE_DENY_WRITE_EXEC))) > return -EACCES; > get_file(new_exe_file); > } > rcu_assign_pointer(mm->exe_file, new_exe_file); > if (old_exe_file) { > - allow_write_access(old_exe_file); > + allow_write_access(old_exe_file, > INODE_DENY_WRITE_EXEC); > fput(old_exe_file); > } > return 0; > @@ -1464,7 +1465,7 @@ int replace_mm_exe_file(struct mm_struct *mm, > struct file *new_exe_file) > return ret; > } > > - ret = deny_write_access(new_exe_file); > + ret = deny_write_access(new_exe_file, > INODE_DENY_WRITE_EXEC); > if (ret) > return -EACCES; > get_file(new_exe_file); > @@ -1476,7 +1477,7 @@ int replace_mm_exe_file(struct mm_struct *mm, > struct file *new_exe_file) > mmap_write_unlock(mm); > > if (old_exe_file) { > - allow_write_access(old_exe_file); > + allow_write_access(old_exe_file, > INODE_DENY_WRITE_EXEC); > fput(old_exe_file); > } > return 0; > diff --git a/security/integrity/evm/evm_main.c > b/security/integrity/evm/evm_main.c > index 62fe66dd53ce..b83dfb295d85 100644 > --- a/security/integrity/evm/evm_main.c > +++ b/security/integrity/evm/evm_main.c > @@ -1084,7 +1084,7 @@ static void evm_file_release(struct file *file) > if (!S_ISREG(inode->i_mode) || !(mode & FMODE_WRITE)) > return; > > - if (iint && atomic_read(&inode->i_writecount) == 1) > + if (iint && atomic_read(&inode- > >i_writecount[INODE_DENY_WRITE_ALL]) == 1) > iint->flags &= ~EVM_NEW_FILE; > } > > diff --git a/security/integrity/ima/ima_main.c > b/security/integrity/ima/ima_main.c > index f04f43af651c..7aed80a70692 100644 > --- a/security/integrity/ima/ima_main.c > +++ b/security/integrity/ima/ima_main.c > @@ -164,7 +164,7 @@ static void ima_check_last_writer(struct > ima_iint_cache *iint, > return; > > mutex_lock(&iint->mutex); > - if (atomic_read(&inode->i_writecount) == 1) { > + if (atomic_read(&inode->i_writecount[INODE_DENY_WRITE_ALL]) > == 1) { > struct kstat stat; > > update = test_and_clear_bit(IMA_UPDATE_XATTR,
On Wed, May 29, 2024 at 04:41:32PM -0400, Josef Bacik wrote: > +++ b/drivers/md/md.c > @@ -7231,7 +7231,7 @@ static int set_bitmap_file(struct mddev *mddev, int fd) > pr_warn("%s: error: bitmap file must open for write\n", > mdname(mddev)); > err = -EBADF; > - } else if (atomic_read(&inode->i_writecount) != 1) { > + } else if (atomic_read(&inode->i_writecount[INODE_DENY_WRITE_ALL]) != 1) { I think this needs an abstraction because I have no idea what this means. } else if (!write_access_ok(inode, INODE_DENY_WRITE_ALL)) { perhaps?
On Wed, May 29, 2024 at 04:41:32PM -0400, Josef Bacik wrote: > NOTE: > This is compile tested only. It's also based on an out of tree feature branch > from Amir that I'm extending to add page fault content events to allow us to > have on-demand binary loading at exec time. If you need more context please let > me know, I'll push my current branch somewhere and wire up how I plan to use > this patch so you can see it in better context, but hopefully I've described > what I'm trying to accomplish enough that this leads to useful discussion. > > > Currently we have ->i_writecount to control write access to an inode. > Callers may deny write access by calling deny_write_access(), which will > cause ->i_writecount to go negative, and allow_write_access() to push it > back up to 0. > > This is used in a few ways, the biggest user being exec. Historically > we've blocked write access to files that are opened for executing. > fsverity is the other notable user. > > With the upcoming fanotify feature that allows for on-demand population > of files, this blanket policy of denying writes to files opened for > executing creates a problem. We have to issue events right before > denying access, and the entire file must be populated before we can > continue with the exec. > > This creates a problem for users who have large binaries they want to > populate on demand. Inside of Meta we often have multi-gigabyte > binaries, even on the order of tens of gigabytes. Pre-loading these > large files is costly, especially when large portions of the binary may > never be read (think debuginfo). > > In order to facilitate on-demand loading of binaries we need to have a > way to punch a hole in this exec related write denial. Hm. I suggest we try to tackle this in a completely different way. Maybe I mentioned it during LSFMM but instead of doing this dance we should try and remove the deny_write_access() mechanisms for exec completely. Back in 2021 we removed the remaining VM_DENYWRITE bits but left in deny_write_access() for exec. Back then I had thought that this was a bit risky for not too much gain. But looking at this code here I think we now have an even stronger reason to try and get rid of this restriction. And I've since changed my mind. See my notes on the _completely untested_ RFC patch I appended. Ofc depends on whether Linus still agrees that removing this might be something we could try. > This patch accomplishes this by doing something similar to the freeze > levels on the super block. We have two levels, one for all write > denial, and one for exec. People wishing to deny writes will specify > the level they're denying. Users wishing to get write access must go > through all of the levels and increment each levels counter until it > increments them all, or encounters a level that is currently denied, at > which point they undo their increments and return an error. > > Future patches will use the get_write_access_level() helper in order to > skip the level they wish to be allowed to access. Any inode being > populated via the pre-content fanotify mechanism will be marked with a > special flag, and the open path will use get_write_access_level() in > order to bypass the exec restriction. > > This is a significant behavior change, as it allows us to write to a > file that is currently being exec'ed. However this will be limited to a > very narrow use case. > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > --- > drivers/md/md.c | 2 +- > fs/binfmt_elf.c | 4 +- > fs/exec.c | 6 +-- > fs/ext4/file.c | 4 +- > fs/inode.c | 3 +- > fs/kernel_read_file.c | 4 +- > fs/locks.c | 2 +- > fs/quota/dquot.c | 2 +- > fs/verity/enable.c | 4 +- > include/linux/fs.h | 90 +++++++++++++++++++++++++++---- > include/trace/events/filelock.h | 2 +- > kernel/fork.c | 11 ++-- > security/integrity/evm/evm_main.c | 2 +- > security/integrity/ima/ima_main.c | 2 +- > 14 files changed, 104 insertions(+), 34 deletions(-) > > diff --git a/drivers/md/md.c b/drivers/md/md.c > index aff9118ff697..134cefbd7bef 100644 > --- a/drivers/md/md.c > +++ b/drivers/md/md.c > @@ -7231,7 +7231,7 @@ static int set_bitmap_file(struct mddev *mddev, int fd) > pr_warn("%s: error: bitmap file must open for write\n", > mdname(mddev)); > err = -EBADF; > - } else if (atomic_read(&inode->i_writecount) != 1) { > + } else if (atomic_read(&inode->i_writecount[INODE_DENY_WRITE_ALL]) != 1) { > pr_warn("%s: error: bitmap file is already in use\n", > mdname(mddev)); > err = -EBUSY; > diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c > index a43897b03ce9..9a6fcf8ba17c 100644 > --- a/fs/binfmt_elf.c > +++ b/fs/binfmt_elf.c > @@ -1216,7 +1216,7 @@ static int load_elf_binary(struct linux_binprm *bprm) > } > reloc_func_desc = interp_load_addr; > > - allow_write_access(interpreter); > + allow_write_access(interpreter, INODE_DENY_WRITE_EXEC); > fput(interpreter); > > kfree(interp_elf_ex); > @@ -1308,7 +1308,7 @@ static int load_elf_binary(struct linux_binprm *bprm) > kfree(interp_elf_ex); > kfree(interp_elf_phdata); > out_free_file: > - allow_write_access(interpreter); > + allow_write_access(interpreter, INODE_DENY_WRITE_EXEC); > if (interpreter) > fput(interpreter); > out_free_ph: > diff --git a/fs/exec.c b/fs/exec.c > index 18f057ba64a5..6b2900ee448d 100644 > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -971,7 +971,7 @@ static struct file *do_open_execat(int fd, struct filename *name, int flags) > if (err) > goto exit; > > - err = deny_write_access(file); > + err = deny_write_access(file, INODE_DENY_WRITE_EXEC); > if (err) > goto exit; > > @@ -1545,7 +1545,7 @@ static void do_close_execat(struct file *file) > { > if (!file) > return; > - allow_write_access(file); > + allow_write_access(file, INODE_DENY_WRITE_EXEC); > fput(file); > } > > @@ -1865,7 +1865,7 @@ static int exec_binprm(struct linux_binprm *bprm) > bprm->file = bprm->interpreter; > bprm->interpreter = NULL; > > - allow_write_access(exec); > + allow_write_access(exec, INODE_DENY_WRITE_EXEC); > if (unlikely(bprm->have_execfd)) { > if (bprm->executable) { > fput(exec); > diff --git a/fs/ext4/file.c b/fs/ext4/file.c > index c89e434db6b7..6196f449649c 100644 > --- a/fs/ext4/file.c > +++ b/fs/ext4/file.c > @@ -171,8 +171,8 @@ static int ext4_release_file(struct inode *inode, struct file *filp) > } > /* if we are the last writer on the inode, drop the block reservation */ > if ((filp->f_mode & FMODE_WRITE) && > - (atomic_read(&inode->i_writecount) == 1) && > - !EXT4_I(inode)->i_reserved_data_blocks) { > + (atomic_read(&inode->i_writecount[INODE_DENY_WRITE_ALL]) == 1) && > + !EXT4_I(inode)->i_reserved_data_blocks) { > down_write(&EXT4_I(inode)->i_data_sem); > ext4_discard_preallocations(inode); > up_write(&EXT4_I(inode)->i_data_sem); > diff --git a/fs/inode.c b/fs/inode.c > index 3a41f83a4ba5..fb6155412252 100644 > --- a/fs/inode.c > +++ b/fs/inode.c > @@ -173,7 +173,8 @@ int inode_init_always(struct super_block *sb, struct inode *inode) > inode->i_opflags |= IOP_XATTR; > i_uid_write(inode, 0); > i_gid_write(inode, 0); > - atomic_set(&inode->i_writecount, 0); > + for (int i = 0; i < INODE_DENY_WRITE_LEVEL; i++) > + atomic_set(&inode->i_writecount[i], 0); > inode->i_size = 0; > inode->i_write_hint = WRITE_LIFE_NOT_SET; > inode->i_blocks = 0; > diff --git a/fs/kernel_read_file.c b/fs/kernel_read_file.c > index c429c42a6867..9af82d20aa1f 100644 > --- a/fs/kernel_read_file.c > +++ b/fs/kernel_read_file.c > @@ -48,7 +48,7 @@ ssize_t kernel_read_file(struct file *file, loff_t offset, void **buf, > if (!S_ISREG(file_inode(file)->i_mode)) > return -EINVAL; > > - ret = deny_write_access(file); > + ret = deny_write_access(file, INODE_DENY_WRITE_ALL); > if (ret) > return ret; > > @@ -119,7 +119,7 @@ ssize_t kernel_read_file(struct file *file, loff_t offset, void **buf, > } > > out: > - allow_write_access(file); > + allow_write_access(file, INODE_DENY_WRITE_ALL); > return ret == 0 ? copied : ret; > } > EXPORT_SYMBOL_GPL(kernel_read_file); > diff --git a/fs/locks.c b/fs/locks.c > index 90c8746874de..3e6a62f56528 100644 > --- a/fs/locks.c > +++ b/fs/locks.c > @@ -1763,7 +1763,7 @@ check_conflicting_open(struct file *filp, const int arg, int flags) > else if (filp->f_mode & FMODE_READ) > self_rcount = 1; > > - if (atomic_read(&inode->i_writecount) != self_wcount || > + if (atomic_read(&inode->i_writecount[INODE_DENY_WRITE_ALL]) != self_wcount || > atomic_read(&inode->i_readcount) != self_rcount) > return -EAGAIN; > > diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c > index 627eb2f72ef3..fa470d76f0b3 100644 > --- a/fs/quota/dquot.c > +++ b/fs/quota/dquot.c > @@ -1031,7 +1031,7 @@ static int add_dquot_ref(struct super_block *sb, int type) > list_for_each_entry(inode, &sb->s_inodes, i_sb_list) { > spin_lock(&inode->i_lock); > if ((inode->i_state & (I_FREEING|I_WILL_FREE|I_NEW)) || > - !atomic_read(&inode->i_writecount) || > + !inode_is_open_for_write(inode) || > !dqinit_needed(inode, type)) { > spin_unlock(&inode->i_lock); > continue; > diff --git a/fs/verity/enable.c b/fs/verity/enable.c > index c284f46d1b53..a0e0d49c6ccc 100644 > --- a/fs/verity/enable.c > +++ b/fs/verity/enable.c > @@ -371,7 +371,7 @@ int fsverity_ioctl_enable(struct file *filp, const void __user *uarg) > if (err) /* -EROFS */ > return err; > > - err = deny_write_access(filp); > + err = deny_write_access(filp, INODE_DENY_WRITE_ALL); > if (err) /* -ETXTBSY */ > goto out_drop_write; > > @@ -397,7 +397,7 @@ int fsverity_ioctl_enable(struct file *filp, const void __user *uarg) > * allow_write_access() is needed to pair with deny_write_access(). > * Regardless, the filesystem won't allow writing to verity files. > */ > - allow_write_access(filp); > + allow_write_access(filp, INODE_DENY_WRITE_ALL); > out_drop_write: > mnt_drop_write_file(filp); > return err; > diff --git a/include/linux/fs.h b/include/linux/fs.h > index 0283cf366c2a..f0720cd0ab45 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -620,6 +620,22 @@ is_uncached_acl(struct posix_acl *acl) > #define IOP_XATTR 0x0008 > #define IOP_DEFAULT_READLINK 0x0010 > > +/* > + * These are used with the *write_access helpers. > + * > + * INODE_DENY_WRITE_ALL - Do not allow writes at all. > + * INODE_DENY_WRITE_EXEC - Do not allow writes because we are > + * exec'ing the file. This can be bypassed > + * in cases where the file needs to be > + * filled in via the fanotify pre-content > + * hooks. > + */ > +enum inode_write_level { > + INODE_DENY_WRITE_ALL, > + INODE_DENY_WRITE_EXEC, > + INODE_DENY_WRITE_LEVEL, > +}; > + > /* > * Keep mostly read-only and often accessed (especially for > * the RCU path lookup and 'stat' data) fields at the beginning > @@ -701,7 +717,7 @@ struct inode { > atomic64_t i_sequence; /* see futex */ > atomic_t i_count; > atomic_t i_dio_count; > - atomic_t i_writecount; > + atomic_t i_writecount[INODE_DENY_WRITE_LEVEL]; > #if defined(CONFIG_IMA) || defined(CONFIG_FILE_LOCKING) > atomic_t i_readcount; /* struct files open RO */ > #endif > @@ -2920,38 +2936,90 @@ static inline void kiocb_end_write(struct kiocb *iocb) > * put_write_access() releases this write permission. > * deny_write_access() denies write access to a file. > * allow_write_access() re-enables write access to a file. > + * __get_write_access_level() gets write permission for a file for the given > + * level. > + * put_write_access_level() releases the level write permission. > * > - * The i_writecount field of an inode can have the following values: > + * The level indicates which level we're denying writes for. Some levels are > + * allowed to be bypassed in special circumstances. In the cases that the write > + * level needs to be bypassed the user must use the > + * get_write_access_level()/put_write_access_level() pairs of the helpers, which > + * will allow the user to bypass the given level, but none of the others. > + * > + * The i_writecount[level] field of an inode can have the following values: > * 0: no write access, no denied write access > - * < 0: (-i_writecount) users that denied write access to the file. > - * > 0: (i_writecount) users that have write access to the file. > + * < 0: (-i_writecount[level]) users that denied write access to the file. > + * > 0: (i_writecount[level]) users that have write access to the file. > * > * Normally we operate on that counter with atomic_{inc,dec} and it's safe > * except for the cases where we don't hold i_writecount yet. Then we need to > * use {get,deny}_write_access() - these functions check the sign and refuse > * to do the change if sign is wrong. > */ > +static inline int __get_write_access(struct inode *inode, > + enum inode_write_level skip_level) > +{ > + int i; > + > + /* We are not allowed to skip INODE_DENY_WRITE_ALL. */ > + WARN_ON_ONCE(skip_level == INODE_DENY_WRITE_ALL); > + > + for (i = 0; i < INODE_DENY_WRITE_LEVEL; i++) { > + if (i == skip_level) > + continue; > + if (!atomic_inc_unless_negative(&inode->i_writecount[i])) > + goto fail; > + } > + return 0; > +fail: > + while (--i >= 0) { > + if (i == skip_level) > + continue; > + atomic_dec(&inode->i_writecount[i]); > + } > + return -ETXTBSY; > +} > static inline int get_write_access(struct inode *inode) > { > - return atomic_inc_unless_negative(&inode->i_writecount) ? 0 : -ETXTBSY; > + return __get_write_access(inode, INODE_DENY_WRITE_LEVEL); > } > -static inline int deny_write_access(struct file *file) > +static inline int get_write_access_level(struct inode *inode, > + enum inode_write_level skip_level) > +{ > + return __get_write_access(inode, skip_level); > +} > +static inline int deny_write_access(struct file *file, > + enum inode_write_level level) > { > struct inode *inode = file_inode(file); > - return atomic_dec_unless_positive(&inode->i_writecount) ? 0 : -ETXTBSY; > + return atomic_dec_unless_positive(&inode->i_writecount[level]) ? 0 : -ETXTBSY; > +} > +static inline void __put_write_access(struct inode *inode, > + enum inode_write_level skip_level) > +{ > + for (int i = 0; i < INODE_DENY_WRITE_LEVEL; i++) { > + if (i == skip_level) > + continue; > + atomic_dec(&inode->i_writecount[i]); > + } > } > static inline void put_write_access(struct inode * inode) > { > - atomic_dec(&inode->i_writecount); > + __put_write_access(inode, INODE_DENY_WRITE_LEVEL); > } > -static inline void allow_write_access(struct file *file) > +static inline void put_write_access_level(struct inode *inode, > + enum inode_write_level skip_level) > +{ > + __put_write_access(inode, skip_level); > +} > +static inline void allow_write_access(struct file *file, enum inode_write_level level) > { > if (file) > - atomic_inc(&file_inode(file)->i_writecount); > + atomic_inc(&file_inode(file)->i_writecount[level]); > } > static inline bool inode_is_open_for_write(const struct inode *inode) > { > - return atomic_read(&inode->i_writecount) > 0; > + return atomic_read(&inode->i_writecount[INODE_DENY_WRITE_ALL]) > 0; > } > > #if defined(CONFIG_IMA) || defined(CONFIG_FILE_LOCKING) > diff --git a/include/trace/events/filelock.h b/include/trace/events/filelock.h > index b8d1e00a7982..ad076e87a956 100644 > --- a/include/trace/events/filelock.h > +++ b/include/trace/events/filelock.h > @@ -187,7 +187,7 @@ TRACE_EVENT(generic_add_lease, > TP_fast_assign( > __entry->s_dev = inode->i_sb->s_dev; > __entry->i_ino = inode->i_ino; > - __entry->wcount = atomic_read(&inode->i_writecount); > + __entry->wcount = atomic_read(&inode->i_writecount[INODE_DENY_WRITE_ALL]); > __entry->rcount = atomic_read(&inode->i_readcount); > __entry->icount = atomic_read(&inode->i_count); > __entry->owner = fl->c.flc_owner; > diff --git a/kernel/fork.c b/kernel/fork.c > index 99076dbe27d8..b6dc7aed2ebf 100644 > --- a/kernel/fork.c > +++ b/kernel/fork.c > @@ -620,7 +620,7 @@ static void dup_mm_exe_file(struct mm_struct *mm, struct mm_struct *oldmm) > * We depend on the oldmm having properly denied write access to the > * exe_file already. > */ > - if (exe_file && deny_write_access(exe_file)) > + if (exe_file && deny_write_access(exe_file, INODE_DENY_WRITE_EXEC)) > pr_warn_once("deny_write_access() failed in %s\n", __func__); > } > > @@ -1417,13 +1417,14 @@ int set_mm_exe_file(struct mm_struct *mm, struct file *new_exe_file) > * We expect the caller (i.e., sys_execve) to already denied > * write access, so this is unlikely to fail. > */ > - if (unlikely(deny_write_access(new_exe_file))) > + if (unlikely(deny_write_access(new_exe_file, > + INODE_DENY_WRITE_EXEC))) > return -EACCES; > get_file(new_exe_file); > } > rcu_assign_pointer(mm->exe_file, new_exe_file); > if (old_exe_file) { > - allow_write_access(old_exe_file); > + allow_write_access(old_exe_file, INODE_DENY_WRITE_EXEC); > fput(old_exe_file); > } > return 0; > @@ -1464,7 +1465,7 @@ int replace_mm_exe_file(struct mm_struct *mm, struct file *new_exe_file) > return ret; > } > > - ret = deny_write_access(new_exe_file); > + ret = deny_write_access(new_exe_file, INODE_DENY_WRITE_EXEC); > if (ret) > return -EACCES; > get_file(new_exe_file); > @@ -1476,7 +1477,7 @@ int replace_mm_exe_file(struct mm_struct *mm, struct file *new_exe_file) > mmap_write_unlock(mm); > > if (old_exe_file) { > - allow_write_access(old_exe_file); > + allow_write_access(old_exe_file, INODE_DENY_WRITE_EXEC); > fput(old_exe_file); > } > return 0; > diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c > index 62fe66dd53ce..b83dfb295d85 100644 > --- a/security/integrity/evm/evm_main.c > +++ b/security/integrity/evm/evm_main.c > @@ -1084,7 +1084,7 @@ static void evm_file_release(struct file *file) > if (!S_ISREG(inode->i_mode) || !(mode & FMODE_WRITE)) > return; > > - if (iint && atomic_read(&inode->i_writecount) == 1) > + if (iint && atomic_read(&inode->i_writecount[INODE_DENY_WRITE_ALL]) == 1) > iint->flags &= ~EVM_NEW_FILE; > } > > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c > index f04f43af651c..7aed80a70692 100644 > --- a/security/integrity/ima/ima_main.c > +++ b/security/integrity/ima/ima_main.c > @@ -164,7 +164,7 @@ static void ima_check_last_writer(struct ima_iint_cache *iint, > return; > > mutex_lock(&iint->mutex); > - if (atomic_read(&inode->i_writecount) == 1) { > + if (atomic_read(&inode->i_writecount[INODE_DENY_WRITE_ALL]) == 1) { > struct kstat stat; > > update = test_and_clear_bit(IMA_UPDATE_XATTR, > -- > 2.43.0 >
On Thu, May 30, 2024 at 1:32 PM Christian Brauner <brauner@kernel.org> wrote: > > On Wed, May 29, 2024 at 04:41:32PM -0400, Josef Bacik wrote: > > NOTE: > > This is compile tested only. It's also based on an out of tree feature branch > > from Amir that I'm extending to add page fault content events to allow us to > > have on-demand binary loading at exec time. If you need more context please let > > me know, I'll push my current branch somewhere and wire up how I plan to use > > this patch so you can see it in better context, but hopefully I've described > > what I'm trying to accomplish enough that this leads to useful discussion. > > > > > > Currently we have ->i_writecount to control write access to an inode. > > Callers may deny write access by calling deny_write_access(), which will > > cause ->i_writecount to go negative, and allow_write_access() to push it > > back up to 0. > > > > This is used in a few ways, the biggest user being exec. Historically > > we've blocked write access to files that are opened for executing. > > fsverity is the other notable user. > > > > With the upcoming fanotify feature that allows for on-demand population > > of files, this blanket policy of denying writes to files opened for > > executing creates a problem. We have to issue events right before > > denying access, and the entire file must be populated before we can > > continue with the exec. > > > > This creates a problem for users who have large binaries they want to > > populate on demand. Inside of Meta we often have multi-gigabyte > > binaries, even on the order of tens of gigabytes. Pre-loading these > > large files is costly, especially when large portions of the binary may > > never be read (think debuginfo). > > > > In order to facilitate on-demand loading of binaries we need to have a > > way to punch a hole in this exec related write denial. > > Hm. I suggest we try to tackle this in a completely different way. Maybe > I mentioned it during LSFMM but instead of doing this dance we should > try and remove the deny_write_access() mechanisms for exec completely. > > Back in 2021 we removed the remaining VM_DENYWRITE bits but left in > deny_write_access() for exec. Back then I had thought that this was a > bit risky for not too much gain. But looking at this code here I think > we now have an even stronger reason to try and get rid of this > restriction. And I've since changed my mind. See my notes on the > _completely untested_ RFC patch I appended. > This looks much simpler :) Obviously, a proper patch would need to also turn get_write_access() to void and clean up the error handling code of its callers. > Ofc depends on whether Linus still agrees that removing this might be > something we could try. > Crossing my fingers that this will be acceptable. Thanks, Amir.
On Thu, May 30, 2024 at 12:32:06PM +0200, Christian Brauner wrote: > On Wed, May 29, 2024 at 04:41:32PM -0400, Josef Bacik wrote: > > NOTE: > > This is compile tested only. It's also based on an out of tree feature branch > > from Amir that I'm extending to add page fault content events to allow us to > > have on-demand binary loading at exec time. If you need more context please let > > me know, I'll push my current branch somewhere and wire up how I plan to use > > this patch so you can see it in better context, but hopefully I've described > > what I'm trying to accomplish enough that this leads to useful discussion. > > > > > > Currently we have ->i_writecount to control write access to an inode. > > Callers may deny write access by calling deny_write_access(), which will > > cause ->i_writecount to go negative, and allow_write_access() to push it > > back up to 0. > > > > This is used in a few ways, the biggest user being exec. Historically > > we've blocked write access to files that are opened for executing. > > fsverity is the other notable user. > > > > With the upcoming fanotify feature that allows for on-demand population > > of files, this blanket policy of denying writes to files opened for > > executing creates a problem. We have to issue events right before > > denying access, and the entire file must be populated before we can > > continue with the exec. > > > > This creates a problem for users who have large binaries they want to > > populate on demand. Inside of Meta we often have multi-gigabyte > > binaries, even on the order of tens of gigabytes. Pre-loading these > > large files is costly, especially when large portions of the binary may > > never be read (think debuginfo). > > > > In order to facilitate on-demand loading of binaries we need to have a > > way to punch a hole in this exec related write denial. > > Hm. I suggest we try to tackle this in a completely different way. Maybe > I mentioned it during LSFMM but instead of doing this dance we should > try and remove the deny_write_access() mechanisms for exec completely. > > Back in 2021 we removed the remaining VM_DENYWRITE bits but left in > deny_write_access() for exec. Back then I had thought that this was a > bit risky for not too much gain. But looking at this code here I think > we now have an even stronger reason to try and get rid of this > restriction. And I've since changed my mind. See my notes on the > _completely untested_ RFC patch I appended. > > Ofc depends on whether Linus still agrees that removing this might be > something we could try. > Muahaha you took the bait. I obviously much prefer this solution, and from what I can tell we're all pretty well in agreement about this. Turn it into a real patch and I'll happily add my reviewed-by. Thanks, Josef
On Thu, May 30, 2024 at 12:32:06PM +0200, Christian Brauner wrote: > On Wed, May 29, 2024 at 04:41:32PM -0400, Josef Bacik wrote: > > NOTE: > > This is compile tested only. It's also based on an out of tree feature branch > > from Amir that I'm extending to add page fault content events to allow us to > > have on-demand binary loading at exec time. If you need more context please let > > me know, I'll push my current branch somewhere and wire up how I plan to use > > this patch so you can see it in better context, but hopefully I've described > > what I'm trying to accomplish enough that this leads to useful discussion. > > > > > > Currently we have ->i_writecount to control write access to an inode. > > Callers may deny write access by calling deny_write_access(), which will > > cause ->i_writecount to go negative, and allow_write_access() to push it > > back up to 0. > > > > This is used in a few ways, the biggest user being exec. Historically > > we've blocked write access to files that are opened for executing. > > fsverity is the other notable user. > > > > With the upcoming fanotify feature that allows for on-demand population > > of files, this blanket policy of denying writes to files opened for > > executing creates a problem. We have to issue events right before > > denying access, and the entire file must be populated before we can > > continue with the exec. > > > > This creates a problem for users who have large binaries they want to > > populate on demand. Inside of Meta we often have multi-gigabyte > > binaries, even on the order of tens of gigabytes. Pre-loading these > > large files is costly, especially when large portions of the binary may > > never be read (think debuginfo). > > > > In order to facilitate on-demand loading of binaries we need to have a > > way to punch a hole in this exec related write denial. > > Hm. I suggest we try to tackle this in a completely different way. Maybe > I mentioned it during LSFMM but instead of doing this dance we should > try and remove the deny_write_access() mechanisms for exec completely. > > Back in 2021 we removed the remaining VM_DENYWRITE bits but left in > deny_write_access() for exec. Back then I had thought that this was a > bit risky for not too much gain. But looking at this code here I think > we now have an even stronger reason to try and get rid of this > restriction. And I've since changed my mind. See my notes on the > _completely untested_ RFC patch I appended. > > Ofc depends on whether Linus still agrees that removing this might be > something we could try. (Ignore point (4) on my notes list. I somehow forgot to remove this as it's obviously nonsense.) > > > This patch accomplishes this by doing something similar to the freeze > > levels on the super block. We have two levels, one for all write > > denial, and one for exec. People wishing to deny writes will specify > > the level they're denying. Users wishing to get write access must go > > through all of the levels and increment each levels counter until it > > increments them all, or encounters a level that is currently denied, at > > which point they undo their increments and return an error. > > > > Future patches will use the get_write_access_level() helper in order to > > skip the level they wish to be allowed to access. Any inode being > > populated via the pre-content fanotify mechanism will be marked with a > > special flag, and the open path will use get_write_access_level() in > > order to bypass the exec restriction. > > > > This is a significant behavior change, as it allows us to write to a > > file that is currently being exec'ed. However this will be limited to a > > very narrow use case. > > > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > > --- > > drivers/md/md.c | 2 +- > > fs/binfmt_elf.c | 4 +- > > fs/exec.c | 6 +-- > > fs/ext4/file.c | 4 +- > > fs/inode.c | 3 +- > > fs/kernel_read_file.c | 4 +- > > fs/locks.c | 2 +- > > fs/quota/dquot.c | 2 +- > > fs/verity/enable.c | 4 +- > > include/linux/fs.h | 90 +++++++++++++++++++++++++++---- > > include/trace/events/filelock.h | 2 +- > > kernel/fork.c | 11 ++-- > > security/integrity/evm/evm_main.c | 2 +- > > security/integrity/ima/ima_main.c | 2 +- > > 14 files changed, 104 insertions(+), 34 deletions(-) > > > > diff --git a/drivers/md/md.c b/drivers/md/md.c > > index aff9118ff697..134cefbd7bef 100644 > > --- a/drivers/md/md.c > > +++ b/drivers/md/md.c > > @@ -7231,7 +7231,7 @@ static int set_bitmap_file(struct mddev *mddev, int fd) > > pr_warn("%s: error: bitmap file must open for write\n", > > mdname(mddev)); > > err = -EBADF; > > - } else if (atomic_read(&inode->i_writecount) != 1) { > > + } else if (atomic_read(&inode->i_writecount[INODE_DENY_WRITE_ALL]) != 1) { > > pr_warn("%s: error: bitmap file is already in use\n", > > mdname(mddev)); > > err = -EBUSY; > > diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c > > index a43897b03ce9..9a6fcf8ba17c 100644 > > --- a/fs/binfmt_elf.c > > +++ b/fs/binfmt_elf.c > > @@ -1216,7 +1216,7 @@ static int load_elf_binary(struct linux_binprm *bprm) > > } > > reloc_func_desc = interp_load_addr; > > > > - allow_write_access(interpreter); > > + allow_write_access(interpreter, INODE_DENY_WRITE_EXEC); > > fput(interpreter); > > > > kfree(interp_elf_ex); > > @@ -1308,7 +1308,7 @@ static int load_elf_binary(struct linux_binprm *bprm) > > kfree(interp_elf_ex); > > kfree(interp_elf_phdata); > > out_free_file: > > - allow_write_access(interpreter); > > + allow_write_access(interpreter, INODE_DENY_WRITE_EXEC); > > if (interpreter) > > fput(interpreter); > > out_free_ph: > > diff --git a/fs/exec.c b/fs/exec.c > > index 18f057ba64a5..6b2900ee448d 100644 > > --- a/fs/exec.c > > +++ b/fs/exec.c > > @@ -971,7 +971,7 @@ static struct file *do_open_execat(int fd, struct filename *name, int flags) > > if (err) > > goto exit; > > > > - err = deny_write_access(file); > > + err = deny_write_access(file, INODE_DENY_WRITE_EXEC); > > if (err) > > goto exit; > > > > @@ -1545,7 +1545,7 @@ static void do_close_execat(struct file *file) > > { > > if (!file) > > return; > > - allow_write_access(file); > > + allow_write_access(file, INODE_DENY_WRITE_EXEC); > > fput(file); > > } > > > > @@ -1865,7 +1865,7 @@ static int exec_binprm(struct linux_binprm *bprm) > > bprm->file = bprm->interpreter; > > bprm->interpreter = NULL; > > > > - allow_write_access(exec); > > + allow_write_access(exec, INODE_DENY_WRITE_EXEC); > > if (unlikely(bprm->have_execfd)) { > > if (bprm->executable) { > > fput(exec); > > diff --git a/fs/ext4/file.c b/fs/ext4/file.c > > index c89e434db6b7..6196f449649c 100644 > > --- a/fs/ext4/file.c > > +++ b/fs/ext4/file.c > > @@ -171,8 +171,8 @@ static int ext4_release_file(struct inode *inode, struct file *filp) > > } > > /* if we are the last writer on the inode, drop the block reservation */ > > if ((filp->f_mode & FMODE_WRITE) && > > - (atomic_read(&inode->i_writecount) == 1) && > > - !EXT4_I(inode)->i_reserved_data_blocks) { > > + (atomic_read(&inode->i_writecount[INODE_DENY_WRITE_ALL]) == 1) && > > + !EXT4_I(inode)->i_reserved_data_blocks) { > > down_write(&EXT4_I(inode)->i_data_sem); > > ext4_discard_preallocations(inode); > > up_write(&EXT4_I(inode)->i_data_sem); > > diff --git a/fs/inode.c b/fs/inode.c > > index 3a41f83a4ba5..fb6155412252 100644 > > --- a/fs/inode.c > > +++ b/fs/inode.c > > @@ -173,7 +173,8 @@ int inode_init_always(struct super_block *sb, struct inode *inode) > > inode->i_opflags |= IOP_XATTR; > > i_uid_write(inode, 0); > > i_gid_write(inode, 0); > > - atomic_set(&inode->i_writecount, 0); > > + for (int i = 0; i < INODE_DENY_WRITE_LEVEL; i++) > > + atomic_set(&inode->i_writecount[i], 0); > > inode->i_size = 0; > > inode->i_write_hint = WRITE_LIFE_NOT_SET; > > inode->i_blocks = 0; > > diff --git a/fs/kernel_read_file.c b/fs/kernel_read_file.c > > index c429c42a6867..9af82d20aa1f 100644 > > --- a/fs/kernel_read_file.c > > +++ b/fs/kernel_read_file.c > > @@ -48,7 +48,7 @@ ssize_t kernel_read_file(struct file *file, loff_t offset, void **buf, > > if (!S_ISREG(file_inode(file)->i_mode)) > > return -EINVAL; > > > > - ret = deny_write_access(file); > > + ret = deny_write_access(file, INODE_DENY_WRITE_ALL); > > if (ret) > > return ret; > > > > @@ -119,7 +119,7 @@ ssize_t kernel_read_file(struct file *file, loff_t offset, void **buf, > > } > > > > out: > > - allow_write_access(file); > > + allow_write_access(file, INODE_DENY_WRITE_ALL); > > return ret == 0 ? copied : ret; > > } > > EXPORT_SYMBOL_GPL(kernel_read_file); > > diff --git a/fs/locks.c b/fs/locks.c > > index 90c8746874de..3e6a62f56528 100644 > > --- a/fs/locks.c > > +++ b/fs/locks.c > > @@ -1763,7 +1763,7 @@ check_conflicting_open(struct file *filp, const int arg, int flags) > > else if (filp->f_mode & FMODE_READ) > > self_rcount = 1; > > > > - if (atomic_read(&inode->i_writecount) != self_wcount || > > + if (atomic_read(&inode->i_writecount[INODE_DENY_WRITE_ALL]) != self_wcount || > > atomic_read(&inode->i_readcount) != self_rcount) > > return -EAGAIN; > > > > diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c > > index 627eb2f72ef3..fa470d76f0b3 100644 > > --- a/fs/quota/dquot.c > > +++ b/fs/quota/dquot.c > > @@ -1031,7 +1031,7 @@ static int add_dquot_ref(struct super_block *sb, int type) > > list_for_each_entry(inode, &sb->s_inodes, i_sb_list) { > > spin_lock(&inode->i_lock); > > if ((inode->i_state & (I_FREEING|I_WILL_FREE|I_NEW)) || > > - !atomic_read(&inode->i_writecount) || > > + !inode_is_open_for_write(inode) || > > !dqinit_needed(inode, type)) { > > spin_unlock(&inode->i_lock); > > continue; > > diff --git a/fs/verity/enable.c b/fs/verity/enable.c > > index c284f46d1b53..a0e0d49c6ccc 100644 > > --- a/fs/verity/enable.c > > +++ b/fs/verity/enable.c > > @@ -371,7 +371,7 @@ int fsverity_ioctl_enable(struct file *filp, const void __user *uarg) > > if (err) /* -EROFS */ > > return err; > > > > - err = deny_write_access(filp); > > + err = deny_write_access(filp, INODE_DENY_WRITE_ALL); > > if (err) /* -ETXTBSY */ > > goto out_drop_write; > > > > @@ -397,7 +397,7 @@ int fsverity_ioctl_enable(struct file *filp, const void __user *uarg) > > * allow_write_access() is needed to pair with deny_write_access(). > > * Regardless, the filesystem won't allow writing to verity files. > > */ > > - allow_write_access(filp); > > + allow_write_access(filp, INODE_DENY_WRITE_ALL); > > out_drop_write: > > mnt_drop_write_file(filp); > > return err; > > diff --git a/include/linux/fs.h b/include/linux/fs.h > > index 0283cf366c2a..f0720cd0ab45 100644 > > --- a/include/linux/fs.h > > +++ b/include/linux/fs.h > > @@ -620,6 +620,22 @@ is_uncached_acl(struct posix_acl *acl) > > #define IOP_XATTR 0x0008 > > #define IOP_DEFAULT_READLINK 0x0010 > > > > +/* > > + * These are used with the *write_access helpers. > > + * > > + * INODE_DENY_WRITE_ALL - Do not allow writes at all. > > + * INODE_DENY_WRITE_EXEC - Do not allow writes because we are > > + * exec'ing the file. This can be bypassed > > + * in cases where the file needs to be > > + * filled in via the fanotify pre-content > > + * hooks. > > + */ > > +enum inode_write_level { > > + INODE_DENY_WRITE_ALL, > > + INODE_DENY_WRITE_EXEC, > > + INODE_DENY_WRITE_LEVEL, > > +}; > > + > > /* > > * Keep mostly read-only and often accessed (especially for > > * the RCU path lookup and 'stat' data) fields at the beginning > > @@ -701,7 +717,7 @@ struct inode { > > atomic64_t i_sequence; /* see futex */ > > atomic_t i_count; > > atomic_t i_dio_count; > > - atomic_t i_writecount; > > + atomic_t i_writecount[INODE_DENY_WRITE_LEVEL]; > > #if defined(CONFIG_IMA) || defined(CONFIG_FILE_LOCKING) > > atomic_t i_readcount; /* struct files open RO */ > > #endif > > @@ -2920,38 +2936,90 @@ static inline void kiocb_end_write(struct kiocb *iocb) > > * put_write_access() releases this write permission. > > * deny_write_access() denies write access to a file. > > * allow_write_access() re-enables write access to a file. > > + * __get_write_access_level() gets write permission for a file for the given > > + * level. > > + * put_write_access_level() releases the level write permission. > > * > > - * The i_writecount field of an inode can have the following values: > > + * The level indicates which level we're denying writes for. Some levels are > > + * allowed to be bypassed in special circumstances. In the cases that the write > > + * level needs to be bypassed the user must use the > > + * get_write_access_level()/put_write_access_level() pairs of the helpers, which > > + * will allow the user to bypass the given level, but none of the others. > > + * > > + * The i_writecount[level] field of an inode can have the following values: > > * 0: no write access, no denied write access > > - * < 0: (-i_writecount) users that denied write access to the file. > > - * > 0: (i_writecount) users that have write access to the file. > > + * < 0: (-i_writecount[level]) users that denied write access to the file. > > + * > 0: (i_writecount[level]) users that have write access to the file. > > * > > * Normally we operate on that counter with atomic_{inc,dec} and it's safe > > * except for the cases where we don't hold i_writecount yet. Then we need to > > * use {get,deny}_write_access() - these functions check the sign and refuse > > * to do the change if sign is wrong. > > */ > > +static inline int __get_write_access(struct inode *inode, > > + enum inode_write_level skip_level) > > +{ > > + int i; > > + > > + /* We are not allowed to skip INODE_DENY_WRITE_ALL. */ > > + WARN_ON_ONCE(skip_level == INODE_DENY_WRITE_ALL); > > + > > + for (i = 0; i < INODE_DENY_WRITE_LEVEL; i++) { > > + if (i == skip_level) > > + continue; > > + if (!atomic_inc_unless_negative(&inode->i_writecount[i])) > > + goto fail; > > + } > > + return 0; > > +fail: > > + while (--i >= 0) { > > + if (i == skip_level) > > + continue; > > + atomic_dec(&inode->i_writecount[i]); > > + } > > + return -ETXTBSY; > > +} > > static inline int get_write_access(struct inode *inode) > > { > > - return atomic_inc_unless_negative(&inode->i_writecount) ? 0 : -ETXTBSY; > > + return __get_write_access(inode, INODE_DENY_WRITE_LEVEL); > > } > > -static inline int deny_write_access(struct file *file) > > +static inline int get_write_access_level(struct inode *inode, > > + enum inode_write_level skip_level) > > +{ > > + return __get_write_access(inode, skip_level); > > +} > > +static inline int deny_write_access(struct file *file, > > + enum inode_write_level level) > > { > > struct inode *inode = file_inode(file); > > - return atomic_dec_unless_positive(&inode->i_writecount) ? 0 : -ETXTBSY; > > + return atomic_dec_unless_positive(&inode->i_writecount[level]) ? 0 : -ETXTBSY; > > +} > > +static inline void __put_write_access(struct inode *inode, > > + enum inode_write_level skip_level) > > +{ > > + for (int i = 0; i < INODE_DENY_WRITE_LEVEL; i++) { > > + if (i == skip_level) > > + continue; > > + atomic_dec(&inode->i_writecount[i]); > > + } > > } > > static inline void put_write_access(struct inode * inode) > > { > > - atomic_dec(&inode->i_writecount); > > + __put_write_access(inode, INODE_DENY_WRITE_LEVEL); > > } > > -static inline void allow_write_access(struct file *file) > > +static inline void put_write_access_level(struct inode *inode, > > + enum inode_write_level skip_level) > > +{ > > + __put_write_access(inode, skip_level); > > +} > > +static inline void allow_write_access(struct file *file, enum inode_write_level level) > > { > > if (file) > > - atomic_inc(&file_inode(file)->i_writecount); > > + atomic_inc(&file_inode(file)->i_writecount[level]); > > } > > static inline bool inode_is_open_for_write(const struct inode *inode) > > { > > - return atomic_read(&inode->i_writecount) > 0; > > + return atomic_read(&inode->i_writecount[INODE_DENY_WRITE_ALL]) > 0; > > } > > > > #if defined(CONFIG_IMA) || defined(CONFIG_FILE_LOCKING) > > diff --git a/include/trace/events/filelock.h b/include/trace/events/filelock.h > > index b8d1e00a7982..ad076e87a956 100644 > > --- a/include/trace/events/filelock.h > > +++ b/include/trace/events/filelock.h > > @@ -187,7 +187,7 @@ TRACE_EVENT(generic_add_lease, > > TP_fast_assign( > > __entry->s_dev = inode->i_sb->s_dev; > > __entry->i_ino = inode->i_ino; > > - __entry->wcount = atomic_read(&inode->i_writecount); > > + __entry->wcount = atomic_read(&inode->i_writecount[INODE_DENY_WRITE_ALL]); > > __entry->rcount = atomic_read(&inode->i_readcount); > > __entry->icount = atomic_read(&inode->i_count); > > __entry->owner = fl->c.flc_owner; > > diff --git a/kernel/fork.c b/kernel/fork.c > > index 99076dbe27d8..b6dc7aed2ebf 100644 > > --- a/kernel/fork.c > > +++ b/kernel/fork.c > > @@ -620,7 +620,7 @@ static void dup_mm_exe_file(struct mm_struct *mm, struct mm_struct *oldmm) > > * We depend on the oldmm having properly denied write access to the > > * exe_file already. > > */ > > - if (exe_file && deny_write_access(exe_file)) > > + if (exe_file && deny_write_access(exe_file, INODE_DENY_WRITE_EXEC)) > > pr_warn_once("deny_write_access() failed in %s\n", __func__); > > } > > > > @@ -1417,13 +1417,14 @@ int set_mm_exe_file(struct mm_struct *mm, struct file *new_exe_file) > > * We expect the caller (i.e., sys_execve) to already denied > > * write access, so this is unlikely to fail. > > */ > > - if (unlikely(deny_write_access(new_exe_file))) > > + if (unlikely(deny_write_access(new_exe_file, > > + INODE_DENY_WRITE_EXEC))) > > return -EACCES; > > get_file(new_exe_file); > > } > > rcu_assign_pointer(mm->exe_file, new_exe_file); > > if (old_exe_file) { > > - allow_write_access(old_exe_file); > > + allow_write_access(old_exe_file, INODE_DENY_WRITE_EXEC); > > fput(old_exe_file); > > } > > return 0; > > @@ -1464,7 +1465,7 @@ int replace_mm_exe_file(struct mm_struct *mm, struct file *new_exe_file) > > return ret; > > } > > > > - ret = deny_write_access(new_exe_file); > > + ret = deny_write_access(new_exe_file, INODE_DENY_WRITE_EXEC); > > if (ret) > > return -EACCES; > > get_file(new_exe_file); > > @@ -1476,7 +1477,7 @@ int replace_mm_exe_file(struct mm_struct *mm, struct file *new_exe_file) > > mmap_write_unlock(mm); > > > > if (old_exe_file) { > > - allow_write_access(old_exe_file); > > + allow_write_access(old_exe_file, INODE_DENY_WRITE_EXEC); > > fput(old_exe_file); > > } > > return 0; > > diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c > > index 62fe66dd53ce..b83dfb295d85 100644 > > --- a/security/integrity/evm/evm_main.c > > +++ b/security/integrity/evm/evm_main.c > > @@ -1084,7 +1084,7 @@ static void evm_file_release(struct file *file) > > if (!S_ISREG(inode->i_mode) || !(mode & FMODE_WRITE)) > > return; > > > > - if (iint && atomic_read(&inode->i_writecount) == 1) > > + if (iint && atomic_read(&inode->i_writecount[INODE_DENY_WRITE_ALL]) == 1) > > iint->flags &= ~EVM_NEW_FILE; > > } > > > > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c > > index f04f43af651c..7aed80a70692 100644 > > --- a/security/integrity/ima/ima_main.c > > +++ b/security/integrity/ima/ima_main.c > > @@ -164,7 +164,7 @@ static void ima_check_last_writer(struct ima_iint_cache *iint, > > return; > > > > mutex_lock(&iint->mutex); > > - if (atomic_read(&inode->i_writecount) == 1) { > > + if (atomic_read(&inode->i_writecount[INODE_DENY_WRITE_ALL]) == 1) { > > struct kstat stat; > > > > update = test_and_clear_bit(IMA_UPDATE_XATTR, > > -- > > 2.43.0 > >
On Thu, 30 May 2024 at 03:32, Christian Brauner <brauner@kernel.org> wrote: > > Ofc depends on whether Linus still agrees that removing this might be > something we could try. I _definitely_ do not want to see any more complex deny_write_access(). So yes, if people have good reasons to override the inode write access, I'd rather remove it entirely than make it some eldritch horror that is even worse than what we have now. It would obviously have to be tested in case some odd case actually depends on the ETXTBSY semantics, since we *have* supported it for a long time. But iirc nobody even noticed when we removed it from shared libraries, so... That said, verity seems to depend on it as a way to do the "enable_verity()" atomically with no concurrent writes, and I see some i_writecount noise in the integrity code too. But maybe that's just a belt-and-suspenders thing? Because if execve() no longer does it, I think we should just remove that i_writecount thing entirely. Linus
On Thu, May 30, 2024 at 08:49:12AM -0700, Linus Torvalds wrote: > On Thu, 30 May 2024 at 03:32, Christian Brauner <brauner@kernel.org> wrote: > > > > Ofc depends on whether Linus still agrees that removing this might be > > something we could try. > > I _definitely_ do not want to see any more complex deny_write_access(). > > So yes, if people have good reasons to override the inode write > access, I'd rather remove it entirely than make it some eldritch > horror that is even worse than what we have now. > > It would obviously have to be tested in case some odd case actually > depends on the ETXTBSY semantics, since we *have* supported it for a > long time. But iirc nobody even noticed when we removed it from > shared libraries, so... > > That said, verity seems to depend on it as a way to do the > "enable_verity()" atomically with no concurrent writes, and I see some > i_writecount noise in the integrity code too. > > But maybe that's just a belt-and-suspenders thing? > > Because if execve() no longer does it, I think we should just remove > that i_writecount thing entirely. deny_write_access() is being used from kernel_read_file() which has a few wrappers around it and they are used in various places: (1) kernel_read_file() based helpers: (1.1) kernel_read_file_from_path() (1.2) kernel_read_file_from_path_initns() (1.3) kernel_read_file_from_fd() (2) kernel_read_file() users: (2.1) kernel/module/main.c:init_module_from_file() (2.2) security/loadpin/loadpin.c:read_trusted_verity_root_digests() (3) kernel_read_file_from_path() users: (3.1) security/integrity/digsig.c:integrity_load_x509() (3.2) security/integrity/ima/ima_fs.c:ima_read_busy() (4) kernel_read_file_from_path_initns() users: (4.1) drivers/base/firmware_loader/main.c:fw_get_filesystem_firmware() (5) kernel_read_file_from_fd() users: (5.1) kernel/kexec_file.c:kimage_file_prepare_segments() In order to remove i_writecount completely we would need to do this in multiple steps as some of that stuff seems potentially sensitive. The exec deny write mechanism can be removed because we have a decent understanding of the implications and there's decent justification for removing it. So I propose that I do various testing (LTP) etc. now, send the patch and then put this into -next to see if anything breaks?
On Fri, May 31, 2024 at 12:02:16PM +0200, Christian Brauner wrote: > On Thu, May 30, 2024 at 08:49:12AM -0700, Linus Torvalds wrote: > > On Thu, 30 May 2024 at 03:32, Christian Brauner <brauner@kernel.org> wrote: > > > > > > Ofc depends on whether Linus still agrees that removing this might be > > > something we could try. > > > > I _definitely_ do not want to see any more complex deny_write_access(). > > > > So yes, if people have good reasons to override the inode write > > access, I'd rather remove it entirely than make it some eldritch > > horror that is even worse than what we have now. > > > > It would obviously have to be tested in case some odd case actually > > depends on the ETXTBSY semantics, since we *have* supported it for a > > long time. But iirc nobody even noticed when we removed it from > > shared libraries, so... > > > > That said, verity seems to depend on it as a way to do the > > "enable_verity()" atomically with no concurrent writes, and I see some > > i_writecount noise in the integrity code too. > > > > But maybe that's just a belt-and-suspenders thing? > > > > Because if execve() no longer does it, I think we should just remove > > that i_writecount thing entirely. > > deny_write_access() is being used from kernel_read_file() which has a > few wrappers around it and they are used in various places: > > (1) kernel_read_file() based helpers: > (1.1) kernel_read_file_from_path() > (1.2) kernel_read_file_from_path_initns() > (1.3) kernel_read_file_from_fd() > > (2) kernel_read_file() users: > (2.1) kernel/module/main.c:init_module_from_file() > (2.2) security/loadpin/loadpin.c:read_trusted_verity_root_digests() > > (3) kernel_read_file_from_path() users: > (3.1) security/integrity/digsig.c:integrity_load_x509() > (3.2) security/integrity/ima/ima_fs.c:ima_read_busy() > > (4) kernel_read_file_from_path_initns() users: > (4.1) drivers/base/firmware_loader/main.c:fw_get_filesystem_firmware() > > (5) kernel_read_file_from_fd() users: > (5.1) kernel/kexec_file.c:kimage_file_prepare_segments() > > In order to remove i_writecount completely we would need to do this in Sorry, typo s/i_write_count/deny_write_access()/g (I don't think we can remove i_writecount itself as it's used for file leases and locks.) > multiple steps as some of that stuff seems potentially sensitive. > > The exec deny write mechanism can be removed because we have a decent > understanding of the implications and there's decent justification for > removing it. > > So I propose that I do various testing (LTP) etc. now, send the patch > and then put this into -next to see if anything breaks?
On Fri, May 31, 2024 at 3:32 PM Christian Brauner <brauner@kernel.org> wrote: > > On Fri, May 31, 2024 at 12:02:16PM +0200, Christian Brauner wrote: > > On Thu, May 30, 2024 at 08:49:12AM -0700, Linus Torvalds wrote: > > > On Thu, 30 May 2024 at 03:32, Christian Brauner <brauner@kernel.org> wrote: > > > > > > > > Ofc depends on whether Linus still agrees that removing this might be > > > > something we could try. > > > > > > I _definitely_ do not want to see any more complex deny_write_access(). > > > > > > So yes, if people have good reasons to override the inode write > > > access, I'd rather remove it entirely than make it some eldritch > > > horror that is even worse than what we have now. > > > > > > It would obviously have to be tested in case some odd case actually > > > depends on the ETXTBSY semantics, since we *have* supported it for a > > > long time. But iirc nobody even noticed when we removed it from > > > shared libraries, so... > > > > > > That said, verity seems to depend on it as a way to do the > > > "enable_verity()" atomically with no concurrent writes, and I see some > > > i_writecount noise in the integrity code too. This one is a bit more challenging. The IMA ima_bprm_check() LSM hook (called from exec_binprm() context) may read the file (in ima_collect_measurement()) and record the signature of the file to be executed, assuming that it cannot be modified. Not sure how to deal with this expectation. Only thing I could think of is that IMA would be allowed to deny_write_access() and set FMODE_EXEC_DENY_WRITE as a hint for do_close_execat() to allow_write_access(), but it's pretty ugly, I admit. > > > > > > But maybe that's just a belt-and-suspenders thing? > > > > > > Because if execve() no longer does it, I think we should just remove > > > that i_writecount thing entirely. > > > > deny_write_access() is being used from kernel_read_file() which has a > > few wrappers around it and they are used in various places: > > > > (1) kernel_read_file() based helpers: > > (1.1) kernel_read_file_from_path() > > (1.2) kernel_read_file_from_path_initns() > > (1.3) kernel_read_file_from_fd() > > > > (2) kernel_read_file() users: > > (2.1) kernel/module/main.c:init_module_from_file() > > (2.2) security/loadpin/loadpin.c:read_trusted_verity_root_digests() > > > > (3) kernel_read_file_from_path() users: > > (3.1) security/integrity/digsig.c:integrity_load_x509() > > (3.2) security/integrity/ima/ima_fs.c:ima_read_busy() > > > > (4) kernel_read_file_from_path_initns() users: > > (4.1) drivers/base/firmware_loader/main.c:fw_get_filesystem_firmware() > > > > (5) kernel_read_file_from_fd() users: > > (5.1) kernel/kexec_file.c:kimage_file_prepare_segments() > > > > In order to remove i_writecount completely we would need to do this in > > Sorry, typo s/i_write_count/deny_write_access()/g > (I don't think we can remove i_writecount itself as it's used for file > leases and locks.) Indeed, i_writecount (as does i_readcount) is used by fs/locks.c: check_conflicting_open(), but not as a synchronization primitive. > > > multiple steps as some of that stuff seems potentially sensitive. > > > > The exec deny write mechanism can be removed because we have a decent > > understanding of the implications and there's decent justification for > > removing it. > > > > So I propose that I do various testing (LTP) etc. now, send the patch > > and then put this into -next to see if anything breaks? Wouldn't hurt to see what else we are missing. Thanks, Amir.
On Fri, May 31, 2024 at 04:09:17PM +0300, Amir Goldstein wrote: > On Fri, May 31, 2024 at 3:32 PM Christian Brauner <brauner@kernel.org> wrote: > > > > On Fri, May 31, 2024 at 12:02:16PM +0200, Christian Brauner wrote: > > > On Thu, May 30, 2024 at 08:49:12AM -0700, Linus Torvalds wrote: > > > > On Thu, 30 May 2024 at 03:32, Christian Brauner <brauner@kernel.org> wrote: > > > > > > > > > > Ofc depends on whether Linus still agrees that removing this might be > > > > > something we could try. > > > > > > > > I _definitely_ do not want to see any more complex deny_write_access(). > > > > > > > > So yes, if people have good reasons to override the inode write > > > > access, I'd rather remove it entirely than make it some eldritch > > > > horror that is even worse than what we have now. > > > > > > > > It would obviously have to be tested in case some odd case actually > > > > depends on the ETXTBSY semantics, since we *have* supported it for a > > > > long time. But iirc nobody even noticed when we removed it from > > > > shared libraries, so... > > > > > > > > That said, verity seems to depend on it as a way to do the > > > > "enable_verity()" atomically with no concurrent writes, and I see some > > > > i_writecount noise in the integrity code too. > > This one is a bit more challenging. > The IMA ima_bprm_check() LSM hook (called from exec_binprm() context) > may read the file (in ima_collect_measurement()) and record the signature > of the file to be executed, assuming that it cannot be modified. > Not sure how to deal with this expectation. This is very annoying. And it probably doesn't work for dynamic binaries. 90% of the code will be in libraries to which one can happily write. Plus there's various attacks to break this: https://svs.informatik.uni-hamburg.de/publications/2020/2020-08-27-Bohling-IMA.pdf Anyway, I really really don't want to add more complex deny_write_access() either. It's hard to understand, it's hard to document and it punches holes into this anyway. The freezing levels are annoying enough already. So then I propose we just make the deny write stuff during exec conditional on IMA being active. At the end it's small- vs chicken pox. (I figure it won't be enough for IMA to read the executable after it has been mapped MS_PRIVATE?) > Only thing I could think of is that IMA would be allowed to > deny_write_access() and set FMODE_EXEC_DENY_WRITE > as a hint for do_close_execat() to allow_write_access(), but > it's pretty ugly, I admit. > > > > > > > > > But maybe that's just a belt-and-suspenders thing? > > > > > > > > Because if execve() no longer does it, I think we should just remove > > > > that i_writecount thing entirely. > > > > > > deny_write_access() is being used from kernel_read_file() which has a > > > few wrappers around it and they are used in various places: > > > > > > (1) kernel_read_file() based helpers: > > > (1.1) kernel_read_file_from_path() > > > (1.2) kernel_read_file_from_path_initns() > > > (1.3) kernel_read_file_from_fd() > > > > > > (2) kernel_read_file() users: > > > (2.1) kernel/module/main.c:init_module_from_file() > > > (2.2) security/loadpin/loadpin.c:read_trusted_verity_root_digests() > > > > > > (3) kernel_read_file_from_path() users: > > > (3.1) security/integrity/digsig.c:integrity_load_x509() > > > (3.2) security/integrity/ima/ima_fs.c:ima_read_busy() > > > > > > (4) kernel_read_file_from_path_initns() users: > > > (4.1) drivers/base/firmware_loader/main.c:fw_get_filesystem_firmware() > > > > > > (5) kernel_read_file_from_fd() users: > > > (5.1) kernel/kexec_file.c:kimage_file_prepare_segments() > > > > > > In order to remove i_writecount completely we would need to do this in > > > > Sorry, typo s/i_write_count/deny_write_access()/g > > (I don't think we can remove i_writecount itself as it's used for file > > leases and locks.) > > Indeed, i_writecount (as does i_readcount) is used by fs/locks.c: > check_conflicting_open(), but not as a synchronization primitive. > > > > > > multiple steps as some of that stuff seems potentially sensitive. > > > > > > The exec deny write mechanism can be removed because we have a decent > > > understanding of the implications and there's decent justification for > > > removing it. > > > > > > So I propose that I do various testing (LTP) etc. now, send the patch > > > and then put this into -next to see if anything breaks? > > Wouldn't hurt to see what else we are missing. > > Thanks, > Amir.
On Fri, May 31, 2024 at 04:50:16PM +0200, Christian Brauner wrote: > So then I propose we just make the deny write stuff during exec > conditional on IMA being active. At the end it's small- vs chicken pox. > > (I figure it won't be enough for IMA to read the executable after it has > been mapped MS_PRIVATE?) do you mean MAP_PRIVATE? If so, you have a misapprehension. We can change the contents of the pagecache after MAP_PRIVATE and that will not cause COW. COW only occurs if someone writes through a MAP_PRIVATE.
On Fri, May 31, 2024 at 04:47:20PM +0100, Matthew Wilcox wrote: > On Fri, May 31, 2024 at 04:50:16PM +0200, Christian Brauner wrote: > > So then I propose we just make the deny write stuff during exec > > conditional on IMA being active. At the end it's small- vs chicken pox. > > > > (I figure it won't be enough for IMA to read the executable after it has > > been mapped MS_PRIVATE?) > > do you mean MAP_PRIVATE? > > If so, you have a misapprehension. We can change the contents of the > pagecache after MAP_PRIVATE and that will not cause COW. COW only > occurs if someone writes through a MAP_PRIVATE. If IMA does want to prevent writes, I suggest it puts a lease on the file. That's a mechanism that all writes must honour, rather than it being an IMA speciality.
diff --git a/drivers/md/md.c b/drivers/md/md.c index aff9118ff697..134cefbd7bef 100644 --- a/drivers/md/md.c +++ b/drivers/md/md.c @@ -7231,7 +7231,7 @@ static int set_bitmap_file(struct mddev *mddev, int fd) pr_warn("%s: error: bitmap file must open for write\n", mdname(mddev)); err = -EBADF; - } else if (atomic_read(&inode->i_writecount) != 1) { + } else if (atomic_read(&inode->i_writecount[INODE_DENY_WRITE_ALL]) != 1) { pr_warn("%s: error: bitmap file is already in use\n", mdname(mddev)); err = -EBUSY; diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c index a43897b03ce9..9a6fcf8ba17c 100644 --- a/fs/binfmt_elf.c +++ b/fs/binfmt_elf.c @@ -1216,7 +1216,7 @@ static int load_elf_binary(struct linux_binprm *bprm) } reloc_func_desc = interp_load_addr; - allow_write_access(interpreter); + allow_write_access(interpreter, INODE_DENY_WRITE_EXEC); fput(interpreter); kfree(interp_elf_ex); @@ -1308,7 +1308,7 @@ static int load_elf_binary(struct linux_binprm *bprm) kfree(interp_elf_ex); kfree(interp_elf_phdata); out_free_file: - allow_write_access(interpreter); + allow_write_access(interpreter, INODE_DENY_WRITE_EXEC); if (interpreter) fput(interpreter); out_free_ph: diff --git a/fs/exec.c b/fs/exec.c index 18f057ba64a5..6b2900ee448d 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -971,7 +971,7 @@ static struct file *do_open_execat(int fd, struct filename *name, int flags) if (err) goto exit; - err = deny_write_access(file); + err = deny_write_access(file, INODE_DENY_WRITE_EXEC); if (err) goto exit; @@ -1545,7 +1545,7 @@ static void do_close_execat(struct file *file) { if (!file) return; - allow_write_access(file); + allow_write_access(file, INODE_DENY_WRITE_EXEC); fput(file); } @@ -1865,7 +1865,7 @@ static int exec_binprm(struct linux_binprm *bprm) bprm->file = bprm->interpreter; bprm->interpreter = NULL; - allow_write_access(exec); + allow_write_access(exec, INODE_DENY_WRITE_EXEC); if (unlikely(bprm->have_execfd)) { if (bprm->executable) { fput(exec); diff --git a/fs/ext4/file.c b/fs/ext4/file.c index c89e434db6b7..6196f449649c 100644 --- a/fs/ext4/file.c +++ b/fs/ext4/file.c @@ -171,8 +171,8 @@ static int ext4_release_file(struct inode *inode, struct file *filp) } /* if we are the last writer on the inode, drop the block reservation */ if ((filp->f_mode & FMODE_WRITE) && - (atomic_read(&inode->i_writecount) == 1) && - !EXT4_I(inode)->i_reserved_data_blocks) { + (atomic_read(&inode->i_writecount[INODE_DENY_WRITE_ALL]) == 1) && + !EXT4_I(inode)->i_reserved_data_blocks) { down_write(&EXT4_I(inode)->i_data_sem); ext4_discard_preallocations(inode); up_write(&EXT4_I(inode)->i_data_sem); diff --git a/fs/inode.c b/fs/inode.c index 3a41f83a4ba5..fb6155412252 100644 --- a/fs/inode.c +++ b/fs/inode.c @@ -173,7 +173,8 @@ int inode_init_always(struct super_block *sb, struct inode *inode) inode->i_opflags |= IOP_XATTR; i_uid_write(inode, 0); i_gid_write(inode, 0); - atomic_set(&inode->i_writecount, 0); + for (int i = 0; i < INODE_DENY_WRITE_LEVEL; i++) + atomic_set(&inode->i_writecount[i], 0); inode->i_size = 0; inode->i_write_hint = WRITE_LIFE_NOT_SET; inode->i_blocks = 0; diff --git a/fs/kernel_read_file.c b/fs/kernel_read_file.c index c429c42a6867..9af82d20aa1f 100644 --- a/fs/kernel_read_file.c +++ b/fs/kernel_read_file.c @@ -48,7 +48,7 @@ ssize_t kernel_read_file(struct file *file, loff_t offset, void **buf, if (!S_ISREG(file_inode(file)->i_mode)) return -EINVAL; - ret = deny_write_access(file); + ret = deny_write_access(file, INODE_DENY_WRITE_ALL); if (ret) return ret; @@ -119,7 +119,7 @@ ssize_t kernel_read_file(struct file *file, loff_t offset, void **buf, } out: - allow_write_access(file); + allow_write_access(file, INODE_DENY_WRITE_ALL); return ret == 0 ? copied : ret; } EXPORT_SYMBOL_GPL(kernel_read_file); diff --git a/fs/locks.c b/fs/locks.c index 90c8746874de..3e6a62f56528 100644 --- a/fs/locks.c +++ b/fs/locks.c @@ -1763,7 +1763,7 @@ check_conflicting_open(struct file *filp, const int arg, int flags) else if (filp->f_mode & FMODE_READ) self_rcount = 1; - if (atomic_read(&inode->i_writecount) != self_wcount || + if (atomic_read(&inode->i_writecount[INODE_DENY_WRITE_ALL]) != self_wcount || atomic_read(&inode->i_readcount) != self_rcount) return -EAGAIN; diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c index 627eb2f72ef3..fa470d76f0b3 100644 --- a/fs/quota/dquot.c +++ b/fs/quota/dquot.c @@ -1031,7 +1031,7 @@ static int add_dquot_ref(struct super_block *sb, int type) list_for_each_entry(inode, &sb->s_inodes, i_sb_list) { spin_lock(&inode->i_lock); if ((inode->i_state & (I_FREEING|I_WILL_FREE|I_NEW)) || - !atomic_read(&inode->i_writecount) || + !inode_is_open_for_write(inode) || !dqinit_needed(inode, type)) { spin_unlock(&inode->i_lock); continue; diff --git a/fs/verity/enable.c b/fs/verity/enable.c index c284f46d1b53..a0e0d49c6ccc 100644 --- a/fs/verity/enable.c +++ b/fs/verity/enable.c @@ -371,7 +371,7 @@ int fsverity_ioctl_enable(struct file *filp, const void __user *uarg) if (err) /* -EROFS */ return err; - err = deny_write_access(filp); + err = deny_write_access(filp, INODE_DENY_WRITE_ALL); if (err) /* -ETXTBSY */ goto out_drop_write; @@ -397,7 +397,7 @@ int fsverity_ioctl_enable(struct file *filp, const void __user *uarg) * allow_write_access() is needed to pair with deny_write_access(). * Regardless, the filesystem won't allow writing to verity files. */ - allow_write_access(filp); + allow_write_access(filp, INODE_DENY_WRITE_ALL); out_drop_write: mnt_drop_write_file(filp); return err; diff --git a/include/linux/fs.h b/include/linux/fs.h index 0283cf366c2a..f0720cd0ab45 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -620,6 +620,22 @@ is_uncached_acl(struct posix_acl *acl) #define IOP_XATTR 0x0008 #define IOP_DEFAULT_READLINK 0x0010 +/* + * These are used with the *write_access helpers. + * + * INODE_DENY_WRITE_ALL - Do not allow writes at all. + * INODE_DENY_WRITE_EXEC - Do not allow writes because we are + * exec'ing the file. This can be bypassed + * in cases where the file needs to be + * filled in via the fanotify pre-content + * hooks. + */ +enum inode_write_level { + INODE_DENY_WRITE_ALL, + INODE_DENY_WRITE_EXEC, + INODE_DENY_WRITE_LEVEL, +}; + /* * Keep mostly read-only and often accessed (especially for * the RCU path lookup and 'stat' data) fields at the beginning @@ -701,7 +717,7 @@ struct inode { atomic64_t i_sequence; /* see futex */ atomic_t i_count; atomic_t i_dio_count; - atomic_t i_writecount; + atomic_t i_writecount[INODE_DENY_WRITE_LEVEL]; #if defined(CONFIG_IMA) || defined(CONFIG_FILE_LOCKING) atomic_t i_readcount; /* struct files open RO */ #endif @@ -2920,38 +2936,90 @@ static inline void kiocb_end_write(struct kiocb *iocb) * put_write_access() releases this write permission. * deny_write_access() denies write access to a file. * allow_write_access() re-enables write access to a file. + * __get_write_access_level() gets write permission for a file for the given + * level. + * put_write_access_level() releases the level write permission. * - * The i_writecount field of an inode can have the following values: + * The level indicates which level we're denying writes for. Some levels are + * allowed to be bypassed in special circumstances. In the cases that the write + * level needs to be bypassed the user must use the + * get_write_access_level()/put_write_access_level() pairs of the helpers, which + * will allow the user to bypass the given level, but none of the others. + * + * The i_writecount[level] field of an inode can have the following values: * 0: no write access, no denied write access - * < 0: (-i_writecount) users that denied write access to the file. - * > 0: (i_writecount) users that have write access to the file. + * < 0: (-i_writecount[level]) users that denied write access to the file. + * > 0: (i_writecount[level]) users that have write access to the file. * * Normally we operate on that counter with atomic_{inc,dec} and it's safe * except for the cases where we don't hold i_writecount yet. Then we need to * use {get,deny}_write_access() - these functions check the sign and refuse * to do the change if sign is wrong. */ +static inline int __get_write_access(struct inode *inode, + enum inode_write_level skip_level) +{ + int i; + + /* We are not allowed to skip INODE_DENY_WRITE_ALL. */ + WARN_ON_ONCE(skip_level == INODE_DENY_WRITE_ALL); + + for (i = 0; i < INODE_DENY_WRITE_LEVEL; i++) { + if (i == skip_level) + continue; + if (!atomic_inc_unless_negative(&inode->i_writecount[i])) + goto fail; + } + return 0; +fail: + while (--i >= 0) { + if (i == skip_level) + continue; + atomic_dec(&inode->i_writecount[i]); + } + return -ETXTBSY; +} static inline int get_write_access(struct inode *inode) { - return atomic_inc_unless_negative(&inode->i_writecount) ? 0 : -ETXTBSY; + return __get_write_access(inode, INODE_DENY_WRITE_LEVEL); } -static inline int deny_write_access(struct file *file) +static inline int get_write_access_level(struct inode *inode, + enum inode_write_level skip_level) +{ + return __get_write_access(inode, skip_level); +} +static inline int deny_write_access(struct file *file, + enum inode_write_level level) { struct inode *inode = file_inode(file); - return atomic_dec_unless_positive(&inode->i_writecount) ? 0 : -ETXTBSY; + return atomic_dec_unless_positive(&inode->i_writecount[level]) ? 0 : -ETXTBSY; +} +static inline void __put_write_access(struct inode *inode, + enum inode_write_level skip_level) +{ + for (int i = 0; i < INODE_DENY_WRITE_LEVEL; i++) { + if (i == skip_level) + continue; + atomic_dec(&inode->i_writecount[i]); + } } static inline void put_write_access(struct inode * inode) { - atomic_dec(&inode->i_writecount); + __put_write_access(inode, INODE_DENY_WRITE_LEVEL); } -static inline void allow_write_access(struct file *file) +static inline void put_write_access_level(struct inode *inode, + enum inode_write_level skip_level) +{ + __put_write_access(inode, skip_level); +} +static inline void allow_write_access(struct file *file, enum inode_write_level level) { if (file) - atomic_inc(&file_inode(file)->i_writecount); + atomic_inc(&file_inode(file)->i_writecount[level]); } static inline bool inode_is_open_for_write(const struct inode *inode) { - return atomic_read(&inode->i_writecount) > 0; + return atomic_read(&inode->i_writecount[INODE_DENY_WRITE_ALL]) > 0; } #if defined(CONFIG_IMA) || defined(CONFIG_FILE_LOCKING) diff --git a/include/trace/events/filelock.h b/include/trace/events/filelock.h index b8d1e00a7982..ad076e87a956 100644 --- a/include/trace/events/filelock.h +++ b/include/trace/events/filelock.h @@ -187,7 +187,7 @@ TRACE_EVENT(generic_add_lease, TP_fast_assign( __entry->s_dev = inode->i_sb->s_dev; __entry->i_ino = inode->i_ino; - __entry->wcount = atomic_read(&inode->i_writecount); + __entry->wcount = atomic_read(&inode->i_writecount[INODE_DENY_WRITE_ALL]); __entry->rcount = atomic_read(&inode->i_readcount); __entry->icount = atomic_read(&inode->i_count); __entry->owner = fl->c.flc_owner; diff --git a/kernel/fork.c b/kernel/fork.c index 99076dbe27d8..b6dc7aed2ebf 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -620,7 +620,7 @@ static void dup_mm_exe_file(struct mm_struct *mm, struct mm_struct *oldmm) * We depend on the oldmm having properly denied write access to the * exe_file already. */ - if (exe_file && deny_write_access(exe_file)) + if (exe_file && deny_write_access(exe_file, INODE_DENY_WRITE_EXEC)) pr_warn_once("deny_write_access() failed in %s\n", __func__); } @@ -1417,13 +1417,14 @@ int set_mm_exe_file(struct mm_struct *mm, struct file *new_exe_file) * We expect the caller (i.e., sys_execve) to already denied * write access, so this is unlikely to fail. */ - if (unlikely(deny_write_access(new_exe_file))) + if (unlikely(deny_write_access(new_exe_file, + INODE_DENY_WRITE_EXEC))) return -EACCES; get_file(new_exe_file); } rcu_assign_pointer(mm->exe_file, new_exe_file); if (old_exe_file) { - allow_write_access(old_exe_file); + allow_write_access(old_exe_file, INODE_DENY_WRITE_EXEC); fput(old_exe_file); } return 0; @@ -1464,7 +1465,7 @@ int replace_mm_exe_file(struct mm_struct *mm, struct file *new_exe_file) return ret; } - ret = deny_write_access(new_exe_file); + ret = deny_write_access(new_exe_file, INODE_DENY_WRITE_EXEC); if (ret) return -EACCES; get_file(new_exe_file); @@ -1476,7 +1477,7 @@ int replace_mm_exe_file(struct mm_struct *mm, struct file *new_exe_file) mmap_write_unlock(mm); if (old_exe_file) { - allow_write_access(old_exe_file); + allow_write_access(old_exe_file, INODE_DENY_WRITE_EXEC); fput(old_exe_file); } return 0; diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c index 62fe66dd53ce..b83dfb295d85 100644 --- a/security/integrity/evm/evm_main.c +++ b/security/integrity/evm/evm_main.c @@ -1084,7 +1084,7 @@ static void evm_file_release(struct file *file) if (!S_ISREG(inode->i_mode) || !(mode & FMODE_WRITE)) return; - if (iint && atomic_read(&inode->i_writecount) == 1) + if (iint && atomic_read(&inode->i_writecount[INODE_DENY_WRITE_ALL]) == 1) iint->flags &= ~EVM_NEW_FILE; } diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index f04f43af651c..7aed80a70692 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -164,7 +164,7 @@ static void ima_check_last_writer(struct ima_iint_cache *iint, return; mutex_lock(&iint->mutex); - if (atomic_read(&inode->i_writecount) == 1) { + if (atomic_read(&inode->i_writecount[INODE_DENY_WRITE_ALL]) == 1) { struct kstat stat; update = test_and_clear_bit(IMA_UPDATE_XATTR,
NOTE: This is compile tested only. It's also based on an out of tree feature branch from Amir that I'm extending to add page fault content events to allow us to have on-demand binary loading at exec time. If you need more context please let me know, I'll push my current branch somewhere and wire up how I plan to use this patch so you can see it in better context, but hopefully I've described what I'm trying to accomplish enough that this leads to useful discussion. Currently we have ->i_writecount to control write access to an inode. Callers may deny write access by calling deny_write_access(), which will cause ->i_writecount to go negative, and allow_write_access() to push it back up to 0. This is used in a few ways, the biggest user being exec. Historically we've blocked write access to files that are opened for executing. fsverity is the other notable user. With the upcoming fanotify feature that allows for on-demand population of files, this blanket policy of denying writes to files opened for executing creates a problem. We have to issue events right before denying access, and the entire file must be populated before we can continue with the exec. This creates a problem for users who have large binaries they want to populate on demand. Inside of Meta we often have multi-gigabyte binaries, even on the order of tens of gigabytes. Pre-loading these large files is costly, especially when large portions of the binary may never be read (think debuginfo). In order to facilitate on-demand loading of binaries we need to have a way to punch a hole in this exec related write denial. This patch accomplishes this by doing something similar to the freeze levels on the super block. We have two levels, one for all write denial, and one for exec. People wishing to deny writes will specify the level they're denying. Users wishing to get write access must go through all of the levels and increment each levels counter until it increments them all, or encounters a level that is currently denied, at which point they undo their increments and return an error. Future patches will use the get_write_access_level() helper in order to skip the level they wish to be allowed to access. Any inode being populated via the pre-content fanotify mechanism will be marked with a special flag, and the open path will use get_write_access_level() in order to bypass the exec restriction. This is a significant behavior change, as it allows us to write to a file that is currently being exec'ed. However this will be limited to a very narrow use case. Signed-off-by: Josef Bacik <josef@toxicpanda.com> --- drivers/md/md.c | 2 +- fs/binfmt_elf.c | 4 +- fs/exec.c | 6 +-- fs/ext4/file.c | 4 +- fs/inode.c | 3 +- fs/kernel_read_file.c | 4 +- fs/locks.c | 2 +- fs/quota/dquot.c | 2 +- fs/verity/enable.c | 4 +- include/linux/fs.h | 90 +++++++++++++++++++++++++++---- include/trace/events/filelock.h | 2 +- kernel/fork.c | 11 ++-- security/integrity/evm/evm_main.c | 2 +- security/integrity/ima/ima_main.c | 2 +- 14 files changed, 104 insertions(+), 34 deletions(-)