| 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
| ||
|
Message-ID: <20130911165942.GA29193@kroah.com>
Date: Wed, 11 Sep 2013 09:59:42 -0700
From: Greg KH <gregkh@...ux.com>
To: Jeff Mahoney <jeffm@...e.com>
Cc: ext4 development <linux-ext4@...r.kernel.org>,
Theodore Ts'o <tytso@....edu>
Subject: Re: [PATCH 1/2] kobject: introduce kobj_completion
On Wed, Sep 11, 2013 at 12:53:45PM -0400, Jeff Mahoney wrote:
> On 9/11/13 12:50 AM, Greg KH wrote:
> > On Tue, Sep 10, 2013 at 02:44:18PM -0400, Jeff Mahoney wrote:
> >> On 9/10/13 2:33 PM, Jeff Mahoney wrote:
> >>> On 9/10/13 2:06 PM, Greg KH wrote:
> >>>> On Tue, Sep 10, 2013 at 01:44:10PM -0400, Jeff Mahoney wrote:
> >>>>> ext4 exports per-filesystem information via sysfs. The lifetime rules
> >>>>> have historically been painful for this but the solution has been to pair
> >>>>> the kobject with a completion and call complete in the kobject's
> >>>>> release function.
> >>>>>
> >>>>> Since this is a pattern I've used in btrfs as well, it makes sense to
> >>>>> turn the pairing into a convenience structure with a standard API.
> >>>>>
> >>>>> Signed-off-by: Jeff Mahoney <jeffm@...e.com>
> >>>>> ---
> >>>>> include/linux/kobj_completion.h | 18 +++++++++++++++
> >>>>> lib/kobject.c | 47 ++++++++++++++++++++++++++++++++++++++++
> >>>>> 2 files changed, 65 insertions(+)
> >>>>>
> >>>>> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> >>>>> +++ b/include/linux/kobj_completion.h 2013-09-10 12:58:03.530554144 -0400
> >>>>> @@ -0,0 +1,18 @@
> >>>>> +#ifndef _KOBJ_COMPLETION_H_
> >>>>> +#define _KOBJ_COMPLETION_H_
> >>>>> +
> >>>>> +#include <linux/kobject.h>
> >>>>> +#include <linux/completion.h>
> >>>>> +
> >>>>> +struct kobj_completion {
> >>>>> + struct kobject kc_kobj;
> >>>>> + struct completion kc_unregister;
> >>>>> +};
> >>>>> +
> >>>>> +#define kobj_to_kobj_completion(kobj) \
> >>>>> + container_of(kobj, struct kobj_completion, kc_kobj)
> >>>>> +
> >>>>> +void kobj_completion_init(struct kobj_completion *kc, struct kobj_type *ktype);
> >>>>> +void kobj_completion_release(struct kobject *kobj);
> >>>>> +void kobj_completion_del_and_wait(struct kobj_completion *kc);
> >>>>> +#endif /* _KOBJ_COMPLETION_H_ */
> >>>>> --- a/lib/kobject.c 2013-09-10 12:57:54.198666613 -0400
> >>>>> +++ b/lib/kobject.c 2013-09-10 13:16:31.750607946 -0400
> >>>>> @@ -13,6 +13,7 @@
> >>>>> */
> >>>>>
> >>>>> #include <linux/kobject.h>
> >>>>> +#include <linux/kobj_completion.h>
> >>>>> #include <linux/string.h>
> >>>>> #include <linux/export.h>
> >>>>> #include <linux/stat.h>
> >>>>> @@ -711,6 +712,52 @@ const struct sysfs_ops kobj_sysfs_ops =
> >>>>> };
> >>>>>
> >>>>> /**
> >>>>> + * kobj_completion_init - initialize a kobj_completion object.
> >>>>> + * @kc: kobj_completion
> >>>>> + * @ktype: type of kobject to initialize
> >>>>> + *
> >>>>> + * kobj_completion structures can be embedded within structures with different
> >>>>> + * lifetime rules. During the release of the enclosing object, we can
> >>>>> + * wait on the release of the kobject so that we don't free it while it's
> >>>>> + * still busy.
> >>>>> + */
> >>>>> +void kobj_completion_init(struct kobj_completion *kc, struct kobj_type *ktype)
> >>>>> +{
> >>>>> + init_completion(&kc->kc_unregister);
> >>>>> + kobject_init(&kc->kc_kobj, ktype);
> >>>>> +}
> >>>>> +EXPORT_SYMBOL_GPL(kobj_completion_init);
> >>>>> +
> >>>>> +/**
> >>>>> + * kobj_completion_release - release a kobj_completion object
> >>>>> + * @kobj: kobject embedded in kobj_completion
> >>>>> + *
> >>>>> + * Used with kobject_release to notify waiters that the kobject has been
> >>>>> + * released.
> >>>>> + */
> >>>>> +void kobj_completion_release(struct kobject *kobj)
> >>>>> +{
> >>>>> + struct kobj_completion *kc = kobj_to_kobj_completion(kobj);
> >>>>> + complete(&kc->kc_unregister);
> >>>>> +}
> >>>>> +EXPORT_SYMBOL_GPL(kobj_completion_release);
> >>>>> +
> >>>>> +/**
> >>>>> + * kobj_completion_del_and_wait - release the kobject and wait for it
> >>>>> + * @kc: kobj_completion object to release
> >>>>> + *
> >>>>> + * Delete the kobject from sysfs and drop the reference count. Then wait
> >>>>> + * until any outstanding references are also dropped.
> >>>>> + */
> >>>>> +void kobj_completion_del_and_wait(struct kobj_completion *kc)
> >>>>> +{
> >>>>> + kobject_del(&kc->kc_kobj);
> >>>>> + kobject_put(&kc->kc_kobj);
> >>>>
> >>>> Why the extra kobject_put() call? Who added this extra reference to the
> >>>> object?
> >>>
> >>> There's an assumption that kobject_add will have been called on the
> >>> initialized kobject. If it hasn't been called, the object can just be
> >>> deleted without the completion. It makes the calling code easier to
> >>> read, so would it work for you if I documented that assumption in
> >>> _del_and_wait?
> >>
> >> Actually, that's not it. The refcounting works out in my tests.
> >>
> >> The kobject's refcount is initialized to 1. kobject_add doesn't
> >> increment it and kobject_del doesn't decrement it. So we need the
> >> kobject_put to trigger the release function.
> >>
> >> Or am I missing something here?
> >
> > Ugh, no, you are right, this is all good as-is. Do you want to take
> > these through the ext4 tree, or do you want me to take them?
>
> I'd prefer to get this one in now and wait until Ted's available to
> discuss the ext4 change. My btrfs changes depend on this as well and I'd
> like to get those moving now.
>
> I have a newer version with an updated comment that explains the
> lifetime a bit better that I'll send separately as a single patch.
Ok, I'll be glad to queue that one up to get into 3.12 so that you can
push the other patches separately.
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists