| Message ID | 20240925131357.9468-1-julien.meunier@nokia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | coresight: etm4x: Fix PID tracing when perf is run in an init PID namespace | expand |
On 25/09/2024 14:13, Julien Meunier wrote: > The previous implementation limited the tracing capabilities when perf > was run in the init PID namespace, making it impossible to trace > applications in non-init PID namespaces. > > This update improves the tracing process by verifying the event owner. > This allows us to determine whether the user has the necessary > permissions to trace the application. > > Cc: stable@vger.kernel.org > Fixes: aab473867fed ("coresight: etm4x: Don't trace PID for non-root PID namespace") > Signed-off-by: Julien Meunier <julien.meunier@nokia.com> Thanks for the fix, I will queue this for v6.13 Suzuki > --- > drivers/hwtracing/coresight/coresight-etm4x-core.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c > index bf01f01964cf..8365307b1aec 100644 > --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c > +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c > @@ -695,7 +695,7 @@ static int etm4_parse_event_config(struct coresight_device *csdev, > > /* Only trace contextID when runs in root PID namespace */ > if ((attr->config & BIT(ETM_OPT_CTXTID)) && > - task_is_in_init_pid_ns(current)) > + task_is_in_init_pid_ns(event->owner)) > /* bit[6], Context ID tracing bit */ > config->cfg |= TRCCONFIGR_CID; > > @@ -710,7 +710,7 @@ static int etm4_parse_event_config(struct coresight_device *csdev, > goto out; > } > /* Only trace virtual contextID when runs in root PID namespace */ > - if (task_is_in_init_pid_ns(current)) > + if (task_is_in_init_pid_ns(event->owner)) > config->cfg |= TRCCONFIGR_VMID | TRCCONFIGR_VMIDOPT; > } >
Hi Julien, On Wed, Sep 25, 2024 at 03:13:56PM +0200, Julien Meunier wrote: > The previous implementation limited the tracing capabilities when perf > was run in the init PID namespace, making it impossible to trace > applications in non-init PID namespaces. > > This update improves the tracing process by verifying the event owner. > This allows us to determine whether the user has the necessary > permissions to trace the application. The original commit aab473867fed is not for constraint permission. It is about PID namespace mismatching issue. E.g. Perf runs in non-root namespace, thus it records process info in the non-root PID namespace. On the other hand, Arm CoreSight traces PID for root namespace, as a result, it will lead mess when decoding. With this change, I am not convinced that Arm CoreSight can trace PID for non-root PID namespace. Seems to me, the concerned issue is still existed - it might cause PID mismatching issue between hardware trace data and Perf's process info. I think we need to check using the software context switch event. With more clear idea, I will get back at here. Thanks, Leo > Cc: stable@vger.kernel.org > Fixes: aab473867fed ("coresight: etm4x: Don't trace PID for non-root PID namespace") > Signed-off-by: Julien Meunier <julien.meunier@nokia.com> > --- > drivers/hwtracing/coresight/coresight-etm4x-core.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c > index bf01f01964cf..8365307b1aec 100644 > --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c > +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c > @@ -695,7 +695,7 @@ static int etm4_parse_event_config(struct coresight_device *csdev, > > /* Only trace contextID when runs in root PID namespace */ > if ((attr->config & BIT(ETM_OPT_CTXTID)) && > - task_is_in_init_pid_ns(current)) > + task_is_in_init_pid_ns(event->owner)) > /* bit[6], Context ID tracing bit */ > config->cfg |= TRCCONFIGR_CID; > > @@ -710,7 +710,7 @@ static int etm4_parse_event_config(struct coresight_device *csdev, > goto out; > } > /* Only trace virtual contextID when runs in root PID namespace */ > - if (task_is_in_init_pid_ns(current)) > + if (task_is_in_init_pid_ns(event->owner)) > config->cfg |= TRCCONFIGR_VMID | TRCCONFIGR_VMIDOPT; > } > > -- > 2.34.1 > >
On 10/7/2024 9:05 PM, Leo Yan wrote: > > Hi Julien, > > On Wed, Sep 25, 2024 at 03:13:56PM +0200, Julien Meunier wrote: >> The previous implementation limited the tracing capabilities when perf >> was run in the init PID namespace, making it impossible to trace >> applications in non-init PID namespaces. >> >> This update improves the tracing process by verifying the event owner. >> This allows us to determine whether the user has the necessary >> permissions to trace the application. > > The original commit aab473867fed is not for constraint permission. It is > about PID namespace mismatching issue. > > E.g. Perf runs in non-root namespace, thus it records process info in the > non-root PID namespace. On the other hand, Arm CoreSight traces PID for > root namespace, as a result, it will lead mess when decoding. > > With this change, I am not convinced that Arm CoreSight can trace PID for > non-root PID namespace. Seems to me, the concerned issue is still existed > - it might cause PID mismatching issue between hardware trace data and > Perf's process info. I thought again and found I was wrong with above conclusion. This patch is a good fixing for the perf running in root namespace to profile programs in non-root namespace. Sorry for noise. Maybe it is good to improve a bit comments to avoid confusion. See below. [...] >> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c >> index bf01f01964cf..8365307b1aec 100644 >> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c >> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c >> @@ -695,7 +695,7 @@ static int etm4_parse_event_config(struct coresight_device *csdev, >> >> /* Only trace contextID when runs in root PID namespace */ We can claim the requirement for the *tool* running in root PID namespae. /* Only trace contextID when the tool runs in root PID namespace */ >> if ((attr->config & BIT(ETM_OPT_CTXTID)) && >> - task_is_in_init_pid_ns(current)) >> + task_is_in_init_pid_ns(event->owner)) >> /* bit[6], Context ID tracing bit */ >> config->cfg |= TRCCONFIGR_CID; >> >> @@ -710,7 +710,7 @@ static int etm4_parse_event_config(struct coresight_device *csdev, >> goto out; >> } >> /* Only trace virtual contextID when runs in root PID namespace */ Ditto. /* Only trace virtual contextID when the tool runs in root PID namespace */ With above change: Reviewed-by: Leo Yan <leo.yan@arm.com> >> - if (task_is_in_init_pid_ns(current)) >> + if (task_is_in_init_pid_ns(event->owner)) >> config->cfg |= TRCCONFIGR_VMID | TRCCONFIGR_VMIDOPT; >> } >> >> -- >> 2.34.1 >> >>
On 08/10/2024 07:52, Leo Yan wrote: > On 10/7/2024 9:05 PM, Leo Yan wrote: >> >> Hi Julien, >> >> On Wed, Sep 25, 2024 at 03:13:56PM +0200, Julien Meunier wrote: >>> The previous implementation limited the tracing capabilities when perf >>> was run in the init PID namespace, making it impossible to trace >>> applications in non-init PID namespaces. >>> >>> This update improves the tracing process by verifying the event owner. >>> This allows us to determine whether the user has the necessary >>> permissions to trace the application. >> >> The original commit aab473867fed is not for constraint permission. It is >> about PID namespace mismatching issue. >> >> E.g. Perf runs in non-root namespace, thus it records process info in the >> non-root PID namespace. On the other hand, Arm CoreSight traces PID for >> root namespace, as a result, it will lead mess when decoding. >> >> With this change, I am not convinced that Arm CoreSight can trace PID for >> non-root PID namespace. Seems to me, the concerned issue is still existed >> - it might cause PID mismatching issue between hardware trace data and >> Perf's process info. > > I thought again and found I was wrong with above conclusion. This patch is a > good fixing for the perf running in root namespace to profile programs in > non-root namespace. Sorry for noise. > > Maybe it is good to improve a bit comments to avoid confusion. See below. > > [...] > >>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c >>> index bf01f01964cf..8365307b1aec 100644 >>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c >>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c >>> @@ -695,7 +695,7 @@ static int etm4_parse_event_config(struct coresight_device *csdev, >>> >>> /* Only trace contextID when runs in root PID namespace */ > > We can claim the requirement for the *tool* running in root PID namespae. > > /* Only trace contextID when the tool runs in root PID namespace */ minor nit: I wouldn't call "tool". Let keep it "event owner". /* Only trace contextID when the event owner is in root PID namespace */ Julien, Please could you respin the patch with the comments addressed. Kind regards Suzuki > > >>> if ((attr->config & BIT(ETM_OPT_CTXTID)) && >>> - task_is_in_init_pid_ns(current)) >>> + task_is_in_init_pid_ns(event->owner)) >>> /* bit[6], Context ID tracing bit */ >>> config->cfg |= TRCCONFIGR_CID; >>> >>> @@ -710,7 +710,7 @@ static int etm4_parse_event_config(struct coresight_device *csdev, >>> goto out; >>> } >>> /* Only trace virtual contextID when runs in root PID namespace */ > > Ditto. > > /* Only trace virtual contextID when the tool runs in root PID namespace */ > > With above change: > > Reviewed-by: Leo Yan <leo.yan@arm.com> > >>> - if (task_is_in_init_pid_ns(current)) >>> + if (task_is_in_init_pid_ns(event->owner)) >>> config->cfg |= TRCCONFIGR_VMID | TRCCONFIGR_VMIDOPT; >>> } >>> >>> -- >>> 2.34.1 >>> >>>
On 08/10/2024 11:59, Suzuki K Poulose wrote: > On 08/10/2024 07:52, Leo Yan wrote: >> On 10/7/2024 9:05 PM, Leo Yan wrote: [...] >> >> I thought again and found I was wrong with above conclusion. This >> patch is a >> good fixing for the perf running in root namespace to profile programs in >> non-root namespace. Sorry for noise. >> >> Maybe it is good to improve a bit comments to avoid confusion. See below. >> >> [...] >> >>>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/ >>>> drivers/hwtracing/coresight/coresight-etm4x-core.c >>>> index bf01f01964cf..8365307b1aec 100644 >>>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c >>>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c >>>> @@ -695,7 +695,7 @@ static int etm4_parse_event_config(struct >>>> coresight_device *csdev, >>>> >>>> /* Only trace contextID when runs in root PID namespace */ >> >> We can claim the requirement for the *tool* running in root PID namespae. >> >> /* Only trace contextID when the tool runs in root PID namespace */ > > minor nit: I wouldn't call "tool". Let keep it "event owner". > > /* Only trace contextID when the event owner is in root PID > namespace */ > > > Julien, > > Please could you respin the patch with the comments addressed. Sure, I will send a v2 with comments updated. > > Kind regards > Suzuki > > >> >> >>>> if ((attr->config & BIT(ETM_OPT_CTXTID)) && >>>> - task_is_in_init_pid_ns(current)) >>>> + task_is_in_init_pid_ns(event->owner)) >>>> /* bit[6], Context ID tracing bit */ >>>> config->cfg |= TRCCONFIGR_CID; >>>> >>>> @@ -710,7 +710,7 @@ static int etm4_parse_event_config(struct >>>> coresight_device *csdev, >>>> goto out; >>>> } >>>> /* Only trace virtual contextID when runs in root PID >>>> namespace */ >> >> Ditto. >> >> /* Only trace virtual contextID when the tool runs in root PID >> namespace */ >> >> With above change: >> >> Reviewed-by: Leo Yan <leo.yan@arm.com> Regards, Julien
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index bf01f01964cf..8365307b1aec 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -695,7 +695,7 @@ static int etm4_parse_event_config(struct coresight_device *csdev, /* Only trace contextID when runs in root PID namespace */ if ((attr->config & BIT(ETM_OPT_CTXTID)) && - task_is_in_init_pid_ns(current)) + task_is_in_init_pid_ns(event->owner)) /* bit[6], Context ID tracing bit */ config->cfg |= TRCCONFIGR_CID; @@ -710,7 +710,7 @@ static int etm4_parse_event_config(struct coresight_device *csdev, goto out; } /* Only trace virtual contextID when runs in root PID namespace */ - if (task_is_in_init_pid_ns(current)) + if (task_is_in_init_pid_ns(event->owner)) config->cfg |= TRCCONFIGR_VMID | TRCCONFIGR_VMIDOPT; }
The previous implementation limited the tracing capabilities when perf was run in the init PID namespace, making it impossible to trace applications in non-init PID namespaces. This update improves the tracing process by verifying the event owner. This allows us to determine whether the user has the necessary permissions to trace the application. Cc: stable@vger.kernel.org Fixes: aab473867fed ("coresight: etm4x: Don't trace PID for non-root PID namespace") Signed-off-by: Julien Meunier <julien.meunier@nokia.com> --- drivers/hwtracing/coresight/coresight-etm4x-core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)