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

[Simon Horman <horms@kernel.org>]

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)

[ ... ]