[PATCH v2] net: sched: Add support for packet bursting.

[PATCH v2] net: sched: Add support for packet bursting.

[Niclas Hedam]

From 71843907bdb9cdc4e24358f0c16a8778f2762dc7 Mon Sep 17 00:00:00 2001
From: Niclas Hedam <nhed@itu.dk>
Date: Fri, 25 Jun 2021 13:37:18 +0200
Subject: [PATCH] net: sched: Add support for packet bursting.

This commit implements packet bursting in the NetEm scheduler.
This allows system administrators to hold back outgoing
packets and release them at a multiple of a time quantum.
This feature can be used to prevent timing attacks caused
by network latency.

Signed-off-by: Niclas Hedam <nhed@itu.dk>
---
v2: add enum at end of list (Cong Wang)
include/uapi/linux/pkt_sched.h |  2 ++
net/sched/sch_netem.c          | 24 +++++++++++++++++++++---
2 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/include/uapi/linux/pkt_sched.h b/include/uapi/linux/pkt_sched.h
index 79a699f106b1..1ba49f141dae 100644
--- a/include/uapi/linux/pkt_sched.h
+++ b/include/uapi/linux/pkt_sched.h
@@ -603,6 +603,7 @@ enum {
	TCA_NETEM_JITTER64,
	TCA_NETEM_SLOT,
	TCA_NETEM_SLOT_DIST,
+        TCA_NETEM_BURSTING,
	__TCA_NETEM_MAX,
};

@@ -615,6 +616,7 @@ struct tc_netem_qopt {
	__u32	gap;		/* re-ordering gap (0 for none) */
	__u32   duplicate;	/* random packet dup  (0=none ~0=100%) */
	__u32	jitter;		/* random jitter in latency (us) */
+	__u32	bursting;	/* send packets in bursts (us) */
};

