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
| ||
|
Date: Thu, 11 Nov 2021 13:10:21 +0000 From: German Gomez <german.gomez@....com> To: Leo Yan <leo.yan@...aro.org> Cc: Namhyung Kim <namhyung@...nel.org>, linux-kernel <linux-kernel@...r.kernel.org>, linux-perf-users <linux-perf-users@...r.kernel.org>, Arnaldo Carvalho de Melo <acme@...nel.org>, Mark Rutland <mark.rutland@....com>, Alexander Shishkin <alexander.shishkin@...ux.intel.com>, Jiri Olsa <jolsa@...hat.com>, John Garry <john.garry@...wei.com>, Will Deacon <will@...nel.org>, Mathieu Poirier <mathieu.poirier@...aro.org>, linux-arm-kernel@...ts.infradead.org Subject: Re: [PATCH v2 4/4] perf arm-spe: Support hardware-based PID tracing Hi, thanks for looking into it On 11/11/2021 12:42, Leo Yan wrote: > On Thu, Nov 11, 2021 at 12:23:08PM +0000, German Gomez wrote: >> On 11/11/2021 08:30, Leo Yan wrote: >>> On Wed, Nov 10, 2021 at 11:59:05PM -0800, Namhyung Kim wrote: > [...] > >>>>>>> +static int arm_spe_set_tid(struct arm_spe_queue *speq, pid_t tid) >>>>>>> +{ >>>>>>> + struct arm_spe *spe = speq->spe; >>>>>>> + int err = machine__set_current_tid(spe->machine, speq->cpu, tid, tid); >>>>>> I think we should pass -1 as pid as we don't know the real pid. >>>>>> >>>>> AFAICT, I observe one case for machine__set_current_tid() returning error >>>>> is 'speq->cpu' is -1 (this is the case for per-thread tracing). In >>>>> this case, if pass '-1' for pid/tid, it still will return failure. >>>>> >>>>> So here should return the error as it is. Am I missing anything? >>>> I'm not saying about the error. It's about thread status. >>>> In the machine__set_current_tid(), it calls >>>> machine__findnew_thread() with given pid and tid. >>>> >>>> I suspect it can set pid to a wrong value if the thread has >>>> no pid value at the moment. >>> Here we should avoid to write pid '-1' with >>> machine__set_current_tid(). >> If the kernel is writing the tids to the contextidr, isn't it wrong to >> assume tid == pid when decoding the context packets here? I haven't >> observed any impact in the built-in commands though, so there must be >> something I'm not seeing. > Okay, let me correct myself :) > > I checked Intel-pt's implementation, I understand now that we need to > distinguish the cases for pid/tid from context switch event and only tid > from SPE context packet. > > Since the context switch event contains the correct pid and tid > values, we should set both of them, see Intel-PT's implementation [1]. > > As Namhyung pointed, we need to set pid as '-1' when we only know the > tid value from SPE context packet; see [2]. I will correct this and resend the patch for SPE. Thanks, German > > So we should use the same with Intel-pt. > > Sorry for I didn't really understand well Namhyung's suggestion and > thanks you both pointed out the issue. > > Leo > > P.s. an offline topic is we should send a patch to fix cs-etm issue > as well [3]. > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/perf/util/intel-pt.c#n2920 > [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/perf/util/intel-pt.c#n2215 > [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/perf/util/cs-etm.c#n1121
Powered by blists - more mailing lists