Date:   Fri, 17 Dec 2021 13:50:42 -0800
From:   Josh Poimboeuf <jpoimboe@redhat.com>
To:     Petr Mladek <pmladek@suse.com>
Cc:     David Vernet <void@manifault.com>, live-patching@vger.kernel.org,
        linux-kernel@vger.kernel.org, jikos@kernel.org, mbenes@suse.cz,
        joe.lawrence@redhat.com, corbet@lwn.net, songliubraving@fb.com,
        gregkh@linuxfoundation.org
Subject: Re: [PATCH v2] livepatch: Fix leak on klp_init_patch_early failure
 path
Message-ID: <20211217215042.76m5qn5e63ptgrjq@treble>
References: <20211214220124.2911264-1-void@manifault.com>
 <20211214235128.ckaozqsvcr6iqcnu@treble>
 <Ybm+FyhLnuH4JThq@alley>
 <YboHpHmu3D+0hxKp@dev0025.ash9.facebook.com>
 <YbyV7nsLXbQ6/44S@alley>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <YbyV7nsLXbQ6/44S@alley>
Precedence: bulk

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@suse.com> 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