[PATCH] tracing/hist: bound expression string construction

[PATCH] tracing/hist: bound expression string construction

[Pengpeng Hou]

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@iscas.ac.cn>
---
 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;
+}
+
+static bool expr_field_str(struct hist_field *field, char *expr, size_t *len)
+{
+	if (field->flags & HIST_FIELD_FL_VAR_REF) {
+		if (!expr_append(expr, len, "$"))
+			return false;
+	} 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);
+		if (!expr_append(expr, len, str))
+			return false;
 	}
 
-	strcat(expr, 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);
+			if (!expr_append(expr, len, ".") ||
+			    !expr_append(expr, len, flags_str))
+				return false;
 		}
 	}
+
+	return true;
 }
 
 static char *expr_str(struct hist_field *field, unsigned int level)
 {
 	char *expr;
+	size_t len = 0;
 
 	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;
-- 
2.50.1 (Apple Git-155)

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

[Steven Rostedt]

On Tue, 7 Apr 2026 14:09:10 +0800
Pengpeng Hou <pengpeng@iscas.ac.cn> 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@iscas.ac.cn>
> ---
>  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;

[PATCH v2] tracing/hist: bound expression strings with seq_buf

[Pengpeng Hou]

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.

Build the expression strings with seq_buf and propagate failures back to
the expression parser when the rendered name would exceed
MAX_FILTER_STR_VAL.

Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
Changes since v1:
- replace the previous bounded append helper and manual length tracking
  with seq_buf as suggested by Steven Rostedt
- keep the -E2BIG propagation back into parse_unary() and parse_expr()

 kernel/trace/trace_events_hist.c | 93 ++++++++++++++++++++++----------
 1 file changed, 64 insertions(+), 29 deletions(-)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 73ea180cad55..09aaedb92993 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -8,6 +8,7 @@
 #include <linux/module.h>
 #include <linux/kallsyms.h>
 #include <linux/security.h>
+#include <linux/seq_buf.h>
 #include <linux/mutex.h>
 #include <linux/slab.h>
 #include <linux/stacktrace.h>
@@ -1738,85 +1739,104 @@ 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_field_str(struct hist_field *field, struct seq_buf *s)
 {
+	const char *field_name;
+
 	if (field->flags & HIST_FIELD_FL_VAR_REF)
-		strcat(expr, "$");
-	else if (field->flags & HIST_FIELD_FL_CONST) {
-		char str[HIST_CONST_DIGITS_MAX];
+		seq_buf_putc(s, '$');
+	else if (field->flags & HIST_FIELD_FL_CONST)
+		seq_buf_printf(s, "%llu", field->constant);
 
-		snprintf(str, HIST_CONST_DIGITS_MAX, "%llu", field->constant);
-		strcat(expr, str);
-	}
+	field_name = hist_field_name(field, 0);
+	if (!field_name)
+		return false;
 
-	strcat(expr, hist_field_name(field, 0));
+	seq_buf_puts(s, field_name);
 
 	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);
-		}
+		if (flags_str)
+			seq_buf_printf(s, ".%s", flags_str);
 	}
+
+	return !seq_buf_has_overflowed(s);
 }
 
 static char *expr_str(struct hist_field *field, unsigned int level)
 {
 	char *expr;
+	struct seq_buf s;
+	int ret = -E2BIG;
 
 	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);
