linux-kernel - Re: [PATCHv5 4/4] asm-generic/io: Add logging support for MMIO accessors

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAK8P3a1k-1_m7r-u0uO1nW1m43bt_hR9u+UeW=SqK40+Ltb+iA@mail.gmail.com>
Date:   Mon, 6 Dec 2021 10:09:57 +0100
From:   Arnd Bergmann <arnd@...db.de>
To:     Sai Prakash Ranjan <quic_saipraka@...cinc.com>
Cc:     Will Deacon <will@...nel.org>,
        Catalin Marinas <catalin.marinas@....com>,
        Arnd Bergmann <arnd@...db.de>,
        Steven Rostedt <rostedt@...dmis.org>,
        Marc Zyngier <maz@...nel.org>,
        gregkh <gregkh@...uxfoundation.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        quic_psodagud@...cinc.com
Subject: Re: [PATCHv5 4/4] asm-generic/io: Add logging support for MMIO accessors

On Mon, Dec 6, 2021 at 9:28 AM Sai Prakash Ranjan
<quic_saipraka@...cinc.com> wrote:
> +#if IS_ENABLED(CONFIG_TRACE_MMIO_ACCESS) && !(defined(__DISABLE_TRACE_MMIO__))
> +#include <linux/tracepoint-defs.h>
> +
> +DECLARE_TRACEPOINT(rwmmio_write);
> +DECLARE_TRACEPOINT(rwmmio_read);
> +
> +#define rwmmio_tracepoint_active(t) tracepoint_enabled(t)
> +void log_write_mmio(u64 val, u8 width, volatile void __iomem *addr);
> +void log_read_mmio(u8 width, const volatile void __iomem *addr);
> +
> +#else
> +
> +#define rwmmio_tracepoint_active(t) false
> +static inline void log_write_mmio(u64 val, u8 width, volatile void __iomem *addr) {}
> +static inline void log_read_mmio(u8 width, const volatile void __iomem *addr) {}
> +
> +#endif /* CONFIG_TRACE_MMIO_ACCESS */
>
>  /*
>   * __raw_{read,write}{b,w,l,q}() access memory in native endianness.
> @@ -149,6 +166,8 @@ static inline u8 readb(const volatile void __iomem *addr)
>  {
>         u8 val;
>
> +       if (rwmmio_tracepoint_active(rwmmio_read))
> +               log_read_mmio(8, addr);
>         __io_br();
>         val = __raw_readb(addr);
>         __io_ar(val);

For readability, it may be nicer to fold the two lines you add for each
helper into one, such as

void __log_write_mmio(u64 val, u8 width, volatile void __iomem *addr);
#define log_write_mmio(val, widtg, addr) do { \
     if (tracepoint_enabled(rwmmio_read)) \
               __log_write_mmio((val), (width), (addr)); \
} while (0)

I wonder if it may even be better to not check for tracepoint_active() in the
inline function at all but always enter the external function when built-in.
This means we do run into the branch, but it also reduces the i-cache footprint.

For general functionality, I think it would be better to trace the returned
value from the read, but I don't know if that defeats the purpose you
are interested in, since it requires the tracing to come after the __raw_read.

         Arnd