From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nicolas Dichtel Subject: Re: [PATCH] procfs: Fix error handling of proc_register() Date: Thu, 23 Oct 2014 09:23:44 +0200 Message-ID: <5448AD00.50809@6wind.com> References: <1413492362-7083-1-git-send-email-dbanerje@akamai.com> <20141022154033.3e2064d866e86a5b1c3c8230@linux-foundation.org> Reply-To: nicolas.dichtel@6wind.com Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: kdevcore@akamai.com, linux-fsdevel@vger.kernel.org To: Andrew Morton , Debabrata Banerjee Return-path: Received: from mail-wg0-f49.google.com ([74.125.82.49]:52714 "EHLO mail-wg0-f49.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751282AbaJWHXr (ORCPT ); Thu, 23 Oct 2014 03:23:47 -0400 Received: by mail-wg0-f49.google.com with SMTP id x12so440360wgg.8 for ; Thu, 23 Oct 2014 00:23:46 -0700 (PDT) In-Reply-To: <20141022154033.3e2064d866e86a5b1c3c8230@linux-foundation.org> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: Le 23/10/2014 00:40, Andrew Morton a =E9crit : > 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 proper= ly >> unrolling allocations and returning an appropriate error. It's actu= ally >> 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 i= t > 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 =3D &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 =3D dir->subdir; tmp; tmp =3D tmp->next) >> if (strcmp(tmp->name, dp->name) =3D=3D 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 refcount= s. > > Signed-off-by: Debabrata Banerjee > Cc: Alexander Viro > Signed-off-by: Andrew Morton Acked-by: Nicolas Dichtel -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel= " in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html