linux-kernel - Re: [PATCH v5 36/37] s390/kmsan: Implement the architecture-specific functions

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: <CAG_fn=XcDDFBAxq_0pij0VFU7ODJ6cUKd3AqBD-EgkbfnSLJNg@mail.gmail.com>
Date: Thu, 20 Jun 2024 16:19:04 +0200
From: Alexander Potapenko <glider@...gle.com>
To: Ilya Leoshkevich <iii@...ux.ibm.com>
Cc: Alexander Gordeev <agordeev@...ux.ibm.com>, Andrew Morton <akpm@...ux-foundation.org>, 
	Christoph Lameter <cl@...ux.com>, David Rientjes <rientjes@...gle.com>, Heiko Carstens <hca@...ux.ibm.com>, 
	Joonsoo Kim <iamjoonsoo.kim@....com>, Marco Elver <elver@...gle.com>, 
	Masami Hiramatsu <mhiramat@...nel.org>, Pekka Enberg <penberg@...nel.org>, 
	Steven Rostedt <rostedt@...dmis.org>, Vasily Gorbik <gor@...ux.ibm.com>, 
	Vlastimil Babka <vbabka@...e.cz>, Christian Borntraeger <borntraeger@...ux.ibm.com>, 
	Dmitry Vyukov <dvyukov@...gle.com>, Hyeonggon Yoo <42.hyeyoo@...il.com>, kasan-dev@...glegroups.com, 
	linux-kernel@...r.kernel.org, linux-mm@...ck.org, linux-s390@...r.kernel.org, 
	linux-trace-kernel@...r.kernel.org, Mark Rutland <mark.rutland@....com>, 
	Roman Gushchin <roman.gushchin@...ux.dev>, Sven Schnelle <svens@...ux.ibm.com>
Subject: Re: [PATCH v5 36/37] s390/kmsan: Implement the architecture-specific functions

On Thu, Jun 20, 2024 at 4:18 PM Alexander Potapenko <glider@...gle.com> wrote:
>
> On Thu, Jun 20, 2024 at 3:38 PM Ilya Leoshkevich <iii@...ux.ibm.com> wrote:
> >
> > On Thu, 2024-06-20 at 11:25 +0200, Alexander Gordeev wrote:
> > > On Wed, Jun 19, 2024 at 05:44:11PM +0200, Ilya Leoshkevich wrote:
> > >
> > > Hi Ilya,
> > >
> > > > +static inline bool is_lowcore_addr(void *addr)
> > > > +{
> > > > +   return addr >= (void *)&S390_lowcore &&
> > > > +          addr < (void *)(&S390_lowcore + 1);
> > > > +}
> > > > +
> > > > +static inline void *arch_kmsan_get_meta_or_null(void *addr, bool
> > > > is_origin)
> > > > +{
> > > > +   if (is_lowcore_addr(addr)) {
> > > > +           /*
> > > > +            * Different lowcores accessed via S390_lowcore
> > > > are described
> > > > +            * by the same struct page. Resolve the prefix
> > > > manually in
> > > > +            * order to get a distinct struct page.
> > > > +            */
> > >
> > > > +           addr += (void
> > > > *)lowcore_ptr[raw_smp_processor_id()] -
> > > > +                   (void *)&S390_lowcore;
> > >
> > > If I am not mistaken neither raw_smp_processor_id() itself, nor
> > > lowcore_ptr[raw_smp_processor_id()] are atomic. Should the preemption
> > > be disabled while the addr is calculated?
> > >
> > > But then the question arises - how meaningful the returned value is?
> > > AFAICT kmsan_get_metadata() is called from a preemptable context.
> > > So if the CPU is changed - how useful the previous CPU lowcore meta
> > > is?
> >
> > This code path will only be triggered by instrumented code that
> > accesses lowcore. That code is supposed to disable preemption;
> > if it didn't, it's a bug in that code and it should be fixed there.
> >
> > >
> > > Is it a memory block that needs to be ignored instead?
> > >
> > > > +           if (WARN_ON_ONCE(is_lowcore_addr(addr)))
> > > > +                   return NULL;
> > >
> > > lowcore_ptr[] pointing into S390_lowcore is rather a bug.
> >
> > Right, but AFAIK BUG() calls are discouraged. I guess in a debug tool
> > the rules are more relaxed, but we can recover from this condition here
> > easily, that's why I still went for WARN_ON_ONCE().
>
> We have KMSAN_WARN_ON() for that, sorry for not pointing it out
> earlier: https://elixir.bootlin.com/linux/latest/source/mm/kmsan/kmsan.h#L46

Apart from that:

Reviewed-by: Alexander Potapenko <glider@...gle.com>