Add example of using static_key_enabled to patch-author-guild.md

[wardenjohn]

Add an example of using static_key_enabled to patch-author-guild.md

[jpoimboe]

This doesn't actually use static_key_enabled()?

[wardenjohn]

Oh! In my use case, I found that using static_branch_unlikely(&memcg_kmem_enabled_key) -> unlikely(&(&memcg_kmem_enabled_key)->key) works well. It may be a use case of fixing static key. To adjust to the patch-author-guide, I have add an new case using static_key_enabled while keeping this original use case.

[wardenjohn]

I rewrite my document and changed my test case. In this newer commit, static_key_enabled is rewrote in a macros. However, if we change the definition in header file may cause too much negative effect. So, I also provide a better solution to this situation. Thanks~

[joe-lawrence]

IMHO (re)defining a kernel macro like static_branch_unlikely is a recipe for developer / review confusion. I think the best practice would be substituting with unlikely(static_key_enabled(&memcg_kmem_enabled_key))

[wardenjohn]

Redefining a kernel macro is a choice if this macro effect limit functions. However, it is not a good idea to redifine it in most cases which is mentioned in the following text. Of course, I can rewrite this description in a more appropriate method.

[joe-lawrence]

Touching header files is another invitation to developer funtimes. Longer builds, more create-diff-object work, potential KABI recalculation, etc. I would just drop the idea since the text already admits it may not be the best idea.

[wardenjohn]

True, I will fulfill the description with the suggestion you given. Thanks, joe~

[joe-lawrence]

Defining a new function (and updating callers) may be a reasonable idea, though this implementation still looks confusing. Wouldn't unlikely(static_key_enabled(&memcg_kmem_enabled_key)) suffice?

[wardenjohn]

As the previous comment, when I tried to use static_key_enabled(&memcg_kmem_enabled_key) instead, but got create-diff-obejct error. I will fix my text after I truly figure out why this method is not work in this change.

[wardenjohn]

It is a little strange. When I use return unlikely(static_key_enabled(&memcg_kmem_enabled_key)). create-diff-object report that jump label found, shoud use static_key_enabled instead.

[wardenjohn]

@joe-lawrence @jpoimboe I had resubmmit and fixed some description. Please come to have a look. Thanks~~

[joe-lawrence]

@joe-lawrence @jpoimboe I had resubmmit and fixed some description. Please come to have a look. Thanks~~

Thanks, will try to take a look this week.

[joe-lawrence]

Maybe it would more straightforward to just include a full example. Unfortunately the affected "real" kpatches that we've created to solve CVEs weren't simple enough to use here (they all required a bunch of other unrelated workarounds that would be confusing).

How about something like:

