Re: [PATCH v2] kset: fix memory leak when kset_register() returns error

[Yang Yingliang]

Hi,

On 2022/10/25 5:25, Luben Tuikov wrote:
> On 2022-10-24 17:06, Luben Tuikov wrote:
> > On 2022-10-24 08:19, Yang Yingliang wrote:
> > > Inject fault while loading module, kset_register() may fail.
> > > If it fails, the name allocated by kobject_set_name() which
> > > is called before kset_register() is leaked, because refcount
> > > of kobject is hold in kset_init().
> > "is hold" --> "was set".
> >
> > Also, I'd say "which must be called" instead of "is", since
> > we cannot register kobj/kset without a name--the kobj code crashes,
> > and we want to make this clear. IOW, a novice user may wonder
> > where "is" it called, as opposed to learning that they "must"
> > call it to allocate/set a name, before calling kset_register().
> >
> > So, I'd say this:
> >
> > "If it fails, the name allocated by kobject_set_name() which must
> >   be called before a call to kset_regsiter() is leaked, since
> >   refcount of kobj was set in kset_init()."
> Actually, to be a bit more clear:
>
> "If kset_register() fails, the name allocated by kobject_set_name(),
>   namely kset.kobj.name, which must be called before a call to kset_register(),
>   may be leaked, if the caller doesn't explicitly free it, say by calling kset_put().
>
>   To mitigate this, we free the name in kset_register() when an error is encountered,
>   i.e. when kset_register() returns an error."
Thanks for you suggestion.
> > > As a kset may be embedded in a larger structure which needs
> > > be freed in release() function or error path in callers, we
> > Drop "As", start with "A kset". "which needs _to_ be".
> > Also please specify that the release is part of the ktype,
> > like this:
> >
> > "A kset may be embedded in a larger structure which needs to be
> >   freed in ktype.release() or error path in callers, we ..."
> >
> > > can not call kset_put() in kset_register(), or it will cause
> > > double free, so just call kfree_const() to free the name and
> > > set it to NULL.
> > >
> > > With this fix, the callers don't need to care about the name
> > > freeing and call an extra kset_put() if kset_register() fails.
> > This is unclear because you're *missing* a verb:
> > "and call an extra kset_put()".
> > Please add the proper verb _between_ "and call", something like,
> >
> > "With this fix, the callers don't need to care about freeing
> >   the name of the kset, and _can_ call kset_put() if kset_register() fails."
I was mean
the callers don't need to care about freeing the name of the kset and
the callers don't need to care about calling kset_put()

Thanks,
Yang
> > Choose a proper verb here: can, should, cannot, should not, etc.
> >
> > We can do this because you set "kset.kobj.name to NULL, and this
> > is checked for in kobject_cleanup(). We just need to stipulate
> > whether they should/shouldn't have to call kset_put(), or can free the kset
> > and/or the embedding object themselves. This really depends
> > on how we want kset_register() to behave in the future, and on
> > user's own ktype.release implementation...
> Forgot "may", "may not".
>
> So, do we want to say "may call kset_put()", like:
>
> "With this fix, the callers need not care about freeing
>   the name of the kset, and _may_ call kset_put() if kset_register() fails."
>
> Or do we want to say "should" or even "must"--it really depends on
> what else is (would be) going on in kobj registration.
>
> Although, the user may have additional work to be done in the ktype.release()
> callback for the embedding object. It would be good to give them the freedom,
> i.e. "may", to call kset_put(). If that's not the case, this must be explicitly
> stipulated with the proper verb.
>
> Regards,
> Luben
>
> .