| 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: <2iainwxojhulyf7oyga5xboufhiztz4boiqaofppopeduicwjt@pxj756ofltru>
Date: Sat, 1 Feb 2025 16:24:38 -0500
From: Kent Overstreet <kent.overstreet@...ux.dev>
To: Jeongjun Park <aha310510@...il.com>
Cc: linux-bcachefs@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] bcachefs: fix deadlock in journal_entry_open()
On Sun, Feb 02, 2025 at 04:50:44AM +0900, Jeongjun Park wrote:
> In the previous commit b3d82c2f2761, code was added to prevent journal sequence
> overflow. Among them, the code added to journal_entry_open() uses the
> bch2_fs_fatal_err_on() function to handle errors.
>
> However, __journal_res_get() , which calls journal_entry_open() , calls
> journal_entry_open() while holding journal->lock , but bch2_fs_fatal_err_on()
> internally tries to acquire journal->lock , which results in a deadlock.
>
> So we need to add a locked helper like bch2_journal_halt_locked() to handle
> fatal errors even when the journal->lock is held.
>
> Fixes: b3d82c2f2761 ("bcachefs: Guard against journal seq overflow")
> Signed-off-by: Jeongjun Park <aha310510@...il.com>
> ---
> fs/bcachefs/error.c | 6 +++---
> fs/bcachefs/fs-ioctl.c | 4 ++--
> fs/bcachefs/journal.c | 22 +++++++++++++++++-----
> fs/bcachefs/journal.h | 3 ++-
> fs/bcachefs/recovery.c | 2 +-
> fs/bcachefs/super.c | 4 ++--
> fs/bcachefs/super.h | 2 +-
> 7 files changed, 28 insertions(+), 15 deletions(-)
>
> diff --git a/fs/bcachefs/error.c b/fs/bcachefs/error.c
> index 038da6a61f6b..2482eb33b1ec 100644
> --- a/fs/bcachefs/error.c
> +++ b/fs/bcachefs/error.c
> @@ -20,7 +20,7 @@ bool bch2_inconsistent_error(struct bch_fs *c)
> return false;
> case BCH_ON_ERROR_fix_safe:
> case BCH_ON_ERROR_ro:
> - if (bch2_fs_emergency_read_only(c))
> + if (bch2_fs_emergency_read_only(c, false))
No, don't add a bool parameter. It'll be a lot less churny, and clearer
reading, if you add a separate
bch2_fs_emergency_read_only_locked()
helper
> bch_err(c, "inconsistency detected - emergency read only at journal seq %llu",
> journal_cur_seq(&c->journal));
> return true;
> @@ -46,7 +46,7 @@ int bch2_topology_error(struct bch_fs *c)
>
> void bch2_fatal_error(struct bch_fs *c)
> {
> - if (bch2_fs_emergency_read_only(c))
> + if (bch2_fs_emergency_read_only(c, false))
> bch_err(c, "fatal error - emergency read only");
> }
>
> @@ -62,7 +62,7 @@ void bch2_io_error_work(struct work_struct *work)
> if (dev
> ? __bch2_dev_set_state(c, ca, BCH_MEMBER_STATE_ro,
> BCH_FORCE_IF_DEGRADED)
> - : bch2_fs_emergency_read_only(c))
> + : bch2_fs_emergency_read_only(c, false))
> bch_err(ca,
> "too many IO errors, setting %s RO",
> dev ? "device" : "filesystem");
> diff --git a/fs/bcachefs/fs-ioctl.c b/fs/bcachefs/fs-ioctl.c
> index 15725b4ce393..92becb6ae5aa 100644
> --- a/fs/bcachefs/fs-ioctl.c
> +++ b/fs/bcachefs/fs-ioctl.c
> @@ -354,14 +354,14 @@ static int bch2_ioc_goingdown(struct bch_fs *c, u32 __user *arg)
> if (ret)
> break;
> bch2_journal_flush(&c->journal);
> - bch2_fs_emergency_read_only(c);
> + bch2_fs_emergency_read_only(c, false);
> bdev_thaw(c->vfs_sb->s_bdev);
> break;
> case FSOP_GOING_FLAGS_LOGFLUSH:
> bch2_journal_flush(&c->journal);
> fallthrough;
> case FSOP_GOING_FLAGS_NOLOGFLUSH:
> - bch2_fs_emergency_read_only(c);
> + bch2_fs_emergency_read_only(c, false);
> break;
> default:
> ret = -EINVAL;
> diff --git a/fs/bcachefs/journal.c b/fs/bcachefs/journal.c
> index 2cd20114b74b..4b3bdbfd7782 100644
> --- a/fs/bcachefs/journal.c
> +++ b/fs/bcachefs/journal.c
> @@ -310,14 +310,23 @@ static void __journal_entry_close(struct journal *j, unsigned closed_val, bool t
> __bch2_journal_buf_put(j, old.idx, le64_to_cpu(buf->data->seq));
> }
>
> -void bch2_journal_halt(struct journal *j)
> +void bch2_journal_halt_locked(struct journal *j)
> {
> - spin_lock(&j->lock);
> + lockdep_assert_held(&j->lock);
> +
> __journal_entry_close(j, JOURNAL_ENTRY_ERROR_VAL, true);
> if (!j->err_seq)
> j->err_seq = journal_cur_seq(j);
> journal_wake(j);
> - spin_unlock(&j->lock);
> +}
> +
> +void bch2_journal_halt(struct journal *j, bool locked)
> +{
> + if (!locked)
> + spin_lock(&j->lock);
> + bch2_journal_halt_locked(j);
> + if (!locked)
> + spin_unlock(&j->lock);
> }
>
> static bool journal_entry_want_write(struct journal *j)
> @@ -382,9 +391,12 @@ static int journal_entry_open(struct journal *j)
> if (nr_unwritten_journal_entries(j) == ARRAY_SIZE(j->buf))
> return JOURNAL_ERR_max_in_flight;
>
> - if (bch2_fs_fatal_err_on(journal_cur_seq(j) >= JOURNAL_SEQ_MAX,
> - c, "cannot start: journal seq overflow"))
> + if (journal_cur_seq(j) >= JOURNAL_SEQ_MAX) {
> + bch_err(c, "cannot start: journal seq overflow");
> + if (bch2_fs_emergency_read_only(c, true))
> + bch_err(c, "fatal error - emergency read only");
> return JOURNAL_ERR_insufficient_devices; /* -EROFS */
> + }
>
> BUG_ON(!j->cur_entry_sectors);
>
> diff --git a/fs/bcachefs/journal.h b/fs/bcachefs/journal.h
> index cb0df0663946..23f5fd970b7d 100644
> --- a/fs/bcachefs/journal.h
> +++ b/fs/bcachefs/journal.h
> @@ -407,7 +407,8 @@ int bch2_journal_flush(struct journal *);
> bool bch2_journal_noflush_seq(struct journal *, u64, u64);
> int bch2_journal_meta(struct journal *);
>
> -void bch2_journal_halt(struct journal *);
> +void bch2_journal_halt(struct journal *, bool);
> +void bch2_journal_halt_locked(struct journal *);
>
> static inline int bch2_journal_error(struct journal *j)
> {
> diff --git a/fs/bcachefs/recovery.c b/fs/bcachefs/recovery.c
> index 98825437381c..31549181888c 100644
> --- a/fs/bcachefs/recovery.c
> +++ b/fs/bcachefs/recovery.c
> @@ -1076,7 +1076,7 @@ int bch2_fs_recovery(struct bch_fs *c)
> return ret;
> err:
> fsck_err:
> - bch2_fs_emergency_read_only(c);
> + bch2_fs_emergency_read_only(c, false);
> goto out;
> }
>
> diff --git a/fs/bcachefs/super.c b/fs/bcachefs/super.c
> index d97ea7bd1171..d3d11fb39632 100644
> --- a/fs/bcachefs/super.c
> +++ b/fs/bcachefs/super.c
> @@ -400,11 +400,11 @@ static void bch2_fs_read_only_async(struct bch_fs *c)
> queue_work(system_long_wq, &c->read_only_work);
> }
>
> -bool bch2_fs_emergency_read_only(struct bch_fs *c)
> +bool bch2_fs_emergency_read_only(struct bch_fs *c, bool locked)
> {
> bool ret = !test_and_set_bit(BCH_FS_emergency_ro, &c->flags);
>
> - bch2_journal_halt(&c->journal);
> + bch2_journal_halt(&c->journal, locked);
> bch2_fs_read_only_async(c);
>
> wake_up(&bch2_read_only_wait);
> diff --git a/fs/bcachefs/super.h b/fs/bcachefs/super.h
> index fa6d52216510..e9ff83e06ca2 100644
> --- a/fs/bcachefs/super.h
> +++ b/fs/bcachefs/super.h
> @@ -28,7 +28,7 @@ int bch2_dev_offline(struct bch_fs *, struct bch_dev *, int);
> int bch2_dev_resize(struct bch_fs *, struct bch_dev *, u64);
> struct bch_dev *bch2_dev_lookup(struct bch_fs *, const char *);
>
> -bool bch2_fs_emergency_read_only(struct bch_fs *);
> +bool bch2_fs_emergency_read_only(struct bch_fs *, bool);
> void bch2_fs_read_only(struct bch_fs *);
>
> int bch2_fs_read_write(struct bch_fs *);
> --
Powered by blists - more mailing lists