Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] Preexisting memory leak of VAR_REF hist_field objects when a variable is referenced 3 or more times. - [High] Preexisting memory leak of VAR_REF hist_field in create_var_ref error path. -- commit 899dd39af79bb308de7335dd48b6656e713c7a62 Author: Donggeun Yoo <[email protected]> tracing: hist: free the field rejected for a bad modifier Writing a hist trigger whose value or variable carries a modifier that is not allowed there leaks the fields that were built for it. This patch fixes the leak by using destroy_hist_field() to free the rejected fields when a bad modifier is encountered. > diff --git a/kernel/trace/trace_events_hist.c > b/kernel/trace/trace_events_hist.c > index 893bd8b0e48a8..6b5572ef2d4ba 100644 > --- a/kernel/trace/trace_events_hist.c > +++ b/kernel/trace/trace_events_hist.c > @@ -4331,6 +4331,7 @@ static int __create_val_field(struct hist_trigger_data > *hist_data, > return ret; > err: > hist_err(file->tr, HIST_ERR_BAD_FIELD_MODIFIER, errpos(field_str)); > + destroy_hist_field(hist_field, 0); > return -EINVAL; > } [Severity: High] This is a pre-existing issue, and was not introduced by the patch under review, but since the commit message mentions that destroy_hist_field() returns early for HIST_FIELD_FL_VAR_REF, does this behavior cause a memory leak in the error path of create_var_ref()? Looking at create_var_ref() in kernel/trace/trace_events_hist.c, if init_var_ref() fails: if (init_var_ref(ref_field, var_field, system, event_name)) { destroy_hist_field(ref_field, 0); return NULL; } destroy_hist_field() explicitly skips freeing fields with the HIST_FIELD_FL_VAR_REF flag: if (hist_field->flags & HIST_FIELD_FL_VAR_REF) return; /* var refs will be destroyed separately */ Since the ref_field failed initialization and hasn't been added to the global hist_data->var_refs array yet, will this object be permanently leaked rather than freed during global trigger teardown? [Severity: High] This is also a pre-existing issue, and was not introduced by the patch under review, but is there a permanent memory leak of VAR_REF hist_field objects when a variable is referenced 3 or more times? For example, when a trigger is created referencing the same variable multiple times, or fails during parsing after reaching 3 references: hist:keys=pid:val1=$x:val2=$x:val3=$x destroy_hist_fields() calls __destroy_hist_field() exactly once per unique variable in hist_data->var_refs: __destroy_hist_field(hist_data->var_refs[i]); hist_data->var_refs[i] = NULL; However, in __destroy_hist_field(), there is a refcount check: if (--hist_field->ref > 1) return; If a variable is used 3 or more times, its initial refcount would be at least 3. When __destroy_hist_field() decrements it by one, the refcount remains 2 or higher, causing an early return without freeing the memory. Since destroy_hist_fields() sets the pointer to NULL immediately after, could this permanently leak the VAR_REF object because no other code drops references for these objects? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
