linux-kernel - Re: [PATCH 2/7] kcov: factor out struct kcov

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] [day] [month] [year] [list]

Message-ID: <CAG_fn=W9_QEhwoSwc9efY9cEFkagBTC=Q6u=wtf1rA+aJqa-Zg@mail.gmail.com>
Date: Wed, 18 Jun 2025 19:41:26 +0200
From: Alexander Potapenko <glider@...gle.com>
To: Marco Elver <elver@...gle.com>
Cc: quic_jiangenj@...cinc.com, linux-kernel@...r.kernel.org, 
	kasan-dev@...glegroups.com, Aleksandr Nogikh <nogikh@...gle.com>, 
	Andrey Konovalov <andreyknvl@...il.com>, Borislav Petkov <bp@...en8.de>, 
	Dave Hansen <dave.hansen@...ux.intel.com>, Dmitry Vyukov <dvyukov@...gle.com>, 
	Ingo Molnar <mingo@...hat.com>, Josh Poimboeuf <jpoimboe@...nel.org>, 
	Peter Zijlstra <peterz@...radead.org>, Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH 2/7] kcov: factor out struct kcov_state

> > ---
> >  MAINTAINERS                |   1 +
> >  include/linux/kcov-state.h |  31 ++++++++
>
> Looking at <linux/sched.h>, a lot of the headers introduced to factor
> out types are called "foo_types.h", so this probably should be
> "kcov_types.h".

Yeah, it makes sense, thank you!


> > +
> > +#ifdef CONFIG_KCOV
> > +struct kcov_state {
> > +       /* See kernel/kcov.c for more details. */
> > +       /*
> > +        * Coverage collection mode enabled for this task (0 if disabled).
> > +        * This field is used for synchronization, so it is kept outside of
> > +        * the below struct.
> > +        */
> > +       unsigned int mode;
> > +
>
> It'd be nice to have a comment why the below is in an anon struct "s"
Ack

> - AFAIK it's to be able to copy it around easily.
Yes, correct.

> However, thinking about it more, why does "mode" have to be in
> "kcov_state"? Does it logically make sense?

You might be right. Almost everywhere we are accessing mode and
kcov_state independently, so there isn't too much profit in keeping
them in the same struct.
Logically, they are protected by the same lock, but that lock protects
other members of struct kcov anyway.

> We also have this inconsistency where before we had the instance in
> "struct kcov" be "enum kcov_mode", and the one in task_struct be
> "unsigned int". Now they're both unsigned int - which I'm not sure is
> better.

You are right, this slipped my mind.

> Could we instead do this:
> - keep "mode" outside the struct (a bit more duplication, but I think
> it's clearer)
Ack

> - move enum kcov_mode to kcov_types.h
Ack

> - define all instances of "mode" consistently as "enum kcov_mode"

There is one tricky place where kcov_get_mode() handily returns either
an enum, or an error value. Not sure we want to change that (and the
declaration of "mode" in kcov_ioctl_locked()).
Or otherwise we could define two modes corresponding to -EINVAL and
-ENOTSUPP to preserve the existing behavior.

> - make kcov_state just contain what is now in "kcov_state::s", and
> effectively get rid of the nested "s"

Yeah, this is doable.


> > @@ -54,24 +55,16 @@ struct kcov {
> >          *  - each code section for remote coverage collection
> >          */
> >         refcount_t refcount;
> > -       /* The lock protects mode, size, area and t. */
> > +       /* The lock protects state and t. */
>
> Unlike previously, this implies it also protects "s.sequence" now.
> (Aside: as-is this will also make annotating it with __guarded_by
> rather difficult.)

As far as I can see, s.sequence is accessed under the same lock
anyway, so it is not too late to make it part of the protected state.
Or am I missing something?