From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Morton Subject: Re: [PATCH] procfs: Fix error handling of proc_register() Date: Wed, 22 Oct 2014 15:40:33 -0700 Message-ID: <20141022154033.3e2064d866e86a5b1c3c8230@linux-foundation.org> References: <1413492362-7083-1-git-send-email-dbanerje@akamai.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: kdevcore@akamai.com, linux-fsdevel@vger.kernel.org, Nicolas Dichtel To: Debabrata Banerjee Return-path: Received: from mail.linuxfoundation.org ([140.211.169.12]:59888 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933227AbaJVWke (ORCPT ); Wed, 22 Oct 2014 18:40:34 -0400 In-Reply-To: <1413492362-7083-1-git-send-email-dbanerje@akamai.com> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Thu, 16 Oct 2014 16:46:02 -0400 Debabrata Banerjee wrote: > I don't see why this should print warnings at all instead of properly > unrolling allocations and returning an appropriate error. It's actually > leaking resources currently. I think the warnings are useful - a duplicate name in /proc is a significant kernel bug and we'll want to know precisely what caused it and get it fixed up quickly. So let's keep that bit. > --- a/fs/proc/generic.c > +++ b/fs/proc/generic.c > @@ -304,6 +304,7 @@ static int proc_register(struct proc_dir_entry * dir, struct proc_dir_entry * dp > dp->proc_iops = &proc_file_inode_operations; > } else { > WARN_ON(1); > + proc_free_inum(dp->low_ino); > return -EINVAL; > } > > @@ -311,9 +312,13 @@ static int proc_register(struct proc_dir_entry * dir, struct proc_dir_entry * dp > > for (tmp = dir->subdir; tmp; tmp = tmp->next) > if (strcmp(tmp->name, dp->name) == 0) { > - WARN(1, "proc_dir_entry '%s/%s' already registered\n", > - dir->name, dp->name); > - break; > + spin_unlock(&proc_subdir_lock); > + > + if (S_ISDIR(dp->mode)) > + dir->nlink--; > + > + proc_free_inum(dp->low_ino); > + return -EEXIST; > } Your patch conflicts somewhat with Nicolas's "fs/proc: use a rb tree for the directory entries". Here's what I ended up with: From: Debabrata Banerjee Subject: procfs: fix error handling of proc_register() proc_register() error paths are leaking inodes and directory refcounts. Signed-off-by: Debabrata Banerjee Cc: Alexander Viro Signed-off-by: Andrew Morton --- fs/proc/generic.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff -puN fs/proc/generic.c~procfs-fix-error-handling-of-proc_register fs/proc/generic.c --- a/fs/proc/generic.c~procfs-fix-error-handling-of-proc_register +++ a/fs/proc/generic.c @@ -369,14 +369,21 @@ static int proc_register(struct proc_dir dp->proc_iops = &proc_file_inode_operations; } else { WARN_ON(1); + proc_free_inum(dp->low_ino); return -EINVAL; } spin_lock(&proc_subdir_lock); dp->parent = dir; - if (pde_subdir_insert(dir, dp) == false) + if (pde_subdir_insert(dir, dp) == false) { WARN(1, "proc_dir_entry '%s/%s' already registered\n", dir->name, dp->name); + spin_unlock(&proc_subdir_lock); + if (S_ISDIR(dp->mode)) + dir->nlink--; + proc_free_inum(dp->low_ino); + return -EEXIST; + } spin_unlock(&proc_subdir_lock); return 0; _