[PATCH v1 0/4] coresight: etm: Correct (virtual) contextID tracing for namespace

[PATCH v1 0/4] coresight: etm: Correct (virtual) contextID tracing for namespace

[Leo Yan]

If a profiling program runs in a non-root PID namespace, if CoreSight
driver enables contextID tracing, it can lead to mismatching issue
between the context ID in hardware trace data and the allocated PID in
the non-root namespace.

CoreSight driver has tried to address this issue for the contextID
related interfaces under sysfs, but it misses other parts: it doesn't
prevent user to set VMID (virtual contextID) for kernel runs in EL2 with
VHE, and furthermore, it misses to handle the perf mode when the
profiling tool (e.g. perf) doesn't run in root PID namespace.

For this reason, this patch series is to correct contextID tracing for
non-root namespace.

Patch 01 is to use spinlock to protect reading virtual context ID
comparator.

Patch 02 corrects the virtual contextID tracing for non-root PID
namespace.

Patch 03/04 are used to fix the contextID tracing for perf mode.

I only verified this patch series on Juno board in the root PID
namespace and confirmed the patches don't introduce any regression for
root PID namespace.


Leo Yan (4):
  coresight: etm4x: Add lock for reading virtual context ID comparator
  coresight: etm4x: Don't use virtual contextID for non-root PID
    namespace
  coresight: etm4x: Don't trace contextID for non-root namespace in perf
    mode
  coresight: etm3x: Don't trace contextID for non-root namespace in perf
    mode

 .../coresight/coresight-etm3x-core.c          |  4 +++
 .../coresight/coresight-etm4x-core.c          | 10 +++++--
 .../coresight/coresight-etm4x-sysfs.c         | 30 +++++++++++++++++++
 3 files changed, 42 insertions(+), 2 deletions(-)

-- 
2.25.1

[PATCH v1 1/4] coresight: etm4x: Add lock for reading virtual context ID comparator

[Leo Yan]

When read the virtual context ID comparator value via sysfs node, the
driver firstly reads out the index, then reads the comparator value
based on index.

This patch adds the spinlock to protect the atomicity for up two steps.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 drivers/hwtracing/coresight/coresight-etm4x-sysfs.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
index a0640fa5c55b..e4c8c44d04ef 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
@@ -2111,7 +2111,9 @@ static ssize_t vmid_val_show(struct device *dev,
 	struct etmv4_drvdata *drvdata = dev_get_drvdata(dev->parent);
 	struct etmv4_config *config = &drvdata->config;
 
+	spin_lock(&drvdata->spinlock);
 	val = (unsigned long)config->vmid_val[config->vmid_idx];
+	spin_unlock(&drvdata->spinlock);
 	return scnprintf(buf, PAGE_SIZE, "%#lx\n", val);
 }
 
-- 
2.25.1

[PATCH v1 2/4] coresight: etm4x: Don't use virtual contextID for non-root PID namespace

[Leo Yan]

As commented in the function ctxid_pid_store(), it can cause the PID
values mismatching between context ID tracing and PID allocated in a
non-root namespace, and it can leak kernel information.

For this reason, when a process runs in non-root PID namespace, the
driver doesn't allow contextID tracing and returns failure when access
contextID related sysfs nodes.

VMID works for virtual contextID when the kernel runs in EL2 mode with
VHE; on the other hand, the driver doesn't prevent users from accessing
it when programs run in the non-root namespace.  Thus this can lead
to same issues with contextID described above.

