On Fri, Dec 17, 2021 at 02:51:42PM +0100, Petr Mladek wrote: > On Wed 2021-12-15 07:20:04, David Vernet wrote: > > Petr Mladek <pmladek@xxxxxxxx> wrote on Wed [2021-Dec-15 11:06:15 +0100]: > > > Well, I still believe that this is just a cargo cult. And I would prefer > > > to finish the discussion about it, first, see > > > https://lore.kernel.org/all/YbmlL0ZyfSuek9OB@alley/ > > > > No problem, I won't send out v3 until we've finished the discussion and > > have consensus. I'll assume that the discussion on whether or not there is > > a leak will continue on the thread you linked to above, so I won't comment > > on it here. > > > > > Note that klp_init_*_early() functions iterate through the arrays > > > using klp_for_each_*_static. While klp_free_*() functions iterate > > > via the lists using klp_for_each_*_safe(). > > > > Correct, as I've understood it, klp_for_each_*_safe() should only iterate > > over the objects that have been added to the patch and klp_object's lists, > > and thus for which kobject_init() has been invoked. So if we fail a check > > on 'struct klp_object' N, then we'll only iterate over the first N - 1 > > objects in klp_for_each_*_safe(). > > > > > We should not need the pre-early-init check when the lists include only > > > structures with initialized kobjects. > > > > Not sure I quite follow. We have to do NULL checks for obj->funcs at some > > point, and per Josh's suggestion it seems cleaner to do it outside the > > critical section, and before we actually invoke kobject_init(). Apologies > > if I've misunderstood your point. > > The original purpose of klp_init_*_early() was to allow calling > klp_free_patch_*() when klp_init_*() fails. The idea was to > initialize all fields properly so that free functions would > do the right thing. > > 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. And if klp_init_object_early() returns void then it would make sense for klp_init_patch_early() to also return void. > Also I am not sure what to do with the existing checks > in klp_init_patch_early(). I am uneasy with removing them > and hoping that pre-early-init() did the right job. klp_init_patch_early() already depends on some of the other checks in klp_enable_patch() anyway. I don't see that as much of a problem since it only has one caller. The benefit of moving the rest of the checks out is that it simplifies the error handling, with no possibility of half-baked structures. > But if we keep the checks then klp_init_patch_early() then it > might fail and the code will not be ready for this. > > > 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. -- Josh
