lkml.org 
[lkml]   [2021]   [Dec]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2] livepatch: Fix leak on klp_init_patch_early failure path
    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

    \
     
     \ /
      Last update: 2021-12-20 10:49    [W:2.956 / U:1.200 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site