This patch imposes the checking on VMID related sysfs knobs, it returns
failure if current process runs in non-root PID namespace.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 .../coresight/coresight-etm4x-sysfs.c         | 28 +++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
index e4c8c44d04ef..e218281703b0 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
@@ -2111,6 +2111,13 @@ static ssize_t vmid_val_show(struct device *dev,
 	struct etmv4_drvdata *drvdata = dev_get_drvdata(dev->parent);
 	struct etmv4_config *config = &drvdata->config;
 
+	/*
+	 * Don't use virtual contextID tracing if coming from a PID namespace.
+	 * See comment in ctxid_pid_store().
+	 */
+	if (task_active_pid_ns(current) != &init_pid_ns)
+		return -EINVAL;
+
 	spin_lock(&drvdata->spinlock);
 	val = (unsigned long)config->vmid_val[config->vmid_idx];
 	spin_unlock(&drvdata->spinlock);
@@ -2125,6 +2132,13 @@ static ssize_t vmid_val_store(struct device *dev,
 	struct etmv4_drvdata *drvdata = dev_get_drvdata(dev->parent);
 	struct etmv4_config *config = &drvdata->config;
 
+	/*
+	 * Don't use virtual contextID tracing if coming from a PID namespace.
+	 * See comment in ctxid_pid_store().
+	 */
+	if (task_active_pid_ns(current) != &init_pid_ns)
+		return -EINVAL;
+
 	/*
 	 * only implemented when vmid tracing is enabled, i.e. at least one
 	 * vmid comparator is implemented and at least 8 bit vmid size
@@ -2148,6 +2162,13 @@ static ssize_t vmid_masks_show(struct device *dev,
 	struct etmv4_drvdata *drvdata = dev_get_drvdata(dev->parent);
 	struct etmv4_config *config = &drvdata->config;
 
+	/*
+	 * Don't use virtual contextID tracing if coming from a PID namespace.
+	 * See comment in ctxid_pid_store().
+	 */
+	if (task_active_pid_ns(current) != &init_pid_ns)
+		return -EINVAL;
+
 	spin_lock(&drvdata->spinlock);
 	val1 = config->vmid_mask0;
 	val2 = config->vmid_mask1;
@@ -2165,6 +2186,13 @@ static ssize_t vmid_masks_store(struct device *dev,
 	struct etmv4_config *config = &drvdata->config;
 	int nr_inputs;
 
+	/*
+	 * Don't use virtual contextID tracing if coming from a PID namespace.
+	 * See comment in ctxid_pid_store().
+	 */
+	if (task_active_pid_ns(current) != &init_pid_ns)
+		return -EINVAL;
+
 	/*
 	 * only implemented when vmid tracing is enabled, i.e. at least one
 	 * vmid comparator is implemented and at least 8 bit vmid size
-- 
2.25.1

[PATCH v1 3/4] coresight: etm4x: Don't trace contextID for non-root namespace in perf mode

[Leo Yan]

When runs in perf mode, the driver always enables the contextID tracing.
This can lead to confusion if the program runs in non-root PID namespace
and potentially leak kernel information.

When programs running in perf mode, this patch changes to only enable
contextID tracing for root PID namespace.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 drivers/hwtracing/coresight/coresight-etm4x-core.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index e24252eaf8e4..6e614bfb38c6 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -615,7 +615,9 @@ static int etm4_parse_event_config(struct coresight_device *csdev,
 		config->cfg |= BIT(11);
 	}
 
-	if (attr->config & BIT(ETM_OPT_CTXTID))
+	/* Only trace contextID when runs in root PID namespace */
+	if ((attr->config & BIT(ETM_OPT_CTXTID)) &&
+	    (task_active_pid_ns(current) == &init_pid_ns))
 		/* bit[6], Context ID tracing bit */
 		config->cfg |= BIT(ETM4_CFG_BIT_CTXTID);
 
@@ -629,7 +631,11 @@ static int etm4_parse_event_config(struct coresight_device *csdev,
 			ret = -EINVAL;
 			goto out;
 		}
-		config->cfg |= BIT(ETM4_CFG_BIT_VMID) | BIT(ETM4_CFG_BIT_VMID_OPT);
+
+		/* Only trace virtual contextID when runs in root PID namespace */
+		if (task_active_pid_ns(current) == &init_pid_ns)
+			config->cfg |= BIT(ETM4_CFG_BIT_VMID) |
+				       BIT(ETM4_CFG_BIT_VMID_OPT);
 	}
 
 	/* return stack - enable if selected and supported */
-- 
2.25.1

[PATCH v1 4/4] coresight: etm3x: Don't trace contextID for non-root namespace in perf mode

[Leo Yan]

ETMv3 driver doesn't handle the case when programs run in non-root PID
namespace, so it allows the contextID tracing by directly using perf
config.

This patch changes to only enable contextID tracing for root PID
namespace.  Note, the hardware supports VMID tracing from ETMv3.5, but
the driver never enables VMID trace, so this patch doesn't handle VMID
trace (bit 30 in ETMCR register) particularly.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 drivers/hwtracing/coresight/coresight-etm3x-core.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-etm3x-core.c b/drivers/hwtracing/coresight/coresight-etm3x-core.c
index cf64ce73a741..0621ab0c71d9 100644
--- a/drivers/hwtracing/coresight/coresight-etm3x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm3x-core.c
@@ -340,6 +340,10 @@ static int etm_parse_event_config(struct etm_drvdata *drvdata,
 
 	config->ctrl = attr->config;
 
+	/* Don't trace contextID when runs in non-root PID namespace */
+	if (task_active_pid_ns(current) != &init_pid_ns)
+		config->ctrl &= ~ETMCR_CTXID_SIZE;
+
 	/*
 	 * Possible to have cores with PTM (supports ret stack) and ETM
 	 * (never has ret stack) on the same SoC. So if we have a request
-- 
2.25.1

Re: [PATCH v1 1/4] coresight: etm4x: Add lock for reading virtual context ID comparator

[Suzuki K Poulose]

Hi Leo

On 31/10/2021 14:42, Leo Yan wrote:
> When read the virtual context ID comparator value via sysfs node, the
> driver firstly reads out the index, then reads the comparator value
> based on index. >
> This patch adds the spinlock to protect the atomicity for up two steps.

minor nit: This could be :

"Updates to the values and the index are protected via the
spinlock. Ensure we use the same lock to read the value
safely"

With that:

Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>

Re: [PATCH v1 1/4] coresight: etm4x: Add lock for reading virtual context ID comparator

[Leo Yan]

Hi Suzuki,

On Mon, Nov 01, 2021 at 09:49:06AM +0000, Suzuki Kuruppassery Poulose wrote:
> Hi Leo
> 
> On 31/10/2021 14:42, Leo Yan wrote:
> > When read the virtual context ID comparator value via sysfs node, the
> > driver firstly reads out the index, then reads the comparator value
> > based on index. >
> > This patch adds the spinlock to protect the atomicity for up two steps.
> 
> minor nit: This could be :
> 
> "Updates to the values and the index are protected via the
> spinlock. Ensure we use the same lock to read the value
> safely"

Yeah, this is more simple and clear.  Will use it.

> With that:
> 
> Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>

Thanks for review.

Leo

Re: [PATCH v1 2/4] coresight: etm4x: Don't use virtual contextID for non-root PID namespace

[Suzuki K Poulose]

Hi Leo

On 31/10/2021 14:42, Leo Yan wrote:
> As commented in the function ctxid_pid_store(), it can cause the PID
> values mismatching between context ID tracing and PID allocated in a
> non-root namespace, and it can leak kernel information.
> 
> For this reason, when a process runs in non-root PID namespace, the
> driver doesn't allow contextID tracing and returns failure when access
> contextID related sysfs nodes.
> 
> VMID works for virtual contextID when the kernel runs in EL2 mode with
> VHE; on the other hand, the driver doesn't prevent users from accessing
> it when programs run in the non-root namespace.  Thus this can lead
> to same issues with contextID described above.
> 
> This patch imposes the checking on VMID related sysfs knobs, it returns
> failure if current process runs in non-root PID namespace.
> 
> Signed-off-by: Leo Yan <leo.yan@linaro.org>

Patch looks good to me. Please see minor comment below.


> ---
>   .../coresight/coresight-etm4x-sysfs.c         | 28 +++++++++++++++++++
>   1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
> index e4c8c44d04ef..e218281703b0 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
> @@ -2111,6 +2111,13 @@ static ssize_t vmid_val_show(struct device *dev,
>   	struct etmv4_drvdata *drvdata = dev_get_drvdata(dev->parent);
>   	struct etmv4_config *config = &drvdata->config;
>   
> +	/*
> +	 * Don't use virtual contextID tracing if coming from a PID namespace.
> +	 * See comment in ctxid_pid_store().
> +	 */
> +	if (task_active_pid_ns(current) != &init_pid_ns)
> +		return -EINVAL;
> +
>   	spin_lock(&drvdata->spinlock);
>   	val = (unsigned long)config->vmid_val[config->vmid_idx];
>   	spin_unlock(&drvdata->spinlock);
> @@ -2125,6 +2132,13 @@ static ssize_t vmid_val_store(struct device *dev,
>   	struct etmv4_drvdata *drvdata = dev_get_drvdata(dev->parent);
>   	struct etmv4_config *config = &drvdata->config;
>   
> +	/*
> +	 * Don't use virtual contextID tracing if coming from a PID namespace.
> +	 * See comment in ctxid_pid_store().
> +	 */
> +	if (task_active_pid_ns(current) != &init_pid_ns)

Please could we add a helper function to make this obvious ?

e.g: task_is_in_root_ns(task) ?

Suzuki

Re: [PATCH v1 2/4] coresight: etm4x: Don't use virtual contextID for non-root PID namespace

[Leo Yan]

On Thu, Nov 04, 2021 at 03:07:45PM +0000, Suzuki Kuruppassery Poulose wrote:

[...]

> > @@ -2111,6 +2111,13 @@ static ssize_t vmid_val_show(struct device *dev,
> >   	struct etmv4_drvdata *drvdata = dev_get_drvdata(dev->parent);
> >   	struct etmv4_config *config = &drvdata->config;
> > +	/*
> > +	 * Don't use virtual contextID tracing if coming from a PID namespace.
> > +	 * See comment in ctxid_pid_store().
> > +	 */
> > +	if (task_active_pid_ns(current) != &init_pid_ns)
> > +		return -EINVAL;
> > +
> >   	spin_lock(&drvdata->spinlock);
> >   	val = (unsigned long)config->vmid_val[config->vmid_idx];
> >   	spin_unlock(&drvdata->spinlock);
> > @@ -2125,6 +2132,13 @@ static ssize_t vmid_val_store(struct device *dev,
> >   	struct etmv4_drvdata *drvdata = dev_get_drvdata(dev->parent);
> >   	struct etmv4_config *config = &drvdata->config;
> > +	/*
> > +	 * Don't use virtual contextID tracing if coming from a PID namespace.
> > +	 * See comment in ctxid_pid_store().
> > +	 */
> > +	if (task_active_pid_ns(current) != &init_pid_ns)
> 
> Please could we add a helper function to make this obvious ?
> 
> e.g: task_is_in_root_ns(task) ?

Yeah, I will use a separate patch set to add this helper function and
polish relevant codes in the kernel.  Thanks for suggestion.

Leo

Re: [PATCH v1 3/4] coresight: etm4x: Don't trace contextID for non-root namespace in perf mode

[Suzuki K Poulose]

Hi Leo,

On 31/10/2021 14:42, Leo Yan wrote:
> When runs in perf mode, the driver always enables the contextID tracing.
> This can lead to confusion if the program runs in non-root PID namespace
> and potentially leak kernel information.
> 
> When programs running in perf mode, this patch changes to only enable
> contextID tracing for root PID namespace.
> 

The only concern with the patch here is we silently ignore the CTXTID
flag and the perf assumes the CTXTID is traced, when traced from a 
non-root namespace. Does the decoder handle this case gracefully ? We are
fine if that is the case.

Either way, we don't want to enforce the policy in the perf tool, if we
can transparently handle the missing CTXTID and allow the trace session
and decode complete. That said, your approach is the safe bet here.


> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>   drivers/hwtracing/coresight/coresight-etm4x-core.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> index e24252eaf8e4..6e614bfb38c6 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> @@ -615,7 +615,9 @@ static int etm4_parse_event_config(struct coresight_device *csdev,
>   		config->cfg |= BIT(11);
>   	}
>   
> -	if (attr->config & BIT(ETM_OPT_CTXTID))
> +	/* Only trace contextID when runs in root PID namespace */
> +	if ((attr->config & BIT(ETM_OPT_CTXTID)) &&
> +	    (task_active_pid_ns(current) == &init_pid_ns))
>   		/* bit[6], Context ID tracing bit */
>   		config->cfg |= BIT(ETM4_CFG_BIT_CTXTID);
>   

As mentioned in the previous comment, please add a helper here, than 
open coding the check.

Kind regards
Suzuki

Re: [PATCH v1 0/4] coresight: etm: Correct (virtual) contextID tracing for namespace

[Suzuki K Poulose]

Hi Leo


On 31/10/2021 14:42, Leo Yan wrote:
> If a profiling program runs in a non-root PID namespace, if CoreSight
> driver enables contextID tracing, it can lead to mismatching issue
> between the context ID in hardware trace data and the allocated PID in
> the non-root namespace.
> 
> CoreSight driver has tried to address this issue for the contextID
> related interfaces under sysfs, but it misses other parts: it doesn't
> prevent user to set VMID (virtual contextID) for kernel runs in EL2 with
> VHE, and furthermore, it misses to handle the perf mode when the
> profiling tool (e.g. perf) doesn't run in root PID namespace.
> 
> For this reason, this patch series is to correct contextID tracing for
> non-root namespace.
> 
> Patch 01 is to use spinlock to protect reading virtual context ID
> comparator.
> 
> Patch 02 corrects the virtual contextID tracing for non-root PID
> namespace.
> 
> Patch 03/04 are used to fix the contextID tracing for perf mode.
> 
> I only verified this patch series on Juno board in the root PID
> namespace and confirmed the patches don't introduce any regression for
> root PID namespace.
> 

I have finished reviewing the set.

Suzuki

Re: [PATCH v1 3/4] coresight: etm4x: Don't trace contextID for non-root namespace in perf mode

[Leo Yan]

Hi Suzuki,

On Tue, Nov 16, 2021 at 09:46:20AM +0000, Suzuki Kuruppassery Poulose wrote:
> Hi Leo,
> 
> On 31/10/2021 14:42, Leo Yan wrote:
> > When runs in perf mode, the driver always enables the contextID tracing.
> > This can lead to confusion if the program runs in non-root PID namespace
> > and potentially leak kernel information.
> > 
> > When programs running in perf mode, this patch changes to only enable
> > contextID tracing for root PID namespace.
> > 
> 
> The only concern with the patch here is we silently ignore the CTXTID
> flag and the perf assumes the CTXTID is traced, when traced from a non-root
> namespace. Does the decoder handle this case gracefully ? We are
> fine if that is the case.

Good point.  As far as I know, if CoreSight trace data doesn't contain
context packets, tidq->tid is initialized as '0' and tidq->pid is
'-1'.  In this case, the decoder will fail to find thread context and
the user space samples will not output anymore, see [1],
cs_etm__mem_access() returns 0 when the thread pointer is NULL and
the user space samples will be skipped.

On the other hand, I observed an unexpected behaviour is the decoder
also fails to output any kernel samples.  From my understanding, the
kernel samples should always be output, I will check furthermore for
this (I can think one possibility is perf tool fails to find a
'correct' vmlinux when parsing symbols).

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/perf/util/cs-etm.c#n720

> Either way, we don't want to enforce the policy in the perf tool, if we
> can transparently handle the missing CTXTID and allow the trace session
> and decode complete. That said, your approach is the safe bet here.

Do you agree below assumption for tracing in non-root PID namespace?

For non-root namespace we doesn't tracing PID, CoreSight trace data
doesn't contain context packet, so the perf decoder cannot find the
corresponding thread context and perf tool will not generate any
samples for user mode.  But the decoder should generate kernel
samples.

If you agree with this, in theory I think we should not change anything
in perf tool (but let me confirm the decoder kernel samples can output
properly).

> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > ---
> >   drivers/hwtracing/coresight/coresight-etm4x-core.c | 10 ++++++++--
> >   1 file changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > index e24252eaf8e4..6e614bfb38c6 100644
> > --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > @@ -615,7 +615,9 @@ static int etm4_parse_event_config(struct coresight_device *csdev,
> >   		config->cfg |= BIT(11);
> >   	}
> > -	if (attr->config & BIT(ETM_OPT_CTXTID))
> > +	/* Only trace contextID when runs in root PID namespace */
> > +	if ((attr->config & BIT(ETM_OPT_CTXTID)) &&
> > +	    (task_active_pid_ns(current) == &init_pid_ns))
> >   		/* bit[6], Context ID tracing bit */
> >   		config->cfg |= BIT(ETM4_CFG_BIT_CTXTID);
> 
> As mentioned in the previous comment, please add a helper here, than open
> coding the check.

Agreed, will add new helper for checking root namespace.

Thanks for reviewing.

Leo