kpatch-build: enable option -R|--replace to build replace klp

[liu-song-6]

Since 5.1 kernel, klp_patch supports a "replace" option, which does atomic
replace of cumulative patches. Enable building such patch with an option.

Signed-off-by: Song Liu song@kernel.org

[joe-lawrence]

Atomic replace was backported to rhel-8's kernel-4.18.0-147.3.el8. It was never backported to rhel-7.

[liu-song-6]

I think we can do rhel_kernel_version_gte 4.18.0-147.el8 in kpatch-build. But I am not sure how to check it here. Is RHEL_RELEASE_CODE >= RHEL_RELEASE_VERSION(8, 0) sufficient?

[joe-lawrence]

For the .c code, we can use RHEL_RELEASE_CODE(8, 2) since atomic replace support was only added in 8.2 GA kernels onward.

For the kpatch-build code, we could have used rhel_kernel_version_gte 4.18.0-147.3.el8 (the trailing .3 would have been important), but if the kmod code can only check RHEL release numbers, then let's match these two checks. So then it would be rhel_kernel_version_gte 4.18.0-193.el8 as -193 was the 8.2 GA kernel.

[joe-lawrence]

I don't know how I feel about this approach -- in a way, atomic replace and other features that directly affect the klp_patch object are more than just build options, they may change the way the kpatch is coded. I wonder if we should try to move this in the kpatch itself somehow? This might be too difficult to implement (how to check that said feature is supported, etc.) but I ask in case others have any good ideas.

[liu-song-6]

With current klp_patch->replace design, which unloads all loaded patch with the new patch, I think the decision to use replace or not is done at kpatch-build time. Therefore, we don't need much flexibility in kpatch. (I may have missed some meaningful scenarios, though. )

[joe-lawrence]

Right, the question I have is whether --replace might tempt somebody to use the same input patch without the flag. If klp_patch->replace behavior is encoded in the kpatch itself, then the kpatch author be responsible for doing so. That is to say, the patch author wrote it in the style for atomic replace and expects it to be built that way.

[joe-lawrence]

Song - for your kpatch infrastructure, would you be intending to build all kpatches with atomic replace? I ask, as maybe another way to look at this is to default all kpatches as klp->replace where available. Just curious what your use case is.

[liu-song-6]

In our use cases, I would like to build all kpatches with atomic replace. Each host will have at most one kpatch load at anytime. I think this would give fewer confusions.

[liu-song-6]

@joe-lawrence How does this version look to you? Do we need more work (in kpatch?) to move forward here? Thanks!

[jpoimboe]

Since cumulative updates are the recommended and standard practice, I'd vote to make klp->replace = true the default. The only problem is that this breaks backwards compatibility, so we should be sure to at least bump the minor version for the next release. Or maybe it's time for 1.0!

Also, instead of the in-place 'sed' of the .c file, how about putting the default boolean value in a macro:

#ifndef KLP_REPLACE_ENABLE
#define KLP_REPLACE_ENABLE true
#endif

Then kpatch-build can change the default if needed by passing the -D option to gcc.

Also, I think some update to the patch author guide document is needed, with respect to callbacks. There's a subtle difference between replace and non-replace. With replace, a cumulative update will call the prior patch's pre-unpatch callback which could have an unintended effect if the patch author isn't careful. Maybe we should consider adding a new pre-replace callback.

[liu-song-6]

Since cumulative updates are the recommended and standard practice, I'd vote to make klp->replace = true the default. The only problem is that this breaks backwards compatibility, so we should be sure to at least bump the minor version for the next release. Or maybe it's time for 1.0!

If we use replace as default, shall we add -N|--non-replace? Any better name for this option? Personally, I still think non-replace should be the default (considering the pre-unpatch pre-replace issue).

Also, instead of the in-place 'sed' of the .c file, how about putting the default boolean value in a macro:

#ifndef KLP_REPLACE_ENABLE
#define KLP_REPLACE_ENABLE true
#endif

Yes, this is better. Let me fix it.

Then kpatch-build can change the default if needed by passing the -D option to gcc.

Also, I think some update to the patch author guide document is needed, with respect to callbacks. There's a subtle difference between replace and non-replace. With replace, a cumulative update will call the prior patch's pre-unpatch callback which could have an unintended effect if the patch author isn't careful. Maybe we should consider adding a new pre-replace callback.

Let me also take a look at documentation and pre-replace. Thanks!

[joe-lawrence]

Since cumulative updates are the recommended and standard practice, I'd vote to make klp->replace = true the default. The only problem is that this breaks backwards compatibility, so we should be sure to at least bump the minor version for the next release. Or maybe it's time for 1.0!

Version 1.0 sounds good to me 🥳 Unless you want to reserve that for eventual kpatch.ko support removal.

