[kpatch] Preparing a patch - pitfalls

[Evgenii Shatokhin]

Hi,

28.09.2015 20:35, Josh Poimboeuf пишет:
> On Mon, Sep 28, 2015 at 04:59:17PM +0300, Evgenii Shatokhin wrote:
>> 25.09.2015 21:51, Evgenii Shatokhin пишет:
>>> Hi,
>>>
>>> In the presentation about using Kpatch in RHEL 7, Seth Jennings mentions
>>> the following:
>>>
>>>   "The patches in the upstream kernel to fix CVEs are not designed to be
>>> applied at runtime. There are certain pitfalls that need to be checked
>>> for when generating a live patch."
>>>
>>> Could you elaborate? What are these pitfalls that should be checked for
>>> when preparing a Kpatch-based patch? What kind of analysis should be done?
>>
>> I suppose, one of these pitfalls related to the handling of global data,
>> discussed in this ML some time ago. The changes in such data are not
>> detected and require workarounds. Stumbled upon this when trying to make a
>> Kpatch-based patch from https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=30927520dbae297182990bb21d08762bcc35ce1d.
>>
>> Any other pitfalls to consider?
>
> Hi Evgenii,
>
> This is actually a very large and difficult topic.  The short answer is
> that you need to fully understand all the implications of the patch and
> how it would be applied in the live patching process.  And then you
> often have to get creative.
>
> The long answer is a book which unfortunately we haven't written yet.
> We will eventually be writing a live patch creation guide to try to
> collect and document best practices.  There are many types of cases to
> consider and often several ways to solve them.
>
> For the patch you pointed to, you'll need to remove the following hunk
> because kpatch intentionally rejects data structure initialization
> changes:
>
> @@ -450,6 +465,7 @@ static struct tcp_congestion_ops cubictcp __read_mostly = {
>          .cong_avoid     = bictcp_cong_avoid,
>          .set_state      = bictcp_state,
>          .undo_cwnd      = bictcp_undo_cwnd,
> +       .cwnd_event     = bictcp_cwnd_event,
>          .pkts_acked     = bictcp_acked,
>          .owner          = THIS_MODULE,
>          .name           = "cubic",
>
> Instead of that initialization you'll need to write some code which
> accomplishes the same task.
>
> One option would be to make a new function which updates the
> cubictcp.cwnd_event pointer at runtime.  You can tell kpatch to call
> this new function during the patching process using the KPATCH_LOAD_HOOK
> macro.  You'd also need to inspect the kernel code to make sure there
> are no concurrency issues related to upating this pointer at runtime.

Yes, when experimenting with the patch, I removed that hunk and added 
load/unload hooks:

+void kpatch_load_cubictcp_cwnd_event(void)
+{
+ cubictcp.cwnd_event = bictcp_cwnd_event;
+}
+
+void kpatch_unload_cubictcp_cwnd_event(void)
+{
+ cubictcp.cwnd_event = NULL;
+}
+
+#include "kpatch-macros.h"
+KPATCH_LOAD_HOOK(kpatch_load_cubictcp_cwnd_event);
+KPATCH_UNLOAD_HOOK(kpatch_unload_cubictcp_cwnd_event);

It works now, but you are right, I still need to prove that changing 
that field in runtime causes no issues. If some kernel code, for 
example, checks if cubictcp.cwnd_event is NULL concurrently with the 
loading of the patch, it may mean trouble, indeed.

Thanks for pointing this out!

>
> Another option would be to patch the function(s) which call using the
> cwnd_event() function pointer.  They could be changed to call
> bictcp_cwnd_event() directly when they detect the cubictcp struct.
>

Interesting. Did not think about that.

As for the guide you mentioned - yes, that would be very useful.

At the present time, perhaps, there are some drafts, random pieces of 
advice or may be collections of patches that needed additional analysis 
and rewriting? Such examples of the patches would be extremely useful 
even without explanations.

I am asking because we are now considering using Kpatch at least for 
regular testing of some updates for the kernels in our systems. Then 
(who knows) our experts might decide to deliver security updates this 
way to our customers too. So, one must be extra careful indeed, when 
preparing the patches. Better to know the pitfalls earlier.

Thanks again!

Regards,
Evgenii