All of lore.kernel.org
 help / color / mirror / Atom feed
From: Josh Poimboeuf <jpoimboe@redhat.com>
To: David Vernet <void@manifault.com>
Cc: pmladek@suse.com, linux-doc@vger.kernel.org,
	live-patching@vger.kernel.org, linux-kernel@vger.kernel.org,
	jikos@kernel.org, mbenes@suse.cz, joe.lawrence@redhat.com,
	corbet@lwn.net, yhs@fb.com, songliubraving@fb.com
Subject: Re: [PATCH] livepatch: Fix leak on klp_init_patch_early failure path
Date: Mon, 13 Dec 2021 12:10:22 -0800	[thread overview]
Message-ID: <20211213201022.dhalhtc2bpey55gh@treble> (raw)
In-Reply-To: <20211213191734.3238783-1-void@manifault.com>

On Mon, Dec 13, 2021 at 11:17:35AM -0800, David Vernet wrote:
> When enabling a KLP patch with `klp_enable_patch`, we invoke
> `klp_init_patch_early` to initialize the kobjects for the patch itself, as
> well as the `struct klp_object*`'s and `struct klp_func*`'s that comprise
> it. However, there are some paths where we may fail to do an
> early-initialization of an object or its functions if certain conditions
> are not met, such as an object having a `NULL` funcs pointer. In these
> paths, we may currently leak the `struct klp_patch*`'s kobject, as well as
> any of its objects or functions, as we don't free the patch in
> `klp_enable_patch` if `klp_init_patch_early` returns an error code. For
> example, if we added the following object entry to the sample livepatch
> code, it would cause us to leak the vmlinux `klp_object`, and its `struct
> klp_func` which updates `cmdline_proc_show`:
> 
> ```
> static struct klp_object objs[] = {
>         {
>                 .name = "kvm",
>         }, { }
> };
> ```
> 
> Without this change, if we enable `CONFIG_DEBUG_KOBJECT` and try to `kpatch
> load livepatch-sample.ko`, we don't observe the kobjects being released
> (though of course we do observe `insmod` failing to insert the module).
> With the change, we do observe that the `kobject` for the patch and its
> `vmlinux` object are released.
> 
> Signed-off-by: David Vernet <void@manifault.com>

Thanks for reporting the issue and submitting the patch!

The patch description needs a few tweaks.  In the kernel we don't use
Markdown for patch descriptions.

A function can be postfixed with a trailing pair of parentheses, like
klp_enable_patch().

Other symbols can be enclosed with single quotes, like 'struct
klp_object'.

I'd also recommend avoiding the excessive use of "we", in favor of more
imperative-type language.

See Documentation/process/submitting-patches.rst for more details.  It's
also a good idea to look at some kernel commit logs to get a general
idea of the kernel patch description style.

> @@ -1052,10 +1052,7 @@ int klp_enable_patch(struct klp_patch *patch)
>  	}
>  
>  	ret = klp_init_patch_early(patch);
> -	if (ret) {
> -		mutex_unlock(&klp_mutex);
> -		return ret;
> -	}
> +		goto err;
>  
>  	ret = klp_init_patch(patch);
>  	if (ret)

I don't think the fix will be quite that simple.  For example, if
klp_init_patch_early() fails, that means try_module_get() hasn't been
done, so klp_free_patch_finish() will wrongly do a module_put().

-- 
Josh


  reply	other threads:[~2021-12-13 20:10 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-12-13 19:17 [PATCH] livepatch: Fix leak on klp_init_patch_early failure path David Vernet
2021-12-13 20:10 ` Josh Poimboeuf [this message]
2021-12-13 22:58   ` David Vernet
2021-12-13 23:32     ` Song Liu
2021-12-14  3:13     ` Josh Poimboeuf
2021-12-14  8:45 ` Petr Mladek
2021-12-14  9:17   ` Miroslav Benes
2021-12-14 15:26     ` David Vernet
2021-12-14 15:50       ` Greg Kroah-Hartman
2021-12-15  8:19         ` Petr Mladek
2021-12-15 15:35           ` Greg Kroah-Hartman
2021-12-16 14:14             ` Petr Mladek
2021-12-16 14:35               ` Greg Kroah-Hartman
2021-12-17 13:10                 ` Petr Mladek
2021-12-16 15:14               ` David Vernet
2021-12-17 13:53                 ` Petr Mladek
2021-12-15  8:33       ` Miroslav Benes
2021-12-14 15:52   ` Greg Kroah-Hartman
2021-12-14 18:26     ` David Vernet

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20211213201022.dhalhtc2bpey55gh@treble \
    --to=jpoimboe@redhat.com \
    --cc=corbet@lwn.net \
    --cc=jikos@kernel.org \
    --cc=joe.lawrence@redhat.com \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=live-patching@vger.kernel.org \
    --cc=mbenes@suse.cz \
    --cc=pmladek@suse.com \
    --cc=songliubraving@fb.com \
    --cc=void@manifault.com \
    --cc=yhs@fb.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.