Re: [PATCH] tracing/hist: bound expression string construction

[Steven Rostedt]

On Tue, 7 Apr 2026 14:09:10 +0800
Pengpeng Hou <pengpeng@xxxxxxxxxxx> wrote:

> expr_str() allocates a fixed MAX_FILTER_STR_VAL buffer and then builds
> expression names with a series of raw strcat() appends. Nested operands,
> constants and field flags can push the rendered string past that fixed
> limit before the name is attached to the hist field.
> 
> Convert the construction helpers to explicit bounded appends and
> propagate failures back to the expression parser when the rendered name
> would exceed MAX_FILTER_STR_VAL.
> 
> Signed-off-by: Pengpeng Hou <pengpeng@xxxxxxxxxxx>
> ---
>  kernel/trace/trace_events_hist.c | 101 +++++++++++++++++++++++--------
>  1 file changed, 76 insertions(+), 25 deletions(-)
> 
> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index 73ea180cad55..caaa262360d2 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -1738,85 +1738,121 @@ static const char *get_hist_field_flags(struct hist_field *hist_field)
>  	return flags_str;
>  }
>  
> -static void expr_field_str(struct hist_field *field, char *expr)
> +static bool expr_append(char *expr, size_t *len, const char *str)
>  {
> -	if (field->flags & HIST_FIELD_FL_VAR_REF)
> -		strcat(expr, "$");
> -	else if (field->flags & HIST_FIELD_FL_CONST) {
> +	size_t str_len = strlen(str);
> +
> +	if (*len + str_len >= MAX_FILTER_STR_VAL)
> +		return false;
> +
> +	memcpy(expr + *len, str, str_len + 1);
> +	*len += str_len;
> +	return true;
> +}
> +

This looks like a better job for seq_buf.


> +static bool expr_field_str(struct hist_field *field, char *expr, size_t *len)
> +{

	struct seq_buf s;

	seq_buf_init(&s, expr, MAX_FILTER_STR_VAL);

> +	if (field->flags & HIST_FIELD_FL_VAR_REF) {
> +		if (!expr_append(expr, len, "$"))
> +			return false;

		seq_buf_putc(&s, '$');

> +	} else if (field->flags & HIST_FIELD_FL_CONST) {
>  		char str[HIST_CONST_DIGITS_MAX];
> +		int ret;
> +
> +		ret = snprintf(str, sizeof(str), "%llu", field->constant);
> +		if (ret >= sizeof(str))
> +			return false;
>  
> -		snprintf(str, HIST_CONST_DIGITS_MAX, "%llu", field->constant);
> -		strcat(expr, str);

		seq_buf_printf(&s, "%llu", field->constant);

> +		if (!expr_append(expr, len, str))
> +			return false;
>  	}
>  
> -	strcat(expr, hist_field_name(field, 0));

	seq_buf_puts(&s, hist_field_name(field, 0));

> +	if (!expr_append(expr, len, hist_field_name(field, 0)))
> +		return false;
>  
>  	if (field->flags && !(field->flags & HIST_FIELD_FL_VAR_REF)) {
>  		const char *flags_str = get_hist_field_flags(field);
>  
>  		if (flags_str) {
> -			strcat(expr, ".");
> -			strcat(expr, flags_str);

			seq_buf_printf(&s, ".%s", flags_str);

> +			if (!expr_append(expr, len, ".") ||
> +			    !expr_append(expr, len, flags_str))
> +				return false;
>  		}
>  	}

	/* Add terminating character */
	seq_buf_str(&s);

	return seq_buf_overflow(&s) ? false : true;

> +
> +	return true;
>  }
>  
>  static char *expr_str(struct hist_field *field, unsigned int level)
>  {
>  	char *expr;
> +	size_t len = 0;
>  

This could all be converted too.

-- Steve

>  	if (level > 1)
> -		return NULL;
> +		return ERR_PTR(-EINVAL);
>  
>  	expr = kzalloc(MAX_FILTER_STR_VAL, GFP_KERNEL);
>  	if (!expr)
> -		return NULL;
> +		return ERR_PTR(-ENOMEM);
>  
>  	if (!field->operands[0]) {
> -		expr_field_str(field, expr);
> +		if (!expr_field_str(field, expr, &len))
> +			goto free;
>  		return expr;
>  	}
>  
>  	if (field->operator == FIELD_OP_UNARY_MINUS) {
>  		char *subexpr;
>  
> -		strcat(expr, "-(");
> +		if (!expr_append(expr, &len, "-("))
> +			goto free;
>  		subexpr = expr_str(field->operands[0], ++level);
>  		if (!subexpr) {
> -			kfree(expr);
> -			return NULL;
> +			goto free;
> +		}
> +		if (!expr_append(expr, &len, subexpr) ||
> +		    !expr_append(expr, &len, ")")) {
> +			kfree(subexpr);
> +			goto free;
>  		}
> -		strcat(expr, subexpr);
> -		strcat(expr, ")");
>  
>  		kfree(subexpr);
>  
>  		return expr;
>  	}
>  
> -	expr_field_str(field->operands[0], expr);
> +	if (!expr_field_str(field->operands[0], expr, &len))
> +		goto free;
>  
>  	switch (field->operator) {
>  	case FIELD_OP_MINUS:
> -		strcat(expr, "-");
> +		if (!expr_append(expr, &len, "-"))
> +			goto free;
>  		break;
>  	case FIELD_OP_PLUS:
> -		strcat(expr, "+");
> +		if (!expr_append(expr, &len, "+"))
> +			goto free;
>  		break;
>  	case FIELD_OP_DIV:
> -		strcat(expr, "/");
> +		if (!expr_append(expr, &len, "/"))
> +			goto free;
>  		break;
>  	case FIELD_OP_MULT:
> -		strcat(expr, "*");
> +		if (!expr_append(expr, &len, "*"))
> +			goto free;
>  		break;
>  	default:
> -		kfree(expr);
> -		return NULL;
> +		goto free;
>  	}
>  
> -	expr_field_str(field->operands[1], expr);
> +	if (!expr_field_str(field->operands[1], expr, &len))
> +		goto free;
>  
>  	return expr;
> +
> +free:
> +	kfree(expr);
> +	return ERR_PTR(-E2BIG);
>  }
>  
>  /*
> @@ -2630,6 +2666,11 @@ static struct hist_field *parse_unary(struct hist_trigger_data *hist_data,
>  	expr->is_signed = operand1->is_signed;
>  	expr->operator = FIELD_OP_UNARY_MINUS;
>  	expr->name = expr_str(expr, 0);
> +	if (IS_ERR(expr->name)) {
> +		ret = PTR_ERR(expr->name);
> +		expr->name = NULL;
> +		goto free;
> +	}
>  	expr->type = kstrdup_const(operand1->type, GFP_KERNEL);
>  	if (!expr->type) {
>  		ret = -ENOMEM;
> @@ -2842,6 +2883,11 @@ static struct hist_field *parse_expr(struct hist_trigger_data *hist_data,
>  		destroy_hist_field(operand1, 0);
>  
>  		expr->name = expr_str(expr, 0);
> +		if (IS_ERR(expr->name)) {
> +			ret = PTR_ERR(expr->name);
> +			expr->name = NULL;
> +			goto free_expr;
> +		}
>  	} else {
>  		/* The operand sizes should be the same, so just pick one */
>  		expr->size = operand1->size;
> @@ -2855,6 +2901,11 @@ static struct hist_field *parse_expr(struct hist_trigger_data *hist_data,
>  		}
>  
>  		expr->name = expr_str(expr, 0);
> +		if (IS_ERR(expr->name)) {
> +			ret = PTR_ERR(expr->name);
> +			expr->name = NULL;
> +			goto free_expr;
> +		}
>  	}
>  
>  	return expr;