| 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: <6610d627-dfab-7b9b-c616-cccaf40ea948@huawei.com>
Date: Wed, 26 Mar 2025 16:04:37 +0800
From: Zhihao Cheng <chengzhihao1@...wei.com>
To: Wang Zhaolong <wangzhaolong1@...wei.com>, <dwmw2@...radead.org>,
<richard@....at>
CC: <linux-mtd@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
<yi.zhang@...wei.com>
Subject: Re: [RFC 1/1] fs/jffs2: Avoid a possible deadlock in
jffs2_wbuf_recover
在 2025/3/19 10:47, Wang Zhaolong 写道:
> jffs2_wbuf_recover can deadlock when called from __jffs2_flush_wbuf
> error path, because both functions try to acquire c->wbuf_sem.
>
> jffs2_write_end
> jffs2_write_inode_range
> jffs2_write_dnode
> jffs2_flash_writev
> down_write(&c->wbuf_sem) # first hold lock
> __jffs2_flush_wbuf
> mtd_write() # return error
> jffs2_wbuf_recover
> jffs2_reserve_space_gc
> jffs2_do_reserve_space
> jffs2_flush_wbuf_pad
> down_write(&c->wbuf_sem) # AA deadlock
>
> Fix this by adding a wbuf_sem_held parameter to jffs2_reserve_space_gc
> and jffs2_do_reserve_space, which can be used to indicate that the caller
> already holds c->wbuf_sem. If this is the case, jffs2_do_reserve_space
> will directly call __jffs2_flush_wbuf instead of going through
> jffs2_flush_wbuf_pad, which would try to acquire the lock again.
>
> Signed-off-by: Wang Zhaolong <wangzhaolong1@...wei.com>
> ---
> fs/jffs2/gc.c | 12 ++++++------
> fs/jffs2/nodelist.h | 12 +++++++++++-
> fs/jffs2/nodemgmt.c | 16 ++++++++++------
> fs/jffs2/os-linux.h | 1 +
> fs/jffs2/wbuf.c | 13 ++-----------
> fs/jffs2/write.c | 4 ++--
> fs/jffs2/xattr.c | 4 ++--
> 7 files changed, 34 insertions(+), 28 deletions(-)
>
[...]
> @@ -416,11 +416,15 @@ static int jffs2_do_reserve_space(struct jffs2_sb_info *c, uint32_t minsize,
>
> if (jffs2_wbuf_dirty(c)) {
> spin_unlock(&c->erase_completion_lock);
> jffs2_dbg(1, "%s(): Flushing write buffer\n",
> __func__);
> - jffs2_flush_wbuf_pad(c);
> + if (wbuf_sem_held) {
> + __jffs2_flush_wbuf(c, PAD_NOACCOUNT);
> + } else {
> + jffs2_flush_wbuf_pad(c);
> + }
Hi, Zhaolong
Perhaps the __jffs2_flush_wbuf is not equalivelent to the lockless
version of jffs2_flush_wbuf_pad, for 2 reasons:
1. jffs2_flush_wbuf_pad will do nothing when CONFIG_JFFS2_FS_WRITEBUFFER
is disabled.
2. jffs2_flush_wbuf_pad also checks 'c->wbuf', will it be ok to remove
the check?
> spin_lock(&c->erase_completion_lock);
> jeb = c->nextblock;
> goto restart;
> }
>
> diff --git a/fs/jffs2/os-linux.h b/fs/jffs2/os-linux.h
> index 86ab014a349c..c9dda7126850 100644
> --- a/fs/jffs2/os-linux.h
> +++ b/fs/jffs2/os-linux.h
> @@ -76,10 +76,11 @@ static inline void jffs2_init_inode_info(struct jffs2_inode_info *f)
> #define jffs2_cleanmarker_oob(c) (0)
> #define jffs2_write_nand_cleanmarker(c,jeb) (-EIO)
>
> #define jffs2_flash_write(c, ofs, len, retlen, buf) jffs2_flash_direct_write(c, ofs, len, retlen, buf)
> #define jffs2_flash_read(c, ofs, len, retlen, buf) (mtd_read((c)->mtd, ofs, len, retlen, buf))
> +#define __jffs2_flush_wbuf(c, pad) ({ do{} while(0); (void)(c), 0; })
> #define jffs2_flush_wbuf_pad(c) ({ do{} while(0); (void)(c), 0; })
> #define jffs2_flush_wbuf_gc(c, i) ({ do{} while(0); (void)(c), (void) i, 0; })
> #define jffs2_write_nand_badblock(c,jeb,bad_offset) (1)
> #define jffs2_nand_flash_setup(c) (0)
> #define jffs2_nand_flash_cleanup(c) do {} while(0)
> diff --git a/fs/jffs2/wbuf.c b/fs/jffs2/wbuf.c
> index 4061e0ba7010..799559760dc1 100644
> --- a/fs/jffs2/wbuf.c
> +++ b/fs/jffs2/wbuf.c
> @@ -384,11 +384,11 @@ static void jffs2_wbuf_recover(struct jffs2_sb_info *c)
> }
> /* OK... we're to rewrite (end-start) bytes of data from first_raw onwards.
> Either 'buf' contains the data, or we find it in the wbuf */
>
> /* ... and get an allocation of space from a shiny new block instead */
> - ret = jffs2_reserve_space_gc(c, end-start, &len, JFFS2_SUMMARY_NOSUM_SIZE);
> + ret = jffs2_reserve_space_gc(c, end-start, &len, JFFS2_SUMMARY_NOSUM_SIZE, 1);
> if (ret) {
> pr_warn("Failed to allocate space for wbuf recovery. Data loss ensues.\n");
> kfree(buf);
> return;
> }
> @@ -566,20 +566,11 @@ static void jffs2_wbuf_recover(struct jffs2_sb_info *c)
> jffs2_dbg(1, "wbuf recovery completed OK. wbuf_ofs 0x%08x, len 0x%x\n",
> c->wbuf_ofs, c->wbuf_len);
>
> }
>
> -/* Meaning of pad argument:
> - 0: Do not pad. Probably pointless - we only ever use this when we can't pad anyway.
> - 1: Pad, do not adjust nextblock free_size
> - 2: Pad, adjust nextblock free_size
> -*/
> -#define NOPAD 0
> -#define PAD_NOACCOUNT 1
> -#define PAD_ACCOUNTING 2
> -
> -static int __jffs2_flush_wbuf(struct jffs2_sb_info *c, int pad)
> +int __jffs2_flush_wbuf(struct jffs2_sb_info *c, int pad)
> {
> struct jffs2_eraseblock *wbuf_jeb;
> int ret;
> size_t retlen;
>
> diff --git a/fs/jffs2/write.c b/fs/jffs2/write.c
> index cda9a361368e..1764cf0466fc 100644
> --- a/fs/jffs2/write.c
> +++ b/fs/jffs2/write.c
> @@ -137,11 +137,11 @@ struct jffs2_full_dnode *jffs2_write_dnode(struct jffs2_sb_info *c, struct jffs2
> jffs2_dbg_acct_sanity_check(c,jeb);
> jffs2_dbg_acct_paranoia_check(c, jeb);
>
> if (alloc_mode == ALLOC_GC) {
> ret = jffs2_reserve_space_gc(c, sizeof(*ri) + datalen, &dummy,
> - JFFS2_SUMMARY_INODE_SIZE);
> + JFFS2_SUMMARY_INODE_SIZE, 0);
> } else {
> /* Locking pain */
> mutex_unlock(&f->sem);
> jffs2_complete_reservation(c);
>
> @@ -289,11 +289,11 @@ struct jffs2_full_dirent *jffs2_write_dirent(struct jffs2_sb_info *c, struct jff
> jffs2_dbg_acct_sanity_check(c,jeb);
> jffs2_dbg_acct_paranoia_check(c, jeb);
>
> if (alloc_mode == ALLOC_GC) {
> ret = jffs2_reserve_space_gc(c, sizeof(*rd) + namelen, &dummy,
> - JFFS2_SUMMARY_DIRENT_SIZE(namelen));
> + JFFS2_SUMMARY_DIRENT_SIZE(namelen), 0);
> } else {
> /* Locking pain */
> mutex_unlock(&f->sem);
> jffs2_complete_reservation(c);
>
> diff --git a/fs/jffs2/xattr.c b/fs/jffs2/xattr.c
> index defb4162c3d5..b7b1eb8f012c 100644
> --- a/fs/jffs2/xattr.c
> +++ b/fs/jffs2/xattr.c
> @@ -1241,11 +1241,11 @@ int jffs2_garbage_collect_xattr_datum(struct jffs2_sb_info *c, struct jffs2_xatt
> goto out;
> }
> old_ofs = ref_offset(xd->node);
> totlen = PAD(sizeof(struct jffs2_raw_xattr)
> + xd->name_len + 1 + xd->value_len);
> - rc = jffs2_reserve_space_gc(c, totlen, &length, JFFS2_SUMMARY_XATTR_SIZE);
> + rc = jffs2_reserve_space_gc(c, totlen, &length, JFFS2_SUMMARY_XATTR_SIZE, 0);
> if (rc) {
> JFFS2_WARNING("jffs2_reserve_space_gc()=%d, request=%u\n", rc, totlen);
> goto out;
> }
> rc = save_xattr_datum(c, xd);
> @@ -1274,11 +1274,11 @@ int jffs2_garbage_collect_xattr_ref(struct jffs2_sb_info *c, struct jffs2_xattr_
> goto out;
>
> old_ofs = ref_offset(ref->node);
> totlen = ref_totlen(c, c->gcblock, ref->node);
>
> - rc = jffs2_reserve_space_gc(c, totlen, &length, JFFS2_SUMMARY_XREF_SIZE);
> + rc = jffs2_reserve_space_gc(c, totlen, &length, JFFS2_SUMMARY_XREF_SIZE, 0);
> if (rc) {
> JFFS2_WARNING("%s: jffs2_reserve_space_gc() = %d, request = %u\n",
> __func__, rc, totlen);
> goto out;
> }
>
Powered by blists - more mailing lists