rdcycle from userland with RISCV_PMU_SBI=y

rdcycle from userland with RISCV_PMU_SBI=y

[Aurelien Jarno]

Hi all,

Mathieu Malaterre reported an issue in Debian with the highway package,
which uses the rdcycle pseudo instruction, and started to fail recently.

We tracked down an issue to the "perf platform driver based on SBI PMU
extension" recently added in Linux 5.18 [1]. On a board which runs an
SBI with the PMU extension, this causes the rdcycle pseudo instruction
to generate a SIGILL. This is because the driver explicitly set
CSR_SCOUNTEREN to 0x02, giving access from userland to rdtime but not
rdcycle.

I wonder if this change is intentional, and if there is a reason to
forbid using the rdcycle pseudo instruction from userland. If it is the
case, this should probably be changed so that the behaviour does not
differ from board to board depending on the available PMU extension.

Aurelien

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e9991434596f5373dfd75857b445eb92a9253c56
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/perf/riscv_pmu_sbi.c?id=2880e1a175b9f31798f9d9482ee49187f61b5539#n649

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: rdcycle from userland with RISCV_PMU_SBI=y

[Mathieu Malaterre]

Atish,

On Fri, Sep 2, 2022 at 9:12 AM Atish Patra <atishp@atishpatra.org> wrote:
>
>
>
> On Thu, Sep 1, 2022 at 3:34 PM Atish Patra <atishp@atishpatra.org> wrote:
>>
>>
>>
>> On Thu, Sep 1, 2022 at 2:15 PM Aurelien Jarno <aurelien@aurel32.net> wrote:
>>>
>>> Hi all,
>>>
>>> Mathieu Malaterre reported an issue in Debian with the highway package,
>>> which uses the rdcycle pseudo instruction, and started to fail recently.
>>>
>>> We tracked down an issue to the "perf platform driver based on SBI PMU
>>> extension" recently added in Linux 5.18 [1]. On a board which runs an
>>> SBI with the PMU extension, this causes the rdcycle pseudo instruction
>>> to generate a SIGILL. This is because the driver explicitly set
>>> CSR_SCOUNTEREN to 0x02, giving access from userland to rdtime but not
>>> rdcycle.
>>>
>>> I wonder if this change is intentional, and if there is a reason to
>>> forbid using the rdcycle pseudo instruction from userland. If it is the
>>> case, this should probably be changed so that the behaviour does not
>>> differ from board to board depending on the available PMU extension.
>>>
>>
>> Yes. The change was intentional due to security reasons. One rogue process can have access to cycle &
>> instructions of the entire kernel always which can lead to some sort of side channel attacks.
>>
>> However, I agree that we can't break userspace. I was not aware of the fact that there are already users of rdcycle
>> in the userspace. I will send a patch to restore the original behavior by enabling CY, IR bits in scounteren.
>>
>
> Merging the thread from sw-dev:
>
> "
> > > On Thu, Sep 1, 2022 at 7:36 PM Paul Walmsley <paul.walmsley@sifive.com> wrote:
> > >>
> > >> If I recall correctly,  other architectures don't allow direct access to their cycle counters from userspace for security reasons.   Any reason why RISC-V shouldn't follow the same approach?
> > >
> > >
> > > That was the intention behind disabling the access in the PMU driver. At that time, I didn’t find any users. Obviously I was wrong 😂. But is that a sufficient reason to existing break user space ?
> >
> > As already pointed out, we can't compromise security by having all
> > apps unrestricted access to the cycle counter. We already have the
> > Linux perf subsystem for managing counters so apps should always use
> > Linux perf syscalls.
> >
> > IMO, the package "highway" directly accessing the cycle counter should
> > be fixed instead of fixing the Linux SBI PMU driver.
> >
> > Further investigating the highway project
> > (https://github.com/google/highway), it seems this project is using
> > "rdcycle" to track timer ticks which is totally wrong. Instead the use
> > of "rdcycle" should be replaced with "rdtime" in this project.
> > (Refer, line 156 of
> > https://github.com/google/highway/blob/master/hwy/nanobenchmark.cc)
>
> Pay attention that quite a few projects are using rdcycle in user-space already:
>
> * https://codesearch.debian.net/search?q=%22rdcycle+%250%22
>
> Anyway if you believe rdtime is the right fix, we can fix one project
> at a time..."
>
> Thanks for sharing the list. I took a quick look. The first few ones at least  (firefox, llvm, chrome)
> seems to use rdcycle for timestamp. All of the packages use "rdtsc" for x86.
> The equivalent instruction in RISC-V should be rdtime not rdcycle.

[CC-ing highway main dev]

Here is his comment on the rdtime patch proposal:

> For benchmarking we really would prefer a cycle count, rather than "real time clock ticks", for which the spec makes no guarantees. If/when those turn out to be millisecond-resolution, they are not very useful.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: rdcycle from userland with RISCV_PMU_SBI=y

[Anup Patel]

On Fri, Sep 2, 2022 at 1:23 PM Mathieu Malaterre <malat@debian.org> wrote:
>
> Atish,
>
> On Fri, Sep 2, 2022 at 9:12 AM Atish Patra <atishp@atishpatra.org> wrote:
> >
> >
> >
> > On Thu, Sep 1, 2022 at 3:34 PM Atish Patra <atishp@atishpatra.org> wrote:
> >>
> >>
> >>
> >> On Thu, Sep 1, 2022 at 2:15 PM Aurelien Jarno <aurelien@aurel32.net> wrote:
> >>>
> >>> Hi all,
> >>>
> >>> Mathieu Malaterre reported an issue in Debian with the highway package,
> >>> which uses the rdcycle pseudo instruction, and started to fail recently.
> >>>
> >>> We tracked down an issue to the "perf platform driver based on SBI PMU
> >>> extension" recently added in Linux 5.18 [1]. On a board which runs an
> >>> SBI with the PMU extension, this causes the rdcycle pseudo instruction
> >>> to generate a SIGILL. This is because the driver explicitly set
> >>> CSR_SCOUNTEREN to 0x02, giving access from userland to rdtime but not
> >>> rdcycle.
> >>>
> >>> I wonder if this change is intentional, and if there is a reason to
> >>> forbid using the rdcycle pseudo instruction from userland. If it is the
> >>> case, this should probably be changed so that the behaviour does not
> >>> differ from board to board depending on the available PMU extension.
> >>>
> >>
> >> Yes. The change was intentional due to security reasons. One rogue process can have access to cycle &
> >> instructions of the entire kernel always which can lead to some sort of side channel attacks.
> >>
> >> However, I agree that we can't break userspace. I was not aware of the fact that there are already users of rdcycle
> >> in the userspace. I will send a patch to restore the original behavior by enabling CY, IR bits in scounteren.
> >>
> >
> > Merging the thread from sw-dev:
> >
> > "
> > > > On Thu, Sep 1, 2022 at 7:36 PM Paul Walmsley <paul.walmsley@sifive.com> wrote:
> > > >>
> > > >> If I recall correctly,  other architectures don't allow direct access to their cycle counters from userspace for security reasons.   Any reason why RISC-V shouldn't follow the same approach?
> > > >
> > > >
> > > > That was the intention behind disabling the access in the PMU driver. At that time, I didn’t find any users. Obviously I was wrong 😂. But is that a sufficient reason to existing break user space ?
> > >
> > > As already pointed out, we can't compromise security by having all
> > > apps unrestricted access to the cycle counter. We already have the
> > > Linux perf subsystem for managing counters so apps should always use
> > > Linux perf syscalls.
> > >
> > > IMO, the package "highway" directly accessing the cycle counter should
> > > be fixed instead of fixing the Linux SBI PMU driver.
> > >
> > > Further investigating the highway project
> > > (https://github.com/google/highway), it seems this project is using
> > > "rdcycle" to track timer ticks which is totally wrong. Instead the use
> > > of "rdcycle" should be replaced with "rdtime" in this project.
> > > (Refer, line 156 of
> > > https://github.com/google/highway/blob/master/hwy/nanobenchmark.cc)
> >
> > Pay attention that quite a few projects are using rdcycle in user-space already:
> >
> > * https://codesearch.debian.net/search?q=%22rdcycle+%250%22
> >
> > Anyway if you believe rdtime is the right fix, we can fix one project
> > at a time..."
> >
> > Thanks for sharing the list. I took a quick look. The first few ones at least  (firefox, llvm, chrome)
> > seems to use rdcycle for timestamp. All of the packages use "rdtsc" for x86.
> > The equivalent instruction in RISC-V should be rdtime not rdcycle.
>
> [CC-ing highway main dev]
>
> Here is his comment on the rdtime patch proposal:
>
> > For benchmarking we really would prefer a cycle count, rather than "real time clock ticks", for which the spec makes no guarantees. If/when those turn out to be millisecond-resolution, they are not very useful.

The RISC-V spec does not make any guarantee about granularity of the
cycle counter as well. In fact, on systems with cpufreq enabled the
rate of cycle counter increment will vary at runtime.

On other hand, the time CSR (i.e. rdtime) will always increment at a
fixed platform specific rate which can be read from
"/proc/device-tree/cpus/timebase-frequency".

Regards,
Anup


>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: rdcycle from userland with RISCV_PMU_SBI=y

[Jan Wassenberg]

(Forwarding in plain text mode as apparently required by the list,
apologies for the duplication)

On Fri, Sep 2, 2022 at 12:22 PM Jan Wassenberg <janwas@google.com> wrote:
>
> OK, but even if frequency changes, a cycle counter seems more useful for benchmarking than a low-resolution wall time.
>
> FYI it's not just Highway, the Google benchmark library also uses rdcycle.
>
> Some personal opinions on security (disclaimer: I have some experience but am not an expert):
> Disabling cycle counters does not seem to provide any extra security on a multi-core system unless
> it also detects and mitigates software timers.
> See section 6 in http://cal.cs.columbia.edu/papers/isca12_timewarp.pdf. In short, other core(s) can increment a shared counter,
> which is close enough to a cycle counter to serve as a side channel. Detecting such coordination via performance counters
> seems feasible, though it risks false alarms for something like spinlocks.
>
> Reducing timer resolution could still be defeated by interpolating. If we're determined to do something,
> perhaps the cycle counter could have some jitter (> LLC latency), ideally only enabled if
> a possible attack is detected by trapping rdcycle and checking the elapsed time since the last rdcycle.
>
> On Fri, Sep 2, 2022 at 10:19 AM Anup Patel <anup@brainfault.org> wrote:
>>
>> On Fri, Sep 2, 2022 at 1:23 PM Mathieu Malaterre <malat@debian.org> wrote:
>> >
>> > Atish,
>> >
>> > On Fri, Sep 2, 2022 at 9:12 AM Atish Patra <atishp@atishpatra.org> wrote:
>> > >
>> > >
>> > >
>> > > On Thu, Sep 1, 2022 at 3:34 PM Atish Patra <atishp@atishpatra.org> wrote:
>> > >>
>> > >>
>> > >>
>> > >> On Thu, Sep 1, 2022 at 2:15 PM Aurelien Jarno <aurelien@aurel32.net> wrote:
>> > >>>
>> > >>> Hi all,
>> > >>>
>> > >>> Mathieu Malaterre reported an issue in Debian with the highway package,
>> > >>> which uses the rdcycle pseudo instruction, and started to fail recently.
>> > >>>
>> > >>> We tracked down an issue to the "perf platform driver based on SBI PMU
>> > >>> extension" recently added in Linux 5.18 [1]. On a board which runs an
>> > >>> SBI with the PMU extension, this causes the rdcycle pseudo instruction
>> > >>> to generate a SIGILL. This is because the driver explicitly set
>> > >>> CSR_SCOUNTEREN to 0x02, giving access from userland to rdtime but not
>> > >>> rdcycle.
>> > >>>
>> > >>> I wonder if this change is intentional, and if there is a reason to
>> > >>> forbid using the rdcycle pseudo instruction from userland. If it is the
>> > >>> case, this should probably be changed so that the behaviour does not
>> > >>> differ from board to board depending on the available PMU extension.
>> > >>>
>> > >>
>> > >> Yes. The change was intentional due to security reasons. One rogue process can have access to cycle &
>> > >> instructions of the entire kernel always which can lead to some sort of side channel attacks.
>> > >>
>> > >> However, I agree that we can't break userspace. I was not aware of the fact that there are already users of rdcycle
>> > >> in the userspace. I will send a patch to restore the original behavior by enabling CY, IR bits in scounteren.
>> > >>
>> > >
>> > > Merging the thread from sw-dev:
>> > >
>> > > "
>> > > > > On Thu, Sep 1, 2022 at 7:36 PM Paul Walmsley <paul.walmsley@sifive.com> wrote:
>> > > > >>
>> > > > >> If I recall correctly,  other architectures don't allow direct access to their cycle counters from userspace for security reasons.   Any reason why RISC-V shouldn't follow the same approach?
>> > > > >
>> > > > >
>> > > > > That was the intention behind disabling the access in the PMU driver. At that time, I didn’t find any users. Obviously I was wrong 😂. But is that a sufficient reason to existing break user space ?
>> > > >
>> > > > As already pointed out, we can't compromise security by having all
>> > > > apps unrestricted access to the cycle counter. We already have the
>> > > > Linux perf subsystem for managing counters so apps should always use
>> > > > Linux perf syscalls.
>> > > >
>> > > > IMO, the package "highway" directly accessing the cycle counter should
>> > > > be fixed instead of fixing the Linux SBI PMU driver.
>> > > >
>> > > > Further investigating the highway project
>> > > > (https://github.com/google/highway), it seems this project is using
>> > > > "rdcycle" to track timer ticks which is totally wrong. Instead the use
>> > > > of "rdcycle" should be replaced with "rdtime" in this project.
>> > > > (Refer, line 156 of
>> > > > https://github.com/google/highway/blob/master/hwy/nanobenchmark.cc)
>> > >
>> > > Pay attention that quite a few projects are using rdcycle in user-space already:
>> > >
>> > > * https://codesearch.debian.net/search?q=%22rdcycle+%250%22
>> > >
>> > > Anyway if you believe rdtime is the right fix, we can fix one project
>> > > at a time..."
>> > >
>> > > Thanks for sharing the list. I took a quick look. The first few ones at least  (firefox, llvm, chrome)
>> > > seems to use rdcycle for timestamp. All of the packages use "rdtsc" for x86.
>> > > The equivalent instruction in RISC-V should be rdtime not rdcycle.
>> >
>> > [CC-ing highway main dev]
>> >
>> > Here is his comment on the rdtime patch proposal:
>> >
>> > > For benchmarking we really would prefer a cycle count, rather than "real time clock ticks", for which the spec makes no guarantees. If/when those turn out to be millisecond-resolution, they are not very useful.
>>
>> The RISC-V spec does not make any guarantee about granularity of the
>> cycle counter as well. In fact, on systems with cpufreq enabled the
>> rate of cycle counter increment will vary at runtime.
>>
>> On other hand, the time CSR (i.e. rdtime) will always increment at a
>> fixed platform specific rate which can be read from
>> "/proc/device-tree/cpus/timebase-frequency".
>>
>> Regards,
>> Anup
>>
>>
>> >
>> > _______________________________________________
>> > linux-riscv mailing list
>> > linux-riscv@lists.infradead.org
>> > http://lists.infradead.org/mailman/listinfo/linux-riscv

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: rdcycle from userland with RISCV_PMU_SBI=y

[Anup Patel]

On Fri, Sep 2, 2022 at 3:52 PM Jan Wassenberg <janwas@google.com> wrote:
>
> OK, but even if frequency changes, a cycle counter seems more useful for benchmarking than a low-resolution wall time.
>
> FYI it's not just Highway, the Google benchmark library also uses rdcycle.

Since the RISC-V ecosystem is still evolving, it is best time to fix
things now rather
than later.

Currently, only "time" CSR is available to user-space and all other performance
counters should be enabled (or accessed) using Linux perf syscalls.

>
> Some personal opinions on security (disclaimer: I have some experience but am not an expert):
> Disabling cycle counters does not seem to provide any extra security on a multi-core system unless
> it also detects and mitigates software timers.
> See section 6 in http://cal.cs.columbia.edu/papers/isca12_timewarp.pdf. In short, other core(s) can increment a shared counter,
> which is close enough to a cycle counter to serve as a side channel. Detecting such coordination via performance counters
> seems feasible, though it risks false alarms for something like spinlocks.

At the moment, there is no HW mechanism defined in RISC-V to obfuscate "time" or
"cycle" CSRs for user-space. On the other hand, using "cycle" for
measuring passing
time is semantically incorrect.

In the future, someone can always propose a RISC-V ISA extension to obfuscate
"time" CSR values visible to user-space but the "cycle" counter is
purely a performance
counter and accurately reflects cycles taken by the CPU.

>
> Reducing timer resolution could still be defeated by interpolating. If we're determined to do something,
> perhaps the cycle counter could have some jitter (> LLC latency), ideally only enabled if
> a possible attack is detected by trapping rdcycle and checking the elapsed time since the last rdcycle.

The "cycle" counter is extensively used for performance analysis so it
is very unlikely
there might any ISA extension created to add jitters in performance
counters. The
"time" CSR (i.e. rdtime) on other hand is only used for measuring
passing time so
an ISA extension is possible to add some jitter. For example, in H-extension we
already have "htimedelta" CSR to add some delta to the "time" CSR values seen
by each Guest/VM under a hypervisor.

Regards,
Anup

>
> On Fri, Sep 2, 2022 at 10:19 AM Anup Patel <anup@brainfault.org> wrote:
>>
>> On Fri, Sep 2, 2022 at 1:23 PM Mathieu Malaterre <malat@debian.org> wrote:
>> >
>> > Atish,
>> >
>> > On Fri, Sep 2, 2022 at 9:12 AM Atish Patra <atishp@atishpatra.org> wrote:
>> > >
>> > >
>> > >
>> > > On Thu, Sep 1, 2022 at 3:34 PM Atish Patra <atishp@atishpatra.org> wrote:
>> > >>
>> > >>
>> > >>
>> > >> On Thu, Sep 1, 2022 at 2:15 PM Aurelien Jarno <aurelien@aurel32.net> wrote:
>> > >>>
>> > >>> Hi all,
>> > >>>
>> > >>> Mathieu Malaterre reported an issue in Debian with the highway package,
>> > >>> which uses the rdcycle pseudo instruction, and started to fail recently.
>> > >>>
>> > >>> We tracked down an issue to the "perf platform driver based on SBI PMU
>> > >>> extension" recently added in Linux 5.18 [1]. On a board which runs an
>> > >>> SBI with the PMU extension, this causes the rdcycle pseudo instruction
>> > >>> to generate a SIGILL. This is because the driver explicitly set
>> > >>> CSR_SCOUNTEREN to 0x02, giving access from userland to rdtime but not
>> > >>> rdcycle.
>> > >>>
>> > >>> I wonder if this change is intentional, and if there is a reason to
>> > >>> forbid using the rdcycle pseudo instruction from userland. If it is the
>> > >>> case, this should probably be changed so that the behaviour does not
>> > >>> differ from board to board depending on the available PMU extension.
>> > >>>
>> > >>
>> > >> Yes. The change was intentional due to security reasons. One rogue process can have access to cycle &
>> > >> instructions of the entire kernel always which can lead to some sort of side channel attacks.
>> > >>
>> > >> However, I agree that we can't break userspace. I was not aware of the fact that there are already users of rdcycle
>> > >> in the userspace. I will send a patch to restore the original behavior by enabling CY, IR bits in scounteren.
>> > >>
>> > >
>> > > Merging the thread from sw-dev:
>> > >
>> > > "
>> > > > > On Thu, Sep 1, 2022 at 7:36 PM Paul Walmsley <paul.walmsley@sifive.com> wrote:
>> > > > >>
>> > > > >> If I recall correctly,  other architectures don't allow direct access to their cycle counters from userspace for security reasons.   Any reason why RISC-V shouldn't follow the same approach?
>> > > > >
>> > > > >
>> > > > > That was the intention behind disabling the access in the PMU driver. At that time, I didn’t find any users. Obviously I was wrong 😂. But is that a sufficient reason to existing break user space ?
>> > > >
>> > > > As already pointed out, we can't compromise security by having all
>> > > > apps unrestricted access to the cycle counter. We already have the
>> > > > Linux perf subsystem for managing counters so apps should always use
>> > > > Linux perf syscalls.
>> > > >
>> > > > IMO, the package "highway" directly accessing the cycle counter should
>> > > > be fixed instead of fixing the Linux SBI PMU driver.
>> > > >
>> > > > Further investigating the highway project
>> > > > (https://github.com/google/highway), it seems this project is using
>> > > > "rdcycle" to track timer ticks which is totally wrong. Instead the use
>> > > > of "rdcycle" should be replaced with "rdtime" in this project.
>> > > > (Refer, line 156 of
>> > > > https://github.com/google/highway/blob/master/hwy/nanobenchmark.cc)
>> > >
>> > > Pay attention that quite a few projects are using rdcycle in user-space already:
>> > >
>> > > * https://codesearch.debian.net/search?q=%22rdcycle+%250%22
>> > >
>> > > Anyway if you believe rdtime is the right fix, we can fix one project
>> > > at a time..."
>> > >
>> > > Thanks for sharing the list. I took a quick look. The first few ones at least  (firefox, llvm, chrome)
>> > > seems to use rdcycle for timestamp. All of the packages use "rdtsc" for x86.
>> > > The equivalent instruction in RISC-V should be rdtime not rdcycle.
>> >
>> > [CC-ing highway main dev]
>> >
>> > Here is his comment on the rdtime patch proposal:
>> >
>> > > For benchmarking we really would prefer a cycle count, rather than "real time clock ticks", for which the spec makes no guarantees. If/when those turn out to be millisecond-resolution, they are not very useful.
>>
>> The RISC-V spec does not make any guarantee about granularity of the
>> cycle counter as well. In fact, on systems with cpufreq enabled the
>> rate of cycle counter increment will vary at runtime.
>>
>> On other hand, the time CSR (i.e. rdtime) will always increment at a
>> fixed platform specific rate which can be read from
>> "/proc/device-tree/cpus/timebase-frequency".
>>
>> Regards,
>> Anup
>>
>>
>> >
>> > _______________________________________________
>> > linux-riscv mailing list
>> > linux-riscv@lists.infradead.org
>> > http://lists.infradead.org/mailman/listinfo/linux-riscv

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: rdcycle from userland with RISCV_PMU_SBI=y

[Aurelien Jarno]

On 2022-09-02 17:34, Anup Patel wrote:
> On Fri, Sep 2, 2022 at 3:52 PM Jan Wassenberg <janwas@google.com> wrote:
> >
> > OK, but even if frequency changes, a cycle counter seems more useful for benchmarking than a low-resolution wall time.
> >
> > FYI it's not just Highway, the Google benchmark library also uses rdcycle.
> 
> Since the RISC-V ecosystem is still evolving, it is best time to fix
> things now rather
> than later.

Fair enough.

> Currently, only "time" CSR is available to user-space and all other performance
> counters should be enabled (or accessed) using Linux perf syscalls.

This is more complex in practice. For a recent kernel, the availability
of the "cycle" CSR depends on the PMU driver being used or compiled in
the kernel. For instance on both QEMU and a Polarfire Icicle board, the
legacy PMU is getting used, so rdcycle is available:

[   34.420435] Legacy PMU implementation is available

In the meantime, I upgraded QEMU and with it OpenSBI, so the SBI PMU is
used instead, and I should also update it on the Polarfire board, but it
is not so easy. Not that for QEMU, the value of scounteren is ignored,
so that rdcycle is still available.

We should make that consistent so that all users see the same behavior.

Regards
Aurelien

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: rdcycle from userland with RISCV_PMU_SBI=y

[Jan Wassenberg]

> Currently, only "time" CSR is available to user-space and all other performance
> counters should be enabled (or accessed) using Linux perf syscalls.
Unfortunately that would seem to prevent precisely measuring very small code
regions (order of 10 cycles). What is the corresponding advantage/benefit?
As mentioned, there is no security gain from merely disabling rdcycle.

> In the future, someone can always propose a RISC-V ISA extension to obfuscate
> "time" CSR values visible to user-space but the "cycle" counter is
> purely a performance counter and accurately reflects cycles taken by the CPU.
Yes, this is why we use it. I'm curious why we think rdcycle is being
used to measure wall time?

About consistency: I understand there will be a time when rdcycle may
or may not be available on Linux depending on PMU.
But there is also bare metal, and other OSes. Is there a way for
userland to detect whether rdcycle will fault?

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: rdcycle from userland with RISCV_PMU_SBI=y

[Palmer Dabbelt]

[Sorry if this a mess, it's not showing up in my inbox.]

>> Currently, only "time" CSR is available to user-space and all other performance
>> counters should be enabled (or accessed) using Linux perf syscalls.
> Unfortunately that would seem to prevent precisely measuring very small code
> regions (order of 10 cycles). What is the corresponding advantage/benefit?
> As mentioned, there is no security gain from merely disabling rdcycle.
>
>> In the future, someone can always propose a RISC-V ISA extension to obfuscate
>> "time" CSR values visible to user-space but the "cycle" counter is
>> purely a performance counter and accurately reflects cycles taken by the CPU.
> Yes, this is why we use it. I'm curious why we think rdcycle is being
> used to measure wall time?
>
> About consistency: I understand there will be a time when rdcycle may
> or may not be available on Linux depending on PMU.
> But there is also bare metal, and other OSes. Is there a way for
> userland to detect whether rdcycle will fault?

We can't just trap rdcycle as SIGILL into userspace, it's part of the 
uABI.  I get the instruction was removed from the ISA, but that doesn't 
matter: we don't break userspace, regardless of how hard the ISA makes 
that.  

I understand there's some worries about the resolution of these counters 
on some systems, but the solution there is to maintain the uABI by 
trapping/emulating the removed instructions (and then providing some way 
for userspace to ask to turn that off, like we'll have to with a bunch 
of stuff).  That's a way's off, though, so for now we need to just stop 
breaking userspace.  There's also some proposals for sorting out the 
accuracy issues with ISA specs, but that's even farther off.

I just sent 
<https://lore.kernel.org/r/20220928131807.30386-1-palmer@rivosinc.com>, 
which hopefully fixes the issue.  I ended up with COVID so I'm kind of a 
mess, I can try to find some time to test it but it'd be much 
appreciated if someone who has a setup already can do so.

Sorry for the breakage.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv