[PATCH] ext4: don't assume that mmp_nodename/bdevname have NUL

[PATCH] ext4: don't assume that mmp_nodename/bdevname have NUL

[Andreas Dilger]

Don't assume that the mmp_nodename and mmp_bdevname strings are NUL
terminated, since they are filled in by snprintf(), which is not
guaranteed to do so.

Signed-off-by: Andreas Dilger <adilger@dilger.ca>
---
 fs/ext4/mmp.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/fs/ext4/mmp.c b/fs/ext4/mmp.c
index 2305b43..612476e 100644
--- a/fs/ext4/mmp.c
+++ b/fs/ext4/mmp.c
@@ -120,10 +120,10 @@ void __dump_mmp_msg(struct super_block *sb, struct mmp_struct *mmp,
 {
 	__ext4_warning(sb, function, line, "%s", msg);
 	__ext4_warning(sb, function, line,
-		       "MMP failure info: last update time: %llu, last update "
-		       "node: %s, last update device: %s",
-		       (long long unsigned int) le64_to_cpu(mmp->mmp_time),
-		       mmp->mmp_nodename, mmp->mmp_bdevname);
+		       "MMP failure info: last update time: %llu, last update node: %.*s, last update device: %.*s",
+		       (long long unsigned int)le64_to_cpu(mmp->mmp_time),
+		       sizeof(mmp->mmp_nodename), mmp->mmp_nodename,
+		       sizeof(mmp->mmp_bdevname), mmp->mmp_bdevname);
 }
 
 /*
@@ -375,7 +375,8 @@ int ext4_multi_mount_protect(struct super_block *sb,
 	/*
 	 * Start a kernel thread to update the MMP block periodically.
 	 */
