[PATCH 1/2] bcachefs: make offline fsck set read_only fs flag

[PATCH 1/2] bcachefs: make offline fsck set read_only fs flag

[Thomas Bertschinger]

A subsequent change will remove "read_only" as a mount option in favor
of the standard option "ro", meaning the userspace fsck command cannot
pass it to the fsck ioctl. Instead, in offline fsck, set "read_only"
kernel-side without trying to parse it as a mount option.

For compatibility with versions of the "bcachefs fsck" command that try
to pass the "read_only" mount opt, remove it from the mount options
string prior to parsing when it is present.

Signed-off-by: Thomas Bertschinger <tahbertschinger@gmail.com>
---
 fs/bcachefs/chardev.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/fs/bcachefs/chardev.c b/fs/bcachefs/chardev.c
index 9e54323f0f5f..5040c584cd72 100644
--- a/fs/bcachefs/chardev.c
+++ b/fs/bcachefs/chardev.c
@@ -213,6 +213,18 @@ static long bch2_ioctl_fsck_offline(struct bch_ioctl_fsck_offline __user *user_a
 
 	if (arg.opts) {
 		char *optstr = strndup_user((char __user *)(unsigned long) arg.opts, 1 << 16);
+		char *ro, *rest;
+
+		/*
+		 * If passed a "read_only" mount option, remove it because it is
+		 * no longer a valid mount option, and the filesystem will be
+		 * set "read_only" regardless.
+		 */
+		ro = strstr(optstr, "read_only");
+		if (ro) {
+			rest = ro + strlen("read_only");
+			memmove(ro, rest, strlen(rest) + 1);
+		}
 
 		ret =   PTR_ERR_OR_ZERO(optstr) ?:
 			bch2_parse_mount_opts(NULL, &thr->opts, optstr);
@@ -223,6 +235,7 @@ static long bch2_ioctl_fsck_offline(struct bch_ioctl_fsck_offline __user *user_a
 	}
 
 	opt_set(thr->opts, stdio, (u64)(unsigned long)&thr->thr.stdio);
+	opt_set(thr->opts, read_only, 1);
 
 	/* We need request_key() to be called before we punt to kthread: */
 	opt_set(thr->opts, nostart, true);
-- 
2.45.0

[PATCH 2/2] bcachefs: don't expose "read_only" as a mount option

[Thomas Bertschinger]

When "read_only" is exposed as a mount option, it is redundant with the
standard option "ro" and gives users multiple ways to specify that a
bcachefs filesystem should be mounted read-only. This presents the risk
of having inconsistent options specified.

This can be seen when remounting a read-only filesystem in read-write
mode, using mount(8) from util-linux. Because mount(8) parses the
existing mount options from `/proc/mounts` and applies them when
remounting, it can end up applying both "read_only" and "rw":

$ mount img -o ro /mnt
$ strace mount -o remount,rw /mnt
...
fsconfig(4, FSCONFIG_SET_FLAG, "read_only", NULL, 0) = 0
fsconfig(4, FSCONFIG_SET_FLAG, "rw", NULL, 0) = 0
...

Making "read_only" no longer a mount option means this edge case cannot
occur.

Fixes: 62719cf33c3a ("bcachefs: Fix nochanges/read_only interaction")
Signed-off-by: Thomas Bertschinger <tahbertschinger@gmail.com>
---
 fs/bcachefs/opts.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/bcachefs/opts.h b/fs/bcachefs/opts.h
index 25530e0bb2f3..f902793e1810 100644
--- a/fs/bcachefs/opts.h
+++ b/fs/bcachefs/opts.h
@@ -406,7 +406,7 @@ enum fsck_err_opts {
 	  BCH2_NO_SB_OPT,		BCH_SB_SECTOR,			\
 	  "offset",	"Sector offset of superblock")			\
 	x(read_only,			u8,				\
-	  OPT_FS|OPT_MOUNT,						\
+	  OPT_FS,							\
 	  OPT_BOOL(),							\
 	  BCH2_NO_SB_OPT,		false,				\
 	  NULL,		NULL)						\
-- 
2.45.0

Re: [PATCH 1/2] bcachefs: make offline fsck set read_only fs flag

[Kent Overstreet]

On Sun, May 26, 2024 at 01:08:19PM -0600, Thomas Bertschinger wrote:
> A subsequent change will remove "read_only" as a mount option in favor
> of the standard option "ro", meaning the userspace fsck command cannot
> pass it to the fsck ioctl. Instead, in offline fsck, set "read_only"
> kernel-side without trying to parse it as a mount option.
> 
> For compatibility with versions of the "bcachefs fsck" command that try
> to pass the "read_only" mount opt, remove it from the mount options
> string prior to parsing when it is present.
> 
> Signed-off-by: Thomas Bertschinger <tahbertschinger@gmail.com>

these are applied
> ---
>  fs/bcachefs/chardev.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/fs/bcachefs/chardev.c b/fs/bcachefs/chardev.c
> index 9e54323f0f5f..5040c584cd72 100644
> --- a/fs/bcachefs/chardev.c
> +++ b/fs/bcachefs/chardev.c
> @@ -213,6 +213,18 @@ static long bch2_ioctl_fsck_offline(struct bch_ioctl_fsck_offline __user *user_a
>  
>  	if (arg.opts) {
>  		char *optstr = strndup_user((char __user *)(unsigned long) arg.opts, 1 << 16);
> +		char *ro, *rest;
> +
> +		/*
> +		 * If passed a "read_only" mount option, remove it because it is
> +		 * no longer a valid mount option, and the filesystem will be
> +		 * set "read_only" regardless.
> +		 */
> +		ro = strstr(optstr, "read_only");
> +		if (ro) {
> +			rest = ro + strlen("read_only");
> +			memmove(ro, rest, strlen(rest) + 1);
> +		}
>  
>  		ret =   PTR_ERR_OR_ZERO(optstr) ?:
>  			bch2_parse_mount_opts(NULL, &thr->opts, optstr);
> @@ -223,6 +235,7 @@ static long bch2_ioctl_fsck_offline(struct bch_ioctl_fsck_offline __user *user_a
>  	}
>  
>  	opt_set(thr->opts, stdio, (u64)(unsigned long)&thr->thr.stdio);
> +	opt_set(thr->opts, read_only, 1);
>  
>  	/* We need request_key() to be called before we punt to kthread: */
>  	opt_set(thr->opts, nostart, true);
> -- 
> 2.45.0
>