| 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: <aS-IVZyCZgE115f6@eray-kasa>
Date: Wed, 3 Dec 2025 03:46:13 +0300
From: Ahmet Eray Karadag <eraykrdg1@...il.com>
To: Joseph Qi <joseph.qi@...ux.alibaba.com>
Cc: mark@...heh.com, jlbec@...lplan.org, Heming Zhao <heming.zhao@...e.com>,
ocfs2-devel@...ts.linux.dev, linux-kernel@...r.kernel.org,
david.hunter.linux@...il.com, skhan@...uxfoundation.org,
Albin Babu Varghese <albinbabuvarghese20@...il.com>
Subject: Re: [PATCH v3 2/2] ocfs2: Convert remaining read-only checks to
ocfs2_emergency_state
On Tue, Dec 02, 2025 at 03:39:30PM +0800, Joseph Qi wrote:
>
>
> On 2025/12/2 10:54, Ahmet Eray Karadag wrote:
> > To centralize error checking, follow the pattern of other filesystems
> > like ext4 (which uses `ext4_emergency_state()`), and prepare for
> > future enhancements, this patch introduces a new helper function:
> > `ocfs2_emergency_state()`.
> >
> > The purpose of this helper is to provide a single, unified location
> > for checking all filesystem-level emergency conditions. In this
> > initial implementation, the function only checks for the existing
> > hard and soft read-only modes, returning -EROFS if either is set.
> >
> > This provides a foundation where future checks (e.g., for fatal error
> > states returning -EIO, or shutdown states) can be easily added in
> > one place.
> >
> > This patch also adds this new check to the beginning of
> > `ocfs2_setattr()`. This ensures that operations like `ftruncate`
> > (which triggered the original BUG) fail-fast with -EROFS when the
> > filesystem is already in a read-only state.
> >
>
> The above commit log is the same with patch 1 and doesn't reflect
> the following changes.
>
> > Co-developed-by: Albin Babu Varghese <albinbabuvarghese20@...il.com>
> > Signed-off-by: Albin Babu Varghese <albinbabuvarghese20@...il.com>
> > Signed-off-by: Ahmet Eray Karadag <eraykrdg1@...il.com>
> > ---
> > v2:
> > - Use `unlikely()` for status check
> > ---
> > fs/ocfs2/buffer_head_io.c | 4 ++--
> > fs/ocfs2/file.c | 17 ++++++++++-------
> > fs/ocfs2/inode.c | 3 +--
> > fs/ocfs2/move_extents.c | 5 +++--
> > fs/ocfs2/resize.c | 8 +++++---
> > fs/ocfs2/super.c | 2 +-
> > 6 files changed, 22 insertions(+), 17 deletions(-)
> >
> > diff --git a/fs/ocfs2/buffer_head_io.c b/fs/ocfs2/buffer_head_io.c
> > index 8f714406528d..61a0f522c673 100644
> > --- a/fs/ocfs2/buffer_head_io.c
> > +++ b/fs/ocfs2/buffer_head_io.c
> > @@ -434,8 +434,8 @@ int ocfs2_write_super_or_backup(struct ocfs2_super *osb,
> > BUG_ON(buffer_jbd(bh));
> > ocfs2_check_super_or_backup(osb->sb, bh->b_blocknr);
> >
> > - if (ocfs2_is_hard_readonly(osb) || ocfs2_is_soft_readonly(osb)) {
> > - ret = -EROFS;
> > + ret = ocfs2_emergency_state(osb);
> > + if (unlikely(ret)) {
>
> I'd like use the following style:
> if (ocfs2_emergency_state(osb)) {
> ret = -EROFS;
> ...
> }
>
> So that ocfs2_emergency_state() can be expended with other cases(properly
> other return code), without touching these code flows.
In the future, there might be other return codes added to ocfs2_emergency_state().
Correct me if I'm wrong, but with your proposed style, we would only return
-EROFS in any case. Shouldn't we consider future improvements?
Thanks,
Ahmet Eray
>
> Joseph
>
> > mlog_errno(ret);
> > goto out;
> > }
> > diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
> > index 253b4f300127..540b35ec02e2 100644
> > --- a/fs/ocfs2/file.c
> > +++ b/fs/ocfs2/file.c
> > @@ -179,8 +179,9 @@ static int ocfs2_sync_file(struct file *file, loff_t start, loff_t end,
> > file->f_path.dentry->d_name.name,
> > (unsigned long long)datasync);
> >
> > - if (ocfs2_is_hard_readonly(osb) || ocfs2_is_soft_readonly(osb))
> > - return -EROFS;
> > + ret = ocfs2_emergency_state(osb);
> > + if (unlikely(ret))
> > + return ret;
> >
> > err = file_write_and_wait_range(file, start, end);
> > if (err)
> > @@ -209,7 +210,7 @@ int ocfs2_should_update_atime(struct inode *inode,
> > struct timespec64 now;
> > struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
> >
> > - if (ocfs2_is_hard_readonly(osb) || ocfs2_is_soft_readonly(osb))
> > + if (unlikely(ocfs2_emergency_state(osb)))
> > return 0;
> >
> > if ((inode->i_flags & S_NOATIME) ||
> > @@ -1949,8 +1950,9 @@ static int __ocfs2_change_file_space(struct file *file, struct inode *inode,
> > handle_t *handle;
> > unsigned long long max_off = inode->i_sb->s_maxbytes;
> >
> > - if (ocfs2_is_hard_readonly(osb) || ocfs2_is_soft_readonly(osb))
> > - return -EROFS;
> > + ret = ocfs2_emergency_state(osb);
> > + if (unlikely(ret))
> > + return ret;
> >
> > inode_lock(inode);
> >
> > @@ -2713,8 +2715,9 @@ static loff_t ocfs2_remap_file_range(struct file *file_in, loff_t pos_in,
> > return -EINVAL;
> > if (!ocfs2_refcount_tree(osb))
> > return -EOPNOTSUPP;
> > - if (ocfs2_is_hard_readonly(osb) || ocfs2_is_soft_readonly(osb))
> > - return -EROFS;
> > + ret = ocfs2_emergency_state(osb);
> > + if (unlikely(ret))
> > + return ret;
> >
> > /* Lock both files against IO */
> > ret = ocfs2_reflink_inodes_lock(inode_in, &in_bh, inode_out, &out_bh);
> > diff --git a/fs/ocfs2/inode.c b/fs/ocfs2/inode.c
> > index fcc89856ab95..b7dad049cfa3 100644
> > --- a/fs/ocfs2/inode.c
> > +++ b/fs/ocfs2/inode.c
> > @@ -1586,8 +1586,7 @@ static int ocfs2_filecheck_repair_inode_block(struct super_block *sb,
> > trace_ocfs2_filecheck_repair_inode_block(
> > (unsigned long long)bh->b_blocknr);
> >
> > - if (ocfs2_is_hard_readonly(OCFS2_SB(sb)) ||
> > - ocfs2_is_soft_readonly(OCFS2_SB(sb))) {
> > + if (unlikely(ocfs2_emergency_state(OCFS2_SB(sb)))) {
> > mlog(ML_ERROR,
> > "Filecheck: cannot repair dinode #%llu "
> > "on readonly filesystem\n",
> > diff --git a/fs/ocfs2/move_extents.c b/fs/ocfs2/move_extents.c
> > index 86f2631e6360..9d2a2d054aa1 100644
> > --- a/fs/ocfs2/move_extents.c
> > +++ b/fs/ocfs2/move_extents.c
> > @@ -898,8 +898,9 @@ static int ocfs2_move_extents(struct ocfs2_move_extents_context *context)
> > struct buffer_head *di_bh = NULL;
> > struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
> >
> > - if (ocfs2_is_hard_readonly(osb) || ocfs2_is_soft_readonly(osb))
> > - return -EROFS;
> > + status = ocfs2_emergency_state(osb);
> > + if (unlikely(status))
> > + return status;
> >
> > inode_lock(inode);
> >
> > diff --git a/fs/ocfs2/resize.c b/fs/ocfs2/resize.c
> > index b0733c08ed13..ae30ae67e220 100644
> > --- a/fs/ocfs2/resize.c
> > +++ b/fs/ocfs2/resize.c
> > @@ -276,8 +276,9 @@ int ocfs2_group_extend(struct inode * inode, int new_clusters)
> > u32 first_new_cluster;
> > u64 lgd_blkno;
> >
> > - if (ocfs2_is_hard_readonly(osb) || ocfs2_is_soft_readonly(osb))
> > - return -EROFS;
> > + ret = ocfs2_emergency_state(osb);
> > + if (unlikely(ret))
> > + return ret;
> >
> > if (new_clusters < 0)
> > return -EINVAL;
> > @@ -466,7 +467,8 @@ int ocfs2_group_add(struct inode *inode, struct ocfs2_new_group_input *input)
> > u16 cl_bpc;
> > u64 bg_ptr;
> >
> > - if (ocfs2_is_hard_readonly(osb) || ocfs2_is_soft_readonly(osb))
> > + ret = ocfs2_emergency_state(osb);
> > + if (unlikely(ret))
> > return -EROFS;
> >
> > main_bm_inode = ocfs2_get_system_file_inode(osb,
> > diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
> > index 53daa4482406..c6019d260efc 100644
> > --- a/fs/ocfs2/super.c
> > +++ b/fs/ocfs2/super.c
> > @@ -2487,7 +2487,7 @@ static int ocfs2_handle_error(struct super_block *sb)
> > rv = -EIO;
> > } else { /* default option */
> > rv = -EROFS;
> > - if (sb_rdonly(sb) && (ocfs2_is_soft_readonly(osb) || ocfs2_is_hard_readonly(osb)))
> > + if (sb_rdonly(sb) && ocfs2_emergency_state(osb))
> > return rv;
> >
> > pr_crit("OCFS2: File system is now read-only.\n");
>
Powered by blists - more mailing lists