-	EXT4_SB(sb)->s_mmp_tsk = kthread_run(kmmpd, mmpd_data, "kmmpd-%s",
+	EXT4_SB(sb)->s_mmp_tsk = kthread_run(kmmpd, mmpd_data, "kmmpd-%.*s",
+					     sizeof(mmp->mmp_bdevname),
 					     bdevname(bh->b_bdev,
 						      mmp->mmp_bdevname));
 	if (IS_ERR(EXT4_SB(sb)->s_mmp_tsk)) {
-- 
1.8.0

Re: [PATCH] ext4: don't assume that mmp_nodename/bdevname have NUL

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3340 bytes --]

Hi Andreas,

I love your patch! Perhaps something to improve:

[auto build test WARNING on ext4/dev]
[also build test WARNING on tytso-fscrypt/master v5.5-rc7 next-20200124]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Andreas-Dilger/ext4-don-t-assume-that-mmp_nodename-bdevname-have-NUL/20200126-053627
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git dev
config: x86_64-defconfig (attached as .config)
compiler: gcc-7 (Debian 7.5.0-3) 7.5.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   fs/ext4/mmp.c: In function '__dump_mmp_msg':
>> fs/ext4/mmp.c:123:73: warning: field precision specifier '.*' expects argument of type 'int', but argument 6 has type 'long unsigned int' [-Wformat=]
             "MMP failure info: last update time: %llu, last update node: %.*s, last update device: %.*s",
                                                                          ~~^~
   fs/ext4/mmp.c:123:99: warning: field precision specifier '.*' expects argument of type 'int', but argument 8 has type 'long unsigned int' [-Wformat=]
             "MMP failure info: last update time: %llu, last update node: %.*s, last update device: %.*s",
                                                                                                    ~~^~
   In file included from fs/ext4/mmp.c:6:0:
   fs/ext4/mmp.c: In function 'ext4_multi_mount_protect':
   fs/ext4/mmp.c:382:57: warning: field precision specifier '.*' expects argument of type 'int', but argument 5 has type 'long unsigned int' [-Wformat=]
     EXT4_SB(sb)->s_mmp_tsk = kthread_run(kmmpd, mmpd_data, "kmmpd-%.*s",
                                                            ^
   include/linux/kthread.h:26:55: note: in definition of macro 'kthread_create'
     kthread_create_on_node(threadfn, data, NUMA_NO_NODE, namefmt, ##arg)
                                                          ^~~~~~~
>> fs/ext4/mmp.c:382:27: note: in expansion of macro 'kthread_run'
     EXT4_SB(sb)->s_mmp_tsk = kthread_run(kmmpd, mmpd_data, "kmmpd-%.*s",
                              ^~~~~~~~~~~

vim +123 fs/ext4/mmp.c

   114	
   115	/*
   116	 * Dump as much information as possible to help the admin.
   117	 */
   118	void __dump_mmp_msg(struct super_block *sb, struct mmp_struct *mmp,
   119			    const char *function, unsigned int line, const char *msg)
   120	{
   121		__ext4_warning(sb, function, line, "%s", msg);
   122		__ext4_warning(sb, function, line,
 > 123			       "MMP failure info: last update time: %llu, last update node: %.*s, last update device: %.*s",
   124			       (long long unsigned int)le64_to_cpu(mmp->mmp_time),
   125			       sizeof(mmp->mmp_nodename), mmp->mmp_nodename,
   126			       sizeof(mmp->mmp_bdevname), mmp->mmp_bdevname);
   127	}
   128	

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 28851 bytes --]

Re: [PATCH] ext4: don't assume that mmp_nodename/bdevname have NUL

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 6820 bytes --]

Hi Andreas,

I love your patch! Perhaps something to improve:

[auto build test WARNING on ext4/dev]
[also build test WARNING on tytso-fscrypt/master v5.5-rc7 next-20200124]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Andreas-Dilger/ext4-don-t-assume-that-mmp_nodename-bdevname-have-NUL/20200126-053627
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git dev
config: x86_64-randconfig-s0-20200126 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.2-10+deb8u1) 4.9.2
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   fs//ext4/mmp.c: In function '__dump_mmp_msg':
   fs//ext4/mmp.c:126:10: warning: field precision specifier '.*' expects argument of type 'int', but argument 6 has type 'long unsigned int' [-Wformat=]
             sizeof(mmp->mmp_bdevname), mmp->mmp_bdevname);
             ^
   fs//ext4/mmp.c:126:10: warning: field precision specifier '.*' expects argument of type 'int', but argument 8 has type 'long unsigned int' [-Wformat=]
   In file included from fs//ext4/mmp.c:6:0:
   fs//ext4/mmp.c: In function 'ext4_multi_mount_protect':
>> include/linux/kthread.h:45:9: warning: field precision specifier '.*' expects argument of type 'int', but argument 5 has type 'long unsigned int' [-Wformat=]
     struct task_struct *__k         \
            ^
   fs//ext4/mmp.c:382:27: note: in expansion of macro 'kthread_run'
     EXT4_SB(sb)->s_mmp_tsk = kthread_run(kmmpd, mmpd_data, "kmmpd-%.*s",
                              ^
--
   fs/ext4/mmp.c: In function '__dump_mmp_msg':
   fs/ext4/mmp.c:126:10: warning: field precision specifier '.*' expects argument of type 'int', but argument 6 has type 'long unsigned int' [-Wformat=]
             sizeof(mmp->mmp_bdevname), mmp->mmp_bdevname);
             ^
   fs/ext4/mmp.c:126:10: warning: field precision specifier '.*' expects argument of type 'int', but argument 8 has type 'long unsigned int' [-Wformat=]
   In file included from fs/ext4/mmp.c:6:0:
   fs/ext4/mmp.c: In function 'ext4_multi_mount_protect':