+
+	seq_buf_init(&s, expr, MAX_FILTER_STR_VAL);
 
 	if (!field->operands[0]) {
-		expr_field_str(field, expr);
+		if (!expr_field_str(field, &s))
+			goto free;
+		seq_buf_str(&s);
 		return expr;
 	}
 
 	if (field->operator == FIELD_OP_UNARY_MINUS) {
 		char *subexpr;
 
-		strcat(expr, "-(");
+		seq_buf_puts(&s, "-(");
 		subexpr = expr_str(field->operands[0], ++level);
-		if (!subexpr) {
-			kfree(expr);
-			return NULL;
+		if (IS_ERR(subexpr)) {
+			ret = PTR_ERR(subexpr);
+			goto free;
 		}
-		strcat(expr, subexpr);
-		strcat(expr, ")");
+		seq_buf_puts(&s, subexpr);
+		seq_buf_putc(&s, ')');
 
 		kfree(subexpr);
+		if (seq_buf_has_overflowed(&s))
+			goto free;
 
+		seq_buf_str(&s);
 		return expr;
 	}
 
-	expr_field_str(field->operands[0], expr);
+	if (!expr_field_str(field->operands[0], &s))
+		goto free;
 
 	switch (field->operator) {
 	case FIELD_OP_MINUS:
-		strcat(expr, "-");
+		seq_buf_putc(&s, '-');
 		break;
 	case FIELD_OP_PLUS:
-		strcat(expr, "+");
+		seq_buf_putc(&s, '+');
 		break;
 	case FIELD_OP_DIV:
-		strcat(expr, "/");
+		seq_buf_putc(&s, '/');
 		break;
 	case FIELD_OP_MULT:
-		strcat(expr, "*");
+		seq_buf_putc(&s, '*');
 		break;
 	default:
-		kfree(expr);
-		return NULL;
+		ret = -EINVAL;
+		goto free;
 	}
 
-	expr_field_str(field->operands[1], expr);
+	if (seq_buf_has_overflowed(&s) ||
+	    !expr_field_str(field->operands[1], &s))
+		goto free;
 
+	seq_buf_str(&s);
 	return expr;
+
+free:
+	kfree(expr);
+	return ERR_PTR(ret);
 }
 
 /*
@@ -2630,6 +2650,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 +2867,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 +2885,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;
-- 
2.50.1 (Apple Git-155)

Re: [PATCH v2] tracing/hist: bound expression strings with seq_buf

[Steven Rostedt]

On Thu, 9 Apr 2026 10:56:28 +0800
Pengpeng Hou <pengpeng@iscas.ac.cn> wrote:

Hi Pengpeng,

Again, subject should be:

  tracing: Bound histogram expression strings with seq_buf

> 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.
> 
> Build the expression strings with seq_buf and propagate failures back to
> the expression parser when the rendered name would exceed
> MAX_FILTER_STR_VAL.
> 
> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> ---
> Changes since v1:
> - replace the previous bounded append helper and manual length tracking
>   with seq_buf as suggested by Steven Rostedt
> - keep the -E2BIG propagation back into parse_unary() and parse_expr()
> 
>  kernel/trace/trace_events_hist.c | 93 ++++++++++++++++++++++----------
>  1 file changed, 64 insertions(+), 29 deletions(-)
> 
> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index 73ea180cad55..09aaedb92993 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -8,6 +8,7 @@
>  #include <linux/module.h>
>  #include <linux/kallsyms.h>
>  #include <linux/security.h>
> +#include <linux/seq_buf.h>
>  #include <linux/mutex.h>
>  #include <linux/slab.h>
>  #include <linux/stacktrace.h>
> @@ -1738,85 +1739,104 @@ 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_field_str(struct hist_field *field, struct seq_buf *s)
>  {
> +	const char *field_name;
> +
>  	if (field->flags & HIST_FIELD_FL_VAR_REF)
> -		strcat(expr, "$");
> -	else if (field->flags & HIST_FIELD_FL_CONST) {
> -		char str[HIST_CONST_DIGITS_MAX];
> +		seq_buf_putc(s, '$');
> +	else if (field->flags & HIST_FIELD_FL_CONST)
> +		seq_buf_printf(s, "%llu", field->constant);
>  
> -		snprintf(str, HIST_CONST_DIGITS_MAX, "%llu", field->constant);
> -		strcat(expr, str);
> -	}
> +	field_name = hist_field_name(field, 0);
> +	if (!field_name)
> +		return false;
>  
> -	strcat(expr, hist_field_name(field, 0));
> +	seq_buf_puts(s, field_name);
>  
>  	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);
> -		}
> +		if (flags_str)
> +			seq_buf_printf(s, ".%s", flags_str);
>  	}
> +
> +	return !seq_buf_has_overflowed(s);
>  }
>  
>  static char *expr_str(struct hist_field *field, unsigned int level)
>  {
>  	char *expr;
> +	struct seq_buf s;
> +	int ret = -E2BIG;
>  
>  	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);

A patch should do one thing at a time. This patch appears to be doing
two things: fixing the bound strings, returning errors instead of NULL.

Please split this up into two patches. One that changes the return
values from NULL to ERR_PTR() and the other to use seq_buf.

Thanks,

-- Steve

Re: [PATCH v2] tracing/hist: bound expression strings with seq_buf

[Pengpeng Hou]

Hi Steve,

Thanks, I'll respin this.

I'll split it into two patches, one for the `NULL` to `ERR_PTR()`
conversion and one for the `seq_buf` conversion, and I'll use the
tracing-style subjects on the resend.

Thanks,
Pengpeng

[PATCH v3 1/2] tracing: Return ERR_PTR() from expr_str()

[Pengpeng Hou]

expr_str() already has failure cases for invalid recursion depth and
allocation failure, but it currently reports them as a bare NULL. Teach
it to return ERR_PTR()-encoded errors and update parse_unary() and
parse_expr() to propagate those errors.

This keeps the error conversion separate from the string-building change
so the follow-up seq_buf patch can stay focused on the overflow fix
itself.

Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
Changes since v2:
- split the ERR_PTR() conversion out as its own patch as requested by
  Steven Rostedt

 kernel/trace/trace_events_hist.c | 33 +++++++++++++++++-----
 1 file changed, 26 insertions(+), 7 deletions(-)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 73ea180cad55..954e0beb7f0a 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -1764,13 +1764,14 @@ static void expr_field_str(struct hist_field *field, char *expr)
 static char *expr_str(struct hist_field *field, unsigned int level)
 {
 	char *expr;
+	int ret = -EINVAL;
 
 	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);
@@ -1782,9 +1783,9 @@ static char *expr_str(struct hist_field *field, unsigned int level)
 
 		strcat(expr, "-(");
 		subexpr = expr_str(field->operands[0], ++level);
-		if (!subexpr) {
-			kfree(expr);
-			return NULL;
+		if (IS_ERR(subexpr)) {
+			ret = PTR_ERR(subexpr);
+			goto free;
 		}
 		strcat(expr, subexpr);
 		strcat(expr, ")");
@@ -1810,13 +1811,16 @@ static char *expr_str(struct hist_field *field, unsigned int level)
 		strcat(expr, "*");
 		break;
 	default:
-		kfree(expr);
-		return NULL;
+		goto free;
 	}
 
 	expr_field_str(field->operands[1], expr);
 
 	return expr;
+
+free:
+	kfree(expr);
+	return ERR_PTR(ret);
 }
 
 /*
@@ -2630,6 +2634,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 +2851,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 +2869,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;
-- 
2.50.1 (Apple Git-155)

[PATCH v3 2/2] tracing: Bound histogram expression strings with seq_buf

[Pengpeng Hou]

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.

Build the expression strings with seq_buf and return -E2BIG when the
rendered name would exceed MAX_FILTER_STR_VAL.

Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
Changes since v2:
- split the ERR_PTR() conversion into patch 1/2 as requested by Steven
  Rostedt
- keep this patch focused on the seq_buf conversion and overflow
  detection

 kernel/trace/trace_events_hist.c | 67 +++++++++++++++-------
 1 file changed, 45 insertions(+), 22 deletions(-)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 954e0beb7f0a..3a74880ac4d1 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -8,6 +8,7 @@
 #include <linux/module.h>
 #include <linux/kallsyms.h>
 #include <linux/security.h>
+#include <linux/seq_buf.h>
 #include <linux/mutex.h>
 #include <linux/slab.h>
 #include <linux/stacktrace.h>
@@ -1738,32 +1739,35 @@ 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_field_str(struct hist_field *field, struct seq_buf *s)
 {
+	const char *field_name;
+
 	if (field->flags & HIST_FIELD_FL_VAR_REF)
-		strcat(expr, "$");
-	else if (field->flags & HIST_FIELD_FL_CONST) {
-		char str[HIST_CONST_DIGITS_MAX];
+		seq_buf_putc(s, '$');
+	else if (field->flags & HIST_FIELD_FL_CONST)
+		seq_buf_printf(s, "%llu", field->constant);
 
-		snprintf(str, HIST_CONST_DIGITS_MAX, "%llu", field->constant);
-		strcat(expr, str);
-	}
+	field_name = hist_field_name(field, 0);
+	if (!field_name)
+		return false;
 
-	strcat(expr, hist_field_name(field, 0));
+	seq_buf_puts(s, field_name);
 
 	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);
-		}
+		if (flags_str)
+			seq_buf_printf(s, ".%s", flags_str);
 	}
+
+	return !seq_buf_has_overflowed(s);
 }
 
 static char *expr_str(struct hist_field *field, unsigned int level)
 {
 	char *expr;
+	struct seq_buf s;
 	int ret = -EINVAL;
 
 	if (level > 1)
@@ -1773,49 +1777,68 @@ static char *expr_str(struct hist_field *field, unsigned int level)
 	if (!expr)
 		return ERR_PTR(-ENOMEM);
 
+	seq_buf_init(&s, expr, MAX_FILTER_STR_VAL);
+
 	if (!field->operands[0]) {
-		expr_field_str(field, expr);
+		if (!expr_field_str(field, &s)) {
+			ret = -E2BIG;
+			goto free;
+		}
+		seq_buf_str(&s);
 		return expr;
 	}
 
 	if (field->operator == FIELD_OP_UNARY_MINUS) {
 		char *subexpr;
 
-		strcat(expr, "-(");
+		seq_buf_puts(&s, "-(");
 		subexpr = expr_str(field->operands[0], ++level);
 		if (IS_ERR(subexpr)) {
 			ret = PTR_ERR(subexpr);
 			goto free;
 		}
-		strcat(expr, subexpr);
-		strcat(expr, ")");
+		seq_buf_puts(&s, subexpr);
+		seq_buf_putc(&s, ')');
 
 		kfree(subexpr);
+		if (seq_buf_has_overflowed(&s)) {
+			ret = -E2BIG;
+			goto free;
+		}
 
+		seq_buf_str(&s);
 		return expr;
 	}
 
-	expr_field_str(field->operands[0], expr);
+	if (!expr_field_str(field->operands[0], &s)) {
+		ret = -E2BIG;
+		goto free;
+	}
 
 	switch (field->operator) {
 	case FIELD_OP_MINUS:
-		strcat(expr, "-");
+		seq_buf_putc(&s, '-');
 		break;
 	case FIELD_OP_PLUS:
-		strcat(expr, "+");
+		seq_buf_putc(&s, '+');
 		break;
 	case FIELD_OP_DIV:
-		strcat(expr, "/");
+		seq_buf_putc(&s, '/');
 		break;
 	case FIELD_OP_MULT:
-		strcat(expr, "*");
+		seq_buf_putc(&s, '*');
 		break;
 	default:
 		goto free;
 	}
 
-	expr_field_str(field->operands[1], expr);
+	if (seq_buf_has_overflowed(&s) ||
+	    !expr_field_str(field->operands[1], &s)) {
+		ret = -E2BIG;
+		goto free;
+	}
 
+	seq_buf_str(&s);
 	return expr;
 
 free:
	kfree(expr);
	return ERR_PTR(ret);
}
-- 
2.50.1 (Apple Git-155)