On Tue, 2020-02-04 at 18:06 +0300, Dan Carpenter wrote:
> On Tue, Feb 04, 2020 at 06:58:00AM -0800, Srinivas Pandruvada wrote:
> > > lib/kobject.c
> > > 664 static void kobject_cleanup(struct kobject *kobj)
> > > 665 {
> > > 666 struct kobj_type *t = get_ktype(kobj);
> > > 667 const char *name = kobj->name;
> > > 668
> > > 669 pr_debug("kobject: '%s' (%p): %s, parent %p\n",
> > > 670 kobject_name(kobj), kobj, __func__,
> > > kobj-
> > > > parent);
> > > 671
> > > 672 if (t && !t->release)
> > > 673 pr_debug("kobject: '%s' (%p): does not
> > > have a
> > > release() function, it is broken and must be fixed. See
> > > Documentation/kobject.txt.\n",
> > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > ^^^^
> > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > >
> > > 674 kobject_name(kobj), kobj);
> > >
> > There are several usages in kernel without release callback.
>
> There are 19. They mostly have to do with sysfs? I'm still figuring
> these things out so I don't know how it all fits together yet.
So this is patch is not the only one.
>
> > Does the attach patch fixes the warning?
> >
> > Thanks,
> > Srinivas
> >
> >
> > > regards,
> > > dan carpenter
> > >
> > From 959ee9fe784d623960df2560fd13cae215b464d4 Mon Sep 17 00:00:00
> > 2001
> > From: Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx>
> > Date: Tue, 4 Feb 2020 06:50:11 -0800
> > Subject: [PATCH] platform/x86/intel-uncore-freq: Add release
> > function
> >
> > When user mode is reference to the kobject, wait to free the memory
> > during module unload by adding a release callback.
> >
> > Signed-off-by: Srinivas Pandruvada <
> > srinivas.pandruvada@xxxxxxxxxxxxxxx>
> > ---
> > drivers/platform/x86/intel-uncore-frequency.c | 15 +++++++++++++--
> > 1 file changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/platform/x86/intel-uncore-frequency.c
> > b/drivers/platform/x86/intel-uncore-frequency.c
> > index 2b1a0734c3f8..3eda66d0713b 100644
> > --- a/drivers/platform/x86/intel-uncore-frequency.c
> > +++ b/drivers/platform/x86/intel-uncore-frequency.c
> > @@ -217,12 +217,21 @@ static struct attribute *uncore_attrs[] = {
> > NULL
> > };
> >
> > +
> > +static void uncore_sysfs_entry_release(struct kobject *kobj)
> > +{
> > + if (!uncore_max_entries)
> > + kfree(uncore_instances);
> > +}
> > +
> > static struct kobj_type uncore_ktype = {
> > + .release = uncore_sysfs_entry_release,
> > .sysfs_ops = &kobj_sysfs_ops,
> > .default_attrs = uncore_attrs,
> > };
> >
> > static struct kobj_type uncore_root_ktype = {
> > + .release = uncore_sysfs_entry_release,
> > .sysfs_ops = &kobj_sysfs_ops,
> > };
> >
> > @@ -281,9 +290,9 @@ static void uncore_remove_die_entry(int cpu)
> > mutex_lock(&uncore_lock);
> > data = uncore_get_instance(cpu);
> > if (data) {
> > - kobject_put(&data->kobj);
> > data->control_cpu = -1;
> > data->valid = false;
> > + kobject_put(&data->kobj);
> > }
> > mutex_unlock(&uncore_lock);
> > }
> > @@ -424,12 +433,14 @@ static void __exit intel_uncore_exit(void)
> >
> > unregister_pm_notifier(&uncore_pm_nb);
> > cpuhp_remove_state(uncore_hp_state);
> > + mutex_lock(&uncore_lock);
>
> It's not clear what this locking does. The actual
> uncore_sysfs_entry_release()
> can be called from a different work queue depending on the
> .config. See
> how kobject_release() is implemented.
I think your static checker is checking a case where user space has
kobject reference and will try to read/write while and user is
unloading module. The mutex locks are not really necessary.
I am trying to hold the free of the memory.
Thanks,
Srinivas
> regards,
> dan carpenter
>
>
> > for (i = 0; i < uncore_max_entries; ++i) {
> > if (uncore_instances[i].valid)
> > kobject_put(&uncore_instances[i].kobj);
> > }
> > + uncore_max_entries = 0;
> > kobject_put(&uncore_root_kobj);
> > - kfree(uncore_instances);
> > + mutex_unlock(&uncore_lock);
> > }
> > module_exit(intel_uncore_exit)
