The original patch changes the initialization of 'cubictcp' instance of

struct tcp_congestion_ops ('cubictcp.cwnd_event' field). Kpatch

intentionally rejects to process such changes.

This modification of the patch uses Kpatch load/unload hooks to set

'cubictcp.cwnd_event' when the binary patch is loaded and reset it to NULL

when the patch is unloaded.

It is still needed to check if changing that field could be problematic

due to concurrency issues, etc.

'cwnd_event' callback is used only via tcp_ca_event() function.

include/net/tcp.h:

static inline void tcp_ca_event(struct sock *sk, const enum tcp_ca_event event)

{

	const struct inet_connection_sock *icsk = inet_csk(sk);

	if (icsk->icsk_ca_ops->cwnd_event)

		icsk->icsk_ca_ops->cwnd_event(sk, event);

}

In turn, tcp_ca_event() is called in a number of places in

net/ipv4/tcp_output.c and net/ipv4/tcp_input.c.

One problem with this modification of the patch is that it may not be safe

to unload it. If it is possible for tcp_ca_event() to run concurrently with

the unloading of the patch, it may happen that 'icsk->icsk_ca_ops->cwnd_event'

is the address of bictcp_cwnd_event() when tcp_ca_event() checks it but is

set to NULL right after. So 'icsk->icsk_ca_ops->cwnd_event(sk, event)' would

result in a kernel oops.

Whether such scenario is possible or not, it should be analyzed. If it is,

then at least, the body of tcp_ca_event() should be made atomic w.r.t.

changing 'cwnd_event' in the patch somehow. Perhaps, RCU could be suitable

for that: a read-side critical section for the body of tcp_ca_event() with

a single read of icsk->icsk_ca_ops->cwnd_event pointer with rcu_dereference().

The pointer could be set by the patch with rcu_assign_pointer().

An alternative suggested by Josh Poimboeuf would be to patch the functions

that call 'cwnd_event' callback (tcp_ca_event() in this case) so that they

call bictcp_cwnd_event() directly when they detect the cubictcp struct [1].

Note that tcp_ca_event() is inlined in a number of places, so the binary

patch will provide replacements for all of the corresponding functions

rather than for just one. It is still needed to check if replacing these

functions in runtime is safe.

References:

[1] https://www.redhat.com/archives/kpatch/2015-September/msg00005.html

diff --git a/net/ipv4/tcp_cubic.c b/net/ipv4/tcp_cubic.c

index 894b7ce..9bff8a0 100644

--- a/net/ipv4/tcp_cubic.c

+++ b/net/ipv4/tcp_cubic.c

@@ -153,6 +153,27 @@ static void bictcp_init(struct sock *sk)

 		tcp_sk(sk)->snd_ssthresh = initial_ssthresh;

 }

+static void bictcp_cwnd_event(struct sock *sk, enum tcp_ca_event event)

+{

+	if (event == CA_EVENT_TX_START) {

+		struct bictcp *ca = inet_csk_ca(sk);

+		u32 now = tcp_time_stamp;

+		s32 delta;

+

+		delta = now - tcp_sk(sk)->lsndtime;

+

+		/* We were application limited (idle) for a while.

+		 * Shift epoch_start to keep cwnd growth to cubic curve.

+		 */

+		if (ca->epoch_start && delta > 0) {

+			ca->epoch_start += delta;

+			if (after(ca->epoch_start, now))

+				ca->epoch_start = now;

+		}

+		return;

+	}

+}

+

 /* calculate the cubic root of x using a table lookup followed by one

  * Newton-Raphson iteration.

  * Avg err ~= 0.195%

@@ -444,6 +465,20 @@ static struct tcp_congestion_ops cubictcp __read_mostly = {

 	.name		= "cubic",

 };

+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);

+

 static int __init cubictcp_register(void)

 {

 	BUILD_BUG_ON(sizeof(struct bictcp) > ICSK_CA_PRIV_SIZE);