Given a hypothetical patch to the ixgbe driver (kpatch code represented by a nop() to create object code differences):

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 094653e81b97..36372d3c188e 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -2245,6 +2245,10 @@ static struct sk_buff *ixgbe_run_xdp(struct ixgbe_adapter *adapter,
 		xdpf = xdp_convert_buff_to_frame(xdp);
 		if (unlikely(!xdpf))
 			goto out_failure;
+
+		/* kpatch change represented by nop() call */
+		nop();
+
 		ring = ixgbe_determine_xdp_ring(adapter);
 		if (static_branch_unlikely(&ixgbe_xdp_locking_key))
 			spin_lock(&ring->tx_lock);

It will result in kpatch-build errors:

create-diff-object: ERROR: ixgbe_main.o: kpatch_regenerate_special_section: 2661: Found 2 unsupported jump label(s) in the patched code. Use static_key_enabled() instead.
ixgbe_main.o: Found a jump label at ixgbe_poll()+0xa50, using key ixgbe_xdp_locking_key, which is defined in a module.  Use static_key_enabled() instead.
ixgbe_main.o: Found a jump label at ixgbe_poll()+0xa62, using key ixgbe_xdp_locking_key, which is defined in a module.  Use static_key_enabled() instead.

Following kpatch-build advice, the kpatch author needs to manually adjust any static_branch_{un,}likely() and static_key_{true,false}() function calls in kpatch-updated functions only (not the entire module or .c file):

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 4507fba8747a..dd945f58354e 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -2219,11 +2219,15 @@ static struct sk_buff *ixgbe_run_xdp(struct ixgbe_adapter *adapter,
 		xdpf = xdp_convert_buff_to_frame(xdp);
 		if (unlikely(!xdpf))
 			goto out_failure;
+
+		/* kpatch change represented by nop() call */
+		nop();
+
 		ring = ixgbe_determine_xdp_ring(adapter);
-		if (static_branch_unlikely(&ixgbe_xdp_locking_key))
+		if (unlikely(static_key_enabled(&ixgbe_xdp_locking_key)))	/* kpatch workaround */
 			spin_lock(&ring->tx_lock);
 		result = ixgbe_xmit_xdp_ring(ring, xdpf);
-		if (static_branch_unlikely(&ixgbe_xdp_locking_key))
+		if (unlikely(static_key_enabled(&ixgbe_xdp_locking_key)))	/* kpatch workaround */
 			spin_unlock(&ring->tx_lock);
 		if (result == IXGBE_XDP_CONSUMED)
 			goto out_failure;

And then to incorporate your original idea... instead of redefining static_branch_unlikely(), what if we created a kpatch specific version of the macro to include in the kpatch or even move into "kpatch-macros.h" if it is useful:

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 4507fba8747a..e93acae3327c 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -2194,6 +2194,8 @@ static struct sk_buff *ixgbe_build_skb(struct ixgbe_ring *rx_ring,
 	return skb;
 }
 
+#define klp_static_branch_unlikely(x)       unlikely(static_key_enabled(&(x)->key))
+
 static struct sk_buff *ixgbe_run_xdp(struct ixgbe_adapter *adapter,
 				     struct ixgbe_ring *rx_ring,
 				     struct xdp_buff *xdp)
@@ -2219,11 +2221,15 @@ static struct sk_buff *ixgbe_run_xdp(struct ixgbe_adapter *adapter,
 		xdpf = xdp_convert_buff_to_frame(xdp);
 		if (unlikely(!xdpf))
 			goto out_failure;
+
+		/* kpatch change represented by nop() call */
+		nop();
+
 		ring = ixgbe_determine_xdp_ring(adapter);
-		if (static_branch_unlikely(&ixgbe_xdp_locking_key))
+		if (klp_static_branch_unlikely(&ixgbe_xdp_locking_key))	/* kpatch workaround */
 			spin_lock(&ring->tx_lock);
 		result = ixgbe_xmit_xdp_ring(ring, xdpf);
-		if (static_branch_unlikely(&ixgbe_xdp_locking_key))
+		if (klp_static_branch_unlikely(&ixgbe_xdp_locking_key))	/* kpatch workaround */
 			spin_unlock(&ring->tx_lock);
 		if (result == IXGBE_XDP_CONSUMED)
 			goto out_failure;

I know I always have to come back to this documentation when applying the static_key workarounds. IIRC, older kernels RHEL7?) used a non-intuitive interface that I always got wrong. I don't remember the exact details, but if I could just throw "klp_" in front of the existing code, I'd love to not have to worry about the details any further. :)

[wardenjohn]

Puting a full patch in to patch-author-guild.md may be clear enough to users.

And yet, I think your idea of creating a version of macros to static_key also a interesting idea, which can make to slove problem more easily.

When trying to add a macros into "kpatch-macros.h", the description here should be changed. Because we should tell user how to slove the static_key problem with macros.

If you think using macros is better, I'll try to help create a new macro. But the this document should change a different version after the new macros is created.

Thanks, Joe.

[wardenjohn]

@joe-lawrence @jpoimboe Hi, maintainers, is there some suggestion for this commit? Hoping to hear from you.

[joe-lawrence]

Hi @wardenjohn : I think the "kpatch-macros.h" approach is the way to go. Then we can add a full example, like the one I made to ixgbe_run_xdp(), to the doc.

[github-actions]

This PR has been open for 60 days with no activity and no assignee. It will be closed in 7 days unless a comment is added.

[github-actions]

This PR was closed because it was inactive for 7 days after being marked stale.