Messages in this thread | | | Date | Wed, 18 Jul 2007 09:59:35 -0700 | From | "Miles Lane" <> | Subject | Re: [PATCH] sysfs: kill an extra put in sysfs_create_link() failure path |
| |
On 7/18/07, Satyam Sharma <satyam.sharma@gmail.com> wrote: > On 7/18/07, Tejun Heo <htejun@gmail.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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |