diff mbox series

[3/3] btrfs: migrate to the newer memparse_safe() helper

Message ID	6dfa53ded887caa2269c1beeaedcff086342339a.1703324146.git.wqu@suse.com
State	New
Headers	Received-SPF: pass (google.com: domain of linux-kernel+bounces-10372-ouuuleilei=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99; From: Qu Wenruo <wqu@suse.com> To: linux-btrfs@vger.kernel.org, linux-kernel@vger.kernel.org, akpm@linux-foundation.org, christophe.jaillet@wanadoo.fr, andriy.shevchenko@linux.intel.com, David.Laight@ACULAB.COM, ddiss@suse.de Subject: [PATCH 3/3] btrfs: migrate to the newer memparse_safe() helper Date: Sat, 23 Dec 2023 20:28:07 +1030 Message-ID: <6dfa53ded887caa2269c1beeaedcff086342339a.1703324146.git.wqu@suse.com> In-Reply-To: <cover.1703324146.git.wqu@suse.com> References: <cover.1703324146.git.wqu@suse.com> Precedence: bulk MIME-Version: 1.0 Content-Transfer-Encoding: 8bit
Series	kstrtox: introduce memparse_safe() \| [0/3] kstrtox: introduce memparse_safe() [1/3] kstrtox: introduce a safer version of memparse() [2/3] kstrtox: add unit tests for memparse_safe() [3/3] btrfs: migrate to the newer memparse_safe() helper

Commit Message

Qu Wenruo Dec. 23, 2023, 9:58 a.m. UTC

  The new helper has better error report and correct overflow detection,
furthermore the old @retptr behavior is also kept, thus there should be
no behavior change.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/ioctl.c |  8 ++++++--
 fs/btrfs/super.c |  8 ++++++++
 fs/btrfs/sysfs.c | 14 +++++++++++---
 3 files changed, 25 insertions(+), 5 deletions(-)

Comments

kernel test robot Dec. 26, 2023, 4:53 a.m. UTC | #1

Hi Qu,

kernel test robot noticed the following build warnings:

[auto build test WARNING on kdave/for-next]
[also build test WARNING on next-20231222]
[cannot apply to akpm-mm/mm-nonmm-unstable akpm-mm/mm-everything linus/master v6.7-rc7]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Qu-Wenruo/kstrtox-introduce-a-safer-version-of-memparse/20231225-151921
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
patch link:    https://lore.kernel.org/r/6dfa53ded887caa2269c1beeaedcff086342339a.1703324146.git.wqu%40suse.com
patch subject: [PATCH 3/3] btrfs: migrate to the newer memparse_safe() helper
config: powerpc-allmodconfig (https://download.01.org/0day-ci/archive/20231226/202312261215.eFf9J1xV-lkp@intel.com/config)
compiler: clang version 18.0.0git (https://github.com/llvm/llvm-project d3ef86708241a3bee902615c190dead1638c4e09)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231226/202312261215.eFf9J1xV-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312261215.eFf9J1xV-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> fs/btrfs/super.c:403:3: warning: label followed by a declaration is a C23 extension [-Wc23-extensions]
     403 |                 int ret;
         |                 ^
   1 warning generated.


vim +403 fs/btrfs/super.c

   261	
   262	static int btrfs_parse_param(struct fs_context *fc, struct fs_parameter *param)
   263	{
   264		struct btrfs_fs_context *ctx = fc->fs_private;
   265		struct fs_parse_result result;
   266		int opt;
   267	
   268		opt = fs_parse(fc, btrfs_fs_parameters, param, &result);
   269		if (opt < 0)
   270			return opt;
   271	
   272		switch (opt) {
   273		case Opt_degraded:
   274			btrfs_set_opt(ctx->mount_opt, DEGRADED);
   275			break;
   276		case Opt_subvol_empty:
   277			/*
   278			 * This exists because we used to allow it on accident, so we're
   279			 * keeping it to maintain ABI.  See 37becec95ac3 ("Btrfs: allow
   280			 * empty subvol= again").
   281			 */
   282			break;
   283		case Opt_subvol:
   284			kfree(ctx->subvol_name);
   285			ctx->subvol_name = kstrdup(param->string, GFP_KERNEL);
   286			if (!ctx->subvol_name)
   287				return -ENOMEM;
   288			break;
   289		case Opt_subvolid:
   290			ctx->subvol_objectid = result.uint_64;
   291	
   292			/* subvolid=0 means give me the original fs_tree. */
   293			if (!ctx->subvol_objectid)
   294				ctx->subvol_objectid = BTRFS_FS_TREE_OBJECTID;
   295			break;
   296		case Opt_device: {
   297			struct btrfs_device *device;
   298			blk_mode_t mode = sb_open_mode(fc->sb_flags);
   299	
   300			mutex_lock(&uuid_mutex);
   301			device = btrfs_scan_one_device(param->string, mode, false);
   302			mutex_unlock(&uuid_mutex);
   303			if (IS_ERR(device))
   304				return PTR_ERR(device);
   305			break;
   306		}
   307		case Opt_datasum:
   308			if (result.negated) {
   309				btrfs_set_opt(ctx->mount_opt, NODATASUM);
   310			} else {
   311				btrfs_clear_opt(ctx->mount_opt, NODATACOW);
   312				btrfs_clear_opt(ctx->mount_opt, NODATASUM);
   313			}
   314			break;
   315		case Opt_datacow:
   316			if (result.negated) {
   317				btrfs_clear_opt(ctx->mount_opt, COMPRESS);
   318				btrfs_clear_opt(ctx->mount_opt, FORCE_COMPRESS);
   319				btrfs_set_opt(ctx->mount_opt, NODATACOW);
   320				btrfs_set_opt(ctx->mount_opt, NODATASUM);
   321			} else {
   322				btrfs_clear_opt(ctx->mount_opt, NODATACOW);
   323			}
   324			break;
   325		case Opt_compress_force:
   326		case Opt_compress_force_type:
   327			btrfs_set_opt(ctx->mount_opt, FORCE_COMPRESS);
   328			fallthrough;
   329		case Opt_compress:
   330		case Opt_compress_type:
   331			if (opt == Opt_compress || opt == Opt_compress_force) {
   332				ctx->compress_type = BTRFS_COMPRESS_ZLIB;
   333				ctx->compress_level = BTRFS_ZLIB_DEFAULT_LEVEL;
   334				btrfs_set_opt(ctx->mount_opt, COMPRESS);
   335				btrfs_clear_opt(ctx->mount_opt, NODATACOW);
   336				btrfs_clear_opt(ctx->mount_opt, NODATASUM);
   337			} else if (strncmp(param->string, "zlib", 4) == 0) {
   338				ctx->compress_type = BTRFS_COMPRESS_ZLIB;
   339				ctx->compress_level =
   340					btrfs_compress_str2level(BTRFS_COMPRESS_ZLIB,
   341								 param->string + 4);
   342				btrfs_set_opt(ctx->mount_opt, COMPRESS);
   343				btrfs_clear_opt(ctx->mount_opt, NODATACOW);
   344				btrfs_clear_opt(ctx->mount_opt, NODATASUM);
   345			} else if (strncmp(param->string, "lzo", 3) == 0) {
   346				ctx->compress_type = BTRFS_COMPRESS_LZO;
   347				ctx->compress_level = 0;
   348				btrfs_set_opt(ctx->mount_opt, COMPRESS);
   349				btrfs_clear_opt(ctx->mount_opt, NODATACOW);
   350				btrfs_clear_opt(ctx->mount_opt, NODATASUM);
   351			} else if (strncmp(param->string, "zstd", 4) == 0) {
   352				ctx->compress_type = BTRFS_COMPRESS_ZSTD;
   353				ctx->compress_level =
   354					btrfs_compress_str2level(BTRFS_COMPRESS_ZSTD,
   355								 param->string + 4);
   356				btrfs_set_opt(ctx->mount_opt, COMPRESS);
   357				btrfs_clear_opt(ctx->mount_opt, NODATACOW);
   358				btrfs_clear_opt(ctx->mount_opt, NODATASUM);
   359			} else if (strncmp(param->string, "no", 2) == 0) {
   360				ctx->compress_level = 0;
   361				ctx->compress_type = 0;
   362				btrfs_clear_opt(ctx->mount_opt, COMPRESS);
   363				btrfs_clear_opt(ctx->mount_opt, FORCE_COMPRESS);
   364			} else {
   365				btrfs_err(NULL, "unrecognized compression value %s",
   366					  param->string);
   367				return -EINVAL;
   368			}
   369			break;
   370		case Opt_ssd:
   371			if (result.negated) {
   372				btrfs_set_opt(ctx->mount_opt, NOSSD);
   373				btrfs_clear_opt(ctx->mount_opt, SSD);
   374				btrfs_clear_opt(ctx->mount_opt, SSD_SPREAD);
   375			} else {
   376				btrfs_set_opt(ctx->mount_opt, SSD);
   377				btrfs_clear_opt(ctx->mount_opt, NOSSD);
   378			}
   379			break;
   380		case Opt_ssd_spread:
   381			if (result.negated) {
   382				btrfs_clear_opt(ctx->mount_opt, SSD_SPREAD);
   383			} else {
   384				btrfs_set_opt(ctx->mount_opt, SSD);
   385				btrfs_set_opt(ctx->mount_opt, SSD_SPREAD);
   386				btrfs_clear_opt(ctx->mount_opt, NOSSD);
   387			}
   388			break;
   389		case Opt_barrier:
   390			if (result.negated)
   391				btrfs_set_opt(ctx->mount_opt, NOBARRIER);
   392			else
   393				btrfs_clear_opt(ctx->mount_opt, NOBARRIER);
   394			break;
   395		case Opt_thread_pool:
   396			if (result.uint_32 == 0) {
   397				btrfs_err(NULL, "invalid value 0 for thread_pool");
   398				return -EINVAL;
   399			}
   400			ctx->thread_pool_size = result.uint_32;
   401			break;
   402		case Opt_max_inline:
 > 403			int ret;
   404	
   405			ret = memparse_safe(param->string, MEMPARSE_SUFFIXES_DEFAULT,
   406					    &ctx->max_inline, NULL);
   407			if (ret < 0) {
   408				btrfs_err(NULL, "invalid string \"%s\"", param->string);
   409				return ret;
   410			}
   411			ctx->max_inline = memparse(param->string, NULL);
   412			break;
   413		case Opt_acl:
   414			if (result.negated) {
   415				fc->sb_flags &= ~SB_POSIXACL;
   416			} else {
   417	#ifdef CONFIG_BTRFS_FS_POSIX_ACL
   418				fc->sb_flags |= SB_POSIXACL;
   419	#else
   420				btrfs_err(NULL, "support for ACL not compiled in");
   421				return -EINVAL;
   422	#endif
   423			}
   424			/*
   425			 * VFS limits the ability to toggle ACL on and off via remount,
   426			 * despite every file system allowing this.  This seems to be
   427			 * an oversight since we all do, but it'll fail if we're
   428			 * remounting.  So don't set the mask here, we'll check it in
   429			 * btrfs_reconfigure and do the toggling ourselves.
   430			 */
   431			if (fc->purpose != FS_CONTEXT_FOR_RECONFIGURE)
   432				fc->sb_flags_mask |= SB_POSIXACL;
   433			break;
   434		case Opt_treelog:
   435			if (result.negated)
   436				btrfs_set_opt(ctx->mount_opt, NOTREELOG);
   437			else
   438				btrfs_clear_opt(ctx->mount_opt, NOTREELOG);
   439			break;
   440		case Opt_nologreplay:
   441			btrfs_warn(NULL,
   442			"'nologreplay' is deprecated, use 'rescue=nologreplay' instead");
   443			btrfs_set_opt(ctx->mount_opt, NOLOGREPLAY);
   444			break;
   445		case Opt_flushoncommit:
   446			if (result.negated)
   447				btrfs_clear_opt(ctx->mount_opt, FLUSHONCOMMIT);
   448			else
   449				btrfs_set_opt(ctx->mount_opt, FLUSHONCOMMIT);
   450			break;
   451		case Opt_ratio:
   452			ctx->metadata_ratio = result.uint_32;
   453			break;
   454		case Opt_discard:
   455			if (result.negated) {
   456				btrfs_clear_opt(ctx->mount_opt, DISCARD_SYNC);
   457				btrfs_clear_opt(ctx->mount_opt, DISCARD_ASYNC);
   458				btrfs_set_opt(ctx->mount_opt, NODISCARD);
   459			} else {
   460				btrfs_set_opt(ctx->mount_opt, DISCARD_SYNC);
   461				btrfs_clear_opt(ctx->mount_opt, DISCARD_ASYNC);
   462			}
   463			break;
   464		case Opt_discard_mode:
   465			switch (result.uint_32) {
   466			case Opt_discard_sync:
   467				btrfs_clear_opt(ctx->mount_opt, DISCARD_ASYNC);
   468				btrfs_set_opt(ctx->mount_opt, DISCARD_SYNC);
   469				break;
   470			case Opt_discard_async:
   471				btrfs_clear_opt(ctx->mount_opt, DISCARD_SYNC);
   472				btrfs_set_opt(ctx->mount_opt, DISCARD_ASYNC);
   473				break;
   474			default:
   475				btrfs_err(NULL, "unrecognized discard mode value %s",
   476					  param->key);
   477				return -EINVAL;
   478			}
   479			btrfs_clear_opt(ctx->mount_opt, NODISCARD);
   480			break;
   481		case Opt_space_cache:
   482			if (result.negated) {
   483				btrfs_set_opt(ctx->mount_opt, NOSPACECACHE);
   484				btrfs_clear_opt(ctx->mount_opt, SPACE_CACHE);
   485				btrfs_clear_opt(ctx->mount_opt, FREE_SPACE_TREE);
   486			} else {
   487				btrfs_clear_opt(ctx->mount_opt, FREE_SPACE_TREE);
   488				btrfs_set_opt(ctx->mount_opt, SPACE_CACHE);
   489			}
   490			break;
   491		case Opt_space_cache_version:
   492			switch (result.uint_32) {
   493			case Opt_space_cache_v1:
   494				btrfs_set_opt(ctx->mount_opt, SPACE_CACHE);
   495				btrfs_clear_opt(ctx->mount_opt, FREE_SPACE_TREE);
   496				break;
   497			case Opt_space_cache_v2:
   498				btrfs_clear_opt(ctx->mount_opt, SPACE_CACHE);
   499				btrfs_set_opt(ctx->mount_opt, FREE_SPACE_TREE);
   500				break;
   501			default:
   502				btrfs_err(NULL, "unrecognized space_cache value %s",
   503					  param->key);
   504				return -EINVAL;
   505			}
   506			break;
   507		case Opt_rescan_uuid_tree:
   508			btrfs_set_opt(ctx->mount_opt, RESCAN_UUID_TREE);
   509			break;
   510		case Opt_clear_cache:
   511			btrfs_set_opt(ctx->mount_opt, CLEAR_CACHE);
   512			break;
   513		case Opt_user_subvol_rm_allowed:
   514			btrfs_set_opt(ctx->mount_opt, USER_SUBVOL_RM_ALLOWED);
   515			break;
   516		case Opt_enospc_debug:
   517			if (result.negated)
   518				btrfs_clear_opt(ctx->mount_opt, ENOSPC_DEBUG);
   519			else
   520				btrfs_set_opt(ctx->mount_opt, ENOSPC_DEBUG);
   521			break;
   522		case Opt_defrag:
   523			if (result.negated)
   524				btrfs_clear_opt(ctx->mount_opt, AUTO_DEFRAG);
   525			else
   526				btrfs_set_opt(ctx->mount_opt, AUTO_DEFRAG);
   527			break;
   528		case Opt_usebackuproot:
   529			btrfs_warn(NULL,
   530				   "'usebackuproot' is deprecated, use 'rescue=usebackuproot' instead");
   531			btrfs_set_opt(ctx->mount_opt, USEBACKUPROOT);
   532	
   533			/* If we're loading the backup roots we can't trust the space cache. */
   534			btrfs_set_opt(ctx->mount_opt, CLEAR_CACHE);
   535			break;
   536		case Opt_skip_balance:
   537			btrfs_set_opt(ctx->mount_opt, SKIP_BALANCE);
   538			break;
   539		case Opt_fatal_errors:
   540			switch (result.uint_32) {
   541			case Opt_fatal_errors_panic:
   542				btrfs_set_opt(ctx->mount_opt, PANIC_ON_FATAL_ERROR);
   543				break;
   544			case Opt_fatal_errors_bug:
   545				btrfs_clear_opt(ctx->mount_opt, PANIC_ON_FATAL_ERROR);
   546				break;
   547			default:
   548				btrfs_err(NULL, "unrecognized fatal_errors value %s",
   549					  param->key);
   550				return -EINVAL;
   551			}
   552			break;
   553		case Opt_commit_interval:
   554			ctx->commit_interval = result.uint_32;
   555			if (ctx->commit_interval == 0)
   556				ctx->commit_interval = BTRFS_DEFAULT_COMMIT_INTERVAL;
   557			break;
   558		case Opt_rescue:
   559			switch (result.uint_32) {
   560			case Opt_rescue_usebackuproot:
   561				btrfs_set_opt(ctx->mount_opt, USEBACKUPROOT);
   562				break;
   563			case Opt_rescue_nologreplay:
   564				btrfs_set_opt(ctx->mount_opt, NOLOGREPLAY);
   565				break;
   566			case Opt_rescue_ignorebadroots:
   567				btrfs_set_opt(ctx->mount_opt, IGNOREBADROOTS);
   568				break;
   569			case Opt_rescue_ignoredatacsums:
   570				btrfs_set_opt(ctx->mount_opt, IGNOREDATACSUMS);
   571				break;
   572			case Opt_rescue_parameter_all:
   573				btrfs_set_opt(ctx->mount_opt, IGNOREDATACSUMS);
   574				btrfs_set_opt(ctx->mount_opt, IGNOREBADROOTS);
   575				btrfs_set_opt(ctx->mount_opt, NOLOGREPLAY);
   576				break;
   577			default:
   578				btrfs_info(NULL, "unrecognized rescue option '%s'",
   579					   param->key);
   580				return -EINVAL;
   581			}
   582			break;
   583	#ifdef CONFIG_BTRFS_DEBUG
   584		case Opt_fragment:
   585			switch (result.uint_32) {
   586			case Opt_fragment_parameter_all:
   587				btrfs_set_opt(ctx->mount_opt, FRAGMENT_DATA);
   588				btrfs_set_opt(ctx->mount_opt, FRAGMENT_METADATA);
   589				break;
   590			case Opt_fragment_parameter_metadata:
   591				btrfs_set_opt(ctx->mount_opt, FRAGMENT_METADATA);
   592				break;
   593			case Opt_fragment_parameter_data:
   594				btrfs_set_opt(ctx->mount_opt, FRAGMENT_DATA);
   595				break;
   596			default:
   597				btrfs_info(NULL, "unrecognized fragment option '%s'",
   598					   param->key);
   599				return -EINVAL;
   600			}
   601			break;
   602	#endif
   603	#ifdef CONFIG_BTRFS_FS_REF_VERIFY
   604		case Opt_ref_verify:
   605			btrfs_set_opt(ctx->mount_opt, REF_VERIFY);
   606			break;
   607	#endif
   608		default:
   609			btrfs_err(NULL, "unrecognized mount option '%s'", param->key);
   610			return -EINVAL;
   611		}
   612	
   613		return 0;
   614	}
   615

kernel test robot Dec. 26, 2023, 6:06 a.m. UTC | #2

Hi Qu,

kernel test robot noticed the following build errors:

[auto build test ERROR on kdave/for-next]
[also build test ERROR on next-20231222]
[cannot apply to akpm-mm/mm-nonmm-unstable akpm-mm/mm-everything linus/master v6.7-rc7]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Qu-Wenruo/kstrtox-introduce-a-safer-version-of-memparse/20231225-151921
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
patch link:    https://lore.kernel.org/r/6dfa53ded887caa2269c1beeaedcff086342339a.1703324146.git.wqu%40suse.com
patch subject: [PATCH 3/3] btrfs: migrate to the newer memparse_safe() helper
config: x86_64-rhel-8.3-rust (https://download.01.org/0day-ci/archive/20231226/202312261319.pYgA0ivL-lkp@intel.com/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231226/202312261319.pYgA0ivL-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312261319.pYgA0ivL-lkp@intel.com/

All errors (new ones prefixed by >>):

>> fs/btrfs/super.c:403:3: error: expected expression
                   int ret;
                   ^
>> fs/btrfs/super.c:405:3: error: use of undeclared identifier 'ret'
                   ret = memparse_safe(param->string, MEMPARSE_SUFFIXES_DEFAULT,
                   ^
   fs/btrfs/super.c:407:7: error: use of undeclared identifier 'ret'
                   if (ret < 0) {
                       ^
   fs/btrfs/super.c:409:11: error: use of undeclared identifier 'ret'
                           return ret;
                                  ^
   4 errors generated.


vim +403 fs/btrfs/super.c

   261	
   262	static int btrfs_parse_param(struct fs_context *fc, struct fs_parameter *param)
   263	{
   264		struct btrfs_fs_context *ctx = fc->fs_private;
   265		struct fs_parse_result result;
   266		int opt;
   267	
   268		opt = fs_parse(fc, btrfs_fs_parameters, param, &result);
   269		if (opt < 0)
   270			return opt;
   271	
   272		switch (opt) {
   273		case Opt_degraded:
   274			btrfs_set_opt(ctx->mount_opt, DEGRADED);
   275			break;
   276		case Opt_subvol_empty:
   277			/*
   278			 * This exists because we used to allow it on accident, so we're
   279			 * keeping it to maintain ABI.  See 37becec95ac3 ("Btrfs: allow
   280			 * empty subvol= again").
   281			 */
   282			break;
   283		case Opt_subvol:
   284			kfree(ctx->subvol_name);
   285			ctx->subvol_name = kstrdup(param->string, GFP_KERNEL);
   286			if (!ctx->subvol_name)
   287				return -ENOMEM;
   288			break;
   289		case Opt_subvolid:
   290			ctx->subvol_objectid = result.uint_64;
   291	
   292			/* subvolid=0 means give me the original fs_tree. */
   293			if (!ctx->subvol_objectid)
   294				ctx->subvol_objectid = BTRFS_FS_TREE_OBJECTID;
   295			break;
   296		case Opt_device: {
   297			struct btrfs_device *device;
   298			blk_mode_t mode = sb_open_mode(fc->sb_flags);
   299	
   300			mutex_lock(&uuid_mutex);
   301			device = btrfs_scan_one_device(param->string, mode, false);
   302			mutex_unlock(&uuid_mutex);
   303			if (IS_ERR(device))
   304				return PTR_ERR(device);
   305			break;
   306		}
   307		case Opt_datasum:
   308			if (result.negated) {
   309				btrfs_set_opt(ctx->mount_opt, NODATASUM);
   310			} else {
   311				btrfs_clear_opt(ctx->mount_opt, NODATACOW);
   312				btrfs_clear_opt(ctx->mount_opt, NODATASUM);
   313			}
   314			break;
   315		case Opt_datacow:
   316			if (result.negated) {
   317				btrfs_clear_opt(ctx->mount_opt, COMPRESS);
   318				btrfs_clear_opt(ctx->mount_opt, FORCE_COMPRESS);
   319				btrfs_set_opt(ctx->mount_opt, NODATACOW);
   320				btrfs_set_opt(ctx->mount_opt, NODATASUM);
   321			} else {
   322				btrfs_clear_opt(ctx->mount_opt, NODATACOW);
   323			}
   324			break;
   325		case Opt_compress_force:
   326		case Opt_compress_force_type:
   327			btrfs_set_opt(ctx->mount_opt, FORCE_COMPRESS);
   328			fallthrough;
   329		case Opt_compress:
   330		case Opt_compress_type:
   331			if (opt == Opt_compress || opt == Opt_compress_force) {
   332				ctx->compress_type = BTRFS_COMPRESS_ZLIB;
   333				ctx->compress_level = BTRFS_ZLIB_DEFAULT_LEVEL;
   334				btrfs_set_opt(ctx->mount_opt, COMPRESS);
   335				btrfs_clear_opt(ctx->mount_opt, NODATACOW);
   336				btrfs_clear_opt(ctx->mount_opt, NODATASUM);
   337			} else if (strncmp(param->string, "zlib", 4) == 0) {
   338				ctx->compress_type = BTRFS_COMPRESS_ZLIB;
   339				ctx->compress_level =
   340					btrfs_compress_str2level(BTRFS_COMPRESS_ZLIB,
   341								 param->string + 4);
   342				btrfs_set_opt(ctx->mount_opt, COMPRESS);
   343				btrfs_clear_opt(ctx->mount_opt, NODATACOW);
   344				btrfs_clear_opt(ctx->mount_opt, NODATASUM);
   345			} else if (strncmp(param->string, "lzo", 3) == 0) {
   346				ctx->compress_type = BTRFS_COMPRESS_LZO;
   347				ctx->compress_level = 0;
   348				btrfs_set_opt(ctx->mount_opt, COMPRESS);
   349				btrfs_clear_opt(ctx->mount_opt, NODATACOW);
   350				btrfs_clear_opt(ctx->mount_opt, NODATASUM);
   351			} else if (strncmp(param->string, "zstd", 4) == 0) {
   352				ctx->compress_type = BTRFS_COMPRESS_ZSTD;
   353				ctx->compress_level =
   354					btrfs_compress_str2level(BTRFS_COMPRESS_ZSTD,
   355								 param->string + 4);
   356				btrfs_set_opt(ctx->mount_opt, COMPRESS);
   357				btrfs_clear_opt(ctx->mount_opt, NODATACOW);
   358				btrfs_clear_opt(ctx->mount_opt, NODATASUM);
   359			} else if (strncmp(param->string, "no", 2) == 0) {
   360				ctx->compress_level = 0;
   361				ctx->compress_type = 0;
   362				btrfs_clear_opt(ctx->mount_opt, COMPRESS);
   363				btrfs_clear_opt(ctx->mount_opt, FORCE_COMPRESS);
   364			} else {
   365				btrfs_err(NULL, "unrecognized compression value %s",
   366					  param->string);
   367				return -EINVAL;
   368			}
   369			break;
   370		case Opt_ssd:
   371			if (result.negated) {
   372				btrfs_set_opt(ctx->mount_opt, NOSSD);
   373				btrfs_clear_opt(ctx->mount_opt, SSD);
   374				btrfs_clear_opt(ctx->mount_opt, SSD_SPREAD);
   375			} else {
   376				btrfs_set_opt(ctx->mount_opt, SSD);
   377				btrfs_clear_opt(ctx->mount_opt, NOSSD);
   378			}
   379			break;
   380		case Opt_ssd_spread:
   381			if (result.negated) {
   382				btrfs_clear_opt(ctx->mount_opt, SSD_SPREAD);
   383			} else {
   384				btrfs_set_opt(ctx->mount_opt, SSD);
   385				btrfs_set_opt(ctx->mount_opt, SSD_SPREAD);
   386				btrfs_clear_opt(ctx->mount_opt, NOSSD);
   387			}
   388			break;
   389		case Opt_barrier:
   390			if (result.negated)
   391				btrfs_set_opt(ctx->mount_opt, NOBARRIER);
   392			else
   393				btrfs_clear_opt(ctx->mount_opt, NOBARRIER);
   394			break;
   395		case Opt_thread_pool:
   396			if (result.uint_32 == 0) {
   397				btrfs_err(NULL, "invalid value 0 for thread_pool");
   398				return -EINVAL;
   399			}
   400			ctx->thread_pool_size = result.uint_32;
   401			break;
   402		case Opt_max_inline:
 > 403			int ret;
   404	
 > 405			ret = memparse_safe(param->string, MEMPARSE_SUFFIXES_DEFAULT,
   406					    &ctx->max_inline, NULL);
   407			if (ret < 0) {
   408				btrfs_err(NULL, "invalid string \"%s\"", param->string);
   409				return ret;
   410			}
   411			ctx->max_inline = memparse(param->string, NULL);
   412			break;
   413		case Opt_acl:
   414			if (result.negated) {
   415				fc->sb_flags &= ~SB_POSIXACL;
   416			} else {
   417	#ifdef CONFIG_BTRFS_FS_POSIX_ACL
   418				fc->sb_flags |= SB_POSIXACL;
   419	#else
   420				btrfs_err(NULL, "support for ACL not compiled in");
   421				return -EINVAL;
   422	#endif
   423			}
   424			/*
   425			 * VFS limits the ability to toggle ACL on and off via remount,
   426			 * despite every file system allowing this.  This seems to be
   427			 * an oversight since we all do, but it'll fail if we're
   428			 * remounting.  So don't set the mask here, we'll check it in
   429			 * btrfs_reconfigure and do the toggling ourselves.
   430			 */
   431			if (fc->purpose != FS_CONTEXT_FOR_RECONFIGURE)
   432				fc->sb_flags_mask |= SB_POSIXACL;
   433			break;
   434		case Opt_treelog:
   435			if (result.negated)
   436				btrfs_set_opt(ctx->mount_opt, NOTREELOG);
   437			else
   438				btrfs_clear_opt(ctx->mount_opt, NOTREELOG);
   439			break;
   440		case Opt_nologreplay:
   441			btrfs_warn(NULL,
   442			"'nologreplay' is deprecated, use 'rescue=nologreplay' instead");
   443			btrfs_set_opt(ctx->mount_opt, NOLOGREPLAY);
   444			break;
   445		case Opt_flushoncommit:
   446			if (result.negated)
   447				btrfs_clear_opt(ctx->mount_opt, FLUSHONCOMMIT);
   448			else
   449				btrfs_set_opt(ctx->mount_opt, FLUSHONCOMMIT);
   450			break;
   451		case Opt_ratio:
   452			ctx->metadata_ratio = result.uint_32;
   453			break;
   454		case Opt_discard:
   455			if (result.negated) {
   456				btrfs_clear_opt(ctx->mount_opt, DISCARD_SYNC);
   457				btrfs_clear_opt(ctx->mount_opt, DISCARD_ASYNC);
   458				btrfs_set_opt(ctx->mount_opt, NODISCARD);
   459			} else {
   460				btrfs_set_opt(ctx->mount_opt, DISCARD_SYNC);
   461				btrfs_clear_opt(ctx->mount_opt, DISCARD_ASYNC);
   462			}
   463			break;
   464		case Opt_discard_mode:
   465			switch (result.uint_32) {
   466			case Opt_discard_sync:
   467				btrfs_clear_opt(ctx->mount_opt, DISCARD_ASYNC);
   468				btrfs_set_opt(ctx->mount_opt, DISCARD_SYNC);
   469				break;
   470			case Opt_discard_async:
   471				btrfs_clear_opt(ctx->mount_opt, DISCARD_SYNC);
   472				btrfs_set_opt(ctx->mount_opt, DISCARD_ASYNC);
   473				break;
   474			default:
   475				btrfs_err(NULL, "unrecognized discard mode value %s",
   476					  param->key);
   477				return -EINVAL;
   478			}
   479			btrfs_clear_opt(ctx->mount_opt, NODISCARD);
   480			break;
   481		case Opt_space_cache:
   482			if (result.negated) {
   483				btrfs_set_opt(ctx->mount_opt, NOSPACECACHE);
   484				btrfs_clear_opt(ctx->mount_opt, SPACE_CACHE);
   485				btrfs_clear_opt(ctx->mount_opt, FREE_SPACE_TREE);
   486			} else {
   487				btrfs_clear_opt(ctx->mount_opt, FREE_SPACE_TREE);
   488				btrfs_set_opt(ctx->mount_opt, SPACE_CACHE);
   489			}
   490			break;
   491		case Opt_space_cache_version:
   492			switch (result.uint_32) {
   493			case Opt_space_cache_v1:
   494				btrfs_set_opt(ctx->mount_opt, SPACE_CACHE);
   495				btrfs_clear_opt(ctx->mount_opt, FREE_SPACE_TREE);
   496				break;
   497			case Opt_space_cache_v2:
   498				btrfs_clear_opt(ctx->mount_opt, SPACE_CACHE);
   499				btrfs_set_opt(ctx->mount_opt, FREE_SPACE_TREE);
   500				break;
   501			default:
   502				btrfs_err(NULL, "unrecognized space_cache value %s",
   503					  param->key);
   504				return -EINVAL;
   505			}
   506			break;
   507		case Opt_rescan_uuid_tree:
   508			btrfs_set_opt(ctx->mount_opt, RESCAN_UUID_TREE);
   509			break;
   510		case Opt_clear_cache:
   511			btrfs_set_opt(ctx->mount_opt, CLEAR_CACHE);
   512			break;
   513		case Opt_user_subvol_rm_allowed:
   514			btrfs_set_opt(ctx->mount_opt, USER_SUBVOL_RM_ALLOWED);
   515			break;
   516		case Opt_enospc_debug:
   517			if (result.negated)
   518				btrfs_clear_opt(ctx->mount_opt, ENOSPC_DEBUG);
   519			else
   520				btrfs_set_opt(ctx->mount_opt, ENOSPC_DEBUG);
   521			break;
   522		case Opt_defrag:
   523			if (result.negated)
   524				btrfs_clear_opt(ctx->mount_opt, AUTO_DEFRAG);
   525			else
   526				btrfs_set_opt(ctx->mount_opt, AUTO_DEFRAG);
   527			break;
   528		case Opt_usebackuproot:
   529			btrfs_warn(NULL,
   530				   "'usebackuproot' is deprecated, use 'rescue=usebackuproot' instead");
   531			btrfs_set_opt(ctx->mount_opt, USEBACKUPROOT);
   532	
   533			/* If we're loading the backup roots we can't trust the space cache. */
   534			btrfs_set_opt(ctx->mount_opt, CLEAR_CACHE);
   535			break;
   536		case Opt_skip_balance:
   537			btrfs_set_opt(ctx->mount_opt, SKIP_BALANCE);
   538			break;
   539		case Opt_fatal_errors:
   540			switch (result.uint_32) {
   541			case Opt_fatal_errors_panic:
   542				btrfs_set_opt(ctx->mount_opt, PANIC_ON_FATAL_ERROR);
   543				break;
   544			case Opt_fatal_errors_bug:
   545				btrfs_clear_opt(ctx->mount_opt, PANIC_ON_FATAL_ERROR);
   546				break;
   547			default:
   548				btrfs_err(NULL, "unrecognized fatal_errors value %s",
   549					  param->key);
   550				return -EINVAL;
   551			}
   552			break;
   553		case Opt_commit_interval:
   554			ctx->commit_interval = result.uint_32;
   555			if (ctx->commit_interval == 0)
   556				ctx->commit_interval = BTRFS_DEFAULT_COMMIT_INTERVAL;
   557			break;
   558		case Opt_rescue:
   559			switch (result.uint_32) {
   560			case Opt_rescue_usebackuproot:
   561				btrfs_set_opt(ctx->mount_opt, USEBACKUPROOT);
   562				break;
   563			case Opt_rescue_nologreplay:
   564				btrfs_set_opt(ctx->mount_opt, NOLOGREPLAY);
   565				break;
   566			case Opt_rescue_ignorebadroots:
   567				btrfs_set_opt(ctx->mount_opt, IGNOREBADROOTS);
   568				break;
   569			case Opt_rescue_ignoredatacsums:
   570				btrfs_set_opt(ctx->mount_opt, IGNOREDATACSUMS);
   571				break;
   572			case Opt_rescue_parameter_all:
   573				btrfs_set_opt(ctx->mount_opt, IGNOREDATACSUMS);
   574				btrfs_set_opt(ctx->mount_opt, IGNOREBADROOTS);
   575				btrfs_set_opt(ctx->mount_opt, NOLOGREPLAY);
   576				break;
   577			default:
   578				btrfs_info(NULL, "unrecognized rescue option '%s'",
   579					   param->key);
   580				return -EINVAL;
   581			}
   582			break;
   583	#ifdef CONFIG_BTRFS_DEBUG
   584		case Opt_fragment:
   585			switch (result.uint_32) {
   586			case Opt_fragment_parameter_all:
   587				btrfs_set_opt(ctx->mount_opt, FRAGMENT_DATA);
   588				btrfs_set_opt(ctx->mount_opt, FRAGMENT_METADATA);
   589				break;
   590			case Opt_fragment_parameter_metadata:
   591				btrfs_set_opt(ctx->mount_opt, FRAGMENT_METADATA);
   592				break;
   593			case Opt_fragment_parameter_data:
   594				btrfs_set_opt(ctx->mount_opt, FRAGMENT_DATA);
   595				break;
   596			default:
   597				btrfs_info(NULL, "unrecognized fragment option '%s'",
   598					   param->key);
   599				return -EINVAL;
   600			}
   601			break;
   602	#endif
   603	#ifdef CONFIG_BTRFS_FS_REF_VERIFY
   604		case Opt_ref_verify:
   605			btrfs_set_opt(ctx->mount_opt, REF_VERIFY);
   606			break;
   607	#endif
   608		default:
   609			btrfs_err(NULL, "unrecognized mount option '%s'", param->key);
   610			return -EINVAL;
   611		}
   612	
   613		return 0;
   614	}
   615

David Disseldorp Dec. 27, 2023, 6:27 a.m. UTC | #3

On Sat, 23 Dec 2023 20:28:07 +1030, Qu Wenruo wrote:

> The new helper has better error report and correct overflow detection,
> furthermore the old @retptr behavior is also kept, thus there should be
> no behavior change.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/ioctl.c |  8 ++++++--
>  fs/btrfs/super.c |  8 ++++++++
>  fs/btrfs/sysfs.c | 14 +++++++++++---
>  3 files changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 4e50b62db2a8..8bfd4b4ccf02 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -1175,8 +1175,12 @@ static noinline int btrfs_ioctl_resize(struct file *file,
>  			mod = 1;
>  			sizestr++;
>  		}
> -		new_size = memparse(sizestr, &retptr);
> -		if (*retptr != '\0' || new_size == 0) {
> +
> +		ret = memparse_safe(sizestr, MEMPARSE_SUFFIXES_DEFAULT,
> +				    &new_size, &retptr);
> +		if (ret < 0)
> +			goto out_finish;
> +		if (*retptr != '\0') {

Was dropping the -EINVAL return for new_size=0 intentional?

>  			ret = -EINVAL;
>  			goto out_finish;
>  		}
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 3a677b808f0f..2bb6ea525e89 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -400,6 +400,14 @@ static int btrfs_parse_param(struct fs_context *fc, struct fs_parameter *param)
>  		ctx->thread_pool_size = result.uint_32;
>  		break;
>  	case Opt_max_inline:
> +		int ret;
> +
> +		ret = memparse_safe(param->string, MEMPARSE_SUFFIXES_DEFAULT,
> +				    &ctx->max_inline, NULL);
> +		if (ret < 0) {
> +			btrfs_err(NULL, "invalid string \"%s\"", param->string);
> +			return ret;
> +		}
>  		ctx->max_inline = memparse(param->string, NULL);

Looks like you overlooked removal of the old memparse() call above.

Cheers, David

Qu Wenruo Dec. 27, 2023, 8:26 a.m. UTC | #4

On 2023/12/27 16:57, David Disseldorp wrote:
> On Sat, 23 Dec 2023 20:28:07 +1030, Qu Wenruo wrote:
>
>> The new helper has better error report and correct overflow detection,
>> furthermore the old @retptr behavior is also kept, thus there should be
>> no behavior change.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   fs/btrfs/ioctl.c |  8 ++++++--
>>   fs/btrfs/super.c |  8 ++++++++
>>   fs/btrfs/sysfs.c | 14 +++++++++++---
>>   3 files changed, 25 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index 4e50b62db2a8..8bfd4b4ccf02 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -1175,8 +1175,12 @@ static noinline int btrfs_ioctl_resize(struct file *file,
>>   			mod = 1;
>>   			sizestr++;
>>   		}
>> -		new_size = memparse(sizestr, &retptr);
>> -		if (*retptr != '\0' || new_size == 0) {
>> +
>> +		ret = memparse_safe(sizestr, MEMPARSE_SUFFIXES_DEFAULT,
>> +				    &new_size, &retptr);
>> +		if (ret < 0)
>> +			goto out_finish;
>> +		if (*retptr != '\0') {
>
> Was dropping the -EINVAL return for new_size=0 intentional?

Oh, that's unintentional. Although we would reject the invalid string, a
dedicated "0" can still be parsed.
In that case we should still return -EINVAL.

I just got it confused with the old behavior for invalid string (where 0
is returned and @retptr is not advanced).
>
>>   			ret = -EINVAL;
>>   			goto out_finish;
>>   		}
>> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
>> index 3a677b808f0f..2bb6ea525e89 100644
>> --- a/fs/btrfs/super.c
>> +++ b/fs/btrfs/super.c
>> @@ -400,6 +400,14 @@ static int btrfs_parse_param(struct fs_context *fc, struct fs_parameter *param)
>>   		ctx->thread_pool_size = result.uint_32;
>>   		break;
>>   	case Opt_max_inline:
>> +		int ret;
>> +
>> +		ret = memparse_safe(param->string, MEMPARSE_SUFFIXES_DEFAULT,
>> +				    &ctx->max_inline, NULL);
>> +		if (ret < 0) {
>> +			btrfs_err(NULL, "invalid string \"%s\"", param->string);
>> +			return ret;
>> +		}
>>   		ctx->max_inline = memparse(param->string, NULL);
>
> Looks like you overlooked removal of the old memparse() call above.

My bad, I forgot to remove the old line.

Furthermore, the declaration of "ret" inside case block is not allowed,
I'll fix it anyway.

Thanks,
Qu
>
> Cheers, David
>

diff mbox series

Patch

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 4e50b62db2a8..8bfd4b4ccf02 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -1175,8 +1175,12 @@  static noinline int btrfs_ioctl_resize(struct file *file,
 			mod = 1;
 			sizestr++;
 		}
-		new_size = memparse(sizestr, &retptr);
-		if (*retptr != '\0' || new_size == 0) {
+
+		ret = memparse_safe(sizestr, MEMPARSE_SUFFIXES_DEFAULT,
+				    &new_size, &retptr);
+		if (ret < 0)
+			goto out_finish;
+		if (*retptr != '\0') {
 			ret = -EINVAL;
 			goto out_finish;
 		}
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 3a677b808f0f..2bb6ea525e89 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -400,6 +400,14 @@  static int btrfs_parse_param(struct fs_context *fc, struct fs_parameter *param)
 		ctx->thread_pool_size = result.uint_32;
 		break;
 	case Opt_max_inline:
+		int ret;
+
+		ret = memparse_safe(param->string, MEMPARSE_SUFFIXES_DEFAULT,
+				    &ctx->max_inline, NULL);
+		if (ret < 0) {
+			btrfs_err(NULL, "invalid string \"%s\"", param->string);
+			return ret;
+		}
 		ctx->max_inline = memparse(param->string, NULL);
 		break;
 	case Opt_acl:
diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index 84c05246ffd8..6846572496a6 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -762,6 +762,7 @@  static ssize_t btrfs_chunk_size_store(struct kobject *kobj,
 	struct btrfs_fs_info *fs_info = to_fs_info(get_btrfs_kobj(kobj));
 	char *retptr;
 	u64 val;
+	int ret;
 
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
@@ -776,7 +777,10 @@  static ssize_t btrfs_chunk_size_store(struct kobject *kobj,
 	if (space_info->flags & BTRFS_BLOCK_GROUP_SYSTEM)
 		return -EPERM;
 
-	val = memparse(buf, &retptr);
+	ret = memparse_safe(buf, MEMPARSE_SUFFIXES_DEFAULT, &val, &retptr);
+	if (ret < 0)
+		return ret;
+
 	/* There could be trailing '\n', also catch any typos after the value */
 	retptr = skip_spaces(retptr);
 	if (*retptr != 0 || val == 0)
@@ -1779,10 +1783,14 @@  static ssize_t btrfs_devinfo_scrub_speed_max_store(struct kobject *kobj,
 {
 	struct btrfs_device *device = container_of(kobj, struct btrfs_device,
 						   devid_kobj);
-	char *endptr;
 	unsigned long long limit;
+	char *endptr;
+	int ret;
+
+	ret = memparse_safe(buf, MEMPARSE_SUFFIXES_DEFAULT, &limit, &endptr);
+	if (ret < 0)
+		return ret;
 
-	limit = memparse(buf, &endptr);
 	/* There could be trailing '\n', also catch any typos after the value. */
 	endptr = skip_spaces(endptr);
 	if (*endptr != 0)