Also, instead of the in-place 'sed' of the .c file, how about putting the default boolean value in a macro:

#ifndef KLP_REPLACE_ENABLE
#define KLP_REPLACE_ENABLE true
#endif

Then kpatch-build can change the default if needed by passing the -D option to gcc.

Agreed. A -N|--non-replace option works for me. If we decide to default to using atomic replace, we should call that out loudly in the release notes.

Also, I think some update to the patch author guide document is needed, with respect to callbacks. There's a subtle difference between replace and non-replace. With replace, a cumulative update will call the prior patch's pre-unpatch callback which could have an unintended effect if the patch author isn't careful. Maybe we should consider adding a new pre-replace callback.

Yeah, it could definitely use an update here. The kernel docs on cumulative-patches provide a good summary of callback behavior under atomic replace. I think the major difference is that that only the new cumulative callbacks will ever be called. See the atomic replace test in the test-callbacks selftest for example. So I'm not sure if a pre-replace callback is needed, unless we wanted to introduce a change in this behavior upstream (no old callbacks run, except this special one). I think the new cumulative patch's pre-patch callback should fit most use cases, or am I missing something?

[jpoimboe]

Since cumulative updates are the recommended and standard practice, I'd vote to make klp->replace = true the default. The only problem is that this breaks backwards compatibility, so we should be sure to at least bump the minor version for the next release. Or maybe it's time for 1.0!

Version 1.0 sounds good to me partying_face Unless you want to reserve that for eventual kpatch.ko support removal.

kpatch.ko is already deprecated and, IIRC, broken with new kernels anyway, so its eventual removal may be a non-event.

Agreed. A -N|--non-replace option works for me. If we decide to default to using atomic replace, we should call that out loudly in the release notes.

Sounds good to me. Another possibility would be a positively-worded option (-i|--incremental?) but either way works for me.

Also, I think some update to the patch author guide document is needed, with respect to callbacks. There's a subtle difference between replace and non-replace. With replace, a cumulative update will call the prior patch's pre-unpatch callback which could have an unintended effect if the patch author isn't careful. Maybe we should consider adding a new pre-replace callback.

Yeah, it could definitely use an update here. The kernel docs on cumulative-patches provide a good summary of callback behavior under atomic replace. I think the major difference is that that only the new cumulative callbacks will ever be called. See the atomic replace test in the test-callbacks selftest for example. So I'm not sure if a pre-replace callback is needed, unless we wanted to introduce a change in this behavior upstream (no old callbacks run, except this special one). I think the new cumulative patch's pre-patch callback should fit most use cases, or am I missing something?

I guess I was wrong in thinking that the previous patch's pre-unpatch callback gets called. So forget my pre-replace callback idea.

[liu-song-6]

I updated the PR with

• -N|--replace option
• use -DKLP_REPLACE_ENABLE instead of sed
• a brief introduction of the option in patch-author-guide.md

Please let me know your thoughts on this. Thanks!

[liu-song-6]

@joe-lawrence @jpoimboe Could you please share your feedbacks on this version? Thanks!

[jpoimboe]

Thinking about it again, the '-N' bothers me, since it stands for "non" it can mean disabling anything. Can we change to '-R'?

I'd prefer not to print the "Disabling replace for KLP" message as we try to keep the kpatch-build output down to a bare minimum.

Otherwise it's looking good to me.

[jpoimboe]

"-N" -> "-R"

[joe-lawrence]

Sorry on getting back to this one @liu-song-6 , swamped with work items at the moment. I just want to pop in to say that changing default kpatch-build to atomic replace breaks the integration tests when they run for all the older (unsupported) kernels.

We could 1) only use atomic replace default if the target kernel supports it. I think that would be confusing. or 2 ) copy that support_klp_replace() kernel version check into the kpatch-test script so that the integration tests build with -R for all those older kernels. This should work since I don't believe any of our integration test patches are sensitive to atomic replace semantics.

Also, a really small nit: I think your editor automatically clears any trailing whitespace from the files that were edited. Not a big deal, but the diff stat would be cleaner without all those edits.

[joe-lawrence]

@liu-song-6 : looks mostly good, I think updating the manpage and the integration test script should be all that's left

[joe-lawrence]

These whitespace changes are still scattered throughout the PR. Is there a way to omit them so they don't add unrelated changes?

[liu-song-6]

It requires some manual work. But yeah, I can get rid of them.

[joe-lawrence]

Was there a reason not to copy kpatch-build's support_klp_replace() and associated functions? If it's only a few more net LOC, but if it matches exactly, hopefully it will be easier to keep synchronized when any updates are needed.

[liu-song-6]

Will fix.

[joe-lawrence]

Looks good, @liu-song-6 , thanks for working on this one!