Submitting your first patch to the Linux kernel
In the previous blog post, we learned how to set up a dev environment to use mutt to subscribe to Linux kernel mailing lists on vger. We are now receiving patches for any of the lists we subscribed to, and can view them, download and experiment with them, and perform code reviews for our fellow upstream contributors. But how do we actually send a patch of our own?
To answer that, we’ll need to tackle three separate questions:
1. What should we work on? What needs to be "patched"?
2. Once we know what to work on, how do we create a patch?
3. Once we have a patch, how do we actually send it out to be reviewed, and iterate with upstream reviewers?
This post will answer each of these questions one at a time. By the end of it, you’ll be ready to write and submit your own patches to the Linux kernel!
Finding things we can contribute to
Let’s address the initial question first: what should we actually work on?
https://bugzilla.kernel.org has a ton of bugs opened by random automation or individuals. There isn't a strong process behind these bugs, and there is no guarantee that a bug is accurately reported, or hasn't already been fixed.
You can also add things to the Linux kernel that you wish were there. Perhaps a BPF patch that hardens the verifier? Or, perhaps you wish it were easier to track statistics in the kernel’s page cache, so you decide to add some sysfs nodes that you can query to get that information. You can also lurk on the mailing lists to see what people are discussing and working on. This will give you a sense of where the community’s focus is, and you can start to get a sense of the code, and ways that you could improve it. Your imagination is the limiting factor here, and as long as you approach the community respectfully, with a solid technical argument, and an open mind, anything is fair game.
Can’t think of anything? Documentation is always an option!
If you don't know where to begin, or you don’t yet feel ready to send patches updating the kernel itself, there is another sure-fire way to contribute to the Linux kernel: improving its documentation. The Linux kernel has a large corpus of documentation, which is in fact checked into the source tree in the form of reStructuredText files. Contributing to the Linux kernel’s documentation is a great way to get used to the upstream code review process, while also providing a lot of value to the global Linux community.
I'm speaking from experience here – the first patch I sent out that was successfully merged to the codebase was improving the kernel livepatch documentation to render links to function and struct doxygen comments. For example, if you’re reading through where the documentation describes how a livepatch is loaded, you can click on the klp_enable_patch() link to view its function signature and description. Pretty simple, but very handy for anyone trying to better understand how the livepatch subsystem works!
If sending patches that improve documentation sounds like a good starting point, ycheck out the how to help improve the Linux kernel documentation page. It describes several ways you can contribute to the kernel's documentation, and should give you some ideas for where to start.
One small word of warning to note: improving documentation in the kernel does not mean just adding comments and/or function headers. That may sometimes be warranted, but the norm in the Linux kernel is to avoid over-commenting.
Creating a patch
Learning the guidelines for Linux kernel contributions
That answers the question of how to choose something to work on. Next, we need to learn how to actually create patches and send them for review. The first step to doing that is learning how to be an effective members of the upstream Linux kernel community, by reading through (some, not all of) the Introduction to Kernel Development documentation list. Again, you definitely do not have to read through this whole list – I have not and almost certainly never will, and after a certain point it’s largely just a reference manual rather than an “introduction” anyways. What you certainly should read, however, are the pages that describe how to interact with the upstream community, and how to write high quality patches. At a minimum, I recommend that you read through the following resources:
- The Code of Conduct. This is a short but important read that describes how members of the upstream community should communicate and treat each other. Most of the communication you do will happen via emails sent to and from the void with contributors all across the globe, so it’s important to internalize this and apply it to all of your interactions..
- A guide to the kernel development process. This is quite long, but is full of accurate and important information. This will give you an overview of the software development processes driving all Linux kernel development. For example, it describes what it means for code to be in the “mainline” kernel, and when you can submit new features to the mainline kernel (during a “merge window”). You must read this if you want to understand how patches make it into the Linux kernel at the end of the day, and in general, how to do development the “Linux way”..
- Linux kernel coding style. This one is medium-long, but is of course an absolute necessity. This is a massive software project with a highly distributed collaboration model. Know what the coding style expectations are, or risk your patch being rejected (and looking silly for not having read this).
- Submitting patches. This one is similarly medium-long, but is full of 100% accurate information that all contributors will expect you to have read. This page is also somewhat process oriented, and describes all the things you have to keep in mind when submitting a patch.
I know, reading about development processes and coding style is a lot less fun than banging out code, but this is important. Given the size and scale of the Linux community, it’s simply a hard requirement for anyone contributing to the Linux kernel to independently read and learn about how to be a contributor. Note as well that this list is a minimum. If something else looks important, it can’t hurt to read it! You’re doing both yourself and the upstream community a favor by being as well informed as possible.
Creating a patch from a commit
We’ve now done our necessary background reading, and are ready to start coding! What you work on is of course up to you, so at this point I’ll assume that you have some code that you’re ready to commit, and send out for patch review.
First things first, we have to create a commit. Given that you’ve read over the pages I linked above, you’re hopefully aware that you need to write a solid commit summary, and then sign off on your commit with -s. Your commit should look something like this:
commit 5ef3dd20555e8e878ac390a71e658db5fd02845c (HEAD)
Author: David Vernet <void@manifault.com>
Date: Tue Dec 21 07:39:31 2021 -0800
livepatch: Fix kobject refcount bug on klp_init_patch_early failure path
When enabling a klp patch with klp_enable_patch(), klp_init_patch_early()
is invoked to initialize the kobjects for the patch itself, as well as the
'struct klp_object' and 'struct klp_func' objects that comprise it.
However, there are some error paths in klp_enable_patch() where some
kobjects may have been initialized with kobject_init(), but an error code
is still returned due to e.g. a 'struct klp_object' having a NULL funcs
pointer.
...
<snip>
...
Signed-off-by: David Vernet <void@manifault.com>
Reviewed-by: Petr Mladek <pmladek@suse.com>
Acked-by: Miroslav Benes <mbenes@suse.cz>
Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Petr Mladek <pmladek@suse.com>
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 335d988bd811..7d228cdb44c5 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -862,14 +862,11 @@ static void klp_init_object_early(struct klp_patch *patch,
list_add_tail(&obj->node, &patch->obj_list);
}
-static int klp_init_patch_early(struct klp_patch *patch)
+static void klp_init_patch_early(struct klp_patch *patch)
<truncated>
...
Your writing style, tone, etc, may differ from mine. As long as you write a patch summary that addresses all of the points described on the Submitting patches page, you’re good to go.
Moving right along – we have our commit, but how do we actually send that to upstream? The answer is that we create a patch, using git-format-patch. Say that you have a single commit that you want to send to upstream, and your HEAD currently points to that commit. You could create the patch as follows:
$ git format-patch HEAD~1 -o /tmp
/tmp/0001-livepatch-Fix-kobject-refcount-bug-on-klp_init_patch.patch
Great, we now have a patch in /tmp/0001-livepatch-Fix-kobject-refcount-bug-on-klp_init_patch.patch. Let’s see what it looks like:
$ cat /tmp/0001-livepatch-Fix-kobject-refcount-bug-on-klp_init_patch.patch
From 5ef3dd20555e8e878ac390a71e658db5fd02845c Mon Sep 17 00:00:00 2001
From: David Vernet <void@manifault.com>
Date: Tue, 21 Dec 2021 07:39:31 -0800
Subject: [PATCH] livepatch: Fix kobject refcount bug on klp_init_patch_early
failure path
When enabling a klp patch with klp_enable_patch(), klp_init_patch_early()
is invoked to initialize the kobjects for the patch itself, as well as the
'struct klp_object' and 'struct klp_func' objects that comprise it.
However, there are some error paths in klp_enable_patch() where some
kobjects may have been initialized with kobject_init(), but an error code
is still returned due to e.g. a 'struct klp_object' having a NULL funcs
pointer.
In these paths, the initial reference of the kobject of the 'struct
klp_patch' may never be released, along with one or more of its objects and
their functions, as kobject_put() is not invoked on the cleanup path if
klp_init_patch_early() returns an error code.
For example, if an object entry such as the following were added to the
sample livepatch module's klp patch, it would cause the vmlinux klp_object,
and its klp_func which updates 'cmdline_proc_show', to never be released:
static struct klp_object objs[] = {
{
/* name being NULL means vmlinux */
.funcs = funcs,
},
{
/* NULL funcs -- would cause reference leak */
.name = "kvm",
}, { }
};
Without this change, if CONFIG_DEBUG_KOBJECT is enabled, and the sample klp
patch is loaded, the kobjects (the patch, the vmlinux 'struct klp_object',
and its func) are observed as initialized, but never released, in the dmesg
log output. With the change, these kobject references no longer fail to be
released as the error case is properly handled before they are initialized.
Signed-off-by: David Vernet <void@manifault.com>
Reviewed-by: Petr Mladek <pmladek@suse.com>
Acked-by: Miroslav Benes <mbenes@suse.cz>
Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
kernel/livepatch/core.c | 31 +++++++++++++------------------
1 file changed, 13 insertions(+), 18 deletions(-)
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 335d988bd811..7d228cdb44c5 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -862,14 +862,11 @@ static void klp_init_object_early(struct klp_patch *patch,
list_add_tail(&obj->node, &patch->obj_list);
}
-static int klp_init_patch_early(struct klp_patch *patch)
+static void klp_init_patch_early(struct klp_patch *patch)
{
struct klp_object *obj;
struct klp_func *func;
- if (!patch->objs)
- return -EINVAL;
-
INIT_LIST_HEAD(&patch->list);
INIT_LIST_HEAD(&patch->obj_list);
kobject_init(&patch->kobj, &klp_ktype_patch);
@@ -879,20 +876,12 @@ static int klp_init_patch_early(struct klp_patch *patch)
init_completion(&patch->finish);
klp_for_each_object_static(patch, obj) {
- if (!obj->funcs)
- return -EINVAL;
-
klp_init_object_early(patch, obj);
klp_for_each_func_static(obj, func) {
klp_init_func_early(obj, func);
}
}
-
- if (!try_module_get(patch->mod))
- return -ENODEV;
-
- return 0;
}
static int klp_init_patch(struct klp_patch *patch)
@@ -1024,10 +1013,17 @@ static int __klp_enable_patch(struct klp_patch *patch)
int klp_enable_patch(struct klp_patch *patch)
{
int ret;
+ struct klp_object *obj;
- if (!patch || !patch->mod)
+ if (!patch || !patch->mod || !patch->objs)
return -EINVAL;
+ klp_for_each_object_static(patch, obj) {
+ if (!obj->funcs)
+ return -EINVAL;
+ }
+
+
if (!is_livepatch_module(patch->mod)) {
pr_err("module %s is not marked as a livepatch module\n",
patch->mod->name);
@@ -1051,11 +1047,10 @@ int klp_enable_patch(struct klp_patch *patch)
return -EINVAL;
}
- ret = klp_init_patch_early(patch);
- if (ret) {
- mutex_unlock(&klp_mutex);
- return ret;
- }
+ if (!try_module_get(patch->mod))
+ return -ENODEV;
+
+ klp_init_patch_early(patch);
ret = klp_init_patch(patch);
if (ret)
--
2.25.1
That’s the text file that you’ll send to the upstream community. As a quick aside, let me just mention why we do this whole “patch” thing to begin with.
Sanity checking the patch
Before you send the patch to upstream you should always 100% without fail run the scripts/check_patch.pl script on your patch to make sure it looks OK:
$ ./scripts/checkpatch.pl /tmp/0001-livepatch-Fix-kobject-refcount-bug-on-klp_init_patch.patch
total: 0 errors, 0 warnings, 68 lines checked
/tmp/0001-livepatch-Fix-kobject-refcount-bug-on-klp_init_patch.patch has no obvious style problems and is ready for submission.
The patch is good to go! Let’s send it off!
Sending the patch, and iterating with reviewers
We’ve created our commit with a good commit summary, transformed it into a patch with git format-patch, and checked it with scripts/check_patch.pl. We’re now all set to send it out for a review. To do that, we’ll use git send-email.
Sending the patch
There are two a few things you need to do to email the patch:
1. Figure out which maintainers (and lists) should be included on an email containing the patch.
2. Send an email to all of them in 7 or 8 bit plaintext encoding.
There is another handy scrip in the Linux kernel called get_maintainer.pl that answers the first question:
$ ./scripts/get_maintainer.pl /tmp/0001-livepatch-Fix-kobject-refcount-bug-on-klp_init_patch.patch
Josh Poimboeuf <jpoimboe@redhat.com> (maintainer:LIVE PATCHING)
Jiri Kosina <jikos@kernel.org> (maintainer:LIVE PATCHING)
Miroslav Benes <mbenes@suse.cz> (maintainer:LIVE PATCHING)
Petr Mladek <pmladek@suse.com> (maintainer:LIVE PATCHING)
Joe Lawrence <joe.lawrence@redhat.com> (reviewer:LIVE PATCHING)
live-patching@vger.kernel.org (open list:LIVE PATCHING)
linux-kernel@vger.kernel.org (open list)
Voila, we have our list of maintainers whom we need to include on the review. The next step is to actually send the patch to them. To do this, we can use git send-email. Before we do that, however, let’s do two quick things to make sure that git send-email is properly installed and configured:
$ sudo apt install git-email -y
…
$ git config --global sendemail.transferEncoding 7bit
The first command (which may differ depending on what distro and package manager you’re using) downloads git send-email and its dependencies, which don’t ship with the default git. The second command ensures that git send-email uses 7bit encoding. Higher-bit (plaintext) encodings are allowed on Linux kernel mailing lists, but I don't have any characters in my name that require more than 7 bits, so I like to stay minimalist. Of course, if I'm replying to an email that contains another contributor with non-ASCII characters in their name, I'll use UTF-8.
Once we’ve taken care of these minor items, we can invoke git send-email as follows:
$ git send-email --to jpoimboe@redhat.com --to jikos@kernel.org --to mbenes@suse.cz --to pmladek@suse.com --to joe.lawrence@redhat.com --to live-patching@vger.kernel.org --to linux-kernel@vger.kernel.org /tmp/0001-livepatch-Fix-kobject-refcount-bug-on-klp_init_patch.patch
From: David Vernet <void@manifault.com>
To: jpoimboe@redhat.com,
jikos@kernel.org,
mbenes@suse.cz,
pmladek@suse.com,
joe.lawrence@redhat.com,
live-patching@vger.kernel.org,
linux-kernel@vger.kernel.org
Cc: David Vernet <void@manifault.com>
Subject: [PATCH] livepatch: Fix kobject refcount bug on klp_init_patch_early failure path
Date: Sun, 6 Mar 2022 12:51:27 -0500
Message-Id: <20220306175127.5570-1-void@manifault.com>
X-Mailer: git-send-email 2.25.1
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Send this email? ([y]es|[n]o|[e]dit|[q]uit|[a]ll): q
If the patch is ready to be sent, go ahead and send it off by entering y.
Interacting with reviewers
Congratulations, you’ve sent out your first patch! We're not quite done, though. Almost without fail, a reviewer will have a suggestion for how your patch can be improved. This is just a reality about Linux kernel development, and at the end of the day is a good thing as it keeps the bar high, and maximizes the quality of your code.
Once discussion has settled down and you have a clear idea of what needs to be changed for the next version of the patch, you can create a new v2 version that includes any such changes. Doing so is quite simple, you just invoke git format-patch with a -v2 flag:
$ git format-patch HEAD~1 -v 2 -o /tmp
/tmp/v2-0001-livepatch-Fix-kobject-refcount-bug-on-klp_init_pa.patch
This creates a “v2” version of the patch:
$ cat /tmp/v2-0001-livepatch-Fix-kobject-refcount-bug-on-klp_init_pa.patch | grep PATCH
Subject: [PATCH v2] livepatch: Fix kobject refcount bug on klp_init_patch_early
failure path
You can now send the patch back to the upstream community, again using git send-email as you did for the first iteration. If you’d like, you can choose to send out the v2 version of the email as a reply to the initial v1 email you sent out by including the --in-reply-to=<message_id> flag. Just substitute <message_id> with the Message ID for the initial email. It’s not required to do this, and is done at the discretion of the patch author.
Quick aside – using kmail
If you’re like me and you find it tiresome to invoke scripts/get_maintainer.pl and manually include the names when invoking git send-email, you can use a tool that I wrote called kmail to do it for you. I won’t describe the tool in much detail here to avoid scope creep in the post. The main point is that it automatically calls scripts/get_maintainer.pl for you, and then invokes git send-email with all of the necessary email addresses.
Merging the patch
Once the patch has been fully reviewed (and received the necessary Acked-by: <> and Reviewed-by: <> tags by the reviewers), your job is finished! One of the maintainers that has write ownership for the subsystem’s repository will manually apply your patch. Then, at some point, it will make its way into Linus’ tree and the mainline kernel!
What to do if nobody responds to your email
Unfortunately, sometimes your patch may go unanswered. If this happens, first make sure that your patch actually made it to the lists you sent it to. For example, the email may silently fail if you don't use plain-text encoding in the email.
If your email is indeed getting sent to the lists and maintainers, but you still haven't received any response, then I recommend emailing the patch again after ~2 weeks, and then again consistently every 2 weeks if you don't hear from anybody.
In an ideal world this should never happen, but unfortunately it sometimes does. It is the job of a maintainer to respond to patches in a timely manner. If you're not receiving a response, just give it some more time, and how the maintainers that you're serious about the patch being reviewed. Note that this applies to very simple patches as well! Just because your patch isn't some hugely complicated project, doesn't mean that it isn't worth anyone's time.
Don’t be nervous, just do your best, and assume good intent
One more thing I wanted to mention before wrapping up: if you’re feeling nervous or intimidated about sending patches to the Linux kernel, that is totally normal, and should not discourage you from contributing and becoming a member of the community. If you take a closer look at the first patch that I linked above, you’ll notice that it was “v3”, or the third version of the patch. While two upstream reviewers accepted the v3 version with some encouraging words, it took a bit of back and forth to get the patch to a reasonable state.
The first v1 version of the patch had a few issues with it:
1. I used markdown in the patch summary. While some subsystems may allow this, many others will not. I should have checked what the norm was for the livepatching subsystem, but neglected to. How embarrassing! But not really. It was pointed out to me in a review for another patch that patch summaries in the kernel don’t use markdown. The reviewer (who happens to be a prolific and influential kernel developer), just gently pointed out my oversight, and that was that.
2. My initial v1 patch actually made the documentation worse, according to the reviewers! Ugh, no! Mortifying! But not really. The upstream contributor not only took the time to consider my patch, but even built the documentation locally to see what it would look like. Rather than retreating, I thanked the reviewer for taking the time to review my code, and proposed something that seemed more canonical and useful. That suggestion ended up making its way into the final patch, and thanks to the reviewers' feedback, the patch was significantly improved.
Neither of these hiccups ended up being problematic because:
1. I assumed good intent by the reviewers.
2. I took their feedback seriously, and did my best to propose something that made the patch more valuable.
3. I didn’t make the reviewers repeat themselves! It’s not a problem to make a mistake, or to not know something that will come with experience such as patch summaries not using markdown. But since I’ve received that feedback, I’ve made darn sure to never send another patch out that has the same issue!
Wrapping up
That’s all there is to it. Once you’ve written, submitted, and merged your first patch, you’re officially a Linux kernel contributor! Happy hacking, and thanks for reading!
Comments
Sign in or become a Byte Lab member to join the conversation.