[PATCH] net/sched: hhf: clear heavy-hitter state on reset

[PATCH] net/sched: hhf: clear heavy-hitter state on reset

[Samuel Moelius]

HHF reset does not clear the classifier state used to identify heavy
hitters.  Packets after reset can therefore be scheduled using flow
history from before the reset.

The reset operation should return the qdisc to an empty state.

Clear the heavy-hitter classifier tables when HHF is reset.

Assisted-by: Codex:gpt-5.5-cyber-preview
Signed-off-by: Samuel Moelius <sam.moelius@trailofbits.com>
---
 net/sched/sch_hhf.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/net/sched/sch_hhf.c b/net/sched/sch_hhf.c
index 96021f52d835..1698c1b54dec 100644
--- a/net/sched/sch_hhf.c
+++ b/net/sched/sch_hhf.c
@@ -461,12 +461,34 @@ static struct sk_buff *hhf_dequeue(struct Qdisc *sch)
 	return skb;
 }
 
+static void hhf_reset_classifier(struct hhf_sched_data *q)
+{
+	int i;
+
+	for (i = 0; i < HH_FLOWS_CNT; i++) {
+		struct hh_flow_state *flow, *next;
+		struct list_head *head = &q->hh_flows[i];
+
+		list_for_each_entry_safe(flow, next, head, flowchain) {
+			list_del(&flow->flowchain);
+			kfree(flow);
+		}
+	}
+	WRITE_ONCE(q->hh_flows_current_cnt, 0);
+
+	for (i = 0; i < HHF_ARRAYS_CNT; i++)
+		bitmap_zero(q->hhf_valid_bits[i], HHF_ARRAYS_LEN);
+	q->hhf_arrays_reset_timestamp = hhf_time_stamp();
+}
+
 static void hhf_reset(struct Qdisc *sch)
 {
+	struct hhf_sched_data *q = qdisc_priv(sch);
 	struct sk_buff *skb;
 
 	while ((skb = hhf_dequeue(sch)) != NULL)
 		rtnl_kfree_skbs(skb, skb);
+	hhf_reset_classifier(q);
 }
 
 static void hhf_destroy(struct Qdisc *sch)
-- 
2.43.0

Re: [PATCH] net/sched: hhf: clear heavy-hitter state on reset

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
net/sched: hhf: clear heavy-hitter state on reset

This commit clears the heavy-hitter classifier tables when the HHF qdisc
is reset. This prevents packets from being scheduled using flow history
from before the reset operation.

> diff --git a/net/sched/sch_hhf.c b/net/sched/sch_hhf.c
> index 96021f52d835b..1698c1b54dec8 100644
> --- a/net/sched/sch_hhf.c
> +++ b/net/sched/sch_hhf.c

[ ... ]

> @@ -461,12 +461,34 @@ static struct sk_buff *hhf_dequeue(struct Qdisc *sch)
>  	return skb;
>  }
>  
> +static void hhf_reset_classifier(struct hhf_sched_data *q)
> +{
> +	int i;
> +
> +	for (i = 0; i < HH_FLOWS_CNT; i++) {
> +		struct hh_flow_state *flow, *next;
> +		struct list_head *head = &q->hh_flows[i];
> +
> +		list_for_each_entry_safe(flow, next, head, flowchain) {

[Severity: High]
If hhf_init() fails due to a memory allocation error, q->hh_flows will be
NULL. When qdisc_create_dflt() cleans up from the error, it calls 
qdisc_put() -> __qdisc_destroy(), which unconditionally calls qdisc_reset()
and triggers hhf_reset().

Does the pointer arithmetic here yield a NULL-derived pointer that will
cause a kernel panic when list_for_each_entry_safe() dereferences 
head->next?

I noticed hhf_destroy() has a specific check (`if (!q->hh_flows)`) to avoid
this issue during cleanup. Would it make sense to add a similar check here?

> +			list_del(&flow->flowchain);
> +			kfree(flow);
> +		}
> +	}
> +	WRITE_ONCE(q->hh_flows_current_cnt, 0);
> +
> +	for (i = 0; i < HHF_ARRAYS_CNT; i++)
> +		bitmap_zero(q->hhf_valid_bits[i], HHF_ARRAYS_LEN);

[Severity: High]
Similarly, q->hhf_valid_bits[i] will also be NULL on the initialization error
path. 

Because bitmap_zero() internally uses memset(), will passing a NULL
pointer here cause an immediate crash when the cleanup path executes?

> +	q->hhf_arrays_reset_timestamp = hhf_time_stamp();
> +}
> +
>  static void hhf_reset(struct Qdisc *sch)

[ ... ]