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
.
