Re: [PATCH v2] livepatch: Fix leak on klp_init_patch_early failure path

[Petr Mladek]

On Fri 2021-12-17 13:50:42, Josh Poimboeuf wrote:
> On Fri, Dec 17, 2021 at 02:51:42PM +0100, Petr Mladek wrote:
> > On Wed 2021-12-15 07:20:04, David Vernet wrote:
> > Josh's proposal adds pre-early-init() to allow calling
> > klp_free_patch_*() already when klp_init_*_early() fails.
> > The purpose is to make sure that klp_init_*_early()
> > will actually never fail.
> > 
> > This might make things somehow complicated. Any future change
> > in klp_init_*_early() might require change in pre-early-init()
> > to catch eventual problems earlier.
> 
> I'm not sure why that would be a problem.  If we can separate out the
> things which fail from the things which don't, it simplifies things.

I think that there is no right answer. It depends on personal
preferences.


> > My proposal is to use simple trick. klp_free_patch_*() iterate
> > using the dynamic list (list_for_each_entry) while klp_init_*_early()
> > iterate using the arrays.
> > 
> > Now, we just need to make sure that klp_init_*_early() will only add
> > fully initialized structures into the dynamic list. As a result,
> > klp_free_patch() will see only the fully initialized structures
> > and could release them. It will not see that not-yet-initialized
> > structures but it is fine. They are not initialized and they do not
> > need to be freed.
> > 
> > In fact, I think that klp_init_*_early() functions already do
> > the right thing. IMHO, if you move the module_get() then you
> > could safely do:
> > 
> > int klp_enable_patch(struct klp_patch *patch)
> > {
> > [...]
> > 	if (!try_module_get(patch->mod)) {
> > 		mutex_unlock(&klp_mutex);
> > 		return -ENODEV;
> > 	}
> > 
> > 	ret = klp_init_patch_early(patch);
> > 	if (ret)
> > 		goto err;
> > 
> > 
> > Note that it would need to get tested.
> > 
> > Anyway, does it make sense now?
> 
> It would work, but it's too clever/non-obvious.  If we rely on tricks
> then we may eventually forget about them and accidentally break
> assumptions later.

It is not that super magic from my POV. But again, it is a personal
taste.

I do not want to bikeshed about it. Feel free to use the pre-early
thing.

Best Regards,
Petr