diff mbox series

autofs: add: new_inode check in autofs_fill_super()

Message ID	20231116000746.7359-1-raven@themaw.net
State	New
Headers	Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35; sender: ian146@aussiebb.com.au) by smtp01.aussiebb.com.au (Postfix) with ESMTPSA id C1732100290; Thu, 16 Nov 2023 11:07:49 +1100 (AEDT) From: Ian Kent <raven@themaw.net> To: Al Viro <viro@zeniv.linux.org.uk>, Christian Brauner <brauner@kernel.org> Cc: Bill O'Donnell <billodo@redhat.com>, Kernel Mailing List <linux-kernel@vger.kernel.org>, autofs mailing list <autofs@vger.kernel.org>, linux-fsdevel <linux-fsdevel@vger.kernel.org>, Ian Kent <raven@themaw.net>, syzbot+662f87a8ef490f45fa64@syzkaller.appspotmail.com Subject: [PATCH] autofs: add: new_inode check in autofs_fill_super() Date: Thu, 16 Nov 2023 08:07:46 +0800 Message-ID: <20231116000746.7359-1-raven@themaw.net> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk
Series	autofs: add: new_inode check in autofs_fill_super() \| autofs: add: new_inode check in autofs_fill_super()

Commit Message

Ian Kent Nov. 16, 2023, 12:07 a.m. UTC

  Add missing NULL check of root_inode in autofs_fill_super().

While we are at it simplify the logic by taking advantage of the VFS
cleanup procedures and get rid of the goto error handling, as suggested
by Al Viro.

Signed-off-by: Ian Kent <raven@themaw.net>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Christian Brauner <brauner@kernel.org>
Cc: Bill O'Donnell <billodo@redhat.com>
Reported-by: syzbot+662f87a8ef490f45fa64@syzkaller.appspotmail.com
---
 fs/autofs/inode.c | 50 ++++++++++++++++++-----------------------------
 1 file changed, 19 insertions(+), 31 deletions(-)

Comments

kernel test robot Nov. 16, 2023, 11:13 a.m. UTC | #1

Hi Ian,

kernel test robot noticed the following build errors:

