| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAJWu+ooqOTem6yKFOYkC7xn3WZiFMHA1hkvxtV6=R3bN-fjF6g@mail.gmail.com>
Date: Mon, 26 Jun 2017 12:02:12 -0700
From: Joel Fernandes <joelaf@...gle.com>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: LKML <linux-kernel@...r.kernel.org>,
Michael Sartain <mikesart@...il.com>, kernel-team@...roid.com,
Ingo Molnar <mingo@...hat.com>
Subject: Re: [PATCH v4 1/3] tracing: Add support for recording tgid of tasks
On Mon, Jun 26, 2017 at 9:52 AM, Steven Rostedt <rostedt@...dmis.org> wrote:
> On Sun, 25 Jun 2017 22:38:42 -0700
> Joel Fernandes <joelaf@...gle.com> wrote:
>
>> +int *tgid_map;
>> +
>> +void tracing_alloc_tgid_map(void)
>> +{
>> + if (tgid_map)
>> + return;
>> +
>> + tgid_map = kzalloc((PID_MAX_DEFAULT + 1) * sizeof(*tgid_map),
>> + GFP_KERNEL);
>> + WARN_ONCE(!tgid_map, "Allocation of tgid_map failed\n");
>
> This needs to return a value. If it fails to allocate, then we must not
> set the bit to do the recording. More below about why.
>
>> +}
>> +
>> #define SAVED_CMDLINES_DEFAULT 128
>> #define NO_CMDLINE_MAP UINT_MAX
>> static arch_spinlock_t trace_cmdline_lock = __ARCH_SPIN_LOCK_UNLOCKED;
>> @@ -1722,7 +1734,7 @@ struct saved_cmdlines_buffer {
>> static struct saved_cmdlines_buffer *savedcmd;
>>
>> /* temporary disable recording */
>> -static atomic_t trace_record_cmdline_disabled __read_mostly;
>> +static atomic_t trace_record_taskinfo_disabled __read_mostly;
>>
>> static inline char *get_saved_cmdlines(int idx)
>> {
>> @@ -1992,16 +2004,87 @@ void trace_find_cmdline(int pid, char comm[])
>> preempt_enable();
>> }
>>
>> -void tracing_record_cmdline(struct task_struct *tsk)
>> +int trace_find_tgid(int pid)
>> +{
>> + if (unlikely(!tgid_map || !pid || pid > PID_MAX_DEFAULT))
>> + return 0;
>> +
>> + return tgid_map[pid];
>> +}
>> +
>> +static int trace_save_tgid(struct task_struct *tsk)
>> +{
>> + if (unlikely(!tgid_map || !tsk->pid || tsk->pid > PID_MAX_DEFAULT))
>> + return 0;
>
> If we failed to allocate, this will always return 0.
>
>> +
>> + tgid_map[tsk->pid] = tsk->tgid;
>> + return 1;
>> +}
>> +
>> +static bool tracing_record_taskinfo_skip(int flags)
>> +{
>> + if (unlikely(!(flags & (TRACE_RECORD_CMDLINE | TRACE_RECORD_TGID))))
>> + return true;
>> + if (atomic_read(&trace_record_taskinfo_disabled) || !tracing_is_on())
>> + return true;
>> + if (!__this_cpu_read(trace_taskinfo_save))
>> + return true;
>> + return false;
>> +}
>> +
>> +/**
>> + * tracing_record_taskinfo - record the task info of a task
>> + *
>> + * @task - task to record
>> + * @flags - TRACE_RECORD_CMDLINE for recording comm
>> + * - TRACE_RECORD_TGID for recording tgid
>> + */
>> +void tracing_record_taskinfo(struct task_struct *task, int flags)
>> +{
>> + if (tracing_record_taskinfo_skip(flags))
>> + return;
>> + if ((flags & TRACE_RECORD_CMDLINE) && !trace_save_cmdline(task))
>> + return;
>> + if ((flags & TRACE_RECORD_TGID) && !trace_save_tgid(task))
>
> With a failed allocation, this will return here.
>
>> + return;
>> +
>> + __this_cpu_write(trace_taskinfo_save, false);
>
> The above will never be cleared, and this callback will constantly be
> called, slowing down the tracer.
>
>
>> +}
>> +
>> +/**
>> + * tracing_record_taskinfo_sched_switch - record task info for sched_switch
>> + *
>> + * @prev - previous task during sched_switch
>> + * @next - next task during sched_switch
>> + * @flags - TRACE_RECORD_CMDLINE for recording comm
>> + * TRACE_RECORD_TGID for recording tgid
>> + */
>> +void tracing_record_taskinfo_sched_switch(struct task_struct *prev,
>> + struct task_struct *next, int flags)
>> {
>> - if (atomic_read(&trace_record_cmdline_disabled) || !tracing_is_on())
>> + if (tracing_record_taskinfo_skip(flags))
>> + return;
>> +
>> + if ((flags & TRACE_RECORD_CMDLINE) &&
>> + (!trace_save_cmdline(prev) || !trace_save_cmdline(next)))
>> return;
>>
>> - if (!__this_cpu_read(trace_cmdline_save))
>> + if ((flags & TRACE_RECORD_TGID) &&
>> + (!trace_save_tgid(prev) || !trace_save_tgid(next)))
>
> On failed allocation, this will return here.
>
>> return;
>>
>> - if (trace_save_cmdline(tsk))
>> - __this_cpu_write(trace_cmdline_save, false);
>> + __this_cpu_write(trace_taskinfo_save, false);
>
> This will not be cleared.
>
>> +}
>> +
>> +/* Helpers to record a specific task information */
>> +void tracing_record_cmdline(struct task_struct *task)
>> +{
>> + tracing_record_taskinfo(task, TRACE_RECORD_CMDLINE);
>> +}
>> +
>> +void tracing_record_tgid(struct task_struct *task)
>> +{
>> + tracing_record_taskinfo(task, TRACE_RECORD_TGID);
>> }
>>
>> /*
>> @@ -3146,7 +3229,7 @@ static void *s_start(struct seq_file *m, loff_t *pos)
>> #endif
>>
>> if (!iter->snapshot)
>> - atomic_inc(&trace_record_cmdline_disabled);
>> + atomic_inc(&trace_record_taskinfo_disabled);
>>
>> if (*pos != iter->pos) {
>> iter->ent = NULL;
>> @@ -3191,7 +3274,7 @@ static void s_stop(struct seq_file *m, void *p)
>> #endif
>>
>> if (!iter->snapshot)
>> - atomic_dec(&trace_record_cmdline_disabled);
>> + atomic_dec(&trace_record_taskinfo_disabled);
>>
>> trace_access_unlock(iter->cpu_file);
>> trace_event_read_unlock();
>> @@ -4238,6 +4321,9 @@ int set_tracer_flag(struct trace_array *tr, unsigned int mask, int enabled)
>> if (mask == TRACE_ITER_RECORD_CMD)
>> trace_event_enable_cmd_record(enabled);
>>
>> + if (mask == TRACE_ITER_RECORD_TGID)
>> + trace_event_enable_tgid_record(enabled);
>> +
>> if (mask == TRACE_ITER_EVENT_FORK)
>> trace_event_follow_fork(tr, enabled);
>>
>> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
>> index 39fd77330aab..90318b016fd9 100644
>> --- a/kernel/trace/trace.h
>> +++ b/kernel/trace/trace.h
>> @@ -635,8 +635,13 @@ void trace_graph_return(struct ftrace_graph_ret *trace);
>> int trace_graph_entry(struct ftrace_graph_ent *trace);
>> void set_graph_array(struct trace_array *tr);
>>
>> +extern int *tgid_map;
>> +void tracing_alloc_tgid_map(void);
>> void tracing_start_cmdline_record(void);
>> void tracing_stop_cmdline_record(void);
>> +void tracing_start_tgid_record(void);
>> +void tracing_stop_tgid_record(void);
>> +
>> int register_tracer(struct tracer *type);
>> int is_tracing_stopped(void);
>>
>> @@ -697,6 +702,7 @@ static inline void __trace_stack(struct trace_array *tr, unsigned long flags,
>> extern u64 ftrace_now(int cpu);
>>
>> extern void trace_find_cmdline(int pid, char comm[]);
>> +extern int trace_find_tgid(int pid);
>> extern void trace_event_follow_fork(struct trace_array *tr, bool enable);
>>
>> #ifdef CONFIG_DYNAMIC_FTRACE
>> @@ -1107,6 +1113,7 @@ extern int trace_get_user(struct trace_parser *parser, const char __user *ubuf,
>> C(CONTEXT_INFO, "context-info"), /* Print pid/cpu/time */ \
>> C(LATENCY_FMT, "latency-format"), \
>> C(RECORD_CMD, "record-cmd"), \
>> + C(RECORD_TGID, "record-tgid"), \
>> C(OVERWRITE, "overwrite"), \
>> C(STOP_ON_FREE, "disable_on_free"), \
>> C(IRQ_INFO, "irq-info"), \
>> @@ -1423,6 +1430,8 @@ struct ftrace_event_field *
>> trace_find_event_field(struct trace_event_call *call, char *name);
>>
>> extern void trace_event_enable_cmd_record(bool enable);
>> +extern void trace_event_enable_tgid_record(bool enable);
>> +
>> extern int event_trace_add_tracer(struct dentry *parent, struct trace_array *tr);
>> extern int event_trace_del_tracer(struct trace_array *tr);
>>
>> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
>> index e7973e10398c..240c6df95ea6 100644
>> --- a/kernel/trace/trace_events.c
>> +++ b/kernel/trace/trace_events.c
>> @@ -343,6 +343,28 @@ void trace_event_enable_cmd_record(bool enable)
>> mutex_unlock(&event_mutex);
>> }
>>
>> +void trace_event_enable_tgid_record(bool enable)
>
> This should return a value.
>
>> +{
>> + struct trace_event_file *file;
>> + struct trace_array *tr;
>> +
>> + mutex_lock(&event_mutex);
>> + do_for_each_event_file(tr, file) {
>> + if (!(file->flags & EVENT_FILE_FL_ENABLED))
>> + continue;
>> +
>> + if (enable) {
>> + tracing_start_tgid_record();
>
> If we fail to start, the bit should not be set, and we should return
> failure. Note, it can only fail on the first try, as once it is
> allocated, you don't need to worry about it failing. Thus, if it fails,
> break out of the loop now and return failure.
>
That seems Ok with me to do, I think a valid point.
I think that I should do it in the second call to
tracing_start_tgid_record too then (__ftrace_event_enable_disable) to
error out if the allocation fails.
While going this code I again, I noticed another potential issue in
__ftrace_event_enable_disable
Powered by blists - more mailing lists