Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3696.120.41.1.1\))
Subject: Re: [PATCH] hfsplus: Add module parameter to enable force writes
From:   Viacheslav Dubeyko <slava@dubeyko.com>
In-Reply-To: <53821C76-DAFE-4505-9EC8-BE4ACBEA9DD9@live.com>
Date:   Fri, 2 Dec 2022 12:38:19 -0800
Cc:     "willy@infradead.org" <willy@infradead.org>,
        "ira.weiny@intel.com" <ira.weiny@intel.com>,
        "axboe@kernel.dk" <axboe@kernel.dk>,
        "akpm@linux-foundation.org" <akpm@linux-foundation.org>,
        "bvanassche@acm.org" <bvanassche@acm.org>,
        "keescook@chromium.org" <keescook@chromium.org>,
        "songmuchun@bytedance.com" <songmuchun@bytedance.com>,
        "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <6AE11F8A-3C10-46A3-9F42-B5F72F1FC634@dubeyko.com>
References: <53821C76-DAFE-4505-9EC8-BE4ACBEA9DD9@live.com>
To:     Aditya Garg <gargaditya08@live.com>
Precedence: bulk


> On Dec 1, 2022, at 10:01 PM, Aditya Garg <gargaditya08@live.com> =
wrote:
>=20
> From: Aditya Garg <gargaditya08@live.com>
>=20
> This patch enables users to permanently enable writes of HFS+ locked
> and/or journaled volumes using a module parameter.
>=20
> Why module parameter?
> Reason being, its not convenient to manually mount the volume with =
force
> everytime. There are use cases which are fine with force enabling =
writes
> on journaled volumes. I've seen many on various online forums and I am =
one
> of them as well.
>=20
> Isn't it risky?
> Yes obviously it is, as the driver itself warns users for the same. =
But
> any user using the parameter obviously shall be well aware of the =
risks
> involved. To be honest, I've been writing on a 100Gb journaled volume =
for
> a few days, including both large and small files, and haven't faced =
any
> corruption yet.
>=20

If you created HFS+ volume under Linux, then you never have journal
and problem of journal replay (even if you created journaled volume).
So, I see the only one case when you have journal with transactions.
You are using HFS+ volume in Linux and Mac OS X. It means that
Mac OS X can create transactions in the journal and Linux needs
to manage it somehow.

Even if you don=E2=80=99t see any corruptions after such short testing, =
then
it doesn=E2=80=99t mean that you are safe. The key trouble that you can
silently lose the data because some metadata state could sit
in the journal and no replay operation has happened. Yes, you can
ignore the transactions in the journal and continue to store data and
modify metadata. But if journal still contain valid transactions, then
mount operation under Mac OS X will replay journal. And it sounds
that journal replay under Mac OS X can corrupt metadata and data
state that was modified/created under Linux.

So, I believe that your suggestion is slightly dangerous because
people loves to make mistakes and really hates to lose data.

Thanks,
Slava.=20

> Signed-off-by: Aditya Garg <gargaditya08@live.com>
> ---
> fs/hfsplus/super.c | 46 ++++++++++++++++++++++++++++++++++++----------
> 1 file changed, 36 insertions(+), 10 deletions(-)
>=20
> diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
> index 122ed89eb..2367a2407 100644
> --- a/fs/hfsplus/super.c
> +++ b/fs/hfsplus/super.c
> @@ -24,6 +24,16 @@ static void hfsplus_free_inode(struct inode =
*inode);
> #include "hfsplus_fs.h"
> #include "xattr.h"
>=20
> +static unsigned int force_journaled_rw;
> +module_param(force_journaled_rw, uint, 0644);
> +MODULE_PARM_DESC(force_journaled_rw, "Force enable writes on =
Journaled HFS+ volumes. "
> +		"([0] =3D disabled, 1 =3D enabled)");
> +
> +static unsigned int force_locked_rw;
> +module_param(force_locked_rw, uint, 0644);
> +MODULE_PARM_DESC(force_locked_rw, "Force enable writes on locked HFS+ =
volumes. "
> +		"([0] =3D disabled, 1 =3D enabled)");
> +
> static int hfsplus_system_read_inode(struct inode *inode)
> {
> 	struct hfsplus_vh *vhdr =3D HFSPLUS_SB(inode->i_sb)->s_vhdr;
> @@ -346,14 +356,22 @@ static int hfsplus_remount(struct super_block =
*sb, int *flags, char *data)
> 			/* nothing */
> 		} else if (vhdr->attributes &
> 				cpu_to_be32(HFSPLUS_VOL_SOFTLOCK)) {
> -			pr_warn("filesystem is marked locked, leaving =
read-only.\n");
> -			sb->s_flags |=3D SB_RDONLY;
> -			*flags |=3D SB_RDONLY;
> +			if (force_locked_rw) {
> +				pr_warn("filesystem is marked locked, =
but writes have been force enabled.\n");
> +			} else {
> +				pr_warn("filesystem is marked locked, =
leaving read-only.\n");
> +				sb->s_flags |=3D SB_RDONLY;
> +				*flags |=3D SB_RDONLY;
> +			}
> 		} else if (vhdr->attributes &
> 				cpu_to_be32(HFSPLUS_VOL_JOURNALED)) {
> -			pr_warn("filesystem is marked journaled, leaving =
read-only.\n");
> -			sb->s_flags |=3D SB_RDONLY;
> -			*flags |=3D SB_RDONLY;
> +			if (force_journaled_rw) {
> +				pr_warn("filesystem is marked journaled, =
but writes have been force enabled.\n");
> +			} else {
> +				pr_warn("filesystem is marked journaled, =
leaving read-only.\n");
> +				sb->s_flags |=3D SB_RDONLY;
> +				*flags |=3D SB_RDONLY;
> +			}
> 		}
> 	}
> 	return 0;
> @@ -459,12 +477,20 @@ static int hfsplus_fill_super(struct super_block =
*sb, void *data, int silent)
> 	} else if (test_and_clear_bit(HFSPLUS_SB_FORCE, &sbi->flags)) {
> 		/* nothing */
> 	} else if (vhdr->attributes & cpu_to_be32(HFSPLUS_VOL_SOFTLOCK)) =
{
> -		pr_warn("Filesystem is marked locked, mounting =
read-only.\n");
> -		sb->s_flags |=3D SB_RDONLY;
> +		if (force_locked_rw) {
> +			pr_warn("Filesystem is marked locked, but writes =
have been force enabled.\n");
> +		} else {
> +			pr_warn("Filesystem is marked locked, mounting =
read-only.\n");
> +			sb->s_flags |=3D SB_RDONLY;
> +		}
> 	} else if ((vhdr->attributes & =
cpu_to_be32(HFSPLUS_VOL_JOURNALED)) &&
> 			!sb_rdonly(sb)) {
> -		pr_warn("write access to a journaled filesystem is not =
supported, use the force option at your own risk, mounting =
read-only.\n");
> -		sb->s_flags |=3D SB_RDONLY;
> +		if (force_journaled_rw) {
> +			pr_warn("write access to a journaled filesystem =
is not supported, but has been force enabled.\n");
> +		} else {
> +			pr_warn("write access to a journaled filesystem =
is not supported, use the force option at your own risk, mounting =
read-only.\n");
> +			sb->s_flags |=3D SB_RDONLY;
> +		}
> 	}
>=20
> 	err =3D -EINVAL;
> --=20
> 2.38.1
>=20