[auto build test ERROR on brauner-vfs/vfs.all]
[also build test ERROR on linus/master v6.7-rc1 next-20231116]
[cannot apply to vfs-idmapping/for-next]
[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/Ian-Kent/autofs-add-new_inode-check-in-autofs_fill_super/20231116-081017
base:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git vfs.all
patch link:    https://lore.kernel.org/r/20231116000746.7359-1-raven%40themaw.net
patch subject: [PATCH] autofs: add: new_inode check in autofs_fill_super()
config: i386-buildonly-randconfig-001-20231116 (https://download.01.org/0day-ci/archive/20231116/202311161857.bZMQmWsW-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231116/202311161857.bZMQmWsW-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/202311161857.bZMQmWsW-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

   fs/autofs/inode.c: In function 'autofs_fill_super':
>> fs/autofs/inode.c:330:22: error: expected identifier or '*' before '-' token
     330 |                 goto -ENOMEM;
         |                      ^
>> fs/autofs/inode.c:349:25: error: 'ret' undeclared (first use in this function); did you mean 'net'?
     349 |                         ret = invalf(fc, "Could not find process group %d",
         |                         ^~~
         |                         net
   fs/autofs/inode.c:349:25: note: each undeclared identifier is reported only once for each function it appears in
>> fs/autofs/inode.c:312:24: warning: unused variable 'root' [-Wunused-variable]
     312 |         struct dentry *root;
         |                        ^~~~


vim +330 fs/autofs/inode.c

   306	
   307	static int autofs_fill_super(struct super_block *s, struct fs_context *fc)
   308	{
   309		struct autofs_fs_context *ctx = fc->fs_private;
   310		struct autofs_sb_info *sbi = s->s_fs_info;
   311		struct inode *root_inode;
 > 312		struct dentry *root;
   313		struct autofs_info *ino;
   314	
   315		pr_debug("starting up, sbi = %p\n", sbi);
   316	
   317		sbi->sb = s;
   318		s->s_blocksize = 1024;
   319		s->s_blocksize_bits = 10;
   320		s->s_magic = AUTOFS_SUPER_MAGIC;
   321		s->s_op = &autofs_sops;
   322		s->s_d_op = &autofs_dentry_operations;
   323		s->s_time_gran = 1;
   324	
   325		/*
   326		 * Get the root inode and dentry, but defer checking for errors.
   327		 */
   328		ino = autofs_new_ino(sbi);
   329		if (!ino)
 > 330			goto -ENOMEM;
   331	
   332		root_inode = autofs_get_inode(s, S_IFDIR | 0755);
   333		if (root_inode) {
   334			root_inode->i_uid = ctx->uid;
   335			root_inode->i_gid = ctx->gid;
   336			root_inode->i_fop = &autofs_root_operations;
   337			root_inode->i_op = &autofs_dir_inode_operations;
   338		}
   339		s->s_root = d_make_root(root_inode);
   340		if (unlikely(!s->s_root)) {
   341			autofs_free_ino(ino);
   342			return -ENOMEM;
   343		}
   344		s->s_root->d_fsdata = ino;
   345	
   346		if (ctx->pgrp_set) {
   347			sbi->oz_pgrp = find_get_pid(ctx->pgrp);
   348			if (!sbi->oz_pgrp) {
 > 349				ret = invalf(fc, "Could not find process group %d",
   350					     ctx->pgrp);
   351				return ret;
   352			}
   353		} else {
   354			sbi->oz_pgrp = get_task_pid(current, PIDTYPE_PGID);
   355		}
   356	
   357		if (autofs_type_trigger(sbi->type))
   358			/* s->s_root won't be contended so there's little to
   359			 * be gained by not taking the d_lock when setting
   360			 * d_flags, even when a lot mounts are being done.
   361			 */
   362			managed_dentry_set_managed(s->s_root);
   363	
   364		pr_debug("pipe fd = %d, pgrp = %u\n",
   365			 sbi->pipefd, pid_nr(sbi->oz_pgrp));
   366	
   367		sbi->flags &= ~AUTOFS_SBI_CATATONIC;
   368		return 0;
   369	}
   370

kernel test robot Nov. 16, 2023, 11:23 a.m. UTC | #2

Hi Ian,

kernel test robot noticed the following build warnings:

[auto build test WARNING on brauner-vfs/vfs.all]
[also build test WARNING on linus/master v6.7-rc1 next-20231116]
[cannot apply to vfs-idmapping/for-next]
[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/Ian-Kent/autofs-add-new_inode-check-in-autofs_fill_super/20231116-081017
base:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git vfs.all
patch link:    https://lore.kernel.org/r/20231116000746.7359-1-raven%40themaw.net
patch subject: [PATCH] autofs: add: new_inode check in autofs_fill_super()
config: x86_64-rhel-8.3-rust (https://download.01.org/0day-ci/archive/20231116/202311161909.KHau6jEj-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/20231116/202311161909.KHau6jEj-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/202311161909.KHau6jEj-lkp@intel.com/

All warnings (new ones prefixed by >>):

   fs/autofs/inode.c:330:8: error: expected identifier
                   goto -ENOMEM;
                        ^
>> fs/autofs/inode.c:330:8: warning: misleading indentation; statement is not part of the previous 'if' [-Wmisleading-indentation]
   fs/autofs/inode.c:329:2: note: previous statement is here
           if (!ino)
           ^
   fs/autofs/inode.c:349:4: error: use of undeclared identifier 'ret'
                           ret = invalf(fc, "Could not find process group %d",
                           ^
   fs/autofs/inode.c:351:11: error: use of undeclared identifier 'ret'
                           return ret;
                                  ^
>> fs/autofs/inode.c:330:8: warning: expression result unused [-Wunused-value]
                   goto -ENOMEM;
                        ^~~~~~~
   2 warnings and 3 errors generated.


vim +/if +330 fs/autofs/inode.c

   306	
   307	static int autofs_fill_super(struct super_block *s, struct fs_context *fc)
   308	{
   309		struct autofs_fs_context *ctx = fc->fs_private;
   310		struct autofs_sb_info *sbi = s->s_fs_info;
   311		struct inode *root_inode;
   312		struct dentry *root;
   313		struct autofs_info *ino;
   314	
   315		pr_debug("starting up, sbi = %p\n", sbi);
   316	
   317		sbi->sb = s;
   318		s->s_blocksize = 1024;
   319		s->s_blocksize_bits = 10;
   320		s->s_magic = AUTOFS_SUPER_MAGIC;
   321		s->s_op = &autofs_sops;
   322		s->s_d_op = &autofs_dentry_operations;
   323		s->s_time_gran = 1;
   324	
   325		/*
   326		 * Get the root inode and dentry, but defer checking for errors.
   327		 */
   328		ino = autofs_new_ino(sbi);
   329		if (!ino)
 > 330			goto -ENOMEM;
   331	
   332		root_inode = autofs_get_inode(s, S_IFDIR | 0755);
   333		if (root_inode) {
   334			root_inode->i_uid = ctx->uid;
   335			root_inode->i_gid = ctx->gid;
   336			root_inode->i_fop = &autofs_root_operations;
   337			root_inode->i_op = &autofs_dir_inode_operations;
   338		}
   339		s->s_root = d_make_root(root_inode);
   340		if (unlikely(!s->s_root)) {
   341			autofs_free_ino(ino);
   342			return -ENOMEM;
   343		}
   344		s->s_root->d_fsdata = ino;
   345	
   346		if (ctx->pgrp_set) {
   347			sbi->oz_pgrp = find_get_pid(ctx->pgrp);
   348			if (!sbi->oz_pgrp) {
   349				ret = invalf(fc, "Could not find process group %d",
   350					     ctx->pgrp);
   351				return ret;
   352			}
   353		} else {
   354			sbi->oz_pgrp = get_task_pid(current, PIDTYPE_PGID);
   355		}
   356	
   357		if (autofs_type_trigger(sbi->type))
   358			/* s->s_root won't be contended so there's little to
   359			 * be gained by not taking the d_lock when setting
   360			 * d_flags, even when a lot mounts are being done.
   361			 */
   362			managed_dentry_set_managed(s->s_root);
   363	
   364		pr_debug("pipe fd = %d, pgrp = %u\n",
   365			 sbi->pipefd, pid_nr(sbi->oz_pgrp));
   366	
   367		sbi->flags &= ~AUTOFS_SBI_CATATONIC;
   368		return 0;
   369	}
   370

Ian Kent Nov. 19, 2023, 10:23 a.m. UTC | #3

On 16/11/23 19:23, kernel test robot wrote:
> Hi Ian,
>
> kernel test robot noticed the following build warnings:

Crikey, how did this compile ... I think I need to just send a replacement

patch.


Ian

> [auto build test WARNING on brauner-vfs/vfs.all]
> [also build test WARNING on linus/master v6.7-rc1 next-20231116]
> [cannot apply to vfs-idmapping/for-next]
> [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/Ian-Kent/autofs-add-new_inode-check-in-autofs_fill_super/20231116-081017
> base:https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git  vfs.all
> patch link:https://lore.kernel.org/r/20231116000746.7359-1-raven%40themaw.net
> patch subject: [PATCH] autofs: add: new_inode check in autofs_fill_super()
> config: x86_64-rhel-8.3-rust (https://download.01.org/0day-ci/archive/20231116/202311161909.KHau6jEj-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/20231116/202311161909.KHau6jEj-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/202311161909.KHau6jEj-lkp@intel.com/
>
> All warnings (new ones prefixed by >>):
>
>     fs/autofs/inode.c:330:8: error: expected identifier
>                     goto -ENOMEM;
>                          ^
>>> fs/autofs/inode.c:330:8: warning: misleading indentation; statement is not part of the previous 'if' [-Wmisleading-indentation]
>     fs/autofs/inode.c:329:2: note: previous statement is here
>             if (!ino)
>             ^
>     fs/autofs/inode.c:349:4: error: use of undeclared identifier 'ret'
>                             ret = invalf(fc, "Could not find process group %d",
>                             ^
>     fs/autofs/inode.c:351:11: error: use of undeclared identifier 'ret'
>                             return ret;
>                                    ^
>>> fs/autofs/inode.c:330:8: warning: expression result unused [-Wunused-value]
>                     goto -ENOMEM;
>                          ^~~~~~~
>     2 warnings and 3 errors generated.
>
>
> vim +/if +330 fs/autofs/inode.c
>
>     306	
>     307	static int autofs_fill_super(struct super_block *s, struct fs_context *fc)
>     308	{
>     309		struct autofs_fs_context *ctx = fc->fs_private;
>     310		struct autofs_sb_info *sbi = s->s_fs_info;
>     311		struct inode *root_inode;
>     312		struct dentry *root;
>     313		struct autofs_info *ino;
>     314	
>     315		pr_debug("starting up, sbi = %p\n", sbi);
>     316	
>     317		sbi->sb = s;
>     318		s->s_blocksize = 1024;
>     319		s->s_blocksize_bits = 10;
>     320		s->s_magic = AUTOFS_SUPER_MAGIC;
>     321		s->s_op = &autofs_sops;
>     322		s->s_d_op = &autofs_dentry_operations;
>     323		s->s_time_gran = 1;
>     324	
>     325		/*
>     326		 * Get the root inode and dentry, but defer checking for errors.
>     327		 */
>     328		ino = autofs_new_ino(sbi);
>     329		if (!ino)
>   > 330			goto -ENOMEM;
>     331	
>     332		root_inode = autofs_get_inode(s, S_IFDIR | 0755);
>     333		if (root_inode) {
>     334			root_inode->i_uid = ctx->uid;
>     335			root_inode->i_gid = ctx->gid;
>     336			root_inode->i_fop = &autofs_root_operations;
>     337			root_inode->i_op = &autofs_dir_inode_operations;
>     338		}
>     339		s->s_root = d_make_root(root_inode);
>     340		if (unlikely(!s->s_root)) {
>     341			autofs_free_ino(ino);
>     342			return -ENOMEM;
>     343		}
>     344		s->s_root->d_fsdata = ino;
>     345	
>     346		if (ctx->pgrp_set) {
>     347			sbi->oz_pgrp = find_get_pid(ctx->pgrp);
>     348			if (!sbi->oz_pgrp) {
>     349				ret = invalf(fc, "Could not find process group %d",
>     350					     ctx->pgrp);
>     351				return ret;
>     352			}
>     353		} else {
>     354			sbi->oz_pgrp = get_task_pid(current, PIDTYPE_PGID);
>     355		}
>     356	
>     357		if (autofs_type_trigger(sbi->type))
>     358			/* s->s_root won't be contended so there's little to
>     359			 * be gained by not taking the d_lock when setting
>     360			 * d_flags, even when a lot mounts are being done.
>     361			 */
>     362			managed_dentry_set_managed(s->s_root);
>     363	
>     364		pr_debug("pipe fd = %d, pgrp = %u\n",
>     365			 sbi->pipefd, pid_nr(sbi->oz_pgrp));
>     366	
>     367		sbi->flags &= ~AUTOFS_SBI_CATATONIC;
>     368		return 0;
>     369	}
>     370	
>

diff mbox series

Patch

diff --git a/fs/autofs/inode.c b/fs/autofs/inode.c
index a5083d447a62..d5dd4223b461 100644
--- a/fs/autofs/inode.c
+++ b/fs/autofs/inode.c
@@ -311,7 +311,6 @@  static int autofs_fill_super(struct super_block *s, struct fs_context *fc)
 	struct inode *root_inode;
 	struct dentry *root;
 	struct autofs_info *ino;
-	int ret = -ENOMEM;
 
 	pr_debug("starting up, sbi = %p\n", sbi);
 
@@ -328,56 +327,45 @@  static int autofs_fill_super(struct super_block *s, struct fs_context *fc)
 	 */
 	ino = autofs_new_ino(sbi);
 	if (!ino)
-		goto fail;
+		goto -ENOMEM;
 
 	root_inode = autofs_get_inode(s, S_IFDIR | 0755);
-	root_inode->i_uid = ctx->uid;
-	root_inode->i_gid = ctx->gid;
-
-	root = d_make_root(root_inode);
-	if (!root)
-		goto fail_ino;
-
-	root->d_fsdata = ino;
+	if (root_inode) {
+		root_inode->i_uid = ctx->uid;
+		root_inode->i_gid = ctx->gid;
+		root_inode->i_fop = &autofs_root_operations;
+		root_inode->i_op = &autofs_dir_inode_operations;
+	}
+	s->s_root = d_make_root(root_inode);
+	if (unlikely(!s->s_root)) {
+		autofs_free_ino(ino);
+		return -ENOMEM;
+	}
+	s->s_root->d_fsdata = ino;
 
 	if (ctx->pgrp_set) {
 		sbi->oz_pgrp = find_get_pid(ctx->pgrp);
 		if (!sbi->oz_pgrp) {
 			ret = invalf(fc, "Could not find process group %d",
 				     ctx->pgrp);
-			goto fail_dput;
+			return ret;
 		}
 	} else {
 		sbi->oz_pgrp = get_task_pid(current, PIDTYPE_PGID);
 	}
 
 	if (autofs_type_trigger(sbi->type))
-		__managed_dentry_set_managed(root);
-
-	root_inode->i_fop = &autofs_root_operations;
-	root_inode->i_op = &autofs_dir_inode_operations;
+		/* s->s_root won't be contended so there's little to
+		 * be gained by not taking the d_lock when setting
+		 * d_flags, even when a lot mounts are being done.
+		 */
+		managed_dentry_set_managed(s->s_root);
 
 	pr_debug("pipe fd = %d, pgrp = %u\n",
 		 sbi->pipefd, pid_nr(sbi->oz_pgrp));
 
 	sbi->flags &= ~AUTOFS_SBI_CATATONIC;
-
-	/*
-	 * Success! Install the root dentry now to indicate completion.
-	 */
-	s->s_root = root;
 	return 0;
-
-	/*
-	 * Failure ... clean up.
-	 */
-fail_dput:
-	dput(root);
-	goto fail;
-fail_ino:
-	autofs_free_ino(ino);
-fail:
-	return ret;
 }
 
 /*