Hi Joel,
On Wed, Jun 30, 2021 at 06:29:55PM -0400, Joel Fernandes wrote:
On Tue, Jun 29, 2021 at 8:34 PM Paul Burton paulburton@google.com wrote:
The tgid_map array records a mapping from pid to tgid, where the index of an entry within the array is the pid & the value stored at that index is the tgid.
The saved_tgids_next() function iterates over pointers into the tgid_map array & dereferences the pointers which results in the tgid, but then it passes that dereferenced value to trace_find_tgid() which treats it as a pid & does a further lookup within the tgid_map array. It seems likely that the intent here was to skip over entries in tgid_map for which the recorded tgid is zero, but instead we end up skipping over entries for which the thread group leader hasn't yet had its own tgid recorded in tgid_map.
A minimal fix would be to remove the call to trace_find_tgid, turning:
if (trace_find_tgid(*ptr))
into:
if (*ptr)
..but it seems like this logic can be much simpler if we simply let seq_read() iterate over the whole tgid_map array & filter out empty entries by returning SEQ_SKIP from saved_tgids_show(). Here we take that approach, removing the incorrect logic here entirely.
Looks reasonable except for one nit:
Signed-off-by: Paul Burton paulburton@google.com Fixes: d914ba37d714 ("tracing: Add support for recording tgid of tasks") Cc: Steven Rostedt rostedt@goodmis.org Cc: Ingo Molnar mingo@redhat.com Cc: Joel Fernandes joelaf@google.com Cc: stable@vger.kernel.org
kernel/trace/trace.c | 38 +++++++++++++------------------------- 1 file changed, 13 insertions(+), 25 deletions(-)
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c index d23a09d3eb37b..9570667310bcc 100644 --- a/kernel/trace/trace.c +++ b/kernel/trace/trace.c @@ -5608,37 +5608,20 @@ static const struct file_operations tracing_readme_fops = {
static void *saved_tgids_next(struct seq_file *m, void *v, loff_t *pos) {
int *ptr = v;
int pid = ++(*pos);
if (*pos || m->count)
ptr++;
(*pos)++;
for (; ptr <= &tgid_map[PID_MAX_DEFAULT]; ptr++) {
if (trace_find_tgid(*ptr))
return ptr;
It would be great if you can add back the check for !tgid_map to both next() and show() as well, for added robustness (since the old code previously did it).
That condition cannot happen, because both next() & show() are called to iterate through the content of the seq_file & by definition their v argument is non-NULL (else seq_file would have finished iterating already). That argument came from either start() or an earlier call to next(), which would only have returned a non-NULL pointer into tgid_map if tgid_map is non-NULL.
I've added comments to this effect in v2, though the second patch in v2 does wind up effectively adding back the check in next() anyway in order to reuse some code.
I was tempted to just add the redundant checks anyway (pick your battles and all) but for show() in particular it wound up making things seem non-sensical to me ("display the value describing this non-NULL pointer into tgid_map only if tgid_map is not NULL?").
Thanks, Paul
With that change: Reviewed-by: Joel Fernandes (Google) joel@joelfernandes.org
thanks,
-Joel
}
if (pid > PID_MAX_DEFAULT)
return NULL;
return NULL;
return &tgid_map[pid];
}
static void *saved_tgids_start(struct seq_file *m, loff_t *pos) {
void *v;
loff_t l = 0;
if (!tgid_map)
if (!tgid_map || *pos > PID_MAX_DEFAULT) return NULL;
v = &tgid_map[0];
while (l <= *pos) {
v = saved_tgids_next(m, v, &l);
if (!v)
return NULL;
}
return v;
return &tgid_map[*pos];
}
static void saved_tgids_stop(struct seq_file *m, void *v) @@ -5647,9 +5630,14 @@ static void saved_tgids_stop(struct seq_file *m, void *v)
static int saved_tgids_show(struct seq_file *m, void *v) {
int pid = (int *)v - tgid_map;
int *entry = (int *)v;
int pid = entry - tgid_map;
int tgid = *entry;
if (tgid == 0)
return SEQ_SKIP;
seq_printf(m, "%d %d\n", pid, trace_find_tgid(pid));
seq_printf(m, "%d %d\n", pid, tgid); return 0;
}
base-commit: 62fb9874f5da54fdb243003b386128037319b219
2.32.0.93.g670b81a890-goog