* Re: For review: documentation of clone3() system call
From: Michael Kerrisk (man-pages) @ 2019-11-14 12:15 UTC (permalink / raw)
To: Jann Horn
Cc: mtk.manpages, Christian Brauner, Florian Weimer,
Christian Brauner, lkml, linux-man, Kees Cook, Oleg Nesterov,
Arnd Bergmann, David Howells, Pavel Emelyanov, Andrew Morton,
Adrian Reber, Andrei Vagin, Linux API, Ingo Molnar
In-Reply-To: <CAG48ez2of684J6suPZpko7JFV6hg5KQsrP0KAn8B8-C3PM9OfQ@mail.gmail.com>
Hello Jann, Christian,
On 11/11/19 3:55 PM, Jann Horn wrote:
> On Sat, Nov 9, 2019 at 9:10 AM Michael Kerrisk (man-pages)
> <mtk.manpages@gmail.com> wrote:
> [...]
>> On 11/7/19 4:19 PM, Christian Brauner wrote:
>>> On Fri, Oct 25, 2019 at 06:59:31PM +0200, Michael Kerrisk (man-pages) wrote:
> [...]
>>>> The stack argument specifies the location of the stack used by the
>>>> child process. Since the child and calling process may share mem‐
>>>> ory, it is not possible for the child process to execute in the
>>>> same stack as the calling process. The calling process must
>>>> therefore set up memory space for the child stack and pass a
>>>> pointer to this space to clone(). Stacks grow downward on all
>>>
>>> It might be a good idea to advise people to use mmap() to create a
>>> stack. The "canonical" way of doing this would usually be something like
>>>
>>> #define DEFAULT_STACK_SIZE (4 * 1024 * 1024) /* 8 MB usually on Linux */
>>> void *stack = mmap(NULL, DEFAULT_STACK_SIZE, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS | MAP_STACK, -1, 0);
>>>
>>> (Yes, the MAP_STACK is usally a noop but people should always include it
>>> in case some arch will have weird alignment requirement in which case
>>> this flag can be changed to actually do something...)
>>
>> So, I'm getting a little bit of an education here, and maybe you are
>> going to further educate me. Long ago, I added the documentation of
>> MAP_STACK to mmap(2), but I never quite connected the dots.
>>
>> However, you say MAP_STACK is *usually* a noop. As far as I can see,
>> in current kernels it is *always* a noop. And AFAICS, since it was first
>> added in 2.6.27 (2008), it has always been a noop.
>>
>> I wonder if it will always be a noop.
> [...]
>> So, my understanding from the above is that MAP_STACK was added to
>> allow a possible fix on some old architectures, should anyone decide it
>> was worth doing the work of implementing it. But so far, after 12 years,
>> no one did. It kind of looks like no one ever will (since those old
>> architectures become less and less relevant).
>>
>> So, AFAICT, while it's not wrong to tell people to use mmap(MAP_STACKED),
>> it doesn't provide any benefit (and perhaps never will), and it is a
>> more clumsy than plain old malloc().
>>
>> But, it could well be that there's something I still don't know here,
>> and I'd be interested to get further education.
>
> Not on Linux, but on OpenBSD, they do use MAP_STACK now AFAIK; this
> was announced here:
> <http://openbsd-archive.7691.n7.nabble.com/stack-register-checking-td338238.html>.
Indeed, thank you for that pointer. The OpenBSD mmap(2) manual
page also says:
MAP_STACK
Indicate that the mapping is used as a stack. This
flag must be used in combination with MAP_ANON and
MAP_PRIVATE.
> Basically they periodically check whether the userspace stack pointer
> points into a MAP_STACK region, and if not, they kill the process.
And I now see that FreeBSD also has MAP_STACK (already since FreeBSD 3.1,
by the look of things!):
MAP_STACK
MAP_STACK implies MAP_ANON, ando ffset of 0. The fd
argument must be -1 and prot must include at least
PROT_READ and PROT_WRITE.
This option creates a memory region that grows to at
most len bytes in size, starting from the stack top
and growing down. The stack top is the starting ad-
dress returned by the call, plus len bytes. The bot-
tom of the stack at maximum growth is the starting ad-
dress returned by the call.
Stacks created with MAP_STACK automatically grow.
Guards prevent inadvertent use of the regions into
which those stacks can grow without requiring mapping
the whole stack in advance.
And on DragonflyBSD:
MAP_STACK
Map the area as a stack. MAP_ANON is implied.
Offset should be 0, fd must be -1, and prot should
include at least PROT_READ and PROT_WRITE. This
option creates a memory region that grows to at
most len bytes in size, starting from the stack
top and growing down. The stack top is the start‐
ing address returned by the call, plus len bytes.
The bottom of the stack at maximum growth is the
starting address returned by the call.
The entire area is reserved from the point of view
of other mmap() calls, even if not faulted in yet.
WARNING! We currently allow MAP_STACK mappings to
provide a hint that points within an existing
MAP_STACK mapping's space, and this will succeed
as long as no page have been faulted in the area
specified, but this behavior is no longer sup‐
ported unless you also specify the MAP_TRYFIXED
flag.
Note that unless MAP_FIXED or MAP_TRYFIXED is
used, you cannot count on the returned address
matching the hint you have provided.
> So
> even if it's a no-op on Linux, it might make sense to advise people to
> use the flag to improve portability? I'm not sure if that's something
> that belongs in Linux manpages.
Actually, the Linux manual pages frequently carry such hints, so
this is a good point.
> Another reason against malloc() is that when setting up thread stacks
> in proper, reliable software, you'll probably want to place a guard
> page (in other words, a 4K PROT_NONE VMA) at the bottom of the stack
> to reliably catch stack overflows; and you probably don't want to do
> that with malloc, in particular with non-page-aligned allocations.
Ahh yes, another good point.
I've fixed the example code in the manual page to use
mmap(MAP_STACK), rather than malloc(), to allocate the stack.
Thanks,
Michael
--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
^ permalink raw reply
* Re: [PATCH 17/23] y2038: time: avoid timespec usage in settimeofday()
From: Arnd Bergmann @ 2019-11-14 11:06 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Jens Axboe, Corey Minyard, y2038 Mailman List, Linux API,
linux-kernel@vger.kernel.org, Li RongQing, David Howells,
Stephen Boyd, zhengbin, John Stultz, Al Viro, Ingo Molnar,
Christian Brauner, Deepa Dinamani
In-Reply-To: <alpine.DEB.2.21.1911132250010.2507@nanos.tec.linutronix.de>
On Wed, Nov 13, 2019 at 10:53 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Fri, 8 Nov 2019, Arnd Bergmann wrote:
> > -SYSCALL_DEFINE2(settimeofday, struct timeval __user *, tv,
> > +SYSCALL_DEFINE2(settimeofday, struct __kernel_old_timeval __user *, tv,
> > struct timezone __user *, tz)
> > {
> > struct timespec64 new_ts;
> > - struct timeval user_tv;
> > struct timezone new_tz;
> >
> > if (tv) {
> > - if (copy_from_user(&user_tv, tv, sizeof(*tv)))
> > + if (get_user(new_ts.tv_sec, &tv->tv_sec) ||
> > + get_user(new_ts.tv_nsec, &tv->tv_usec))
> > return -EFAULT;
>
> How is that supposed to be correct on a 32bit kernel?
I don't see the problem you are referring to. This should behave the
same way on a 32-bit kernel and on a 64-bit kernel, sign-extending
the tv_sec field, and copying the user tv_usec field into the
kernel tv_nsec, to be multiplied by 1000 a few lines later.
Am I missing something?
> > - if (!timeval_valid(&user_tv))
> > + if (tv->tv_usec > USEC_PER_SEC)
> > return -EINVAL;
>
> That's incomplete:
>
> static inline bool timeval_valid(const struct timeval *tv)
> {
> /* Dates before 1970 are bogus */
> if (tv->tv_sec < 0)
> return false;
>
> /* Can't have more microseconds then a second */
> if (tv->tv_usec < 0 || tv->tv_usec >= USEC_PER_SEC)
> return false;
>
> return true;
> }
My idea was to not duplicate the range check that is done
in do_sys_settimeofday64() and again in do_settimeofday64:
if (!timespec64_valid_settod(ts))
return -EINVAL;
The only check we should need in addition to this is to ensure
that passing an invalid tv_usec number doesn't become an
unexpectedly valid tv_nsec after the multiplication.
I agree the patch looks like I'm missing a check here, but
the code after the patch appears clear enough to me.
Arnd
_______________________________________________
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038
^ permalink raw reply
* Re: [PATCH v15 3/9] namei: LOOKUP_NO_XDEV: block mountpoint crossing
From: Al Viro @ 2019-11-14 5:43 UTC (permalink / raw)
To: Aleksa Sarai
Cc: Jeff Layton, J. Bruce Fields, Arnd Bergmann, David Howells,
Shuah Khan, Shuah Khan, Ingo Molnar, Peter Zijlstra,
Christian Brauner, David Drysdale, Andy Lutomirski,
Linus Torvalds, Eric Biederman, Andrew Morton, Alexei Starovoitov,
Kees Cook, Jann Horn, Tycho Andersen, Chanho Min, Oleg Nesterov,
Ras
In-Reply-To: <20191114044945.ldedzjrb4s7i7irr@yavin.dot.cyphar.com>
On Thu, Nov 14, 2019 at 03:49:45PM +1100, Aleksa Sarai wrote:
> On 2019-11-13, Al Viro <viro@zeniv.linux.org.uk> wrote:
> > On Tue, Nov 05, 2019 at 08:05:47PM +1100, Aleksa Sarai wrote:
> >
> > > @@ -862,6 +870,8 @@ static int nd_jump_root(struct nameidata *nd)
> > > void nd_jump_link(struct path *path)
> > > {
> > > struct nameidata *nd = current->nameidata;
> > > +
> > > + nd->last_magiclink.same_mnt = (nd->path.mnt == path->mnt);
> > > path_put(&nd->path);
> > >
> > > nd->path = *path;
> > > @@ -1082,6 +1092,10 @@ const char *get_link(struct nameidata *nd)
> > > if (nd->flags & LOOKUP_MAGICLINK_JUMPED) {
> > > if (unlikely(nd->flags & LOOKUP_NO_MAGICLINKS))
> > > return ERR_PTR(-ELOOP);
> > > + if (unlikely(nd->flags & LOOKUP_NO_XDEV)) {
> > > + if (!nd->last_magiclink.same_mnt)
> > > + return ERR_PTR(-EXDEV);
> > > + }
> > > }
> >
> > Ugh... Wouldn't it be better to take that logics (some equivalent thereof)
> > into nd_jump_link()? Or just have nd_jump_link() return an error...
>
> This could be done, but the reason for stashing it away in
> last_magiclink is because of the future magic-link re-opening patches
> which can't be implemented like that without putting the open_flags
> inside nameidata (which was decided to be too ugly a while ago).
>
> My point being that I could implement it this way for this series, but
> I'd have to implement something like last_magiclink when I end up
> re-posting the magic-link stuff in a few weeks.
>
> Looking at all the nd_jump_link() users, the other option is to just
> disallow magic-link crossings entirely for LOOKUP_NO_XDEV. The only
> thing allowing them permits is to resolve file descriptors that are
> pointing to the same procfs mount -- and it's unclear to me how useful
> that really is (apparmorfs and nsfs will always give -EXDEV because
> aafs_mnt and nsfs_mnt are internal kernel vfsmounts).
I would rather keep the entire if (nd->flags & LOOKUP_MAGICLINK_JUMPED)
out of the get_link(). If you want to generate some error if
nd_jump_link() has been called, just do it right there. The fewer
pieces of state need to be carried around, the better...
And as for opening them... Why would you need full open_flags in there?
Details, please...
^ permalink raw reply
* Re: [PATCH v15 4/9] namei: LOOKUP_BENEATH: O_BENEATH-like scoped resolution
From: Aleksa Sarai @ 2019-11-14 4:57 UTC (permalink / raw)
To: Al Viro
Cc: linux-ia64, linux-sh, Rasmus Villemoes, Alexei Starovoitov,
linux-kernel, David Howells, linux-kselftest, sparclinux,
Christian Brauner, Shuah Khan, linux-arch, linux-s390, Jiri Olsa,
Alexander Shishkin, Ingo Molnar, linux-arm-kernel, linux-mips,
linux-xtensa, Kees Cook, Arnd Bergmann, Jann Horn, linuxppc-dev,
linux-m68k, Andy
In-Reply-To: <20191113074757.5b4u5vlyx2u6pbn6@yavin.dot.cyphar.com>
[-- Attachment #1: Type: text/plain, Size: 1595 bytes --]
On 2019-11-13, Aleksa Sarai <cyphar@cyphar.com> wrote:
> On 2019-11-13, Al Viro <viro@zeniv.linux.org.uk> wrote:
> > Minor nit here - I'd split "move the conditional call of set_root()
> > into nd_jump_root()" into a separate patch before that one. Makes
> > for fewer distractions in this one. I'd probably fold "and be
> > ready for errors other than -ECHILD" into the same preliminary
> > patch.
>
> Will do.
>
> > > + /* Not currently safe for scoped-lookups. */
> > > + if (unlikely(nd->flags & LOOKUP_IS_SCOPED))
> > > + return ERR_PTR(-EXDEV);
> >
> > Also a candidate for doing in nd_jump_link()...
> >
> > > @@ -1373,8 +1403,11 @@ static int follow_dotdot_rcu(struct nameidata *nd)
> > > struct inode *inode = nd->inode;
> > >
> > > while (1) {
> > > - if (path_equal(&nd->path, &nd->root))
> > > + if (path_equal(&nd->path, &nd->root)) {
> > > + if (unlikely(nd->flags & LOOKUP_BENEATH))
> > > + return -EXDEV;
> >
> > Umm... Are you sure it's not -ECHILD?
>
> It wouldn't hurt to be -ECHILD -- though it's not clear to me how likely
> a success would be in REF-walk if the parent components didn't already
> trigger an unlazy_walk() in RCU-walk.
>
> I guess that also means LOOKUP_NO_XDEV should trigger -ECHILD in
> follow_dotdot_rcu()?
Scratch the last question -- AFAICS we don't need to do that for
LOOKUP_NO_XDEV because we check against mount_lock so it's very unlikely
that -ECHILD will have any benefit.
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH v15 3/9] namei: LOOKUP_NO_XDEV: block mountpoint crossing
From: Aleksa Sarai @ 2019-11-14 4:49 UTC (permalink / raw)
To: Al Viro
Cc: Jeff Layton, J. Bruce Fields, Arnd Bergmann, David Howells,
Shuah Khan, Shuah Khan, Ingo Molnar, Peter Zijlstra,
Christian Brauner, David Drysdale, Andy Lutomirski,
Linus Torvalds, Eric Biederman, Andrew Morton, Alexei Starovoitov,
Kees Cook, Jann Horn, Tycho Andersen, Chanho Min, Oleg Nesterov,
Ras
In-Reply-To: <20191113013630.GZ26530@ZenIV.linux.org.uk>
[-- Attachment #1: Type: text/plain, Size: 1920 bytes --]
On 2019-11-13, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Tue, Nov 05, 2019 at 08:05:47PM +1100, Aleksa Sarai wrote:
>
> > @@ -862,6 +870,8 @@ static int nd_jump_root(struct nameidata *nd)
> > void nd_jump_link(struct path *path)
> > {
> > struct nameidata *nd = current->nameidata;
> > +
> > + nd->last_magiclink.same_mnt = (nd->path.mnt == path->mnt);
> > path_put(&nd->path);
> >
> > nd->path = *path;
> > @@ -1082,6 +1092,10 @@ const char *get_link(struct nameidata *nd)
> > if (nd->flags & LOOKUP_MAGICLINK_JUMPED) {
> > if (unlikely(nd->flags & LOOKUP_NO_MAGICLINKS))
> > return ERR_PTR(-ELOOP);
> > + if (unlikely(nd->flags & LOOKUP_NO_XDEV)) {
> > + if (!nd->last_magiclink.same_mnt)
> > + return ERR_PTR(-EXDEV);
> > + }
> > }
>
> Ugh... Wouldn't it be better to take that logics (some equivalent thereof)
> into nd_jump_link()? Or just have nd_jump_link() return an error...
This could be done, but the reason for stashing it away in
last_magiclink is because of the future magic-link re-opening patches
which can't be implemented like that without putting the open_flags
inside nameidata (which was decided to be too ugly a while ago).
My point being that I could implement it this way for this series, but
I'd have to implement something like last_magiclink when I end up
re-posting the magic-link stuff in a few weeks.
Looking at all the nd_jump_link() users, the other option is to just
disallow magic-link crossings entirely for LOOKUP_NO_XDEV. The only
thing allowing them permits is to resolve file descriptors that are
pointing to the same procfs mount -- and it's unclear to me how useful
that really is (apparmorfs and nsfs will always give -EXDEV because
aafs_mnt and nsfs_mnt are internal kernel vfsmounts).
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH v24 11/12] LRNG - add SP800-90B compliant health tests
From: Stephan Müller @ 2019-11-14 1:46 UTC (permalink / raw)
To: Alexander E. Patrakov
Cc: Arnd Bergmann, Greg Kroah-Hartman, linux-crypto, LKML, Linux API,
Eric W. Biederman, Ahmed S. Darwish, Theodore Y. Ts'o,
Willy Tarreau, Matthew Garrett, Vito Caputo, Andreas Dilger,
Jan Kara, Ray Strode, William Jon McCann, zhangjs,
Andy Lutomirski, Florian Weimer, Lennart Poettering,
Nicolai Stange, "Peter, Matthias" <matthias>
In-Reply-To: <CAN_LGv3LUjJ=8ZZxaJ=c7uDPb=ayvCNPE-UshxgBUK-Jf7qNmQ@mail.gmail.com>
Am Mittwoch, 13. November 2019, 07:02:40 CET schrieb Alexander E. Patrakov:
Hi Alexander,
> ср, 13 нояб. 2019 г. в 05:38, Stephan Müller <smueller@chronox.de>:
> > Am Dienstag, 12. November 2019, 20:58:32 CET schrieb Alexander E.
> > Patrakov:
> >
> > Hi Alexander,
> >
> > > > +config LRNG_HEALTH_TESTS
> > > > + bool "Enable noise source online health tests"
> > > > + help
> > > > + The online health tests validate the noise source at
> > > > + runtime for fatal errors. These tests include SP800-90B
> > > > + compliant tests which are invoked if the system is booted
> > > > + with fips=1. In case of fatal errors during active
> > > > + SP800-90B tests, the issue is logged and the noise
> > > > + data is discarded. These tests are required for full
> > > > + compliance with SP800-90B.
> > >
> > > How have you tested that these tests work at runtime? Maybe add some
> > > code under a new CONFIG item that depends on CONFIG_BROKEN that
> > > deliberately botches the RNG and triggers failures?
> >
> > I am unable to find sensible information about CONFIG_BROKEN in the recent
> > kernel tree.
> >
> > Do you happen to have a pointer on how that option is to be used?
>
> This option is not used on its own. You create a new option,
> CONFIG_TEST_LRNG_FAILURE_MODE or something like that, and put your
> code under the ifdef guarded by that option. And then, to prevent
> ordinary users and allyesconfig from selecting it, in Kconfig you say:
> "depends on BROKEN". This way, the option becomes unselectable in
> menuconfig but still works as intended when added manually to .config
> (obviously, with the BROKEN dependency removed locally).
>
> Definition:
> https://github.com/torvalds/linux/blob/9c7db5004280767566e91a33445bf93aa479
> ef02/init/Kconfig#L68 Example usage:
> https://github.com/torvalds/linux/blob/c87237110f2553b4200a8b3401a01198edfc
> f0d9/drivers/gpu/drm/i915/Kconfig.debug#L166
I added such support to the LRNG.
Thank you for your suggestion.
>
> --
> Alexander E. Patrakov
Ciao
Stephan
^ permalink raw reply
* Re: [PATCH 12/23] y2038: syscalls: change remaining timeval to __kernel_old_timeval
From: Rafael J. Wysocki @ 2019-11-13 22:39 UTC (permalink / raw)
To: Arnd Bergmann
Cc: y2038, Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
Alexander Viro, Pavel Machek, Len Brown, John Stultz,
Thomas Gleixner, linux-kernel, Stephen Boyd, Christian Brauner,
linuxppc-dev, linux-fsdevel, linux-api, linux-pm
In-Reply-To: <20191108211323.1806194-3-arnd@arndb.de>
On Friday, November 8, 2019 10:12:11 PM CET Arnd Bergmann wrote:
> All of the remaining syscalls that pass a timeval (gettimeofday, utime,
> futimesat) can trivially be changed to pass a __kernel_old_timeval
> instead, which has a compatible layout, but avoids ambiguity with
> the timeval type in user space.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
For the change in power/power.h
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> arch/powerpc/include/asm/asm-prototypes.h | 3 ++-
> arch/powerpc/kernel/syscalls.c | 4 ++--
> fs/select.c | 10 +++++-----
> fs/utimes.c | 8 ++++----
> include/linux/syscalls.h | 10 +++++-----
> kernel/power/power.h | 2 +-
> kernel/time/time.c | 2 +-
> 7 files changed, 20 insertions(+), 19 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/asm-prototypes.h b/arch/powerpc/include/asm/asm-prototypes.h
> index 8561498e653c..2c25dc079cb9 100644
> --- a/arch/powerpc/include/asm/asm-prototypes.h
> +++ b/arch/powerpc/include/asm/asm-prototypes.h
> @@ -92,7 +92,8 @@ long sys_swapcontext(struct ucontext __user *old_ctx,
> long sys_debug_setcontext(struct ucontext __user *ctx,
> int ndbg, struct sig_dbg_op __user *dbg);
> int
> -ppc_select(int n, fd_set __user *inp, fd_set __user *outp, fd_set __user *exp, struct timeval __user *tvp);
> +ppc_select(int n, fd_set __user *inp, fd_set __user *outp, fd_set __user *exp,
> + struct __kernel_old_timeval __user *tvp);
> unsigned long __init early_init(unsigned long dt_ptr);
> void __init machine_init(u64 dt_ptr);
> #endif
> diff --git a/arch/powerpc/kernel/syscalls.c b/arch/powerpc/kernel/syscalls.c
> index 3bfb3888e897..078608ec2e92 100644
> --- a/arch/powerpc/kernel/syscalls.c
> +++ b/arch/powerpc/kernel/syscalls.c
> @@ -79,7 +79,7 @@ SYSCALL_DEFINE6(mmap, unsigned long, addr, size_t, len,
> * sys_select() with the appropriate args. -- Cort
> */
> int
> -ppc_select(int n, fd_set __user *inp, fd_set __user *outp, fd_set __user *exp, struct timeval __user *tvp)
> +ppc_select(int n, fd_set __user *inp, fd_set __user *outp, fd_set __user *exp, struct __kernel_old_timeval __user *tvp)
> {
> if ( (unsigned long)n >= 4096 )
> {
> @@ -89,7 +89,7 @@ ppc_select(int n, fd_set __user *inp, fd_set __user *outp, fd_set __user *exp, s
> || __get_user(inp, ((fd_set __user * __user *)(buffer+1)))
> || __get_user(outp, ((fd_set __user * __user *)(buffer+2)))
> || __get_user(exp, ((fd_set __user * __user *)(buffer+3)))
> - || __get_user(tvp, ((struct timeval __user * __user *)(buffer+4))))
> + || __get_user(tvp, ((struct __kernel_old_timeval __user * __user *)(buffer+4))))
> return -EFAULT;
> }
> return sys_select(n, inp, outp, exp, tvp);
> diff --git a/fs/select.c b/fs/select.c
> index 53a0c149f528..11d0285d46b7 100644
> --- a/fs/select.c
> +++ b/fs/select.c
> @@ -321,7 +321,7 @@ static int poll_select_finish(struct timespec64 *end_time,
> switch (pt_type) {
> case PT_TIMEVAL:
> {
> - struct timeval rtv;
> + struct __kernel_old_timeval rtv;
>
> if (sizeof(rtv) > sizeof(rtv.tv_sec) + sizeof(rtv.tv_usec))
> memset(&rtv, 0, sizeof(rtv));
> @@ -698,10 +698,10 @@ int core_sys_select(int n, fd_set __user *inp, fd_set __user *outp,
> }
>
> static int kern_select(int n, fd_set __user *inp, fd_set __user *outp,
> - fd_set __user *exp, struct timeval __user *tvp)
> + fd_set __user *exp, struct __kernel_old_timeval __user *tvp)
> {
> struct timespec64 end_time, *to = NULL;
> - struct timeval tv;
> + struct __kernel_old_timeval tv;
> int ret;
>
> if (tvp) {
> @@ -720,7 +720,7 @@ static int kern_select(int n, fd_set __user *inp, fd_set __user *outp,
> }
>
> SYSCALL_DEFINE5(select, int, n, fd_set __user *, inp, fd_set __user *, outp,
> - fd_set __user *, exp, struct timeval __user *, tvp)
> + fd_set __user *, exp, struct __kernel_old_timeval __user *, tvp)
> {
> return kern_select(n, inp, outp, exp, tvp);
> }
> @@ -810,7 +810,7 @@ SYSCALL_DEFINE6(pselect6_time32, int, n, fd_set __user *, inp, fd_set __user *,
> struct sel_arg_struct {
> unsigned long n;
> fd_set __user *inp, *outp, *exp;
> - struct timeval __user *tvp;
> + struct __kernel_old_timeval __user *tvp;
> };
>
> SYSCALL_DEFINE1(old_select, struct sel_arg_struct __user *, arg)
> diff --git a/fs/utimes.c b/fs/utimes.c
> index 1ba3f7883870..c952b6b3d8a0 100644
> --- a/fs/utimes.c
> +++ b/fs/utimes.c
> @@ -161,9 +161,9 @@ SYSCALL_DEFINE4(utimensat, int, dfd, const char __user *, filename,
> * utimensat() instead.
> */
> static long do_futimesat(int dfd, const char __user *filename,
> - struct timeval __user *utimes)
> + struct __kernel_old_timeval __user *utimes)
> {
> - struct timeval times[2];
> + struct __kernel_old_timeval times[2];
> struct timespec64 tstimes[2];
>
> if (utimes) {
> @@ -190,13 +190,13 @@ static long do_futimesat(int dfd, const char __user *filename,
>
>
> SYSCALL_DEFINE3(futimesat, int, dfd, const char __user *, filename,
> - struct timeval __user *, utimes)
> + struct __kernel_old_timeval __user *, utimes)
> {
> return do_futimesat(dfd, filename, utimes);
> }
>
> SYSCALL_DEFINE2(utimes, char __user *, filename,
> - struct timeval __user *, utimes)
> + struct __kernel_old_timeval __user *, utimes)
> {
> return do_futimesat(AT_FDCWD, filename, utimes);
> }
> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index 2f27bc9d5ef0..e665920fa359 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -51,7 +51,7 @@ struct statx;
> struct __sysctl_args;
> struct sysinfo;
> struct timespec;
> -struct timeval;
> +struct __kernel_old_timeval;
> struct __kernel_timex;
> struct timezone;
> struct tms;
> @@ -732,7 +732,7 @@ asmlinkage long sys_prctl(int option, unsigned long arg2, unsigned long arg3,
> asmlinkage long sys_getcpu(unsigned __user *cpu, unsigned __user *node, struct getcpu_cache __user *cache);
>
> /* kernel/time.c */
> -asmlinkage long sys_gettimeofday(struct timeval __user *tv,
> +asmlinkage long sys_gettimeofday(struct __kernel_old_timeval __user *tv,
> struct timezone __user *tz);
> asmlinkage long sys_settimeofday(struct timeval __user *tv,
> struct timezone __user *tz);
> @@ -1082,9 +1082,9 @@ asmlinkage long sys_time32(old_time32_t __user *tloc);
> asmlinkage long sys_utime(char __user *filename,
> struct utimbuf __user *times);
> asmlinkage long sys_utimes(char __user *filename,
> - struct timeval __user *utimes);
> + struct __kernel_old_timeval __user *utimes);
> asmlinkage long sys_futimesat(int dfd, const char __user *filename,
> - struct timeval __user *utimes);
> + struct __kernel_old_timeval __user *utimes);
> #endif
> asmlinkage long sys_futimesat_time32(unsigned int dfd,
> const char __user *filename,
> @@ -1098,7 +1098,7 @@ asmlinkage long sys_getdents(unsigned int fd,
> struct linux_dirent __user *dirent,
> unsigned int count);
> asmlinkage long sys_select(int n, fd_set __user *inp, fd_set __user *outp,
> - fd_set __user *exp, struct timeval __user *tvp);
> + fd_set __user *exp, struct __kernel_old_timeval __user *tvp);
> asmlinkage long sys_poll(struct pollfd __user *ufds, unsigned int nfds,
> int timeout);
> asmlinkage long sys_epoll_wait(int epfd, struct epoll_event __user *events,
> diff --git a/kernel/power/power.h b/kernel/power/power.h
> index 44bee462ff57..7cdc64dc2373 100644
> --- a/kernel/power/power.h
> +++ b/kernel/power/power.h
> @@ -179,7 +179,7 @@ extern void swsusp_close(fmode_t);
> extern int swsusp_unmark(void);
> #endif
>
> -struct timeval;
> +struct __kernel_old_timeval;
> /* kernel/power/swsusp.c */
> extern void swsusp_show_speed(ktime_t, ktime_t, unsigned int, char *);
>
> diff --git a/kernel/time/time.c b/kernel/time/time.c
> index 7eba7c9a7e3e..bc114f0be8f1 100644
> --- a/kernel/time/time.c
> +++ b/kernel/time/time.c
> @@ -137,7 +137,7 @@ SYSCALL_DEFINE1(stime32, old_time32_t __user *, tptr)
> #endif /* __ARCH_WANT_SYS_TIME32 */
> #endif
>
> -SYSCALL_DEFINE2(gettimeofday, struct timeval __user *, tv,
> +SYSCALL_DEFINE2(gettimeofday, struct __kernel_old_timeval __user *, tv,
> struct timezone __user *, tz)
> {
> if (likely(tv != NULL)) {
>
^ permalink raw reply
* Re: [PATCH 17/23] y2038: time: avoid timespec usage in settimeofday()
From: Thomas Gleixner @ 2019-11-13 21:53 UTC (permalink / raw)
To: Arnd Bergmann
Cc: y2038, John Stultz, linux-kernel, Stephen Boyd, David Howells,
Al Viro, Deepa Dinamani, Christian Brauner, Jens Axboe,
Ingo Molnar, Corey Minyard, zhengbin, Li RongQing, linux-api
In-Reply-To: <20191108211323.1806194-8-arnd@arndb.de>
On Fri, 8 Nov 2019, Arnd Bergmann wrote:
> -SYSCALL_DEFINE2(settimeofday, struct timeval __user *, tv,
> +SYSCALL_DEFINE2(settimeofday, struct __kernel_old_timeval __user *, tv,
> struct timezone __user *, tz)
> {
> struct timespec64 new_ts;
> - struct timeval user_tv;
> struct timezone new_tz;
>
> if (tv) {
> - if (copy_from_user(&user_tv, tv, sizeof(*tv)))
> + if (get_user(new_ts.tv_sec, &tv->tv_sec) ||
> + get_user(new_ts.tv_nsec, &tv->tv_usec))
> return -EFAULT;
How is that supposed to be correct on a 32bit kernel?
>
> - if (!timeval_valid(&user_tv))
> + if (tv->tv_usec > USEC_PER_SEC)
> return -EINVAL;
That's incomplete:
static inline bool timeval_valid(const struct timeval *tv)
{
/* Dates before 1970 are bogus */
if (tv->tv_sec < 0)
return false;
/* Can't have more microseconds then a second */
if (tv->tv_usec < 0 || tv->tv_usec >= USEC_PER_SEC)
return false;
return true;
}
>
> - new_ts.tv_sec = user_tv.tv_sec;
> - new_ts.tv_nsec = user_tv.tv_usec * NSEC_PER_USEC;
> + new_ts.tv_nsec *= NSEC_PER_USEC;
> }
> if (tz) {
> if (copy_from_user(&new_tz, tz, sizeof(*tz)))
> @@ -245,18 +244,17 @@ COMPAT_SYSCALL_DEFINE2(settimeofday, struct old_timeval32 __user *, tv,
> struct timezone __user *, tz)
> {
> struct timespec64 new_ts;
> - struct timeval user_tv;
> struct timezone new_tz;
>
> if (tv) {
> - if (compat_get_timeval(&user_tv, tv))
> + if (get_user(new_ts.tv_sec, &tv->tv_sec) ||
> + get_user(new_ts.tv_nsec, &tv->tv_usec))
> return -EFAULT;
>
> - if (!timeval_valid(&user_tv))
> + if (new_ts.tv_nsec > USEC_PER_SEC)
> return -EINVAL;
Ditto.
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH 00/23] y2038 cleanups
From: Arnd Bergmann @ 2019-11-13 21:40 UTC (permalink / raw)
To: y2038 Mailman List
Cc: linux-kernel@vger.kernel.org, Richard Henderson, Tony Luck,
Paul Burton, Greentime Hu, Helge Deller, Michael Ellerman,
David Miller, Thomas Gleixner, the arch/x86 maintainers,
Jeff Dike, Richard Weinberger, Al Viro, Benjamin LaHaise,
John Stultz, Stephen Boyd, Steven Rostedt, Vincenzo Frascino,
Paul Moore, Stephen Smalley
In-Reply-To: <20191108210236.1296047-1-arnd@arndb.de>
On Fri, Nov 8, 2019 at 10:04 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> This is a series of cleanups for the y2038 work, mostly intended
> for namespace cleaning: the kernel defines the traditional
> time_t, timeval and timespec types that often lead to y2038-unsafe
> code. Even though the unsafe usage is mostly gone from the kernel,
> having the types and associated functions around means that we
> can still grow new users, and that we may be missing conversions
> to safe types that actually matter.
>
> As there is no rush on any of these patches, I would either
> queue them up in linux-next through my y2038 branch, or
> Thomas could add them to the tip tree if he wants.
>
> As mentioned in another series, this is part of a larger
> effort to fix all the remaining bits and pieces that are
> not completed yet from the y2038 conversion, and the full
> set can be found at:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git/log/?h=y2038-endgame
>
> Maintainers, please review and provide Acks.
>
> Let me know if you have any opinion on whether we should do
> the include last two patches of this series or not.
>
> Arnd
>
> Arnd Bergmann (23):
> y2038: remove CONFIG_64BIT_TIME
> y2038: add __kernel_old_timespec and __kernel_old_time_t
> y2038: vdso: change timeval to __kernel_old_timeval
> y2038: vdso: change timespec to __kernel_old_timespec
> y2038: vdso: change time_t to __kernel_old_time_t
> y2038: vdso: nds32: open-code timespec_add_ns()
> y2038: vdso: powerpc: avoid timespec references
> y2038: ipc: remove __kernel_time_t reference from headers
> y2038: stat: avoid 'time_t' in 'struct stat'
> y2038: uapi: change __kernel_time_t to __kernel_old_time_t
> y2038: rusage: use __kernel_old_timeval
> y2038: syscalls: change remaining timeval to __kernel_old_timeval
> y2038: socket: remove timespec reference in timestamping
> y2038: make ns_to_compat_timeval use __kernel_old_timeval
> y2038: elfcore: Use __kernel_old_timeval for process times
> y2038: timerfd: Use timespec64 internally
> y2038: time: avoid timespec usage in settimeofday()
> y2038: itimer: compat handling to itimer.c
> y2038: use compat_{get,set}_itimer on alpha
> y2038: move itimer reset into itimer.c
> y2038: itimer: change implementation to timespec64
> [RFC] y2038: itimer: use ktime_t internally
> y2038: allow disabling time32 system calls
I've dropped the "[RFC] y2038: itimer: use ktime_t internally" patch
for the moment,
and added two other patches from other series:
y2038: remove CONFIG_64BIT_TIME
y2038: socket: use __kernel_old_timespec instead of timespec
Tentatively pushed out the patches with the Acks I have received so
far to my y2038 branch on git.kernel.org so it gets included in linux-next.
If I hear no complaints, I'll send a pull request for the merge window,
along with the compat-ioctl series I have already queued up in the same
branch.
Arnd
^ permalink raw reply
* Re: [man-pages RFC PATCH] statx.2: document STATX_ATTR_VERITY
From: Eric Biggers @ 2019-11-13 20:31 UTC (permalink / raw)
To: walter harms
Cc: linux-man, tytso, darrick.wong, linux-api, linux-f2fs-devel,
dhowells, linux-fscrypt, linux-fsdevel, jaegeuk, linux-ext4,
victorhsieh
In-Reply-To: <5DC714DB.9060007@bfs.de>
On Sat, Nov 09, 2019 at 08:34:51PM +0100, walter harms wrote:
> Am 08.11.2019 20:35, schrieb Eric Biggers:
> > On Fri, Nov 08, 2019 at 09:23:04AM +0100, walter harms wrote:
> >>
> >>
> >> Am 07.11.2019 23:02, schrieb Eric Biggers:
> >>> From: Eric Biggers <ebiggers@google.com>
> >>>
> >>> Document the verity attribute for statx().
> >>>
> >>> Signed-off-by: Eric Biggers <ebiggers@google.com>
> >>> ---
> >>> man2/statx.2 | 4 ++++
> >>> 1 file changed, 4 insertions(+)
> >>>
> >>> RFC since the kernel patches are currently under review.
> >>> The kernel patches can be found here:
> >>> https://lkml.kernel.org/linux-fscrypt/20191029204141.145309-1-ebiggers@kernel.org/T/#u
> >>>
> >>> diff --git a/man2/statx.2 b/man2/statx.2
> >>> index d2f1b07b8..713bd1260 100644
> >>> --- a/man2/statx.2
> >>> +++ b/man2/statx.2
> >>> @@ -461,6 +461,10 @@ See
> >>> .TP
> >>> .B STATX_ATTR_ENCRYPTED
> >>> A key is required for the file to be encrypted by the filesystem.
> >>> +.TP
> >>> +.B STATX_ATTR_VERITY
> >>> +The file has fs-verity enabled. It cannot be written to, and all reads from it
> >>> +will be verified against a Merkle tree.
> >>
> >> Using "Merkle tree" opens a can of worm and what will happen when the methode will change ?
> >> Does it matter at all ? i would suggest "filesystem" here.
> >>
> >
> > Fundamentally, fs-verity guarantees that all data read is verified against a
> > cryptographic hash that covers the entire file. I think it will be helpful to
> > convey that here, e.g. to avoid confusion with non-cryptographic, individual
> > block checksums supported by filesystems like btrfs and zfs.
> >
> > Now, the only sane way to implement this model is with a Merkle tree, and this
> > is part of the fs-verity UAPI (via the file hash), so that's where I'm coming
> > from here. Perhaps the phrase "Merkle tree" could be interpreted too strictly,
> > though, so it would be better to emphasize the more abstract model. How about
> > the following?:
> >
> > The file has fs-verity enabled. It cannot be written to, and all reads
> > from it will be verified against a cryptographic hash that covers the
> > entire file, e.g. via a Merkle tree.
> >
>
> "feels" better,. but from a programmers perspective it is important at what level
> this is actually done. To see my point look at the line before.
> "encrypted by the filesystem" mean i have to read the documentation of the fs first
> so if encryption is supported at all. Or do i think to complicated ?
>
It's filesystem-specific whether encryption and verity are supported. I'm not
sure what your concern is, as statx() won't return the bits if the filesystem
doesn't support them.
Also note, if someone really wants the details about fscrypt and fsverity, they
really should read the documentation we maintain in the kernel tree [1][2].
[1] https://www.kernel.org/doc/html/latest/filesystems/fscrypt.html
[2] https://www.kernel.org/doc/html/latest/filesystems/fsverity.html
- Eric
^ permalink raw reply
* Re: [PATCH 0/4] statx: expose the fs-verity bit
From: Eric Biggers @ 2019-11-13 20:20 UTC (permalink / raw)
To: linux-fscrypt
Cc: Theodore Ts'o, linux-api, linux-f2fs-devel, David Howells,
linux-fsdevel, Jaegeuk Kim, linux-ext4, Victor Hsieh
In-Reply-To: <20191029204141.145309-1-ebiggers@kernel.org>
On Tue, Oct 29, 2019 at 01:41:37PM -0700, Eric Biggers wrote:
> This patchset exposes the verity bit (a.k.a. FS_VERITY_FL) via statx().
>
> This is useful because it allows applications to check whether a file is
> a verity file without opening it. Opening a verity file can be
> expensive because the fsverity_info is set up on open, which involves
> parsing metadata and optionally verifying a cryptographic signature.
>
> This is analogous to how various other bits are exposed through both
> FS_IOC_GETFLAGS and statx(), e.g. the encrypt bit.
>
> This patchset applies to v5.4-rc5.
>
> Eric Biggers (4):
> statx: define STATX_ATTR_VERITY
> ext4: support STATX_ATTR_VERITY
> f2fs: support STATX_ATTR_VERITY
> docs: fs-verity: mention statx() support
>
> Documentation/filesystems/fsverity.rst | 8 ++++++++
> fs/ext4/inode.c | 5 ++++-
> fs/f2fs/file.c | 5 ++++-
> include/linux/stat.h | 3 ++-
> include/uapi/linux/stat.h | 2 +-
> 5 files changed, 19 insertions(+), 4 deletions(-)
Applied to fscrypt.git#fsverity for 5.5.
- Eric
^ permalink raw reply
* Re: [PATCH] Allow restricting permissions in /proc/sys
From: Jann Horn @ 2019-11-13 16:40 UTC (permalink / raw)
To: Topi Miettinen
Cc: Luis Chamberlain, Kees Cook, Alexey Dobriyan,
linux-kernel@vger.kernel.org,
open list:FILESYSTEMS (VFS and infrastructure), Linux API,
Christian Brauner
In-Reply-To: <13bc7935-8341-bb49-74ea-2eb58f72fd1f@gmail.com>
On Wed, Nov 13, 2019 at 5:19 PM Topi Miettinen <toiwoton@gmail.com> wrote:
> On 13.11.2019 18.00, Jann Horn wrote:
> > On Wed, Nov 13, 2019 at 12:22 AM Christian Brauner
> > <christian.brauner@ubuntu.com> wrote:
> >> On Sun, Nov 03, 2019 at 04:55:48PM +0200, Topi Miettinen wrote:
> >>> Several items in /proc/sys need not be accessible to unprivileged
> >>> tasks. Let the system administrator change the permissions, but only
> >>> to more restrictive modes than what the sysctl tables allow.
[...]
> > In kernel/ucount.c, the ->permissions handler set_permissions() grants
> > access based on whether the caller has CAP_SYS_RESOURCE. And in
> > net/sysctl_net.c, the handler net_ctl_permissions() grants access
> > based on whether the caller has CAP_NET_ADMIN. This added check is
> > going to break those, right?
> >
>
> Right. The comment above seems then a bit misleading:
> /*
> * sysctl entries that are not writeable,
> * are _NOT_ writeable, capabilities or not.
> */
I don't see the problem. Those handlers never make a file writable
that doesn't have one of the three write bits (0222) set.
^ permalink raw reply
* Re: [PATCH] Allow restricting permissions in /proc/sys
From: Topi Miettinen @ 2019-11-13 16:19 UTC (permalink / raw)
To: Jann Horn
Cc: Luis Chamberlain, Kees Cook, Alexey Dobriyan,
linux-kernel@vger.kernel.org,
open list:FILESYSTEMS (VFS and infrastructure), Linux API,
Christian Brauner
In-Reply-To: <CAG48ez0j_7NyCyvGn8U8NS2p=CQQb=me-5KTa7k5E6xpHJphaA@mail.gmail.com>
On 13.11.2019 18.00, Jann Horn wrote:
> On Wed, Nov 13, 2019 at 12:22 AM Christian Brauner
> <christian.brauner@ubuntu.com> wrote:
>> On Sun, Nov 03, 2019 at 04:55:48PM +0200, Topi Miettinen wrote:
>>> Several items in /proc/sys need not be accessible to unprivileged
>>> tasks. Let the system administrator change the permissions, but only
>>> to more restrictive modes than what the sysctl tables allow.
>>>
>>> Signed-off-by: Topi Miettinen <toiwoton@gmail.com>
>>> ---
>>> fs/proc/proc_sysctl.c | 41 +++++++++++++++++++++++++++++++----------
>>> 1 file changed, 31 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
>>> index d80989b6c344..88c4ca7d2782 100644
>>> --- a/fs/proc/proc_sysctl.c
>>> +++ b/fs/proc/proc_sysctl.c
>>> @@ -818,6 +818,10 @@ static int proc_sys_permission(struct inode *inode, int
>>> mask)
>>> if ((mask & MAY_EXEC) && S_ISREG(inode->i_mode))
>>> return -EACCES;
>>>
>>> + error = generic_permission(inode, mask);
>>> + if (error)
>>> + return error;
>
> In kernel/ucount.c, the ->permissions handler set_permissions() grants
> access based on whether the caller has CAP_SYS_RESOURCE. And in
> net/sysctl_net.c, the handler net_ctl_permissions() grants access
> based on whether the caller has CAP_NET_ADMIN. This added check is
> going to break those, right?
>
Right. The comment above seems then a bit misleading:
/*
* sysctl entries that are not writeable,
* are _NOT_ writeable, capabilities or not.
*/
-Topi
^ permalink raw reply
* Re: [PATCH] Allow restricting permissions in /proc/sys
From: Jann Horn @ 2019-11-13 16:00 UTC (permalink / raw)
To: Topi Miettinen
Cc: Luis Chamberlain, Kees Cook, Alexey Dobriyan,
linux-kernel@vger.kernel.org,
open list:FILESYSTEMS (VFS and infrastructure), Linux API,
Christian Brauner
In-Reply-To: <20191112232239.yevpeemgxz4wy32b@wittgenstein>
On Wed, Nov 13, 2019 at 12:22 AM Christian Brauner
<christian.brauner@ubuntu.com> wrote:
> On Sun, Nov 03, 2019 at 04:55:48PM +0200, Topi Miettinen wrote:
> > Several items in /proc/sys need not be accessible to unprivileged
> > tasks. Let the system administrator change the permissions, but only
> > to more restrictive modes than what the sysctl tables allow.
> >
> > Signed-off-by: Topi Miettinen <toiwoton@gmail.com>
> > ---
> > fs/proc/proc_sysctl.c | 41 +++++++++++++++++++++++++++++++----------
> > 1 file changed, 31 insertions(+), 10 deletions(-)
> >
> > diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
> > index d80989b6c344..88c4ca7d2782 100644
> > --- a/fs/proc/proc_sysctl.c
> > +++ b/fs/proc/proc_sysctl.c
> > @@ -818,6 +818,10 @@ static int proc_sys_permission(struct inode *inode, int
> > mask)
> > if ((mask & MAY_EXEC) && S_ISREG(inode->i_mode))
> > return -EACCES;
> >
> > + error = generic_permission(inode, mask);
> > + if (error)
> > + return error;
In kernel/ucount.c, the ->permissions handler set_permissions() grants
access based on whether the caller has CAP_SYS_RESOURCE. And in
net/sysctl_net.c, the handler net_ctl_permissions() grants access
based on whether the caller has CAP_NET_ADMIN. This added check is
going to break those, right?
^ permalink raw reply
* Re: [PATCH v4 2/2] pidfd: add tests for NSpid info in fdinfo
From: Christian Brauner @ 2019-11-13 12:20 UTC (permalink / raw)
To: Naresh Kamboju
Cc: Christian Kellner, open list, linux-api, Jann Horn,
Christian Kellner, Christian Brauner, Shuah Khan, Andrew Morton,
Peter Zijlstra (Intel), Ingo Molnar, Michal Hocko,
Thomas Gleixner, Elena Reshetova, Roman Gushchin,
Andrea Arcangeli, Al Viro, Aleksa Sarai, Dmitry V. Levin,
open list:KERNEL SELFTEST FRAMEWORK, joel
In-Reply-To: <CA+G9fYu=Z+mZs7+571PbChODV2drUYrxkdWEb7=XqkK2O3_Tyw@mail.gmail.com>
On Wed, Nov 13, 2019 at 05:22:33PM +0530, Naresh Kamboju wrote:
> Hi Christian,
Hi Naresh,
[+Cc Joel since this is _not related_ to the fdinfo patches but rather
the polling tests]
Thanks for following up here. See for more comments below.
>
> On Tue, 15 Oct 2019 at 15:37, Christian Brauner
> <christian.brauner@ubuntu.com> wrote:
> >
> > On Mon, Oct 14, 2019 at 06:20:33PM +0200, Christian Kellner wrote:
> > > From: Christian Kellner <christian@kellner.me>
> > >
> > > Add a test that checks that if pid namespaces are configured the fdinfo
> > > file of a pidfd contains an NSpid: entry containing the process id in
> > > the current and additionally all nested namespaces. In the case that
> > > a pidfd is from a pid namespace not in the same namespace hierarchy as
> > > the process accessing the fdinfo file, ensure the 'NSpid' shows 0 for
> > > that pidfd, analogous to the 'Pid' entry.
> > >
> > > Signed-off-by: Christian Kellner <christian@kellner.me>
> >
> > That looks reasonable to me.
>
> on arm64 Juno-r2, Hikey (hi6220) and dragonboard 410c and arm32
> Beagleboard x15 test pidfd_test failed.
> and on x86_64 and i386 test fails intermittently with TIMEOUT error.
> Test code is being used from linux next tree.
>
> Juno-r2 test output:
> --------------------------
> # selftests pidfd pidfd_test
> pidfd: pidfd_test_ #
> # TAP version 13
> version: 13_ #
> # 1..4
> : _ #
> # # Parent pid 10586
> Parent: pid_10586 #
> # # Parent Waiting for Child (10587) to complete.
> Parent: Waiting_for #
> # # Time waited for child 0
> Time: waited_for #
> # Bail out! pidfd_poll check for premature notification on child
> thread exec test Failed
> out!: pidfd_poll_check #
> # # Planned tests != run tests (4 != 0)
> Planned: tests_!= #
> # # Pass 0 Fail 0 Xfail 0 Xpass 0 Skip 0 Error 0
> Pass: 0_Fail #
> [FAIL] 1 selftests pidfd pidfd_test # exit=1
> selftests: pidfd_pidfd_test [FAIL]
>
> arm32 x15 output log,
> -----------------------------
> # selftests pidfd pidfd_test
> pidfd: pidfd_test_ #
> [FAIL] 1 selftests pidfd pidfd_test # TIMEOUT
> selftests: pidfd_pidfd_test [FAIL]
>
> x86_64 output log,
> -------------------------
> # selftests pidfd pidfd_test
> pidfd: pidfd_test_ #
> [FAIL] 1 selftests pidfd pidfd_test # TIMEOUT
> selftests: pidfd_pidfd_test [FAIL]
>
> Test results comparison,
> https://qa-reports.linaro.org/lkft/linux-mainline-oe/tests/kselftest/pidfd_pidfd_test
> https://qa-reports.linaro.org/lkft/linux-next-oe/tests/kselftest/pidfd_pidfd_test
>
> link,
> https://lkft.validation.linaro.org/scheduler/job/993549#L17835
Note, that this failure is _not_ related to the fdinfo and NSpid patches
here.
It's rather related to the polling testing code that Joel added. Iirc,
then it is timing sensitive.
I'll try to make some room this week to look into this.
Christian
^ permalink raw reply
* Re: [PATCH v24 00/12] /dev/random - a new approach with full SP800-90B compliance
From: Stephan Müller @ 2019-11-13 12:16 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Arnd Bergmann, Greg Kroah-Hartman, Linux Crypto Mailing List,
LKML, Linux API, Eric W. Biederman, Alexander E. Patrakov,
Ahmed S. Darwish, Theodore Y. Ts'o, Willy Tarreau,
Matthew Garrett, Vito Caputo, Andreas Dilger, Jan Kara,
Ray Strode, William Jon McCann, zhangjs, Florian Weimer,
Lennart Poettering, Nicolai Stange <nstang>
In-Reply-To: <CALCETrVHdauN2ptZLSYAUDm=S3OGkxq=iH4qGxCuH0XCMKjGkQ@mail.gmail.com>
Am Mittwoch, 13. November 2019, 05:48:30 CET schrieb Andy Lutomirski:
Hi Andy,
>
> > - 6a26a3146e5fb90878dca9fde8caa1ca4233156a: My handler for /dev/urandom
> > and
> > getrandom(..., 0) are using one callback which issues a warning in both
> > use
> > cases (see lrng_sdrng_read). So I think this patch may not be applicable
> > as
> > the LRNG code implements warning about being unseeded.
>
> Probably true.
>
> What is the actual semantics of /dev/urandom with your series applied?
> Is there any situation in which it will block?
The LRNG tries to provide a 100% identical user interface to the existing /
dev/random:
- /dev/urandom never blocks
- getrandom(..., 0) blocks until the LRNG has received 256 bits of entropy
(i.e. the LRNG is fully seeded)
Yet, both may issue a warning if CONFIG_WARN_ALL_UNSEEDED_RANDOM is set.
>
> > - 3e8e159da49b44ae0bb08e68fa2be760722fa033: I am happy to take that code
> > which would almost directly apply. The last hunk however would be:
> >
> > if (!(flags & GRND_INSECURE) && unlikely(!lrng_state_operational())) {
> >
> > ==> Shall I apply it to my code base? If yes, how shall the changes to
> > random.h be handled?
>
> This might be a question for Ted. Once the merge window opens, I'll
> resubmit it.
Ok, I will keep it out of the LRNG for now, but once your patch is merged, I
would integrate it.
>
> > - 920e97e7fc508e6f0da9c7dec94c8073fd63ab4d: I would pass on this patch due
> > to the following: it unconditionally starts removing the access to the
> > TRNG (the LRNG's logical equivalent to the blocking_pool). As patch 10/12
> > of the LRNG patch series provides the TRNG that is a compile time option,
> > your patch would logically and functionally be equivalent when
> > deselecting
> > CONFIG_LRNG_TRNG_SUPPORT in the LRNG without any further changes to the
> > LRNG code.
>
> Given your previous email about the TRNG, I'm wondering what the API
> for the TRNG should be. I am willing to grant that there are users
> who need a TRNG for various reasons, and that not all of them can use
> hwrng. (And the current hwrng API is pretty bad.) But I'm not
> convinced that /dev/random or getrandom(..., GRND_RANDOM) is a
> reasonable way to access it. A blocking_pool-style TRNG is a very
> limited resource, and I think it could make sense to require some sort
> of actual permission to use it. GRND_RANDOM has no access control at
> all, and everyone expects /dev/random to be world-readable. The most
> widespread user of /dev/random that I know of is gnupg, and gnupg
> really should not be using it.
>
> Would it make sense to have a /dev/true_random that is 0400 by default
> for users who actually need it? Then /dev/random and GRND_RANDOM
> could work as they do with my patch, and maybe it does the right thing
> for everyone.
That is surely a reasonable way to do it. But I am not sure 0400 should be
applied, but rather 0440. This should allow introducing a group in user space
that processes who need the TRNG are not required to have root privilege, but
rather need to be a member of some otherwise unprivileged group.
>
> > - 693b9ffdf0fdc93456b5ad293ac05edf240a531b: This patch is applicable to
> > the
> > LRNG. In case CONFIG_LRNG_TRNG_SUPPORT is not set, the TRNG is not
> > present.
> > Yet, the /dev/random and getrandom(GRND_RANDOM) would behave blocked until
> > fully initialized. I have now added the general blocking until the LRNG is
> > fully initialized to the common /dev/random and getrandom(GRND_RANDOM)
> > interface function of lrng_trng_read_common. With that, the LRNG would be
> > fully equivalent to this patch if CONFIG_LRNG_TRNG_SUPPORT is not set.
>
> Sounds reasonable.
>
> > By making the TRNG compile-time selectable, I was hoping to serve all
> > users: I wanted to cover the conclusions of the discussion to remove the
> > blocking_pool. On the other hand, however, I want to support requirements
> > that need the blocking behavior.
>
> I find it odd that /dev/random would be either a TRNG or not a TRNG
> depending on kernel configuration. For the small fraction of users
> that actually want a TRNG, wouldn't it be better to have an interface
> that fails outright if the TRNG is not enabled?
Sure, I would have no concerns here.
>
> --Andy
Ciao
Stephan
^ permalink raw reply
* Re: [PATCH v4 2/2] pidfd: add tests for NSpid info in fdinfo
From: Naresh Kamboju @ 2019-11-13 11:52 UTC (permalink / raw)
To: Christian Brauner
Cc: Christian Kellner, open list, linux-api, Jann Horn,
Christian Kellner, Christian Brauner, Shuah Khan, Andrew Morton,
Peter Zijlstra (Intel), Ingo Molnar, Michal Hocko,
Thomas Gleixner, Elena Reshetova, Roman Gushchin,
Andrea Arcangeli, Al Viro, Aleksa Sarai, Dmitry V. Levin,
open list:KERNEL SELFTEST FRAMEWORK
In-Reply-To: <20191015100743.t6gowsic7c347ldv@wittgenstein>
Hi Christian,
On Tue, 15 Oct 2019 at 15:37, Christian Brauner
<christian.brauner@ubuntu.com> wrote:
>
> On Mon, Oct 14, 2019 at 06:20:33PM +0200, Christian Kellner wrote:
> > From: Christian Kellner <christian@kellner.me>
> >
> > Add a test that checks that if pid namespaces are configured the fdinfo
> > file of a pidfd contains an NSpid: entry containing the process id in
> > the current and additionally all nested namespaces. In the case that
> > a pidfd is from a pid namespace not in the same namespace hierarchy as
> > the process accessing the fdinfo file, ensure the 'NSpid' shows 0 for
> > that pidfd, analogous to the 'Pid' entry.
> >
> > Signed-off-by: Christian Kellner <christian@kellner.me>
>
> That looks reasonable to me.
on arm64 Juno-r2, Hikey (hi6220) and dragonboard 410c and arm32
Beagleboard x15 test pidfd_test failed.
and on x86_64 and i386 test fails intermittently with TIMEOUT error.
Test code is being used from linux next tree.
Juno-r2 test output:
--------------------------
# selftests pidfd pidfd_test
pidfd: pidfd_test_ #
# TAP version 13
version: 13_ #
# 1..4
: _ #
# # Parent pid 10586
Parent: pid_10586 #
# # Parent Waiting for Child (10587) to complete.
Parent: Waiting_for #
# # Time waited for child 0
Time: waited_for #
# Bail out! pidfd_poll check for premature notification on child
thread exec test Failed
out!: pidfd_poll_check #
# # Planned tests != run tests (4 != 0)
Planned: tests_!= #
# # Pass 0 Fail 0 Xfail 0 Xpass 0 Skip 0 Error 0
Pass: 0_Fail #
[FAIL] 1 selftests pidfd pidfd_test # exit=1
selftests: pidfd_pidfd_test [FAIL]
arm32 x15 output log,
-----------------------------
# selftests pidfd pidfd_test
pidfd: pidfd_test_ #
[FAIL] 1 selftests pidfd pidfd_test # TIMEOUT
selftests: pidfd_pidfd_test [FAIL]
x86_64 output log,
-------------------------
# selftests pidfd pidfd_test
pidfd: pidfd_test_ #
[FAIL] 1 selftests pidfd pidfd_test # TIMEOUT
selftests: pidfd_pidfd_test [FAIL]
Test results comparison,
https://qa-reports.linaro.org/lkft/linux-mainline-oe/tests/kselftest/pidfd_pidfd_test
https://qa-reports.linaro.org/lkft/linux-next-oe/tests/kselftest/pidfd_pidfd_test
link,
https://lkft.validation.linaro.org/scheduler/job/993549#L17835
- Naresh
^ permalink raw reply
* Re: [PATCH] Allow restricting permissions in /proc/sys
From: Topi Miettinen @ 2019-11-13 10:52 UTC (permalink / raw)
To: Andy Lutomirski, Christian Brauner
Cc: Luis Chamberlain, Kees Cook, Alexey Dobriyan,
linux-kernel@vger.kernel.org,
open list:FILESYSTEMS (VFS and infrastructure), Linux API,
Andrew Morton
In-Reply-To: <CALCETrUEQMdugz1t6HfK5MvDq_kOw13yuF+98euqVJgZ4WR1VA@mail.gmail.com>
On 13.11.2019 6.50, Andy Lutomirski wrote:
> On Tue, Nov 12, 2019 at 3:22 PM Christian Brauner
> <christian.brauner@ubuntu.com> wrote:
>>
>> [Cc+ linux-api@vger.kernel.org]
>>
>> since that's potentially relevant to quite a few people.
>>
>> On Sun, Nov 03, 2019 at 04:55:48PM +0200, Topi Miettinen wrote:
>>> Several items in /proc/sys need not be accessible to unprivileged
>>> tasks. Let the system administrator change the permissions, but only
>>> to more restrictive modes than what the sysctl tables allow.
>>>
>>> Signed-off-by: Topi Miettinen <toiwoton@gmail.com>
>>> ---
>>> fs/proc/proc_sysctl.c | 41 +++++++++++++++++++++++++++++++----------
>>> 1 file changed, 31 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
>>> index d80989b6c344..88c4ca7d2782 100644
>>> --- a/fs/proc/proc_sysctl.c
>>> +++ b/fs/proc/proc_sysctl.c
>>> @@ -818,6 +818,10 @@ static int proc_sys_permission(struct inode *inode, int
>>> mask)
>>> if ((mask & MAY_EXEC) && S_ISREG(inode->i_mode))
>>> return -EACCES;
>>>
>>> + error = generic_permission(inode, mask);
>>> + if (error)
>>> + return error;
>>> +
>>> head = grab_header(inode);
>>> if (IS_ERR(head))
>>> return PTR_ERR(head);
>>> @@ -837,9 +841,35 @@ static int proc_sys_setattr(struct dentry *dentry,
>>> struct iattr *attr)
>>> struct inode *inode = d_inode(dentry);
>>> int error;
>>>
>>> - if (attr->ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID))
>>> + if (attr->ia_valid & (ATTR_UID | ATTR_GID))
>>> return -EPERM;
>
> Supporting at least ATTR_GID would make this much more useful.
Yes, also XATTR/ACL support would be useful. But so far I've tried to
allow only tightening of permissions.
>>>
>>> + if (attr->ia_valid & ATTR_MODE) {
>>> + struct ctl_table_header *head = grab_header(inode);
>>> + struct ctl_table *table = PROC_I(inode)->sysctl_entry;
>>> + umode_t max_mode = 0777; /* Only these bits may change */
>>> +
>>> + if (IS_ERR(head))
>>> + return PTR_ERR(head);
>>> +
>>> + if (!table) /* global root - r-xr-xr-x */
>>> + max_mode &= ~0222;
>>> + else /*
>>> + * Don't allow permissions to become less
>>> + * restrictive than the sysctl table entry
>>> + */
>>> + max_mode &= table->mode;
>
> Style nit: please put braces around multi-line if and else branches,
> even if they're only multi-line because of comments.
OK, thanks.
>>> +
>>> + sysctl_head_finish(head);
>>> +
>>> + /* Execute bits only allowed for directories */
>>> + if (!S_ISDIR(inode->i_mode))
>>> + max_mode &= ~0111;
>
> Why is this needed?
>
In general, /proc/sys does not allow executable permissions for the
files, so I've continued this policy.
-Topi
^ permalink raw reply
* Re: [PATCH] proc: Allow restricting permissions in /proc/sys
From: Topi Miettinen @ 2019-11-13 10:44 UTC (permalink / raw)
To: Luis Chamberlain
Cc: Kees Cook, Alexey Dobriyan, linux-kernel@vger.kernel.org,
open list:FILESYSTEMS (VFS and infrastructure), Eric W. Biederman,
Andrew Morton, linux-api
In-Reply-To: <20191113003524.GQ11244@42.do-not-panic.com>
On 13.11.2019 2.35, Luis Chamberlain wrote:
> On Mon, Nov 04, 2019 at 02:07:29PM +0200, Topi Miettinen wrote:
>> Several items in /proc/sys need not be accessible to unprivileged
>> tasks. Let the system administrator change the permissions, but only
>> to more restrictive modes than what the sysctl tables allow.
>
> Thanks for taking the time for looking into this!
>
> We don't get many eyeballs over this code, so while you're at it, if its
> not too much trouble and since it seems you care: can you list proc sys
> files which are glaring red flags to have their current defaults
> permissions?
I'm not aware if there are any problems with the defaults. It's just
that the defaults make so many files available to unprivileged tasks,
when in reality only a few of the files seem to be really needed or useful.
For example, going through the few Debian Code Search hits for
/proc/sys/debug [1], it seems to me that the default could as well be
0500 for the directory without breaking anything.
1: https://codesearch.debian.net/search?q=%2Fproc%2Fsys%2Fdebug&literal=1
>> Signed-off-by: Topi Miettinen <toiwoton@gmail.com>
>> ---
>> v2: actually keep track of changed permissions instead of relying on inode
>> cache
>> ---
>> fs/proc/proc_sysctl.c | 42 ++++++++++++++++++++++++++++++++++++++----
>> include/linux/sysctl.h | 1 +
>> 2 files changed, 39 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
>> index d80989b6c344..1f75382c49fd 100644
>> --- a/fs/proc/proc_sysctl.c
>> +++ b/fs/proc/proc_sysctl.c
>> @@ -818,6 +818,10 @@ static int proc_sys_permission(struct inode *inode, int
>> mask)
>> if ((mask & MAY_EXEC) && S_ISREG(inode->i_mode))
>> return -EACCES;
>>
>> + error = generic_permission(inode, mask);
>> + if (error)
>> + return error;
>> +
>
> This alone checks to see if the inode's uid and gid are mapped to the
> current namespace, amonst other things. A worthy change in and of
> itself, worthy of it being a separate patch.
OK, will separate.
> Can it regress current uses? Well depends if namespaces exists today
> where root is not mapped to other namespaces, and if that was *expected*
> to work.
>
>> head = grab_header(inode);
>> if (IS_ERR(head))
>> return PTR_ERR(head);
>> @@ -835,17 +839,46 @@ static int proc_sys_permission(struct inode *inode,
>> int mask)
>> static int proc_sys_setattr(struct dentry *dentry, struct iattr *attr)
>> {
>> struct inode *inode = d_inode(dentry);
>> + struct ctl_table_header *head = grab_header(inode);
>> + struct ctl_table *table = PROC_I(inode)->sysctl_entry;
>> int error;
>>
>> - if (attr->ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID))
>> + if (attr->ia_valid & (ATTR_UID | ATTR_GID))
>> return -EPERM;
>>
>> + if (attr->ia_valid & ATTR_MODE) {
>> + umode_t max_mode = 0777; /* Only these bits may change */
>> +
>> + if (IS_ERR(head))
>> + return PTR_ERR(head);
>> +
>> + if (!table) /* global root - r-xr-xr-x */
>> + max_mode &= ~0222;
>
> max_mode &= root->permissions(head, table) ?
Currently, writing is not allowed by default. For /proc/sys/net and
/proc/sys/user, which grant write access to suitably ns_capable tasks, I
think this would allow those tasks also to change the mode to world
writable. So far, I've tried to allow only tightening of permissions.
> But why are we setting this? More in context below.
>
>> + else /*
>> + * Don't allow permissions to become less
>> + * restrictive than the sysctl table entry
>> + */
>> + max_mode &= table->mode;
>> +
>> + /* Execute bits only allowed for directories */
>> + if (!S_ISDIR(inode->i_mode))
>> + max_mode &= ~0111;
>> +
>> + if (attr->ia_mode & ~S_IFMT & ~max_mode)
>
> Shouldn't this error path call sysctl_head_finish(head) ?
Right, will fix.
>> + return -EPERM;
>> + }
>> +
>> error = setattr_prepare(dentry, attr);
>> if (error)
>> return error;
>>
>> setattr_copy(inode, attr);
>> mark_inode_dirty(inode);
>> +
>> + if (table)
>> + table->current_mode = inode->i_mode;
>
> Here we only care about setting this current_mode if the
> table is set is present, but above we did some processing
> when it was not set. Why?
The processing above when there was no table was to ensure that there is
some default (0444 for files, 0555 for directories). Here we store the
changed mode to table, if it is present.
Though if there's no table, the change would remain only in the inode
cache, so using the table for backing storage for the mode looks now to
me as a bad idea. Perhaps struct proc_dir_entry should be used instead.
>> + sysctl_head_finish(head);
>> +
>> return 0;
>> }
>>
>> @@ -861,7 +894,7 @@ static int proc_sys_getattr(const struct path *path,
>> struct kstat *stat,
>>
>> generic_fillattr(inode, stat);
>> if (table)
>> - stat->mode = (stat->mode & S_IFMT) | table->mode;
>> + stat->mode = (stat->mode & S_IFMT) | table->current_mode;
>>
>> sysctl_head_finish(head);
>> return 0;
>> @@ -981,7 +1014,7 @@ static struct ctl_dir *new_dir(struct ctl_table_set
>> *set,
>> memcpy(new_name, name, namelen);
>> new_name[namelen] = '\0';
>> table[0].procname = new_name;
>> - table[0].mode = S_IFDIR|S_IRUGO|S_IXUGO;
>> + table[0].current_mode = table[0].mode = S_IFDIR|S_IRUGO|S_IXUGO;
>> init_header(&new->header, set->dir.header.root, set, node, table);
>>
>> return new;
>> @@ -1155,6 +1188,7 @@ static int sysctl_check_table(const char *path, struct
>> ctl_table *table)
>> if ((table->mode & (S_IRUGO|S_IWUGO)) != table->mode)
>> err |= sysctl_err(path, table, "bogus .mode 0%o",
>> table->mode);
>> + table->current_mode = table->mode;
>> }
>> return err;
>> }
>> @@ -1192,7 +1226,7 @@ static struct ctl_table_header *new_links(struct
>> ctl_dir *dir, struct ctl_table
>> int len = strlen(entry->procname) + 1;
>> memcpy(link_name, entry->procname, len);
>> link->procname = link_name;
>> - link->mode = S_IFLNK|S_IRWXUGO;
>> + link->current_mode = link->mode = S_IFLNK|S_IRWXUGO;
>> link->data = link_root;
>> link_name += len;
>> }
>> diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
>> index 6df477329b76..7c519c35bf9c 100644
>> --- a/include/linux/sysctl.h
>> +++ b/include/linux/sysctl.h
>> @@ -126,6 +126,7 @@ struct ctl_table
>> void *data;
>> int maxlen;
>> umode_t mode;
>> + umode_t current_mode;
>
> Please add kdoc, I know we don't have one, but we have to start, and
> explain at least that mode is the original intended settings, and that
> current_mode can only be stricter settings.
OK, if this remains instead of using proc_dir_entry for storing the mode.
> Also, I see your patch does a good sanity test on the input mask
> and returns it back, howevever, I don't see how proc_sys_permission()
> is using it?
It's not, but the inode mode is checked by generic_permission() added by
the patch.
-Topi
^ permalink raw reply
* Re: [PATCH v15 6/9] namei: LOOKUP_{IN_ROOT,BENEATH}: permit limited ".." resolution
From: Aleksa Sarai @ 2019-11-13 7:52 UTC (permalink / raw)
To: Al Viro
Cc: Jeff Layton, J. Bruce Fields, Arnd Bergmann, David Howells,
Shuah Khan, Shuah Khan, Ingo Molnar, Peter Zijlstra,
Christian Brauner, Jann Horn, Linus Torvalds, Eric Biederman,
Andy Lutomirski, Andrew Morton, Alexei Starovoitov, Kees Cook,
Tycho Andersen, David Drysdale, Chanho Min, Oleg Nesterov
In-Reply-To: <20191113020917.GC26530@ZenIV.linux.org.uk>
[-- Attachment #1: Type: text/plain, Size: 1005 bytes --]
On 2019-11-13, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Tue, Nov 05, 2019 at 08:05:50PM +1100, Aleksa Sarai wrote:
>
> > One other possible alternative (which previous versions of this patch
> > used) would be to check with path_is_under() if there was a racing
> > rename or mount (after re-taking the relevant seqlocks). While this does
> > work, it results in possible O(n*m) behaviour if there are many renames
> > or mounts occuring *anywhere on the system*.
>
> BTW, do you realize that open-by-fhandle (or working nfsd, for that matter)
> will trigger arseloads of write_seqlock(&rename_lock) simply on d_splice_alias()
> bringing disconnected subtrees in contact with parent?
I wasn't aware of that -- that makes path_is_under() even less viable.
I'll reword it to be clearer that path_is_under() isn't a good idea and
why we went with -EAGAIN over an in-kernel retry.
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH v15 4/9] namei: LOOKUP_BENEATH: O_BENEATH-like scoped resolution
From: Aleksa Sarai @ 2019-11-13 7:47 UTC (permalink / raw)
To: Al Viro
Cc: Jeff Layton, J. Bruce Fields, Arnd Bergmann, David Howells,
Shuah Khan, Shuah Khan, Ingo Molnar, Peter Zijlstra,
Christian Brauner, David Drysdale, Andy Lutomirski,
Linus Torvalds, Eric Biederman, Andrew Morton, Alexei Starovoitov,
Kees Cook, Jann Horn, Tycho Andersen, Chanho Min, Oleg Nesterov,
Ras
In-Reply-To: <20191113015534.GA26530@ZenIV.linux.org.uk>
[-- Attachment #1: Type: text/plain, Size: 1296 bytes --]
On 2019-11-13, Al Viro <viro@zeniv.linux.org.uk> wrote:
> Minor nit here - I'd split "move the conditional call of set_root()
> into nd_jump_root()" into a separate patch before that one. Makes
> for fewer distractions in this one. I'd probably fold "and be
> ready for errors other than -ECHILD" into the same preliminary
> patch.
Will do.
> > + /* Not currently safe for scoped-lookups. */
> > + if (unlikely(nd->flags & LOOKUP_IS_SCOPED))
> > + return ERR_PTR(-EXDEV);
>
> Also a candidate for doing in nd_jump_link()...
>
> > @@ -1373,8 +1403,11 @@ static int follow_dotdot_rcu(struct nameidata *nd)
> > struct inode *inode = nd->inode;
> >
> > while (1) {
> > - if (path_equal(&nd->path, &nd->root))
> > + if (path_equal(&nd->path, &nd->root)) {
> > + if (unlikely(nd->flags & LOOKUP_BENEATH))
> > + return -EXDEV;
>
> Umm... Are you sure it's not -ECHILD?
It wouldn't hurt to be -ECHILD -- though it's not clear to me how likely
a success would be in REF-walk if the parent components didn't already
trigger an unlazy_walk() in RCU-walk.
I guess that also means LOOKUP_NO_XDEV should trigger -ECHILD in
follow_dotdot_rcu()?
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH v24 11/12] LRNG - add SP800-90B compliant health tests
From: Alexander E. Patrakov @ 2019-11-13 6:02 UTC (permalink / raw)
To: Stephan Müller
Cc: Arnd Bergmann, Greg Kroah-Hartman, linux-crypto, LKML, Linux API,
Eric W. Biederman, Ahmed S. Darwish, Theodore Y. Ts'o,
Willy Tarreau, Matthew Garrett, Vito Caputo, Andreas Dilger,
Jan Kara, Ray Strode, William Jon McCann, zhangjs,
Andy Lutomirski, Florian Weimer, Lennart Poettering,
Nicolai Stange, "Peter, Matthias" <matthias>
In-Reply-To: <9152597.fJySsU3eCD@positron.chronox.de>
ср, 13 нояб. 2019 г. в 05:38, Stephan Müller <smueller@chronox.de>:
>
> Am Dienstag, 12. November 2019, 20:58:32 CET schrieb Alexander E. Patrakov:
>
> Hi Alexander,
>
> > > +config LRNG_HEALTH_TESTS
> > > + bool "Enable noise source online health tests"
> > > + help
> > > + The online health tests validate the noise source at
> > > + runtime for fatal errors. These tests include SP800-90B
> > > + compliant tests which are invoked if the system is booted
> > > + with fips=1. In case of fatal errors during active
> > > + SP800-90B tests, the issue is logged and the noise
> > > + data is discarded. These tests are required for full
> > > + compliance with SP800-90B.
> >
> > How have you tested that these tests work at runtime? Maybe add some
> > code under a new CONFIG item that depends on CONFIG_BROKEN that
> > deliberately botches the RNG and triggers failures?
>
>
> I am unable to find sensible information about CONFIG_BROKEN in the recent
> kernel tree.
>
> Do you happen to have a pointer on how that option is to be used?
This option is not used on its own. You create a new option,
CONFIG_TEST_LRNG_FAILURE_MODE or something like that, and put your
code under the ifdef guarded by that option. And then, to prevent
ordinary users and allyesconfig from selecting it, in Kconfig you say:
"depends on BROKEN". This way, the option becomes unselectable in
menuconfig but still works as intended when added manually to .config
(obviously, with the BROKEN dependency removed locally).
Definition: https://github.com/torvalds/linux/blob/9c7db5004280767566e91a33445bf93aa479ef02/init/Kconfig#L68
Example usage: https://github.com/torvalds/linux/blob/c87237110f2553b4200a8b3401a01198edfcf0d9/drivers/gpu/drm/i915/Kconfig.debug#L166
--
Alexander E. Patrakov
^ permalink raw reply
* [PATCH v6 2/2] fpga: dfl: fme: add performance reporting support
From: Wu Hao @ 2019-11-13 5:24 UTC (permalink / raw)
To: mdf, will, mark.rutland, linux-fpga, linux-kernel
Cc: linux-api, atull, gregkh, Wu Hao, Luwei Kang, Xu Yilun
In-Reply-To: <1573622695-25607-1-git-send-email-hao.wu@intel.com>
This patch adds support for performance reporting private feature
for FPGA Management Engine (FME). Now it supports several different
performance counters, including 'basic', 'cache', 'fabric', 'vtd'
and 'vtd_sip'. It allows user to use standard linux tools to access
these performance counters.
e.g. List all events by "perf list"
perf list | grep fme
fme0/cache_read_hit/ [Kernel PMU event]
fme0/cache_read_miss/ [Kernel PMU event]
...
fme0/fab_mmio_read/ [Kernel PMU event]
fme0/fab_mmio_write/ [Kernel PMU event]
...
fme0/fab_port_mmio_read,portid=?/ [Kernel PMU event]
fme0/fab_port_mmio_write,portid=?/ [Kernel PMU event]
...
fme0/vtd_port_devtlb_1g_fill,portid=?/ [Kernel PMU event]
fme0/vtd_port_devtlb_2m_fill,portid=?/ [Kernel PMU event]
...
fme0/vtd_sip_iotlb_1g_hit/ [Kernel PMU event]
fme0/vtd_sip_iotlb_1g_miss/ [Kernel PMU event]
...
fme0/clock [Kernel PMU event]
...
e.g. check increased counter value after run one application using
"perf stat" command.
perf stat -e fme0/fab_mmio_read/,fme0/fab_mmio_write/, ./test
Performance counter stats for './test':
1 fme0/fab_mmio_read/
2 fme0/fab_mmio_write/
1.009496520 seconds time elapsed
Please note that fabric counters support both fab_* and fab_port_*, but
actually they are sharing one set of performance counters in hardware.
If user wants to monitor overall data events on fab_* then fab_port_*
can't be supported at the same time, see example below:
perf stat -e fme0/fab_mmio_read/,fme0/fab_port_mmio_write,portid=0/
Performance counter stats for 'system wide':
0 fme0/fab_mmio_read/
<not supported> fme0/fab_port_mmio_write,portid=0/
2.141064085 seconds time elapsed
Signed-off-by: Luwei Kang <luwei.kang@intel.com>
Signed-off-by: Xu Yilun <yilun.xu@intel.com>
Signed-off-by: Wu Hao <hao.wu@intel.com>
---
v3: replace scnprintf with sprintf in sysfs interfaces.
update sysfs doc kernel version and date.
fix sysfs doc issue for fabric counter.
refine PERF_OBJ_ATTR_* macro, doesn't count on __ATTR anymore.
introduce PERF_OBJ_ATTR_F_* macro, as it needs to use different
filenames for some of the sysfs attributes.
remove kobject_del when destroy kobject, kobject_put is enough.
do sysfs_remove_groups first when destroying perf_obj.
WARN_ON_ONCE in case internal parms are wrong in read_*_count().
v4: rework this patch to use standard perf API as user interfaces.
v5: rebase and clean up.
v6: use dev_ext_attribute instead of creating new fme_perf_attribute
use is_visible function to decide which events to expose per
hardware capability, and add event_init checking for all events.
---
drivers/fpga/Makefile | 1 +
drivers/fpga/dfl-fme-main.c | 4 +
drivers/fpga/dfl-fme-perf.c | 943 ++++++++++++++++++++++++++++++++++++++++++++
drivers/fpga/dfl-fme.h | 2 +
drivers/fpga/dfl.h | 2 +
5 files changed, 952 insertions(+)
create mode 100644 drivers/fpga/dfl-fme-perf.c
diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
index 4865b74..d8e21df 100644
--- a/drivers/fpga/Makefile
+++ b/drivers/fpga/Makefile
@@ -40,6 +40,7 @@ obj-$(CONFIG_FPGA_DFL_FME_REGION) += dfl-fme-region.o
obj-$(CONFIG_FPGA_DFL_AFU) += dfl-afu.o
dfl-fme-objs := dfl-fme-main.o dfl-fme-pr.o dfl-fme-error.o
+dfl-fme-objs += dfl-fme-perf.o
dfl-afu-objs := dfl-afu-main.o dfl-afu-region.o dfl-afu-dma-region.o
dfl-afu-objs += dfl-afu-error.o
diff --git a/drivers/fpga/dfl-fme-main.c b/drivers/fpga/dfl-fme-main.c
index 7c930e6..53a1447 100644
--- a/drivers/fpga/dfl-fme-main.c
+++ b/drivers/fpga/dfl-fme-main.c
@@ -580,6 +580,10 @@ static int fme_power_mgmt_init(struct platform_device *pdev,
.ops = &fme_power_mgmt_ops,
},
{
+ .id_table = fme_perf_id_table,
+ .ops = &fme_perf_ops,
+ },
+ {
.ops = NULL,
},
};
diff --git a/drivers/fpga/dfl-fme-perf.c b/drivers/fpga/dfl-fme-perf.c
new file mode 100644
index 0000000..5e51727
--- /dev/null
+++ b/drivers/fpga/dfl-fme-perf.c
@@ -0,0 +1,943 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for FPGA Management Engine (FME) Global Performance Reporting
+ *
+ * Copyright 2019 Intel Corporation, Inc.
+ *
+ * Authors:
+ * Kang Luwei <luwei.kang@intel.com>
+ * Xiao Guangrong <guangrong.xiao@linux.intel.com>
+ * Wu Hao <hao.wu@intel.com>
+ * Xu Yilun <yilun.xu@intel.com>
+ * Joseph Grecco <joe.grecco@intel.com>
+ * Enno Luebbers <enno.luebbers@intel.com>
+ * Tim Whisonant <tim.whisonant@intel.com>
+ * Ananda Ravuri <ananda.ravuri@intel.com>
+ * Mitchel, Henry <henry.mitchel@intel.com>
+ */
+
+#include <linux/perf_event.h>
+#include "dfl.h"
+#include "dfl-fme.h"
+
+/*
+ * Performance Counter Registers for Cache.
+ *
+ * Cache Events are listed below as CACHE_EVNT_*.
+ */
+#define CACHE_CTRL 0x8
+#define CACHE_RESET_CNTR BIT_ULL(0)
+#define CACHE_FREEZE_CNTR BIT_ULL(8)
+#define CACHE_CTRL_EVNT GENMASK_ULL(19, 16)
+#define CACHE_EVNT_RD_HIT 0x0
+#define CACHE_EVNT_WR_HIT 0x1
+#define CACHE_EVNT_RD_MISS 0x2
+#define CACHE_EVNT_WR_MISS 0x3
+#define CACHE_EVNT_RSVD 0x4
+#define CACHE_EVNT_HOLD_REQ 0x5
+#define CACHE_EVNT_DATA_WR_PORT_CONTEN 0x6
+#define CACHE_EVNT_TAG_WR_PORT_CONTEN 0x7
+#define CACHE_EVNT_TX_REQ_STALL 0x8
+#define CACHE_EVNT_RX_REQ_STALL 0x9
+#define CACHE_EVNT_EVICTIONS 0xa
+#define CACHE_EVNT_MAX CACHE_EVNT_EVICTIONS
+#define CACHE_CHANNEL_SEL BIT_ULL(20)
+#define CACHE_CHANNEL_RD 0
+#define CACHE_CHANNEL_WR 1
+#define CACHE_CNTR0 0x10
+#define CACHE_CNTR1 0x18
+#define CACHE_CNTR_EVNT_CNTR GENMASK_ULL(47, 0)
+#define CACHE_CNTR_EVNT GENMASK_ULL(63, 60)
+
+/*
+ * Performance Counter Registers for Fabric.
+ *
+ * Fabric Events are listed below as FAB_EVNT_*
+ */
+#define FAB_CTRL 0x20
+#define FAB_RESET_CNTR BIT_ULL(0)
+#define FAB_FREEZE_CNTR BIT_ULL(8)
+#define FAB_CTRL_EVNT GENMASK_ULL(19, 16)
+#define FAB_EVNT_PCIE0_RD 0x0
+#define FAB_EVNT_PCIE0_WR 0x1
+#define FAB_EVNT_PCIE1_RD 0x2
+#define FAB_EVNT_PCIE1_WR 0x3
+#define FAB_EVNT_UPI_RD 0x4
+#define FAB_EVNT_UPI_WR 0x5
+#define FAB_EVNT_MMIO_RD 0x6
+#define FAB_EVNT_MMIO_WR 0x7
+#define FAB_EVNT_MAX FAB_EVNT_MMIO_WR
+#define FAB_PORT_ID GENMASK_ULL(21, 20)
+#define FAB_PORT_FILTER BIT_ULL(23)
+#define FAB_PORT_FILTER_DISABLE 0
+#define FAB_PORT_FILTER_ENABLE 1
+#define FAB_CNTR 0x28
+#define FAB_CNTR_EVNT_CNTR GENMASK_ULL(59, 0)
+#define FAB_CNTR_EVNT GENMASK_ULL(63, 60)
+
+/*
+ * Performance Counter Registers for Clock.
+ *
+ * Clock Counter can't be reset or frozen by SW.
+ */
+#define CLK_CNTR 0x30
+#define BASIC_EVNT_CLK 0x0
+#define BASIC_EVNT_MAX BASIC_EVNT_CLK
+
+/*
+ * Performance Counter Registers for IOMMU / VT-D.
+ *
+ * VT-D Events are listed below as VTD_EVNT_* and VTD_SIP_EVNT_*
+ */
+#define VTD_CTRL 0x38
+#define VTD_RESET_CNTR BIT_ULL(0)
+#define VTD_FREEZE_CNTR BIT_ULL(8)
+#define VTD_CTRL_EVNT GENMASK_ULL(19, 16)
+#define VTD_EVNT_AFU_MEM_RD_TRANS 0x0
+#define VTD_EVNT_AFU_MEM_WR_TRANS 0x1
+#define VTD_EVNT_AFU_DEVTLB_RD_HIT 0x2
+#define VTD_EVNT_AFU_DEVTLB_WR_HIT 0x3
+#define VTD_EVNT_DEVTLB_4K_FILL 0x4
+#define VTD_EVNT_DEVTLB_2M_FILL 0x5
+#define VTD_EVNT_DEVTLB_1G_FILL 0x6
+#define VTD_EVNT_MAX VTD_EVNT_DEVTLB_1G_FILL
+#define VTD_CNTR 0x40
+#define VTD_CNTR_EVNT_CNTR GENMASK_ULL(47, 0)
+#define VTD_CNTR_EVNT GENMASK_ULL(63, 60)
+
+#define VTD_SIP_CTRL 0x48
+#define VTD_SIP_RESET_CNTR BIT_ULL(0)
+#define VTD_SIP_FREEZE_CNTR BIT_ULL(8)
+#define VTD_SIP_CTRL_EVNT GENMASK_ULL(19, 16)
+#define VTD_SIP_EVNT_IOTLB_4K_HIT 0x0
+#define VTD_SIP_EVNT_IOTLB_2M_HIT 0x1
+#define VTD_SIP_EVNT_IOTLB_1G_HIT 0x2
+#define VTD_SIP_EVNT_SLPWC_L3_HIT 0x3
+#define VTD_SIP_EVNT_SLPWC_L4_HIT 0x4
+#define VTD_SIP_EVNT_RCC_HIT 0x5
+#define VTD_SIP_EVNT_IOTLB_4K_MISS 0x6
+#define VTD_SIP_EVNT_IOTLB_2M_MISS 0x7
+#define VTD_SIP_EVNT_IOTLB_1G_MISS 0x8
+#define VTD_SIP_EVNT_SLPWC_L3_MISS 0x9
+#define VTD_SIP_EVNT_SLPWC_L4_MISS 0xa
+#define VTD_SIP_EVNT_RCC_MISS 0xb
+#define VTD_SIP_EVNT_MAX VTD_SIP_EVNT_SLPWC_L4_MISS
+#define VTD_SIP_CNTR 0X50
+#define VTD_SIP_CNTR_EVNT_CNTR GENMASK_ULL(47, 0)
+#define VTD_SIP_CNTR_EVNT GENMASK_ULL(63, 60)
+
+#define PERF_TIMEOUT 30
+
+#define PERF_MAX_PORT_NUM 1U
+
+/**
+ * struct fme_perf_priv - priv data structure for fme perf driver
+ *
+ * @dev: parent device.
+ * @ioaddr: mapped base address of mmio region.
+ * @pmu: pmu data structure for fme perf counters.
+ * @id: id of this fme performance report private feature.
+ * @fab_users: current user number on fabric counters.
+ * @fab_port_id: used to indicate current working mode of fabric counters.
+ * @fab_lock: lock to protect fabric counters working mode.
+ */
+struct fme_perf_priv {
+ struct device *dev;
+ void __iomem *ioaddr;
+ struct pmu pmu;
+ u64 id;
+
+ u32 fab_users;
+ u32 fab_port_id;
+ spinlock_t fab_lock;
+};
+
+/**
+ * struct fme_perf_event_ops - callbacks for fme perf events
+ *
+ * @event_init: callback invoked during event init.
+ * @event_destroy: callback invoked during event destroy.
+ * @read_counter: callback to read hardware counters.
+ */
+struct fme_perf_event_ops {
+ int (*event_init)(struct fme_perf_priv *priv, u32 event, u32 portid);
+ void (*event_destroy)(struct fme_perf_priv *priv, u32 event,
+ u32 portid);
+ u64 (*read_counter)(struct fme_perf_priv *priv, u32 event, u32 portid);
+};
+
+#define to_fme_perf_priv(_pmu) container_of(_pmu, struct fme_perf_priv, pmu)
+
+static cpumask_t fme_perf_cpumask = CPU_MASK_CPU0;
+
+static ssize_t cpumask_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ return cpumap_print_to_pagebuf(true, buf, &fme_perf_cpumask);
+}
+static DEVICE_ATTR_RO(cpumask);
+
+static struct attribute *fme_perf_cpumask_attrs[] = {
+ &dev_attr_cpumask.attr,
+ NULL,
+};
+
+static struct attribute_group fme_perf_cpumask_group = {
+ .attrs = fme_perf_cpumask_attrs,
+};
+
+#define FME_EVENT_MASK GENMASK_ULL(11, 0)
+#define FME_EVENT_SHIFT 0
+#define FME_EVTYPE_MASK GENMASK_ULL(15, 12)
+#define FME_EVTYPE_SHIFT 12
+#define FME_EVTYPE_BASIC 0
+#define FME_EVTYPE_CACHE 1
+#define FME_EVTYPE_FABRIC 2
+#define FME_EVTYPE_VTD 3
+#define FME_EVTYPE_VTD_SIP 4
+#define FME_EVTYPE_MAX FME_EVTYPE_VTD_SIP
+#define FME_PORTID_MASK GENMASK_ULL(23, 16)
+#define FME_PORTID_SHIFT 16
+#define FME_PORTID_ROOT (0xffU)
+
+#define get_event(_config) FIELD_GET(FME_EVENT_MASK, _config)
+#define get_evtype(_config) FIELD_GET(FME_EVTYPE_MASK, _config)
+#define get_portid(_config) FIELD_GET(FME_PORTID_MASK, _config)
+
+PMU_FORMAT_ATTR(event, "config:0-11");
+PMU_FORMAT_ATTR(evtype, "config:12-15");
+PMU_FORMAT_ATTR(portid, "config:16-23");
+
+static struct attribute *fme_perf_format_attrs[] = {
+ &format_attr_event.attr,
+ &format_attr_evtype.attr,
+ &format_attr_portid.attr,
+ NULL,
+};
+
+static struct attribute_group fme_perf_format_group = {
+ .name = "format",
+ .attrs = fme_perf_format_attrs,
+};
+
+static struct attribute *fme_perf_events_attrs_empty[] = {
+ NULL,
+};
+
+static struct attribute_group fme_perf_events_group = {
+ .name = "events",
+ .attrs = fme_perf_events_attrs_empty,
+};
+
+static const struct attribute_group *fme_perf_groups[] = {
+ &fme_perf_format_group,
+ &fme_perf_cpumask_group,
+ &fme_perf_events_group,
+ NULL,
+};
+
+static bool is_portid_root(u32 portid)
+{
+ return portid == FME_PORTID_ROOT;
+}
+
+static bool is_portid_port(u32 portid)
+{
+ return portid < PERF_MAX_PORT_NUM;
+}
+
+static bool is_portid_root_or_port(u32 portid)
+{
+ return is_portid_root(portid) || is_portid_port(portid);
+}
+
+static int basic_event_init(struct fme_perf_priv *priv, u32 event, u32 portid)
+{
+ if (event <= BASIC_EVNT_MAX && is_portid_root(portid))
+ return 0;
+
+ return -EINVAL;
+}
+
+static u64 basic_read_event_counter(struct fme_perf_priv *priv,
+ u32 event, u32 portid)
+{
+ void __iomem *base = priv->ioaddr;
+
+ return readq(base + CLK_CNTR);
+}
+
+static int cache_event_init(struct fme_perf_priv *priv, u32 event, u32 portid)
+{
+ if (priv->id == FME_FEATURE_ID_GLOBAL_IPERF &&
+ event <= CACHE_EVNT_MAX && is_portid_root(portid))
+ return 0;
+
+ return -EINVAL;
+}
+
+static u64 cache_read_event_counter(struct fme_perf_priv *priv,
+ u32 event, u32 portid)
+{
+ void __iomem *base = priv->ioaddr;
+ u64 v, count;
+ u8 channel;
+
+ if (event == CACHE_EVNT_WR_HIT || event == CACHE_EVNT_WR_MISS ||
+ event == CACHE_EVNT_DATA_WR_PORT_CONTEN ||
+ event == CACHE_EVNT_TAG_WR_PORT_CONTEN)
+ channel = CACHE_CHANNEL_WR;
+ else
+ channel = CACHE_CHANNEL_RD;
+
+ /* set channel access type and cache event code. */
+ v = readq(base + CACHE_CTRL);
+ v &= ~(CACHE_CHANNEL_SEL | CACHE_CTRL_EVNT);
+ v |= FIELD_PREP(CACHE_CHANNEL_SEL, channel);
+ v |= FIELD_PREP(CACHE_CTRL_EVNT, event);
+ writeq(v, base + CACHE_CTRL);
+
+ if (readq_poll_timeout_atomic(base + CACHE_CNTR0, v,
+ FIELD_GET(CACHE_CNTR_EVNT, v) == event,
+ 1, PERF_TIMEOUT)) {
+ dev_err(priv->dev, "timeout, unmatched cache event code in counter register.\n");
+ return 0;
+ }
+
+ v = readq(base + CACHE_CNTR0);
+ count = FIELD_GET(CACHE_CNTR_EVNT_CNTR, v);
+ v = readq(base + CACHE_CNTR1);
+ count += FIELD_GET(CACHE_CNTR_EVNT_CNTR, v);
+
+ return count;
+}
+
+static bool is_fabric_event_supported(struct fme_perf_priv *priv, u32 event,
+ u32 portid)
+{
+ if (event > FAB_EVNT_MAX || !is_portid_root_or_port(portid))
+ return false;
+
+ if (priv->id == FME_FEATURE_ID_GLOBAL_DPERF &&
+ (event == FAB_EVNT_PCIE1_RD || event == FAB_EVNT_UPI_RD ||
+ event == FAB_EVNT_PCIE1_WR || event == FAB_EVNT_UPI_WR))
+ return false;
+
+ return true;
+}
+
+static int fabric_event_init(struct fme_perf_priv *priv, u32 event, u32 portid)
+{
+ void __iomem *base = priv->ioaddr;
+ int ret = 0;
+ u64 v;
+
+ if (!is_fabric_event_supported(priv, event, portid))
+ return -EINVAL;
+
+ /*
+ * as fabric counter set only can be in either overall or port mode.
+ * In overall mode, it counts overall data for FPGA, and in port mode,
+ * it is configured to monitor on one individual port.
+ *
+ * so every time, a new event is initialized, driver checks
+ * current working mode and if someone is using this counter set.
+ */
+ spin_lock(&priv->fab_lock);
+ if (priv->fab_users && priv->fab_port_id != portid) {
+ dev_dbg(priv->dev, "conflict fabric event monitoring mode.\n");
+ ret = -EOPNOTSUPP;
+ goto exit;
+ }
+
+ priv->fab_users++;
+
+ /*
+ * skip if current working mode matches, otherwise change the working
+ * mode per input port_id, to monitor overall data or another port.
+ */
+ if (priv->fab_port_id == portid)
+ goto exit;
+
+ priv->fab_port_id = portid;
+
+ v = readq(base + FAB_CTRL);
+ v &= ~(FAB_PORT_FILTER | FAB_PORT_ID);
+
+ if (is_portid_root(portid)) {
+ v |= FIELD_PREP(FAB_PORT_FILTER, FAB_PORT_FILTER_DISABLE);
+ } else {
+ v |= FIELD_PREP(FAB_PORT_FILTER, FAB_PORT_FILTER_ENABLE);
+ v |= FIELD_PREP(FAB_PORT_ID, portid);
+ }
+ writeq(v, base + FAB_CTRL);
+
+exit:
+ spin_unlock(&priv->fab_lock);
+ return ret;
+}
+
+static void fabric_event_destroy(struct fme_perf_priv *priv, u32 event,
+ u32 portid)
+{
+ spin_lock(&priv->fab_lock);
+ priv->fab_users--;
+ spin_unlock(&priv->fab_lock);
+}
+
+static u64 fabric_read_event_counter(struct fme_perf_priv *priv, u32 event,
+ u32 portid)
+{
+ void __iomem *base = priv->ioaddr;
+ u64 v;
+
+ v = readq(base + FAB_CTRL);
+ v &= ~FAB_CTRL_EVNT;
+ v |= FIELD_PREP(FAB_CTRL_EVNT, event);
+ writeq(v, base + FAB_CTRL);
+
+ if (readq_poll_timeout_atomic(base + FAB_CNTR, v,
+ FIELD_GET(FAB_CNTR_EVNT, v) == event,
+ 1, PERF_TIMEOUT)) {
+ dev_err(priv->dev, "timeout, unmatched fab event code in counter register.\n");
+ return 0;
+ }
+
+ v = readq(base + FAB_CNTR);
+ return FIELD_GET(FAB_CNTR_EVNT_CNTR, v);
+}
+
+static int vtd_event_init(struct fme_perf_priv *priv, u32 event, u32 portid)
+{
+ if (priv->id == FME_FEATURE_ID_GLOBAL_IPERF &&
+ event <= VTD_EVNT_MAX && is_portid_port(portid))
+ return 0;
+
+ return -EINVAL;
+}
+
+static u64 vtd_read_event_counter(struct fme_perf_priv *priv, u32 event,
+ u32 portid)
+{
+ void __iomem *base = priv->ioaddr;
+ u64 v;
+
+ event += (portid * (VTD_EVNT_MAX + 1));
+
+ v = readq(base + VTD_CTRL);
+ v &= ~VTD_CTRL_EVNT;
+ v |= FIELD_PREP(VTD_CTRL_EVNT, event);
+ writeq(v, base + VTD_CTRL);
+
+ if (readq_poll_timeout_atomic(base + VTD_CNTR, v,
+ FIELD_GET(VTD_CNTR_EVNT, v) == event,
+ 1, PERF_TIMEOUT)) {
+ dev_err(priv->dev, "timeout, unmatched vtd event code in counter register.\n");
+ return 0;
+ }
+
+ v = readq(base + VTD_CNTR);
+ return FIELD_GET(VTD_CNTR_EVNT_CNTR, v);
+}
+
+static int vtd_sip_event_init(struct fme_perf_priv *priv, u32 event, u32 portid)
+{
+ if (priv->id == FME_FEATURE_ID_GLOBAL_IPERF &&
+ event <= VTD_SIP_EVNT_MAX && is_portid_root(portid))
+ return 0;
+
+ return -EINVAL;
+}
+
+static u64 vtd_sip_read_event_counter(struct fme_perf_priv *priv, u32 event,
+ u32 portid)
+{
+ void __iomem *base = priv->ioaddr;
+ u64 v;
+
+ v = readq(base + VTD_SIP_CTRL);
+ v &= ~VTD_SIP_CTRL_EVNT;
+ v |= FIELD_PREP(VTD_SIP_CTRL_EVNT, event);
+ writeq(v, base + VTD_SIP_CTRL);
+
+ if (readq_poll_timeout_atomic(base + VTD_SIP_CNTR, v,
+ FIELD_GET(VTD_SIP_CNTR_EVNT, v) == event,
+ 1, PERF_TIMEOUT)) {
+ dev_err(priv->dev, "timeout, unmatched vtd sip event code in counter register\n");
+ return 0;
+ }
+
+ v = readq(base + VTD_SIP_CNTR);
+ return FIELD_GET(VTD_SIP_CNTR_EVNT_CNTR, v);
+}
+
+static struct fme_perf_event_ops fme_perf_event_ops[] = {
+ [FME_EVTYPE_BASIC] = {.event_init = basic_event_init,
+ .read_counter = basic_read_event_counter,},
+ [FME_EVTYPE_CACHE] = {.event_init = cache_event_init,
+ .read_counter = cache_read_event_counter,},
+ [FME_EVTYPE_FABRIC] = {.event_init = fabric_event_init,
+ .event_destroy = fabric_event_destroy,
+ .read_counter = fabric_read_event_counter,},
+ [FME_EVTYPE_VTD] = {.event_init = vtd_event_init,
+ .read_counter = vtd_read_event_counter,},
+ [FME_EVTYPE_VTD_SIP] = {.event_init = vtd_sip_event_init,
+ .read_counter = vtd_sip_read_event_counter,},
+};
+
+static ssize_t fme_perf_event_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct dev_ext_attribute *eattr;
+ unsigned long config;
+ char *ptr = buf;
+
+ eattr = container_of(attr, struct dev_ext_attribute, attr);
+ config = (unsigned long)eattr->var;
+
+ ptr += sprintf(ptr, "event=0x%02x", (unsigned int)get_event(config));
+ ptr += sprintf(ptr, ",evtype=0x%02x", (unsigned int)get_evtype(config));
+
+ if (is_portid_root(get_portid(config)))
+ ptr += sprintf(ptr, ",portid=0x%02x\n", FME_PORTID_ROOT);
+ else
+ ptr += sprintf(ptr, ",portid=?\n");
+
+ return (ssize_t)(ptr - buf);
+}
+
+#define FME_EVENT_ATTR(_name) \
+ __ATTR(_name, 0444, fme_perf_event_show, NULL)
+
+#define FME_PORT_EVENT_CONFIG(_event, _type) \
+ (void *)((((_event) << FME_EVENT_SHIFT) & FME_EVENT_MASK) | \
+ (((_type) << FME_EVTYPE_SHIFT) & FME_EVTYPE_MASK))
+
+#define FME_EVENT_CONFIG(_event, _type) \
+ (void *)((((_event) << FME_EVENT_SHIFT) & FME_EVENT_MASK) | \
+ (((_type) << FME_EVTYPE_SHIFT) & FME_EVTYPE_MASK) | \
+ (FME_PORTID_ROOT << FME_PORTID_SHIFT))
+
+/* FME Perf Basic Events */
+#define FME_EVENT_BASIC(_name, _event) \
+static struct dev_ext_attribute fme_perf_event_##_name = { \
+ .attr = FME_EVENT_ATTR(_name), \
+ .var = FME_EVENT_CONFIG(_event, FME_EVTYPE_BASIC), \
+}
+
+FME_EVENT_BASIC(clock, BASIC_EVNT_CLK);
+
+static struct attribute *fme_perf_basic_events_attrs[] = {
+ &fme_perf_event_clock.attr.attr,
+ NULL,
+};
+
+static const struct attribute_group fme_perf_basic_events_group = {
+ .name = "events",
+ .attrs = fme_perf_basic_events_attrs,
+};
+
+/* FME Perf Fabric Events */
+#define FME_EVENT_FABRIC(_name, _event) \
+static struct dev_ext_attribute fme_perf_event_fab_##_name = { \
+ .attr = FME_EVENT_ATTR(fab_##_name), \
+ .var = FME_EVENT_CONFIG(_event, FME_EVTYPE_FABRIC), \
+}
+
+#define FME_EVENT_FABRIC_PORT(_name, _event) \
+static struct dev_ext_attribute fme_perf_event_fab_port_##_name = { \
+ .attr = FME_EVENT_ATTR(fab_port_##_name), \
+ .var = FME_PORT_EVENT_CONFIG(_event, FME_EVTYPE_FABRIC), \
+}
+
+FME_EVENT_FABRIC(pcie0_read, FAB_EVNT_PCIE0_RD);
+FME_EVENT_FABRIC(pcie0_write, FAB_EVNT_PCIE0_WR);
+FME_EVENT_FABRIC(pcie1_read, FAB_EVNT_PCIE1_RD);
+FME_EVENT_FABRIC(pcie1_write, FAB_EVNT_PCIE1_WR);
+FME_EVENT_FABRIC(upi_read, FAB_EVNT_UPI_RD);
+FME_EVENT_FABRIC(upi_write, FAB_EVNT_UPI_WR);
+FME_EVENT_FABRIC(mmio_read, FAB_EVNT_MMIO_RD);
+FME_EVENT_FABRIC(mmio_write, FAB_EVNT_MMIO_WR);
+
+FME_EVENT_FABRIC_PORT(pcie0_read, FAB_EVNT_PCIE0_RD);
+FME_EVENT_FABRIC_PORT(pcie0_write, FAB_EVNT_PCIE0_WR);
+FME_EVENT_FABRIC_PORT(pcie1_read, FAB_EVNT_PCIE1_RD);
+FME_EVENT_FABRIC_PORT(pcie1_write, FAB_EVNT_PCIE1_WR);
+FME_EVENT_FABRIC_PORT(upi_read, FAB_EVNT_UPI_RD);
+FME_EVENT_FABRIC_PORT(upi_write, FAB_EVNT_UPI_WR);
+FME_EVENT_FABRIC_PORT(mmio_read, FAB_EVNT_MMIO_RD);
+FME_EVENT_FABRIC_PORT(mmio_write, FAB_EVNT_MMIO_WR);
+
+static struct attribute *fme_perf_fabric_events_attrs[] = {
+ &fme_perf_event_fab_pcie0_read.attr.attr,
+ &fme_perf_event_fab_pcie0_write.attr.attr,
+ &fme_perf_event_fab_pcie1_read.attr.attr,
+ &fme_perf_event_fab_pcie1_write.attr.attr,
+ &fme_perf_event_fab_upi_read.attr.attr,
+ &fme_perf_event_fab_upi_write.attr.attr,
+ &fme_perf_event_fab_mmio_read.attr.attr,
+ &fme_perf_event_fab_mmio_write.attr.attr,
+ &fme_perf_event_fab_port_pcie0_read.attr.attr,
+ &fme_perf_event_fab_port_pcie0_write.attr.attr,
+ &fme_perf_event_fab_port_pcie1_read.attr.attr,
+ &fme_perf_event_fab_port_pcie1_write.attr.attr,
+ &fme_perf_event_fab_port_upi_read.attr.attr,
+ &fme_perf_event_fab_port_upi_write.attr.attr,
+ &fme_perf_event_fab_port_mmio_read.attr.attr,
+ &fme_perf_event_fab_port_mmio_write.attr.attr,
+ NULL,
+};
+
+static umode_t fme_perf_fabric_events_visible(struct kobject *kobj,
+ struct attribute *attr, int n)
+{
+ struct pmu *pmu = dev_get_drvdata(kobj_to_dev(kobj));
+ struct fme_perf_priv *priv = to_fme_perf_priv(pmu);
+ struct dev_ext_attribute *eattr;
+ u64 var;
+
+ eattr = container_of(attr, struct dev_ext_attribute, attr.attr);
+ var = (u64)eattr->var;
+
+ if (is_fabric_event_supported(priv, get_event(var), get_portid(var)))
+ return attr->mode;
+
+ return 0;
+}
+
+static const struct attribute_group fme_perf_fabric_events_group = {
+ .name = "events",
+ .attrs = fme_perf_fabric_events_attrs,
+ .is_visible = fme_perf_fabric_events_visible,
+};
+
+/* FME Perf Cache Events */
+#define FME_EVENT_CACHE(_name, _event) \
+static struct dev_ext_attribute fme_perf_event_cache_##_name = { \
+ .attr = FME_EVENT_ATTR(cache_##_name), \
+ .var = FME_EVENT_CONFIG(_event, FME_EVTYPE_CACHE), \
+}
+
+FME_EVENT_CACHE(read_hit, CACHE_EVNT_RD_HIT);
+FME_EVENT_CACHE(read_miss, CACHE_EVNT_RD_MISS);
+FME_EVENT_CACHE(write_hit, CACHE_EVNT_WR_HIT);
+FME_EVENT_CACHE(write_miss, CACHE_EVNT_WR_MISS);
+FME_EVENT_CACHE(hold_request, CACHE_EVNT_HOLD_REQ);
+FME_EVENT_CACHE(tx_req_stall, CACHE_EVNT_TX_REQ_STALL);
+FME_EVENT_CACHE(rx_req_stall, CACHE_EVNT_RX_REQ_STALL);
+FME_EVENT_CACHE(eviction, CACHE_EVNT_EVICTIONS);
+FME_EVENT_CACHE(data_write_port_contention, CACHE_EVNT_DATA_WR_PORT_CONTEN);
+FME_EVENT_CACHE(tag_write_port_contention, CACHE_EVNT_TAG_WR_PORT_CONTEN);
+
+static struct attribute *fme_perf_cache_events_attrs[] = {
+ &fme_perf_event_cache_read_hit.attr.attr,
+ &fme_perf_event_cache_read_miss.attr.attr,
+ &fme_perf_event_cache_write_hit.attr.attr,
+ &fme_perf_event_cache_write_miss.attr.attr,
+ &fme_perf_event_cache_hold_request.attr.attr,
+ &fme_perf_event_cache_tx_req_stall.attr.attr,
+ &fme_perf_event_cache_rx_req_stall.attr.attr,
+ &fme_perf_event_cache_eviction.attr.attr,
+ &fme_perf_event_cache_data_write_port_contention.attr.attr,
+ &fme_perf_event_cache_tag_write_port_contention.attr.attr,
+ NULL,
+};
+
+static umode_t fme_perf_events_visible(struct kobject *kobj,
+ struct attribute *attr, int n)
+{
+ struct pmu *pmu = dev_get_drvdata(kobj_to_dev(kobj));
+ struct fme_perf_priv *priv = to_fme_perf_priv(pmu);
+
+ return (priv->id == FME_FEATURE_ID_GLOBAL_IPERF) ? attr->mode : 0;
+}
+
+static const struct attribute_group fme_perf_cache_events_group = {
+ .name = "events",
+ .attrs = fme_perf_cache_events_attrs,
+ .is_visible = fme_perf_events_visible,
+};
+
+/* FME Perf VTD Events */
+#define FME_EVENT_VTD_PORT(_name, _event) \
+static struct dev_ext_attribute fme_perf_event_vtd_port_##_name = { \
+ .attr = FME_EVENT_ATTR(vtd_port_##_name), \
+ .var = FME_PORT_EVENT_CONFIG(_event, FME_EVTYPE_VTD), \
+}
+
+FME_EVENT_VTD_PORT(read_transaction, VTD_EVNT_AFU_MEM_RD_TRANS);
+FME_EVENT_VTD_PORT(write_transaction, VTD_EVNT_AFU_MEM_WR_TRANS);
+FME_EVENT_VTD_PORT(devtlb_read_hit, VTD_EVNT_AFU_DEVTLB_RD_HIT);
+FME_EVENT_VTD_PORT(devtlb_write_hit, VTD_EVNT_AFU_DEVTLB_WR_HIT);
+FME_EVENT_VTD_PORT(devtlb_4k_fill, VTD_EVNT_DEVTLB_4K_FILL);
+FME_EVENT_VTD_PORT(devtlb_2m_fill, VTD_EVNT_DEVTLB_2M_FILL);
+FME_EVENT_VTD_PORT(devtlb_1g_fill, VTD_EVNT_DEVTLB_1G_FILL);
+
+static struct attribute *fme_perf_vtd_events_attrs[] = {
+ &fme_perf_event_vtd_port_read_transaction.attr.attr,
+ &fme_perf_event_vtd_port_write_transaction.attr.attr,
+ &fme_perf_event_vtd_port_devtlb_read_hit.attr.attr,
+ &fme_perf_event_vtd_port_devtlb_write_hit.attr.attr,
+ &fme_perf_event_vtd_port_devtlb_4k_fill.attr.attr,
+ &fme_perf_event_vtd_port_devtlb_2m_fill.attr.attr,
+ &fme_perf_event_vtd_port_devtlb_1g_fill.attr.attr,
+ NULL,
+};
+
+static const struct attribute_group fme_perf_vtd_events_group = {
+ .name = "events",
+ .attrs = fme_perf_vtd_events_attrs,
+ .is_visible = fme_perf_events_visible,
+};
+
+/* FME Perf VTD SIP Events */
+#define FME_EVENT_VTD_SIP(_name, _event) \
+static struct dev_ext_attribute fme_perf_event_vtd_sip_##_name = { \
+ .attr = FME_EVENT_ATTR(vtd_sip_##_name), \
+ .var = FME_EVENT_CONFIG(_event, FME_EVTYPE_VTD_SIP), \
+}
+
+FME_EVENT_VTD_SIP(iotlb_4k_hit, VTD_SIP_EVNT_IOTLB_4K_HIT);
+FME_EVENT_VTD_SIP(iotlb_2m_hit, VTD_SIP_EVNT_IOTLB_2M_HIT);
+FME_EVENT_VTD_SIP(iotlb_1g_hit, VTD_SIP_EVNT_IOTLB_1G_HIT);
+FME_EVENT_VTD_SIP(slpwc_l3_hit, VTD_SIP_EVNT_SLPWC_L3_HIT);
+FME_EVENT_VTD_SIP(slpwc_l4_hit, VTD_SIP_EVNT_SLPWC_L4_HIT);
+FME_EVENT_VTD_SIP(rcc_hit, VTD_SIP_EVNT_RCC_HIT);
+FME_EVENT_VTD_SIP(iotlb_4k_miss, VTD_SIP_EVNT_IOTLB_4K_MISS);
+FME_EVENT_VTD_SIP(iotlb_2m_miss, VTD_SIP_EVNT_IOTLB_2M_MISS);
+FME_EVENT_VTD_SIP(iotlb_1g_miss, VTD_SIP_EVNT_IOTLB_1G_MISS);
+FME_EVENT_VTD_SIP(slpwc_l3_miss, VTD_SIP_EVNT_SLPWC_L3_MISS);
+FME_EVENT_VTD_SIP(slpwc_l4_miss, VTD_SIP_EVNT_SLPWC_L4_MISS);
+FME_EVENT_VTD_SIP(rcc_miss, VTD_SIP_EVNT_RCC_MISS);
+
+static struct attribute *fme_perf_vtd_sip_events_attrs[] = {
+ &fme_perf_event_vtd_sip_iotlb_4k_hit.attr.attr,
+ &fme_perf_event_vtd_sip_iotlb_2m_hit.attr.attr,
+ &fme_perf_event_vtd_sip_iotlb_1g_hit.attr.attr,
+ &fme_perf_event_vtd_sip_slpwc_l3_hit.attr.attr,
+ &fme_perf_event_vtd_sip_slpwc_l4_hit.attr.attr,
+ &fme_perf_event_vtd_sip_rcc_hit.attr.attr,
+ &fme_perf_event_vtd_sip_iotlb_4k_miss.attr.attr,
+ &fme_perf_event_vtd_sip_iotlb_2m_miss.attr.attr,
+ &fme_perf_event_vtd_sip_iotlb_1g_miss.attr.attr,
+ &fme_perf_event_vtd_sip_slpwc_l3_miss.attr.attr,
+ &fme_perf_event_vtd_sip_slpwc_l4_miss.attr.attr,
+ &fme_perf_event_vtd_sip_rcc_miss.attr.attr,
+ NULL,
+};
+
+static const struct attribute_group fme_perf_vtd_sip_events_group = {
+ .name = "events",
+ .attrs = fme_perf_vtd_sip_events_attrs,
+ .is_visible = fme_perf_events_visible,
+};
+
+static const struct attribute_group *fme_perf_events_groups[] = {
+ &fme_perf_basic_events_group,
+ &fme_perf_cache_events_group,
+ &fme_perf_fabric_events_group,
+ &fme_perf_vtd_events_group,
+ &fme_perf_vtd_sip_events_group,
+ NULL,
+};
+
+static struct fme_perf_event_ops *get_event_ops(u32 evtype)
+{
+ if (evtype > FME_EVTYPE_MAX)
+ return NULL;
+
+ return &fme_perf_event_ops[evtype];
+}
+
+static void fme_perf_event_destroy(struct perf_event *event)
+{
+ struct fme_perf_event_ops *ops = get_event_ops(event->hw.event_base);
+ struct fme_perf_priv *priv = to_fme_perf_priv(event->pmu);
+
+ if (ops->event_destroy)
+ ops->event_destroy(priv, event->hw.idx, event->hw.config_base);
+}
+
+static int fme_perf_event_init(struct perf_event *event)
+{
+ struct fme_perf_priv *priv = to_fme_perf_priv(event->pmu);
+ struct hw_perf_event *hwc = &event->hw;
+ struct fme_perf_event_ops *ops;
+ u32 eventid, evtype, portid;
+
+ /* test the event attr type check for PMU enumeration */
+ if (event->attr.type != event->pmu->type)
+ return -ENOENT;
+
+ /*
+ * fme counters are shared across all cores.
+ * Therefore, it does not support per-process mode.
+ * Also, it does not support event sampling mode.
+ */
+ if (is_sampling_event(event) || event->attach_state & PERF_ATTACH_TASK)
+ return -EINVAL;
+
+ if (event->cpu < 0)
+ return -EINVAL;
+
+ eventid = get_event(event->attr.config);
+ portid = get_portid(event->attr.config);
+ evtype = get_evtype(event->attr.config);
+ if (evtype > FME_EVTYPE_MAX)
+ return -EINVAL;
+
+ hwc->event_base = evtype;
+ hwc->idx = (int)eventid;
+ hwc->config_base = portid;
+
+ event->destroy = fme_perf_event_destroy;
+
+ dev_dbg(priv->dev, "%s event=0x%x, evtype=0x%x, portid=0x%x,\n",
+ __func__, eventid, evtype, portid);
+
+ ops = get_event_ops(evtype);
+ if (ops->event_init)
+ return ops->event_init(priv, eventid, portid);
+
+ return 0;
+}
+
+static void fme_perf_event_update(struct perf_event *event)
+{
+ struct fme_perf_event_ops *ops = get_event_ops(event->hw.event_base);
+ struct fme_perf_priv *priv = to_fme_perf_priv(event->pmu);
+ struct hw_perf_event *hwc = &event->hw;
+ u64 now, prev, delta;
+
+ now = ops->read_counter(priv, (u32)hwc->idx, hwc->config_base);
+ prev = local64_read(&hwc->prev_count);
+ delta = now - prev;
+
+ local64_add(delta, &event->count);
+}
+
+static void fme_perf_event_start(struct perf_event *event, int flags)
+{
+ struct fme_perf_event_ops *ops = get_event_ops(event->hw.event_base);
+ struct fme_perf_priv *priv = to_fme_perf_priv(event->pmu);
+ struct hw_perf_event *hwc = &event->hw;
+ u64 count;
+
+ count = ops->read_counter(priv, (u32)hwc->idx, hwc->config_base);
+ local64_set(&hwc->prev_count, count);
+}
+
+static void fme_perf_event_stop(struct perf_event *event, int flags)
+{
+ fme_perf_event_update(event);
+}
+
+static int fme_perf_event_add(struct perf_event *event, int flags)
+{
+ if (flags & PERF_EF_START)
+ fme_perf_event_start(event, flags);
+
+ return 0;
+}
+
+static void fme_perf_event_del(struct perf_event *event, int flags)
+{
+ fme_perf_event_stop(event, PERF_EF_UPDATE);
+}
+
+static void fme_perf_event_read(struct perf_event *event)
+{
+ fme_perf_event_update(event);
+}
+
+static void fme_perf_setup_hardware(struct fme_perf_priv *priv)
+{
+ void __iomem *base = priv->ioaddr;
+ u64 v;
+
+ /* read and save current working mode for fabric counters */
+ v = readq(base + FAB_CTRL);
+
+ if (FIELD_GET(FAB_PORT_FILTER, v) == FAB_PORT_FILTER_DISABLE)
+ priv->fab_port_id = FME_PORTID_ROOT;
+ else
+ priv->fab_port_id = FIELD_GET(FAB_PORT_ID, v);
+}
+
+static int fme_perf_pmu_register(struct platform_device *pdev,
+ struct fme_perf_priv *priv)
+{
+ struct pmu *pmu = &priv->pmu;
+ char *name;
+ int ret;
+
+ spin_lock_init(&priv->fab_lock);
+
+ fme_perf_setup_hardware(priv);
+
+ pmu->task_ctx_nr = perf_invalid_context;
+ pmu->attr_groups = fme_perf_groups;
+ pmu->attr_update = fme_perf_events_groups;
+ pmu->event_init = fme_perf_event_init;
+ pmu->add = fme_perf_event_add;
+ pmu->del = fme_perf_event_del;
+ pmu->start = fme_perf_event_start;
+ pmu->stop = fme_perf_event_stop;
+ pmu->read = fme_perf_event_read;
+ pmu->capabilities = PERF_PMU_CAP_NO_INTERRUPT |
+ PERF_PMU_CAP_NO_EXCLUDE;
+
+ name = devm_kasprintf(priv->dev, GFP_KERNEL, "fme%d", pdev->id);
+
+ ret = perf_pmu_register(pmu, name, -1);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static void fme_perf_pmu_unregister(struct fme_perf_priv *priv)
+{
+ perf_pmu_unregister(&priv->pmu);
+}
+
+static int fme_perf_init(struct platform_device *pdev,
+ struct dfl_feature *feature)
+{
+ struct fme_perf_priv *priv;
+ int ret;
+
+ priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->dev = &pdev->dev;
+ priv->ioaddr = feature->ioaddr;
+ priv->id = feature->id;
+
+ ret = fme_perf_pmu_register(pdev, priv);
+ if (ret)
+ return ret;
+
+ feature->priv = priv;
+ return 0;
+}
+
+static void fme_perf_uinit(struct platform_device *pdev,
+ struct dfl_feature *feature)
+{
+ struct fme_perf_priv *priv = feature->priv;
+
+ fme_perf_pmu_unregister(priv);
+}
+
+const struct dfl_feature_id fme_perf_id_table[] = {
+ {.id = FME_FEATURE_ID_GLOBAL_IPERF,},
+ {.id = FME_FEATURE_ID_GLOBAL_DPERF,},
+ {0,}
+};
+
+const struct dfl_feature_ops fme_perf_ops = {
+ .init = fme_perf_init,
+ .uinit = fme_perf_uinit,
+};
diff --git a/drivers/fpga/dfl-fme.h b/drivers/fpga/dfl-fme.h
index 6685c8e..4195dd6 100644
--- a/drivers/fpga/dfl-fme.h
+++ b/drivers/fpga/dfl-fme.h
@@ -38,5 +38,7 @@ struct dfl_fme {
extern const struct dfl_feature_ops fme_global_err_ops;
extern const struct dfl_feature_id fme_global_err_id_table[];
extern const struct attribute_group fme_global_err_group;
+extern const struct dfl_feature_ops fme_perf_ops;
+extern const struct dfl_feature_id fme_perf_id_table[];
#endif /* __DFL_FME_H */
diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
index 9f0e656..d312678 100644
--- a/drivers/fpga/dfl.h
+++ b/drivers/fpga/dfl.h
@@ -197,12 +197,14 @@ struct dfl_feature_driver {
* feature dev (platform device)'s reources.
* @ioaddr: mapped mmio resource address.
* @ops: ops of this sub feature.
+ * @priv: priv data of this feature.
*/
struct dfl_feature {
u64 id;
int resource_index;
void __iomem *ioaddr;
const struct dfl_feature_ops *ops;
+ void *priv;
};
#define DEV_STATUS_IN_USE 0
--
1.8.3.1
^ permalink raw reply related
* [PATCH v6 1/2] Documentation: fpga: dfl: add description for performance reporting support
From: Wu Hao @ 2019-11-13 5:24 UTC (permalink / raw)
To: mdf, will, mark.rutland, linux-fpga, linux-kernel
Cc: linux-api, atull, gregkh, Xu Yilun, Wu Hao
In-Reply-To: <1573622695-25607-1-git-send-email-hao.wu@intel.com>
From: Xu Yilun <yilun.xu@intel.com>
This patch adds description for performance reporting support for
Device Feature List (DFL) based FPGA.
Signed-off-by: Xu Yilun <yilun.xu@intel.com>
Signed-off-by: Wu Hao <hao.wu@intel.com>
---
v5: rebase due to format change (txt->rst).
---
Documentation/fpga/dfl.rst | 83 ++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 83 insertions(+)
diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
index 094fc8a..85b137b 100644
--- a/Documentation/fpga/dfl.rst
+++ b/Documentation/fpga/dfl.rst
@@ -118,6 +118,11 @@ More functions are exposed through sysfs
management information (current temperature, thresholds, threshold status,
etc.).
+ Performance reporting
+ performance counters are exposed through perf PMU APIs. Standard perf tool
+ can be used to monitor all available perf events. Please see performance
+ counter section below for more detailed information.
+
FIU - PORT
==========
@@ -378,6 +383,84 @@ The device nodes used for ioctl() or mmap() can be referenced through::
/sys/class/fpga_region/<regionX>/<dfl-port.n>/dev
+Performance Counters
+====================
+Performance reporting is one private feature implemented in FME. It could
+supports several independent, system-wide, device counter sets in hardware to
+monitor and count for performance events, including "basic", "cache", "fabric",
+"vtd" and "vtd_sip" counters. Users could use standard perf tool to monitor
+FPGA cache hit/miss rate, transaction number, interface clock counter of AFU
+and other FPGA performance events.
+
+Different FPGA devices may have different counter sets, it depends on hardware
+implementation. e.g. some discrete FPGA cards don't have any cache. User could
+use "perf list" to check which perf events are supported by target hardware.
+
+In order to allow user to use standard perf API to access these performance
+counters, driver creates a perf PMU, and related sysfs interfaces in
+/sys/bus/event_source/devices/fme* to describe available perf events and
+configuration options.
+
+The "format" directory describes the format of the config field of struct
+perf_event_attr. There are 3 bitfields for config, "evtype" defines which type
+the perf event belongs to. "event" is the identity of the event within its
+category. "portid" is introduced to decide counters set to monitor on FPGA
+overall data or a specific port.
+
+The "events" directory describes the configuration templates for all available
+events which can be used with perf tool directly. For example, fab_mmio_read
+has the configuration "event=0x06,evtype=0x02,portid=0xff", which shows this
+event belongs to fabric type (0x02), the local event id is 0x06 and it is for
+overall monitoring (portid=0xff).
+
+Example usage of perf::
+
+ $# perf list |grep fme
+
+ fme0/fab_mmio_read/ [Kernel PMU event]
+ <...>
+ fme0/fab_port_mmio_read,portid=?/ [Kernel PMU event]
+ <...>
+
+ $# perf stat -a -e fme0/fab_mmio_read/ <command>
+ or
+ $# perf stat -a -e fme0/event=0x06,evtype=0x02,portid=0xff/ <command>
+ or
+ $# perf stat -a -e fme0/config=0xff2006/ <command>
+
+Another example, fab_port_mmio_read monitors mmio read of a specific port. So
+its configuration template is "event=0x06,evtype=0x01,portid=?". The portid
+should be explicitly set.
+
+Its usage of perf::
+
+ $# perf stat -a -e fme0/fab_port_mmio_read,portid=0x0/ <command>
+ or
+ $# perf stat -a -e fme0/event=0x06,evtype=0x02,portid=0x0/ <command>
+ or
+ $# perf stat -a -e fme0/config=0x2006/ <command>
+
+Please note for fabric counters, overall perf events (fab_*) and port perf
+events (fab_port_*) actually share one set of counters in hardware, so it can't
+monitor both at the same time. If this set of counters is configured to monitor
+overall data, then per port perf data is not supported. See below example::
+
+ $# perf stat -e fme0/fab_mmio_read/,fme0/fab_port_mmio_write,\
+ portid=0/ sleep 1
+
+ Performance counter stats for 'system wide':
+
+ 3 fme0/fab_mmio_read/
+ <not supported> fme0/fab_port_mmio_write,portid=0x0/
+
+ 1.001750904 seconds time elapsed
+
+The driver also provides a "cpumask" sysfs attribute, which always shows fixed
+value cpu0 as all perf events are from system-wide counters on FPGA device.
+
+The current driver does not support sampling. So "perf record" is unsupported.
+
+
Add new FIUs support
====================
It's possible that developers made some new function blocks (FIUs) under this
--
1.8.3.1
^ permalink raw reply related
* [PATCH v6 0/2] add performance reporting support to FPGA DFL drivers
From: Wu Hao @ 2019-11-13 5:24 UTC (permalink / raw)
To: mdf, will, mark.rutland, linux-fpga, linux-kernel
Cc: linux-api, atull, gregkh, Wu Hao
Hi Moritz and all,
This patchset adds performance reporting support for FPGA DFL drivers. It
introduces one pmu to expose userspace interfaces via standard perf API.
User could use standard perf tool to access perf events exposed via pmu.
This patchset is generated based on fpga/for-next branch.
Main changes from v5:
- use dev_ext_attribute instead of fme_perf_event_attr.
- use is_visible function to decide which events to expose per
hardware capability, and add event_init checking for all events.
Main changes from v4:
- rebase and clean up.
- update Kconfig for PERF_EVENTS dependency.
Main changes from v3:
- add more descriptions in doc, including how to use perf tool for these
hardware counters. (patch #1)
- use standard perf API instead of sysfs entries. (patch #2)
Wu Hao (1):
fpga: dfl: fme: add performance reporting support
Xu Yilun (1):
Documentation: fpga: dfl: add description for performance reporting
support
Documentation/fpga/dfl.rst | 83 ++++
drivers/fpga/Makefile | 1 +
drivers/fpga/dfl-fme-main.c | 4 +
drivers/fpga/dfl-fme-perf.c | 943 ++++++++++++++++++++++++++++++++++++++++++++
drivers/fpga/dfl-fme.h | 2 +
drivers/fpga/dfl.h | 2 +
6 files changed, 1035 insertions(+)
create mode 100644 drivers/fpga/dfl-fme-perf.c
--
1.8.3.1
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox