| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20240829093733.njnlsyexwknyosxe@quack3>
Date: Thu, 29 Aug 2024 11:37:33 +0200
From: Jan Kara <jack@...e.cz>
To: Michal Hocko <mhocko@...nel.org>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
Christoph Hellwig <hch@....de>, Yafang Shao <laoar.shao@...il.com>,
Kent Overstreet <kent.overstreet@...ux.dev>, jack@...e.cz,
Christian Brauner <brauner@...nel.org>,
Alexander Viro <viro@...iv.linux.org.uk>,
Paul Moore <paul@...l-moore.com>, James Morris <jmorris@...ei.org>,
"Serge E. Hallyn" <serge@...lyn.com>, linux-fsdevel@...r.kernel.org,
linux-mm@...ck.org, linux-bcachefs@...r.kernel.org,
linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org,
Michal Hocko <mhocko@...e.com>
Subject: Re: [PATCH 1/2] bcachefs: do not use PF_MEMALLOC_NORECLAIM
On Mon 26-08-24 10:47:12, Michal Hocko wrote:
> From: Michal Hocko <mhocko@...e.com>
>
> bch2_new_inode relies on PF_MEMALLOC_NORECLAIM to try to allocate a new
> inode to achieve GFP_NOWAIT semantic while holding locks. If this
> allocation fails it will drop locks and use GFP_NOFS allocation context.
>
> We would like to drop PF_MEMALLOC_NORECLAIM because it is really
> dangerous to use if the caller doesn't control the full call chain with
> this flag set. E.g. if any of the function down the chain needed
> GFP_NOFAIL request the PF_MEMALLOC_NORECLAIM would override this and
> cause unexpected failure.
>
> While this is not the case in this particular case using the scoped gfp
> semantic is not really needed bacause we can easily pus the allocation
> context down the chain without too much clutter.
>
> Acked-by: Christoph Hellwig <hch@....de>
> Signed-off-by: Michal Hocko <mhocko@...e.com>
For the VFS changes feel free to add:
Reviewed-by: Jan Kara <jack@...e.cz>
Honza
> ---
> fs/bcachefs/fs.c | 14 ++++++--------
> fs/inode.c | 6 +++---
> include/linux/fs.h | 7 ++++++-
> include/linux/lsm_hooks.h | 2 +-
> include/linux/security.h | 4 ++--
> security/security.c | 8 ++++----
> 6 files changed, 22 insertions(+), 19 deletions(-)
>
> diff --git a/fs/bcachefs/fs.c b/fs/bcachefs/fs.c
> index 15fc41e63b6c..7a55167b9133 100644
> --- a/fs/bcachefs/fs.c
> +++ b/fs/bcachefs/fs.c
> @@ -231,9 +231,9 @@ static struct inode *bch2_alloc_inode(struct super_block *sb)
> BUG();
> }
>
> -static struct bch_inode_info *__bch2_new_inode(struct bch_fs *c)
> +static struct bch_inode_info *__bch2_new_inode(struct bch_fs *c, gfp_t gfp)
> {
> - struct bch_inode_info *inode = kmem_cache_alloc(bch2_inode_cache, GFP_NOFS);
> + struct bch_inode_info *inode = kmem_cache_alloc(bch2_inode_cache, gfp);
> if (!inode)
> return NULL;
>
> @@ -245,7 +245,7 @@ static struct bch_inode_info *__bch2_new_inode(struct bch_fs *c)
> mutex_init(&inode->ei_quota_lock);
> memset(&inode->ei_devs_need_flush, 0, sizeof(inode->ei_devs_need_flush));
>
> - if (unlikely(inode_init_always(c->vfs_sb, &inode->v))) {
> + if (unlikely(inode_init_always_gfp(c->vfs_sb, &inode->v), gfp)) {
> kmem_cache_free(bch2_inode_cache, inode);
> return NULL;
> }
> @@ -258,12 +258,10 @@ static struct bch_inode_info *__bch2_new_inode(struct bch_fs *c)
> */
> static struct bch_inode_info *bch2_new_inode(struct btree_trans *trans)
> {
> - struct bch_inode_info *inode =
> - memalloc_flags_do(PF_MEMALLOC_NORECLAIM|PF_MEMALLOC_NOWARN,
> - __bch2_new_inode(trans->c));
> + struct bch_inode_info *inode = __bch2_new_inode(trans->c, GFP_NOWARN | GFP_NOWAIT);
>
> if (unlikely(!inode)) {
> - int ret = drop_locks_do(trans, (inode = __bch2_new_inode(trans->c)) ? 0 : -ENOMEM);
> + int ret = drop_locks_do(trans, (inode = __bch2_new_inode(trans->c, GFP_NOFS)) ? 0 : -ENOMEM);
> if (ret && inode) {
> __destroy_inode(&inode->v);
> kmem_cache_free(bch2_inode_cache, inode);
> @@ -328,7 +326,7 @@ __bch2_create(struct mnt_idmap *idmap,
> if (ret)
> return ERR_PTR(ret);
> #endif
> - inode = __bch2_new_inode(c);
> + inode = __bch2_new_inode(c, GFP_NOFS);
> if (unlikely(!inode)) {
> inode = ERR_PTR(-ENOMEM);
> goto err;
> diff --git a/fs/inode.c b/fs/inode.c
> index 86670941884b..95fd67a6cac3 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -153,7 +153,7 @@ static int no_open(struct inode *inode, struct file *file)
> * These are initializations that need to be done on every inode
> * allocation as the fields are not initialised by slab allocation.
> */
> -int inode_init_always(struct super_block *sb, struct inode *inode)
> +int inode_init_always(struct super_block *sb, struct inode *inode, gfp_t gfp)
> {
> static const struct inode_operations empty_iops;
> static const struct file_operations no_open_fops = {.open = no_open};
> @@ -230,14 +230,14 @@ int inode_init_always(struct super_block *sb, struct inode *inode)
> #endif
> inode->i_flctx = NULL;
>
> - if (unlikely(security_inode_alloc(inode)))
> + if (unlikely(security_inode_alloc(inode, gfp)))
> return -ENOMEM;
>
> this_cpu_inc(nr_inodes);
>
> return 0;
> }
> -EXPORT_SYMBOL(inode_init_always);
> +EXPORT_SYMBOL(inode_init_always_gfp);
>
> void free_inode_nonrcu(struct inode *inode)
> {
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index fd34b5755c0b..d46ca71a7855 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -3027,7 +3027,12 @@ extern loff_t default_llseek(struct file *file, loff_t offset, int whence);
>
> extern loff_t vfs_llseek(struct file *file, loff_t offset, int whence);
>
> -extern int inode_init_always(struct super_block *, struct inode *);
> +extern int inode_init_always_gfp(struct super_block *, struct inode *, gfp_t);
> +static inline int inode_init_always(struct super_block *sb, struct inode *inode)
> +{
> + return inode_init_always_gfp(sb, inode, GFP_NOFS);
> +}
> +
> extern void inode_init_once(struct inode *);
> extern void address_space_init_once(struct address_space *mapping);
> extern struct inode * igrab(struct inode *);
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index a2ade0ffe9e7..b08472d64765 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -150,6 +150,6 @@ extern struct lsm_info __start_early_lsm_info[], __end_early_lsm_info[];
> __used __section(".early_lsm_info.init") \
> __aligned(sizeof(unsigned long))
>
> -extern int lsm_inode_alloc(struct inode *inode);
> +extern int lsm_inode_alloc(struct inode *inode, gfp_t gfp);
>
> #endif /* ! __LINUX_LSM_HOOKS_H */
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 1390f1efb4f0..7c6b9b038a0d 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -336,7 +336,7 @@ int security_dentry_create_files_as(struct dentry *dentry, int mode,
> struct cred *new);
> int security_path_notify(const struct path *path, u64 mask,
> unsigned int obj_type);
> -int security_inode_alloc(struct inode *inode);
> +int security_inode_alloc(struct inode *inode, gfp_t gfp);
> void security_inode_free(struct inode *inode);
> int security_inode_init_security(struct inode *inode, struct inode *dir,
> const struct qstr *qstr,
> @@ -769,7 +769,7 @@ static inline int security_path_notify(const struct path *path, u64 mask,
> return 0;
> }
>
> -static inline int security_inode_alloc(struct inode *inode)
> +static inline int security_inode_alloc(struct inode *inode, gfp_t gfp)
> {
> return 0;
> }
> diff --git a/security/security.c b/security/security.c
> index 8cee5b6c6e6d..3581262da5ee 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -660,14 +660,14 @@ static int lsm_file_alloc(struct file *file)
> *
> * Returns 0, or -ENOMEM if memory can't be allocated.
> */
> -int lsm_inode_alloc(struct inode *inode)
> +int lsm_inode_alloc(struct inode *inode, gfp_t gfp)
> {
> if (!lsm_inode_cache) {
> inode->i_security = NULL;
> return 0;
> }
>
> - inode->i_security = kmem_cache_zalloc(lsm_inode_cache, GFP_NOFS);
> + inode->i_security = kmem_cache_zalloc(lsm_inode_cache, gfp);
> if (inode->i_security == NULL)
> return -ENOMEM;
> return 0;
> @@ -1582,9 +1582,9 @@ int security_path_notify(const struct path *path, u64 mask,
> *
> * Return: Return 0 if operation was successful.
> */
> -int security_inode_alloc(struct inode *inode)
> +int security_inode_alloc(struct inode *inode, gfp_t gfp)
> {
> - int rc = lsm_inode_alloc(inode);
> + int rc = lsm_inode_alloc(inode, gfp);
>
> if (unlikely(rc))
> return rc;
> --
> 2.46.0
>
--
Jan Kara <jack@...e.com>
SUSE Labs, CR
Powered by blists - more mailing lists