linux-kernel - Re: [PATCH] sysfs: kill an extra put in sysfs_create

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 18 Jul 2007 09:59:35 -0700
From:	"Miles Lane" <miles.lane@...il.com>
To:	"Satyam Sharma" <satyam.sharma@...il.com>
Cc:	"Tejun Heo" <htejun@...il.com>,
	"Gabriel C" <nix.or.die@...glemail.com>,
	"Linux Kernel Mailing List" <linux-kernel@...r.kernel.org>,
	"Christoph Lameter" <clameter@....com>, gregkh@...e.de
Subject: Re: [PATCH] sysfs: kill an extra put in sysfs_create_link() failure path

On 7/18/07, Satyam Sharma <satyam.sharma@...il.com> wrote:
> On 7/18/07, Tejun Heo <htejun@...il.com> wrote:
> > Tejun Heo wrote:
> > > Satyam Sharma wrote:
> > >>>> sysfs_find_dirent() -- to check for -EEXIST -- should be called
> > >>>> *before* we create the new dentry for the to-be-created symlink
> > >>>> in the first place. [ It's weird to grab a reference on the target
> > >>>> for ourselves (and in fact even allocate the new dirent for the
> > >>>> to-be-created symlink) and /then/ check for erroneous usage,
> > >>>> and then go about undoing all that we should never have done
> > >>>> at all. ] So this test could, and should, be made earlier, IMHO.
> > >>> Locking.
> > >> Well s/sysfs_find_dirent/sysfs_get_dirent/ then. And then simply put
> > >> down the reference later.
> > >
> > > Isn't that the current code?
> >
> > Oops, somehow thought you were talking about allocating it first.
> > Gee... what difference does using sysfs_get_dirent() make?  Do you think
> > the following code is correct?
> >
> >         sd = sysfs_get_dirent("some name");
> >         if (sd != NULL)
> >                 return -EEXIST;
> >         lock;
> >         add_new_node("some name");
> >         unlock;
> >         sysfs_put_dirent(sd);
>
> Nopes, it's not, of course. We'd need the parent's i_mutex as well
> as the sysfs_mutex around both the EEXIST check as well as the
> actual sysfs_add_one(), which is precisely what sysfs_addrm_start
> and finish are, so you're right ... I'll factor this in.

I tried to apply the patch you sent earlier, but it was rejected
(2.6.22-rc6-mm1 + Tejun's patch + your patch).  When you and
Tejun agree on an additional patch, I'd be happy to test it for you.

       Miles
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/