struct tc_netem_corr {
diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index 0c345e43a09a..52d796287b86 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -85,6 +85,7 @@ struct netem_sched_data {
	s64 latency;
	s64 jitter;

+	u32 bursting;
	u32 loss;
	u32 ecn;
	u32 limit;
@@ -467,7 +468,7 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch,
	/* If a delay is expected, orphan the skb. (orphaning usually takes
	 * place at TX completion time, so _before_ the link transit delay)
	 */
-	if (q->latency || q->jitter || q->rate)
+	if (q->latency || q->jitter || q->rate || q->bursting)
		skb_orphan_partial(skb);

	/*
@@ -527,8 +528,17 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch,
	qdisc_qstats_backlog_inc(sch, skb);

	cb = netem_skb_cb(skb);
-	if (q->gap == 0 ||		/* not doing reordering */
-	    q->counter < q->gap - 1 ||	/* inside last reordering gap */
+	if (q->bursting > 0) {
+		u64 now;
+
+		now = ktime_get_ns();
+
+		cb->time_to_send = now - (now % q->bursting) + q->bursting;
+
+		++q->counter;
+		tfifo_enqueue(skb, sch);
+	} else if (q->gap == 0 ||		/* not doing reordering */
+	    q->counter < q->gap - 1 ||		/* inside last reordering gap */
	    q->reorder < get_crandom(&q->reorder_cor)) {
		u64 now;
		s64 delay;
@@ -927,6 +937,7 @@ static const struct nla_policy netem_policy[TCA_NETEM_MAX + 1] = {
	[TCA_NETEM_ECN]		= { .type = NLA_U32 },
	[TCA_NETEM_RATE64]	= { .type = NLA_U64 },
	[TCA_NETEM_LATENCY64]	= { .type = NLA_S64 },
+	[TCA_NETEM_BURSTING]	= { .type = NLA_U64 },
	[TCA_NETEM_JITTER64]	= { .type = NLA_S64 },
	[TCA_NETEM_SLOT]	= { .len = sizeof(struct tc_netem_slot) },
};
@@ -1001,6 +1012,7 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt,

	q->latency = PSCHED_TICKS2NS(qopt->latency);
	q->jitter = PSCHED_TICKS2NS(qopt->jitter);
+	q->bursting = PSCHED_TICKS2NS(qopt->bursting);
	q->limit = qopt->limit;
	q->gap = qopt->gap;
	q->counter = 0;
@@ -1032,6 +1044,9 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt,
	if (tb[TCA_NETEM_LATENCY64])
		q->latency = nla_get_s64(tb[TCA_NETEM_LATENCY64]);

+	if (tb[TCA_NETEM_BURSTING])
+		q->bursting = nla_get_u64(tb[TCA_NETEM_BURSTING]);
+
	if (tb[TCA_NETEM_JITTER64])
		q->jitter = nla_get_s64(tb[TCA_NETEM_JITTER64]);

@@ -1150,6 +1165,9 @@ static int netem_dump(struct Qdisc *sch, struct sk_buff *skb)
			     UINT_MAX);
	qopt.jitter = min_t(psched_tdiff_t, PSCHED_NS2TICKS(q->jitter),
			    UINT_MAX);
+	qopt.bursting = min_t(psched_tdiff_t, PSCHED_NS2TICKS(q->bursting),
+			    UINT_MAX);
+
	qopt.limit = q->limit;
	qopt.loss = q->loss;
	qopt.gap = q->gap;
--
2.25.1

Re: [PATCH v2] net: sched: Add support for packet bursting.

[Toke Høiland-Jørgensen]

Niclas Hedam <nhed@itu.dk> writes:

> From 71843907bdb9cdc4e24358f0c16a8778f2762dc7 Mon Sep 17 00:00:00 2001
> From: Niclas Hedam <nhed@itu.dk>
> Date: Fri, 25 Jun 2021 13:37:18 +0200
> Subject: [PATCH] net: sched: Add support for packet bursting.

Something went wrong with the formatting here.

> This commit implements packet bursting in the NetEm scheduler.
> This allows system administrators to hold back outgoing
> packets and release them at a multiple of a time quantum.
> This feature can be used to prevent timing attacks caused
> by network latency.

How is this bursting feature different from the existing slot-based
mechanism?

-Toke

Re: [PATCH v2] net: sched: Add support for packet bursting.

[Niclas Hedam]

>> From 71843907bdb9cdc4e24358f0c16a8778f2762dc7 Mon Sep 17 00:00:00 2001
>> From: Niclas Hedam <nhed@itu.dk>
>> Date: Fri, 25 Jun 2021 13:37:18 +0200
>> Subject: [PATCH] net: sched: Add support for packet bursting.
> 
> Something went wrong with the formatting here.

I'll resubmit with fixed formatting. My bad.

>> 
>> This commit implements packet bursting in the NetEm scheduler.
>> This allows system administrators to hold back outgoing
>> packets and release them at a multiple of a time quantum.
>> This feature can be used to prevent timing attacks caused
>> by network latency.
> 
> How is this bursting feature different from the existing slot-based
> mechanism?

It is similar, but the reason for separating it is the audience that they are catering.
The slots seems to be focused on networking constraints and duty cycles.
My contribution and mechanism is mitigating timing attacks. The complexity of slots are mostly unwanted in this context as we want as few CPU cycles as possible.

> On 28 Jun 2021, at 13:44, Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> 
> Niclas Hedam <nhed@itu.dk> writes:
> 
>> From 71843907bdb9cdc4e24358f0c16a8778f2762dc7 Mon Sep 17 00:00:00 2001
>> From: Niclas Hedam <nhed@itu.dk>
>> Date: Fri, 25 Jun 2021 13:37:18 +0200
>> Subject: [PATCH] net: sched: Add support for packet bursting.
> 
> Something went wrong with the formatting here.
> 
>> This commit implements packet bursting in the NetEm scheduler.
>> This allows system administrators to hold back outgoing
>> packets and release them at a multiple of a time quantum.
>> This feature can be used to prevent timing attacks caused
>> by network latency.
> 
> How is this bursting feature different from the existing slot-based
> mechanism?
> 
> -Toke
> 

[PATCH v3] net: sched: Add support for packet bursting.

[Niclas Hedam]

This commit implements packet bursting in the NetEm scheduler.
This allows system administrators to hold back outgoing
packets and release them at a multiple of a time quantum.
This feature can be used to prevent timing attacks caused
by network latency.

Signed-off-by: Niclas Hedam <niclas@hed.am>
---
v2: add enum at end of list (Cong Wang)
v3: fixed formatting of commit diff (Toke Høiland-Jørgensen)
 include/uapi/linux/pkt_sched.h |  2 ++
 net/sched/sch_netem.c          | 24 +++++++++++++++++++++---
 2 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/include/uapi/linux/pkt_sched.h b/include/uapi/linux/pkt_sched.h
index 79a699f106b1..1ba49f141dae 100644
--- a/include/uapi/linux/pkt_sched.h
+++ b/include/uapi/linux/pkt_sched.h
@@ -603,6 +603,7 @@ enum {
 	TCA_NETEM_JITTER64,
 	TCA_NETEM_SLOT,
 	TCA_NETEM_SLOT_DIST,
+        TCA_NETEM_BURSTING,
 	__TCA_NETEM_MAX,
 };

@@ -615,6 +616,7 @@ struct tc_netem_qopt {
 	__u32	gap;		/* re-ordering gap (0 for none) */
 	__u32   duplicate;	/* random packet dup  (0=none ~0=100%) */
 	__u32	jitter;		/* random jitter in latency (us) */
+	__u32	bursting;	/* send packets in bursts (us) */
 };

 struct tc_netem_corr {
diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index 0c345e43a09a..52d796287b86 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -85,6 +85,7 @@ struct netem_sched_data {
 	s64 latency;
 	s64 jitter;

+	u32 bursting;
 	u32 loss;
 	u32 ecn;
 	u32 limit;
@@ -467,7 +468,7 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 	/* If a delay is expected, orphan the skb. (orphaning usually takes
 	 * place at TX completion time, so _before_ the link transit delay)
 	 */
-	if (q->latency || q->jitter || q->rate)
+	if (q->latency || q->jitter || q->rate || q->bursting)
 		skb_orphan_partial(skb);

 	/*
@@ -527,8 +528,17 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 	qdisc_qstats_backlog_inc(sch, skb);

 	cb = netem_skb_cb(skb);
-	if (q->gap == 0 ||		/* not doing reordering */
-	    q->counter < q->gap - 1 ||	/* inside last reordering gap */
+	if (q->bursting > 0) {
+		u64 now;
+
+		now = ktime_get_ns();
+
+		cb->time_to_send = now - (now % q->bursting) + q->bursting;
+
+		++q->counter;
+		tfifo_enqueue(skb, sch);
+	} else if (q->gap == 0 ||		/* not doing reordering */
+	    q->counter < q->gap - 1 ||		/* inside last reordering gap */
 	    q->reorder < get_crandom(&q->reorder_cor)) {
 		u64 now;
 		s64 delay;
@@ -927,6 +937,7 @@ static const struct nla_policy netem_policy[TCA_NETEM_MAX + 1] = {
 	[TCA_NETEM_ECN]		= { .type = NLA_U32 },
 	[TCA_NETEM_RATE64]	= { .type = NLA_U64 },
 	[TCA_NETEM_LATENCY64]	= { .type = NLA_S64 },
+	[TCA_NETEM_BURSTING]	= { .type = NLA_U64 },
 	[TCA_NETEM_JITTER64]	= { .type = NLA_S64 },
 	[TCA_NETEM_SLOT]	= { .len = sizeof(struct tc_netem_slot) },
 };
@@ -1001,6 +1012,7 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt,

 	q->latency = PSCHED_TICKS2NS(qopt->latency);
 	q->jitter = PSCHED_TICKS2NS(qopt->jitter);
+	q->bursting = PSCHED_TICKS2NS(qopt->bursting);
 	q->limit = qopt->limit;
 	q->gap = qopt->gap;
 	q->counter = 0;
@@ -1032,6 +1044,9 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt,
 	if (tb[TCA_NETEM_LATENCY64])
 		q->latency = nla_get_s64(tb[TCA_NETEM_LATENCY64]);

+	if (tb[TCA_NETEM_BURSTING])
+		q->bursting = nla_get_u64(tb[TCA_NETEM_BURSTING]);
+
 	if (tb[TCA_NETEM_JITTER64])
 		q->jitter = nla_get_s64(tb[TCA_NETEM_JITTER64]);

@@ -1150,6 +1165,9 @@ static int netem_dump(struct Qdisc *sch, struct sk_buff *skb)
 			     UINT_MAX);
 	qopt.jitter = min_t(psched_tdiff_t, PSCHED_NS2TICKS(q->jitter),
 			    UINT_MAX);
+	qopt.bursting = min_t(psched_tdiff_t, PSCHED_NS2TICKS(q->bursting),
+			    UINT_MAX);
+
 	qopt.limit = q->limit;
 	qopt.loss = q->loss;
 	qopt.gap = q->gap;
--
2.25.1

Re: [PATCH v2] net: sched: Add support for packet bursting.

[Toke Høiland-Jørgensen]

Niclas Hedam <nhed@itu.dk> writes:

>>> From 71843907bdb9cdc4e24358f0c16a8778f2762dc7 Mon Sep 17 00:00:00 2001
>>> From: Niclas Hedam <nhed@itu.dk>
>>> Date: Fri, 25 Jun 2021 13:37:18 +0200
>>> Subject: [PATCH] net: sched: Add support for packet bursting.
>> 
>> Something went wrong with the formatting here.
>
> I'll resubmit with fixed formatting. My bad.
>
>>> 
>>> This commit implements packet bursting in the NetEm scheduler.
>>> This allows system administrators to hold back outgoing
>>> packets and release them at a multiple of a time quantum.
>>> This feature can be used to prevent timing attacks caused
>>> by network latency.
>> 
>> How is this bursting feature different from the existing slot-based
>> mechanism?
>
> It is similar, but the reason for separating it is the audience that they are catering.
> The slots seems to be focused on networking constraints and duty cycles.
> My contribution and mechanism is mitigating timing attacks. The
> complexity of slots are mostly unwanted in this context as we want as
> few CPU cycles as possible.

(Adding Dave who wrote the slots code)

But you're still duplicating functionality, then? This has a cost in
terms of maintainability and interactions (what happens if someone turns
on both slots and bursting, for instance)?

If the concern is CPU cost (got benchmarks to back that up?), why not
improve the existing mechanism so it can be used for your use case as
well?

-Toke

Re: [PATCH v2] net: sched: Add support for packet bursting.

[Niclas Hedam]

Thanks for the valuable thoughts, Toke.

The patch started with me being tasked to try and mitigate timing attacks caused by network latencies.
I scouted over the current network stack and didn't find anything that fully matched my use-case.
While I now understand that you can actually leverage the slots functionality for this, I would still opt for a new interface and implementation.

I have not done any CPU benchmarks on the slots system, so I'm not approaching this from the practical performance side per se.
Instead, I argue for seperation with reference to the Seperation of Concern design principle. The slots functionality is not built/designed to cater security guarantees, and my patch is not built to cater duty cycles, etc.
If we opt to merge these two functionalities or discard mine, we have to implement some guarantee that the slots functionality won't become significantly slower or complex, which in my opinion is less maintainable than two similar systems. Also, this patch is very limited in lines of code, so maintaining it is pretty trivial.

I do agree, however, that we should define what would happen if you enable both systems at the same time.

@Dave: Any thoughts on this?

> On 28 Jun 2021, at 14:21, Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> 
> Niclas Hedam <nhed@itu.dk> writes:
> 
>>>> From 71843907bdb9cdc4e24358f0c16a8778f2762dc7 Mon Sep 17 00:00:00 2001
>>>> From: Niclas Hedam <nhed@itu.dk>
>>>> Date: Fri, 25 Jun 2021 13:37:18 +0200
>>>> Subject: [PATCH] net: sched: Add support for packet bursting.
>>> 
>>> Something went wrong with the formatting here.
>> 
>> I'll resubmit with fixed formatting. My bad.
>> 
>>>> 
>>>> This commit implements packet bursting in the NetEm scheduler.
>>>> This allows system administrators to hold back outgoing
>>>> packets and release them at a multiple of a time quantum.
>>>> This feature can be used to prevent timing attacks caused
>>>> by network latency.
>>> 
>>> How is this bursting feature different from the existing slot-based
>>> mechanism?
>> 
>> It is similar, but the reason for separating it is the audience that they are catering.
>> The slots seems to be focused on networking constraints and duty cycles.
>> My contribution and mechanism is mitigating timing attacks. The
>> complexity of slots are mostly unwanted in this context as we want as
>> few CPU cycles as possible.
> 
> (Adding Dave who wrote the slots code)
> 
> But you're still duplicating functionality, then? This has a cost in
> terms of maintainability and interactions (what happens if someone turns
> on both slots and bursting, for instance)?
> 
> If the concern is CPU cost (got benchmarks to back that up?), why not
> improve the existing mechanism so it can be used for your use case as
> well?
> 
> -Toke

Re: [PATCH v3] net: sched: Add support for packet bursting.

[Eric Dumazet]

On 6/28/21 2:08 PM, Niclas Hedam wrote:
> This commit implements packet bursting in the NetEm scheduler.
> This allows system administrators to hold back outgoing
> packets and release them at a multiple of a time quantum.
> This feature can be used to prevent timing attacks caused
> by network latency.
> 
> Signed-off-by: Niclas Hedam <niclas@hed.am>
> ---
> v2: add enum at end of list (Cong Wang)
> v3: fixed formatting of commit diff (Toke Høiland-Jørgensen)
>  include/uapi/linux/pkt_sched.h |  2 ++
>  net/sched/sch_netem.c          | 24 +++++++++++++++++++++---
>  2 files changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/include/uapi/linux/pkt_sched.h b/include/uapi/linux/pkt_sched.h
> index 79a699f106b1..1ba49f141dae 100644
> --- a/include/uapi/linux/pkt_sched.h
> +++ b/include/uapi/linux/pkt_sched.h
> @@ -603,6 +603,7 @@ enum {
>  	TCA_NETEM_JITTER64,
>  	TCA_NETEM_SLOT,
>  	TCA_NETEM_SLOT_DIST,
> +        TCA_NETEM_BURSTING,
>  	__TCA_NETEM_MAX,
>  };
> 
> @@ -615,6 +616,7 @@ struct tc_netem_qopt {
>  	__u32	gap;		/* re-ordering gap (0 for none) */
>  	__u32   duplicate;	/* random packet dup  (0=none ~0=100%) */
>  	__u32	jitter;		/* random jitter in latency (us) */
> +	__u32	bursting;	/* send packets in bursts (us) */
>  };
> 
>  struct tc_netem_corr {
> diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
> index 0c345e43a09a..52d796287b86 100644
> --- a/net/sched/sch_netem.c
> +++ b/net/sched/sch_netem.c
> @@ -85,6 +85,7 @@ struct netem_sched_data {
>  	s64 latency;
>  	s64 jitter;
> 
> +	u32 bursting;
>  	u32 loss;
>  	u32 ecn;
>  	u32 limit;
> @@ -467,7 +468,7 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>  	/* If a delay is expected, orphan the skb. (orphaning usually takes
>  	 * place at TX completion time, so _before_ the link transit delay)
>  	 */
> -	if (q->latency || q->jitter || q->rate)
> +	if (q->latency || q->jitter || q->rate || q->bursting)
>  		skb_orphan_partial(skb);
> 
>  	/*
> @@ -527,8 +528,17 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>  	qdisc_qstats_backlog_inc(sch, skb);
> 
>  	cb = netem_skb_cb(skb);
> -	if (q->gap == 0 ||		/* not doing reordering */
> -	    q->counter < q->gap - 1 ||	/* inside last reordering gap */
> +	if (q->bursting > 0) {
> +		u64 now;
> +
> +		now = ktime_get_ns();
> +
> +		cb->time_to_send = now - (now % q->bursting) + q->bursting;

This wont compile on 32bit arches.

> +
> +		++q->counter;
> +		tfifo_enqueue(skb, sch);
> +	} else if (q->gap == 0 ||		/* not doing reordering */
> +	    q->counter < q->gap - 1 ||		/* inside last reordering gap */
>  	    q->reorder < get_crandom(&q->reorder_cor)) {
>  		u64 now;
>  		s64 delay;
> @@ -927,6 +937,7 @@ static const struct nla_policy netem_policy[TCA_NETEM_MAX + 1] = {
>  	[TCA_NETEM_ECN]		= { .type = NLA_U32 },
>  	[TCA_NETEM_RATE64]	= { .type = NLA_U64 },
>  	[TCA_NETEM_LATENCY64]	= { .type = NLA_S64 },
> +	[TCA_NETEM_BURSTING]	= { .type = NLA_U64 },
>  	[TCA_NETEM_JITTER64]	= { .type = NLA_S64 },
>  	[TCA_NETEM_SLOT]	= { .len = sizeof(struct tc_netem_slot) },
>  };
> @@ -1001,6 +1012,7 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt,
> 
>  	q->latency = PSCHED_TICKS2NS(qopt->latency);
>  	q->jitter = PSCHED_TICKS2NS(qopt->jitter);
> +	q->bursting = PSCHED_TICKS2NS(qopt->bursting);

This is a bit silly to use 64bit user<>kernel interface
but still use the old legacy PSCHED_TICKS2NS conversion,
since this will force a big granularity.

If someone want 10 usec bursts, we should allow it.

I would simply use a 32bit value, in ns unit.

(bursts of more than 2^32 ns make no sense)

>  	q->limit = qopt->limit;
>  	q->gap = qopt->gap;
>  	q->counter = 0;
> @@ -1032,6 +1044,9 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt,
>  	if (tb[TCA_NETEM_LATENCY64])
>  		q->latency = nla_get_s64(tb[TCA_NETEM_LATENCY64]);
> 
> +	if (tb[TCA_NETEM_BURSTING])
> +		q->bursting = nla_get_u64(tb[TCA_NETEM_BURSTING]);
> +
>  	if (tb[TCA_NETEM_JITTER64])
>  		q->jitter = nla_get_s64(tb[TCA_NETEM_JITTER64]);
> 
> @@ -1150,6 +1165,9 @@ static int netem_dump(struct Qdisc *sch, struct sk_buff *skb)
>  			     UINT_MAX);
>  	qopt.jitter = min_t(psched_tdiff_t, PSCHED_NS2TICKS(q->jitter),
>  			    UINT_MAX);
> +	qopt.bursting = min_t(psched_tdiff_t, PSCHED_NS2TICKS(q->bursting),
> +			    UINT_MAX);
> +
>  	qopt.limit = q->limit;
>  	qopt.loss = q->loss;
>  	qopt.gap = q->gap;
> --
> 2.25.1
> 

Re: [PATCH v3] net: sched: Add support for packet bursting.

[Niclas Hedam]

Good point. I’ll change it to a 32 bit uint later today.
Removing the tick based conversion removes the ability to provide the interval in various units, doesn’t it?

> On 28 Jun 2021, at 17.27, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> 
> 
> 
>> On 6/28/21 2:08 PM, Niclas Hedam wrote:
>> This commit implements packet bursting in the NetEm scheduler.
>> This allows system administrators to hold back outgoing
>> packets and release them at a multiple of a time quantum.
>> This feature can be used to prevent timing attacks caused
>> by network latency.
>> 
>> Signed-off-by: Niclas Hedam <niclas@hed.am>
>> ---
>> v2: add enum at end of list (Cong Wang)
>> v3: fixed formatting of commit diff (Toke Høiland-Jørgensen)
>> include/uapi/linux/pkt_sched.h |  2 ++
>> net/sched/sch_netem.c          | 24 +++++++++++++++++++++---
>> 2 files changed, 23 insertions(+), 3 deletions(-)
>> 
>> diff --git a/include/uapi/linux/pkt_sched.h b/include/uapi/linux/pkt_sched.h
>> index 79a699f106b1..1ba49f141dae 100644
>> --- a/include/uapi/linux/pkt_sched.h
>> +++ b/include/uapi/linux/pkt_sched.h
>> @@ -603,6 +603,7 @@ enum {
>>    TCA_NETEM_JITTER64,
>>    TCA_NETEM_SLOT,
>>    TCA_NETEM_SLOT_DIST,
>> +        TCA_NETEM_BURSTING,
>>    __TCA_NETEM_MAX,
>> };
>> 
>> @@ -615,6 +616,7 @@ struct tc_netem_qopt {
>>    __u32    gap;        /* re-ordering gap (0 for none) */
>>    __u32   duplicate;    /* random packet dup  (0=none ~0=100%) */
>>    __u32    jitter;        /* random jitter in latency (us) */
>> +    __u32    bursting;    /* send packets in bursts (us) */
>> };
>> 
>> struct tc_netem_corr {
>> diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
>> index 0c345e43a09a..52d796287b86 100644
>> --- a/net/sched/sch_netem.c
>> +++ b/net/sched/sch_netem.c
>> @@ -85,6 +85,7 @@ struct netem_sched_data {
>>    s64 latency;
>>    s64 jitter;
>> 
>> +    u32 bursting;
>>    u32 loss;
>>    u32 ecn;
>>    u32 limit;
>> @@ -467,7 +468,7 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>>    /* If a delay is expected, orphan the skb. (orphaning usually takes
>>     * place at TX completion time, so _before_ the link transit delay)
>>     */
>> -    if (q->latency || q->jitter || q->rate)
>> +    if (q->latency || q->jitter || q->rate || q->bursting)
>>        skb_orphan_partial(skb);
>> 
>>    /*
>> @@ -527,8 +528,17 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>>    qdisc_qstats_backlog_inc(sch, skb);
>> 
>>    cb = netem_skb_cb(skb);
>> -    if (q->gap == 0 ||        /* not doing reordering */
>> -        q->counter < q->gap - 1 ||    /* inside last reordering gap */
>> +    if (q->bursting > 0) {
>> +        u64 now;
>> +
>> +        now = ktime_get_ns();
>> +
>> +        cb->time_to_send = now - (now % q->bursting) + q->bursting;
> 
> This wont compile on 32bit arches.
> 
>> +
>> +        ++q->counter;
>> +        tfifo_enqueue(skb, sch);
>> +    } else if (q->gap == 0 ||        /* not doing reordering */
>> +        q->counter < q->gap - 1 ||        /* inside last reordering gap */
>>        q->reorder < get_crandom(&q->reorder_cor)) {
>>        u64 now;
>>        s64 delay;
>> @@ -927,6 +937,7 @@ static const struct nla_policy netem_policy[TCA_NETEM_MAX + 1] = {
>>    [TCA_NETEM_ECN]        = { .type = NLA_U32 },
>>    [TCA_NETEM_RATE64]    = { .type = NLA_U64 },
>>    [TCA_NETEM_LATENCY64]    = { .type = NLA_S64 },
>> +    [TCA_NETEM_BURSTING]    = { .type = NLA_U64 },
>>    [TCA_NETEM_JITTER64]    = { .type = NLA_S64 },
>>    [TCA_NETEM_SLOT]    = { .len = sizeof(struct tc_netem_slot) },
>> };
>> @@ -1001,6 +1012,7 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt,
>> 
>>    q->latency = PSCHED_TICKS2NS(qopt->latency);
>>    q->jitter = PSCHED_TICKS2NS(qopt->jitter);
>> +    q->bursting = PSCHED_TICKS2NS(qopt->bursting);
> 
> This is a bit silly to use 64bit user<>kernel interface
> but still use the old legacy PSCHED_TICKS2NS conversion,
> since this will force a big granularity.
> 
> If someone want 10 usec bursts, we should allow it.
> 
> I would simply use a 32bit value, in ns unit.
> 
> (bursts of more than 2^32 ns make no sense)
> 
>>    q->limit = qopt->limit;
>>    q->gap = qopt->gap;
>>    q->counter = 0;
>> @@ -1032,6 +1044,9 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt,
>>    if (tb[TCA_NETEM_LATENCY64])
>>        q->latency = nla_get_s64(tb[TCA_NETEM_LATENCY64]);
>> 
>> +    if (tb[TCA_NETEM_BURSTING])
>> +        q->bursting = nla_get_u64(tb[TCA_NETEM_BURSTING]);
>> +
>>    if (tb[TCA_NETEM_JITTER64])
>>        q->jitter = nla_get_s64(tb[TCA_NETEM_JITTER64]);
>> 
>> @@ -1150,6 +1165,9 @@ static int netem_dump(struct Qdisc *sch, struct sk_buff *skb)
>>                 UINT_MAX);
>>    qopt.jitter = min_t(psched_tdiff_t, PSCHED_NS2TICKS(q->jitter),
>>                UINT_MAX);
>> +    qopt.bursting = min_t(psched_tdiff_t, PSCHED_NS2TICKS(q->bursting),
>> +                UINT_MAX);
>> +
>>    qopt.limit = q->limit;
>>    qopt.loss = q->loss;
>>    qopt.gap = q->gap;
>> --
>> 2.25.1
>> 

Re: [PATCH v2] net: sched: Add support for packet bursting.

[Toke Høiland-Jørgensen]

Niclas Hedam <nhed@itu.dk> writes:

> Thanks for the valuable thoughts, Toke.
>
> The patch started with me being tasked to try and mitigate timing
> attacks caused by network latencies. I scouted over the current
> network stack and didn't find anything that fully matched my use-case.
> While I now understand that you can actually leverage the slots
> functionality for this, I would still opt for a new interface and
> implementation.

So what is the actual use case for this? If this is something you're
actually planning to deploy this in production, I'm not sure netem is
the best place for it...

> I have not done any CPU benchmarks on the slots system, so I'm not
> approaching this from the practical performance side per se.
>
> Instead, I argue for seperation with reference to the Seperation of
> Concern design principle. The slots functionality is not
> built/designed to cater security guarantees, and my patch is not built
> to cater duty cycles, etc.

Separation of concerns is all well and good, but you're still adding
this to an existing qdisc (and one mostly used for emulating networks,
at that), in a way that will silently disable most of the other
functionality. I.e., if the 'bursting' field is set, 'rate' and
'latency' will just silently stop working because you're skipping the
code path that uses those.

So you'll need to reject invalid combinations at configure time, which
AFAICT is any other feature combined with bursting. Also, please add an
unlikely() around the check for the bursting parameter to hint the
compiler that this is not the most commonly used feature.

> If we opt to merge these two functionalities or discard mine, we have
> to implement some guarantee that the slots functionality won't become
> significantly slower or complex, which in my opinion is less
> maintainable than two similar systems. Also, this patch is very
> limited in lines of code, so maintaining it is pretty trivial.

Maintenance is not just about lines of code, it is also about
combination of features (e.g., dealing with things like "my rate limiter
stopped working after I turned on this 'security' feature"). At the very
least you'll need to clearly document interactions (and refuse invalid
combinations as mentioned above).

-Toke

Re: [PATCH v2] net: sched: Add support for packet bursting.

[Dave Taht]

Thx for bringing me in. I don't really have an opinion as to the value
of this patchset, but I do have an opinion on the slot functionality.

The slot functionality was my first attempt at properly emulating wifi
behaviors in netem, and although it does capture how aggregation
behaves in 802.11n, which was VERY important - it falls down miserably
on later standards and on more than one station being present due to
the half duplex nature of wifi, and the ingress and egress queues
being tightly coupled.

Used extremely carefully (along with the packet trace models also now
in netem), you *can* get closer to an emulation of how one or two wifi
stations actually behave in normal and tightly coupled systems with
tcp-friendly protocols, but I often wish I'd *never* released the code
as any result you get from it for tcp behaviors over denser wifi
networks is extremely misleading. (although, still better than
anything else out there in terms of emulating aggregation properly!).

Us not having been (since) able to gain access to low level firmwares
for wifi 6 (though the openwifi project is promising), has made it
really difficult to actually implement the real fixes for wifi (and
starlink) that we have as an outgrowth of the make-wifi-fast project.
In particular, merely getting a "txop is nearly done" interrupt would
make a huge difference if only we could find a chipset to leverage.

We are still forced to construct a two txop standing queue which can
really hurt wifi
performance on every chipset we have tried to improve. Dang it.

To quote from the codel paper:

"The standing queue, resulting from a mismatch between the window and
pipe size, is the essence of bufferbloat. It creates large delays but
no improvement in throughput. It is not a phenomenon treated by
queuing or traffic theory, which, unfortunately, results in it being
almost universally misclassified as congestion (a completely different
and much rarer pathology). These theories usually assume Poisson
arrival processes, which are, by definition, uncorrelated. The
arrivals of a closed-loop, reliable transport process such as TCP are
completely correlated, resulting in an arrival and departure rate
equality that theorists have dismissed as unnatural and wildly
improbable. Since normal cures for congestion such as usage limits or
usage-based billing have no effect on bufferbloat but annoy customers
and discourage network use, addressing the real problem would be
prudent."  - https://queue.acm.org/detail.cfm?id=2209336







On Mon, Jun 28, 2021 at 6:24 AM Niclas Hedam <nhed@itu.dk> wrote:
>
> Thanks for the valuable thoughts, Toke.
>
> The patch started with me being tasked to try and mitigate timing attacks caused by network latencies.
> I scouted over the current network stack and didn't find anything that fully matched my use-case.
> While I now understand that you can actually leverage the slots functionality for this, I would still opt for a new interface and implementation.
>
> I have not done any CPU benchmarks on the slots system, so I'm not approaching this from the practical performance side per se.
> Instead, I argue for seperation with reference to the Seperation of Concern design principle. The slots functionality is not built/designed to cater security guarantees, and my patch is not built to cater duty cycles, etc.
> If we opt to merge these two functionalities or discard mine, we have to implement some guarantee that the slots functionality won't become significantly slower or complex, which in my opinion is less maintainable than two similar systems. Also, this patch is very limited in lines of code, so maintaining it is pretty trivial.
>
> I do agree, however, that we should define what would happen if you enable both systems at the same time.
>
> @Dave: Any thoughts on this?
>
> > On 28 Jun 2021, at 14:21, Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> >
> > Niclas Hedam <nhed@itu.dk> writes:
> >
> >>>> From 71843907bdb9cdc4e24358f0c16a8778f2762dc7 Mon Sep 17 00:00:00 2001
> >>>> From: Niclas Hedam <nhed@itu.dk>
> >>>> Date: Fri, 25 Jun 2021 13:37:18 +0200
> >>>> Subject: [PATCH] net: sched: Add support for packet bursting.
> >>>
> >>> Something went wrong with the formatting here.
> >>
> >> I'll resubmit with fixed formatting. My bad.
> >>
> >>>>
> >>>> This commit implements packet bursting in the NetEm scheduler.
> >>>> This allows system administrators to hold back outgoing
> >>>> packets and release them at a multiple of a time quantum.
> >>>> This feature can be used to prevent timing attacks caused
> >>>> by network latency.
> >>>
> >>> How is this bursting feature different from the existing slot-based
> >>> mechanism?
> >>
> >> It is similar, but the reason for separating it is the audience that they are catering.
> >> The slots seems to be focused on networking constraints and duty cycles.
> >> My contribution and mechanism is mitigating timing attacks. The
> >> complexity of slots are mostly unwanted in this context as we want as
> >> few CPU cycles as possible.
> >
> > (Adding Dave who wrote the slots code)
> >
> > But you're still duplicating functionality, then? This has a cost in
> > terms of maintainability and interactions (what happens if someone turns
> > on both slots and bursting, for instance)?
> >
> > If the concern is CPU cost (got benchmarks to back that up?), why not
> > improve the existing mechanism so it can be used for your use case as
> > well?
> >
> > -Toke
>


--
Latest Podcast:
https://www.linkedin.com/feed/update/urn:li:activity:6791014284936785920/

Dave Täht CTO, TekLibre, LLC