>> include/linux/kthread.h:45:9: warning: field precision specifier '.*' expects argument of type 'int', but argument 5 has type 'long unsigned int' [-Wformat=]
     struct task_struct *__k         \
            ^
   fs/ext4/mmp.c:382:27: note: in expansion of macro 'kthread_run'
     EXT4_SB(sb)->s_mmp_tsk = kthread_run(kmmpd, mmpd_data, "kmmpd-%.*s",
                              ^

vim +45 include/linux/kthread.h

^1da177e4c3f41 Linus Torvalds  2005-04-16   7  
b9075fa968a0a4 Joe Perches     2011-10-31   8  __printf(4, 5)
207205a2ba2655 Eric Dumazet    2011-03-22   9  struct task_struct *kthread_create_on_node(int (*threadfn)(void *data),
^1da177e4c3f41 Linus Torvalds  2005-04-16  10  					   void *data,
207205a2ba2655 Eric Dumazet    2011-03-22  11  					   int node,
b9075fa968a0a4 Joe Perches     2011-10-31  12  					   const char namefmt[], ...);
207205a2ba2655 Eric Dumazet    2011-03-22  13  
e154ccc831b5b5 Jonathan Corbet 2016-10-11  14  /**
e154ccc831b5b5 Jonathan Corbet 2016-10-11  15   * kthread_create - create a kthread on the current node
e154ccc831b5b5 Jonathan Corbet 2016-10-11  16   * @threadfn: the function to run in the thread
e154ccc831b5b5 Jonathan Corbet 2016-10-11  17   * @data: data pointer for @threadfn()
e154ccc831b5b5 Jonathan Corbet 2016-10-11  18   * @namefmt: printf-style format string for the thread name
d16977f3a6cfbb Jonathan Corbet 2017-08-02  19   * @arg...: arguments for @namefmt.
e154ccc831b5b5 Jonathan Corbet 2016-10-11  20   *
e154ccc831b5b5 Jonathan Corbet 2016-10-11  21   * This macro will create a kthread on the current node, leaving it in
e154ccc831b5b5 Jonathan Corbet 2016-10-11  22   * the stopped state.  This is just a helper for kthread_create_on_node();
e154ccc831b5b5 Jonathan Corbet 2016-10-11  23   * see the documentation there for more details.
e154ccc831b5b5 Jonathan Corbet 2016-10-11  24   */
207205a2ba2655 Eric Dumazet    2011-03-22  25  #define kthread_create(threadfn, data, namefmt, arg...) \
e9f069868d6055 Andrew Morton   2015-09-04  26  	kthread_create_on_node(threadfn, data, NUMA_NO_NODE, namefmt, ##arg)
207205a2ba2655 Eric Dumazet    2011-03-22  27  
^1da177e4c3f41 Linus Torvalds  2005-04-16  28  
2a1d446019f9a5 Thomas Gleixner 2012-07-16  29  struct task_struct *kthread_create_on_cpu(int (*threadfn)(void *data),
2a1d446019f9a5 Thomas Gleixner 2012-07-16  30  					  void *data,
2a1d446019f9a5 Thomas Gleixner 2012-07-16  31  					  unsigned int cpu,
2a1d446019f9a5 Thomas Gleixner 2012-07-16  32  					  const char *namefmt);
2a1d446019f9a5 Thomas Gleixner 2012-07-16  33  
^1da177e4c3f41 Linus Torvalds  2005-04-16  34  /**
9e37bd301ee130 Randy Dunlap    2006-06-25  35   * kthread_run - create and wake a thread.
^1da177e4c3f41 Linus Torvalds  2005-04-16  36   * @threadfn: the function to run until signal_pending(current).
^1da177e4c3f41 Linus Torvalds  2005-04-16  37   * @data: data ptr for @threadfn.
^1da177e4c3f41 Linus Torvalds  2005-04-16  38   * @namefmt: printf-style name for the thread.
^1da177e4c3f41 Linus Torvalds  2005-04-16  39   *
^1da177e4c3f41 Linus Torvalds  2005-04-16  40   * Description: Convenient wrapper for kthread_create() followed by
9e37bd301ee130 Randy Dunlap    2006-06-25  41   * wake_up_process().  Returns the kthread or ERR_PTR(-ENOMEM).
9e37bd301ee130 Randy Dunlap    2006-06-25  42   */
^1da177e4c3f41 Linus Torvalds  2005-04-16  43  #define kthread_run(threadfn, data, namefmt, ...)			   \
^1da177e4c3f41 Linus Torvalds  2005-04-16  44  ({									   \
^1da177e4c3f41 Linus Torvalds  2005-04-16 @45  	struct task_struct *__k						   \
^1da177e4c3f41 Linus Torvalds  2005-04-16  46  		= kthread_create(threadfn, data, namefmt, ## __VA_ARGS__); \
^1da177e4c3f41 Linus Torvalds  2005-04-16  47  	if (!IS_ERR(__k))						   \
^1da177e4c3f41 Linus Torvalds  2005-04-16  48  		wake_up_process(__k);					   \
^1da177e4c3f41 Linus Torvalds  2005-04-16  49  	__k;								   \
^1da177e4c3f41 Linus Torvalds  2005-04-16  50  })
^1da177e4c3f41 Linus Torvalds  2005-04-16  51  

:::::: The code at line 45 was first introduced by commit
:::::: 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 Linux-2.6.12-rc2

:::::: TO: Linus Torvalds <torvalds@ppc970.osdl.org>
:::::: CC: Linus Torvalds <torvalds@ppc970.osdl.org>

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 35069 bytes --]

[PATCH] ext4: don't assume that mmp_nodename/bdevname have NUL

[Andreas Dilger]

Don't assume that the mmp_nodename and mmp_bdevname strings are NUL
terminated, since they are filled in by snprintf(), which is not
guaranteed to do so.

Signed-off-by: Andreas Dilger <adilger@dilger.ca>
---
 fs/ext4/mmp.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/fs/ext4/mmp.c b/fs/ext4/mmp.c
index 2305b43..9d00e0d 100644
--- a/fs/ext4/mmp.c
+++ b/fs/ext4/mmp.c
@@ -120,10 +120,10 @@ void __dump_mmp_msg(struct super_block *sb, struct mmp_struct *mmp,
 {
 	__ext4_warning(sb, function, line, "%s", msg);
 	__ext4_warning(sb, function, line,
-		       "MMP failure info: last update time: %llu, last update "
-		       "node: %s, last update device: %s",
-		       (long long unsigned int) le64_to_cpu(mmp->mmp_time),
-		       mmp->mmp_nodename, mmp->mmp_bdevname);
+		       "MMP failure info: last update time: %llu, last update node: %.*s, last update device: %.*s",
+		       (unsigned long long)le64_to_cpu(mmp->mmp_time),
+		       (int)sizeof(mmp->mmp_nodename), mmp->mmp_nodename,
+		       (int)sizeof(mmp->mmp_bdevname), mmp->mmp_bdevname);
 }
 
 /*
@@ -154,6 +154,7 @@ static int kmmpd(void *data)
 	mmp_check_interval = max(EXT4_MMP_CHECK_MULT * mmp_update_interval,
 				 EXT4_MMP_MIN_CHECK_INTERVAL);
 	mmp->mmp_check_interval = cpu_to_le16(mmp_check_interval);
+	BUILD_BUG_ON(sizeof(mmp->mmp_bdevname) < BDEVNAME_SIZE);
 	bdevname(bh->b_bdev, mmp->mmp_bdevname);
 
 	memcpy(mmp->mmp_nodename, init_utsname()->nodename,
@@ -375,7 +376,8 @@ int ext4_multi_mount_protect(struct super_block *sb,
 	/*
 	 * Start a kernel thread to update the MMP block periodically.
 	 */
-	EXT4_SB(sb)->s_mmp_tsk = kthread_run(kmmpd, mmpd_data, "kmmpd-%s",
+	EXT4_SB(sb)->s_mmp_tsk = kthread_run(kmmpd, mmpd_data, "kmmpd-%.*s",
+					     (int)sizeof(mmp->mmp_bdevname),
 					     bdevname(bh->b_bdev,
 						      mmp->mmp_bdevname));
 	if (IS_ERR(EXT4_SB(sb)->s_mmp_tsk)) {
-- 
1.8.0

Re: [PATCH] ext4: don't assume that mmp_nodename/bdevname have NUL

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 2463 bytes --]

On Jan 26, 2020, at 3:03 PM, Andreas Dilger <adilger@dilger.ca> wrote:
> 
> Don't assume that the mmp_nodename and mmp_bdevname strings are NUL
> terminated, since they are filled in by snprintf(), which is not
> guaranteed to do so.
> 
> Signed-off-by: Andreas Dilger <adilger@dilger.ca>
> ---

NB: this is v2 of the patch, which fixes the checkpatch warnings.

Ted, do you also want an ext4 patch with EXT4_LEN_STR() and a change of these
char strings to __u8, along with similar changes to other non-NUL-terminated
strings in the superblock, as was done for e2fsprogs?

Cheers, Andreas

> fs/ext4/mmp.c | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/ext4/mmp.c b/fs/ext4/mmp.c
> index 2305b43..9d00e0d 100644
> --- a/fs/ext4/mmp.c
> +++ b/fs/ext4/mmp.c
> @@ -120,10 +120,10 @@ void __dump_mmp_msg(struct super_block *sb, struct mmp_struct *mmp,
> {
> 	__ext4_warning(sb, function, line, "%s", msg);
> 	__ext4_warning(sb, function, line,
> -		       "MMP failure info: last update time: %llu, last update "
> -		       "node: %s, last update device: %s",
> -		       (long long unsigned int) le64_to_cpu(mmp->mmp_time),
> -		       mmp->mmp_nodename, mmp->mmp_bdevname);
> +		       "MMP failure info: last update time: %llu, last update node: %.*s, last update device: %.*s",
> +		       (unsigned long long)le64_to_cpu(mmp->mmp_time),
> +		       (int)sizeof(mmp->mmp_nodename), mmp->mmp_nodename,
> +		       (int)sizeof(mmp->mmp_bdevname), mmp->mmp_bdevname);
> }
> 
> /*
> @@ -154,6 +154,7 @@ static int kmmpd(void *data)
> 	mmp_check_interval = max(EXT4_MMP_CHECK_MULT * mmp_update_interval,
> 				 EXT4_MMP_MIN_CHECK_INTERVAL);
> 	mmp->mmp_check_interval = cpu_to_le16(mmp_check_interval);
> +	BUILD_BUG_ON(sizeof(mmp->mmp_bdevname) < BDEVNAME_SIZE);
> 	bdevname(bh->b_bdev, mmp->mmp_bdevname);
> 
> 	memcpy(mmp->mmp_nodename, init_utsname()->nodename,
> @@ -375,7 +376,8 @@ int ext4_multi_mount_protect(struct super_block *sb,
> 	/*
> 	 * Start a kernel thread to update the MMP block periodically.
> 	 */
> -	EXT4_SB(sb)->s_mmp_tsk = kthread_run(kmmpd, mmpd_data, "kmmpd-%s",
> +	EXT4_SB(sb)->s_mmp_tsk = kthread_run(kmmpd, mmpd_data, "kmmpd-%.*s",
> +					     (int)sizeof(mmp->mmp_bdevname),
> 					     bdevname(bh->b_bdev,
> 						      mmp->mmp_bdevname));
> 	if (IS_ERR(EXT4_SB(sb)->s_mmp_tsk)) {
> --
> 1.8.0
> 


Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

Re: [PATCH] ext4: don't assume that mmp_nodename/bdevname have NUL

[Theodore Y. Ts'o]

On Sun, Jan 26, 2020 at 03:03:34PM -0700, Andreas Dilger wrote:
> Don't assume that the mmp_nodename and mmp_bdevname strings are NUL
> terminated, since they are filled in by snprintf(), which is not
> guaranteed to do so.
> 
> Signed-off-by: Andreas Dilger <adilger@dilger.ca>

Applied, thanks.

					- Ted

Re: [PATCH] ext4: don't assume that mmp_nodename/bdevname have NUL

[Theodore Y. Ts'o]

On Sun, Jan 26, 2020 at 03:06:36PM -0700, Andreas Dilger wrote:
> Ted, do you also want an ext4 patch with EXT4_LEN_STR() and a change of these
> char strings to __u8, along with similar changes to other non-NUL-terminated
> strings in the superblock, as was done for e2fsprogs?

Yes, it would be great to have that as a separate cleanup patch, thanks.

     	      	       	       - Ted