Received-SPF: pass (google.com: domain of linux-kernel+bounces-215323-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240611133229.527822-1-nogikh@google.com> <CA+fCnZdfB206Bjw=MAkZ9qbKUtf-KeGrrqJnOJ1ZrgH6fGXRhA@mail.gmail.com>
In-Reply-To: <CA+fCnZdfB206Bjw=MAkZ9qbKUtf-KeGrrqJnOJ1ZrgH6fGXRhA@mail.gmail.com>
From: Aleksandr Nogikh <nogikh@google.com>
Date: Fri, 14 Jun 2024 19:13:56 +0200
Message-ID: <CANp29Y6x4Xx-a8z1DhR1NYh9SMuv1ikV1x=JXR5sKUFiqH6w8g@mail.gmail.com>
Subject: Re: [PATCH] kcov: don't lose track of remote references during softirqs
To: Andrey Konovalov <andreyknvl@gmail.com>
Cc: dvyukov@google.com, arnd@arndb.de, akpm@linux-foundation.org, 
	elver@google.com, glider@google.com, syzkaller@googlegroups.com, 
	kasan-dev@googlegroups.com, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, Jun 14, 2024 at 1:02=E2=80=AFAM Andrey Konovalov <andreyknvl@gmail.=
com> wrote:
>
> On Tue, Jun 11, 2024 at 3:32=E2=80=AFPM Aleksandr Nogikh <nogikh@google.c=
om> wrote:
> >
> > In kcov_remote_start()/kcov_remote_stop(), we swap the previous KCOV
> > metadata of the current task into a per-CPU variable. However, the
> > kcov_mode_enabled(mode) check is not sufficient in the case of remote
> > KCOV coverage: current->kcov_mode always remains KCOV_MODE_DISABLED
> > for remote KCOV objects.
> >
> > If the original task that has invoked the KCOV_REMOTE_ENABLE ioctl
> > happens to get interrupted and kcov_remote_start() is called, it
> > ultimately leads to kcov_remote_stop() NOT restoring the original
> > KCOV reference. So when the task exits, all registered remote KCOV
> > handles remain active forever.
> >
> > Fix it by introducing a special kcov_mode that is assigned to the
> > task that owns a KCOV remote object. It makes kcov_mode_enabled()
> > return true and yet does not trigger coverage collection in
> > __sanitizer_cov_trace_pc() and write_comp_data().
> >
> > Signed-off-by: Aleksandr Nogikh <nogikh@google.com>
> > Fixes: 5ff3b30ab57d ("kcov: collect coverage from interrupts")
> > ---
> >  include/linux/kcov.h | 2 ++
> >  kernel/kcov.c        | 1 +
> >  2 files changed, 3 insertions(+)
> >
> > diff --git a/include/linux/kcov.h b/include/linux/kcov.h
> > index b851ba415e03..3b479a3d235a 100644
> > --- a/include/linux/kcov.h
> > +++ b/include/linux/kcov.h
> > @@ -21,6 +21,8 @@ enum kcov_mode {
> >         KCOV_MODE_TRACE_PC =3D 2,
> >         /* Collecting comparison operands mode. */
> >         KCOV_MODE_TRACE_CMP =3D 3,
> > +       /* The process owns a KCOV remote reference. */
> > +       KCOV_MODE_REMOTE =3D 4,
> >  };
> >
> >  #define KCOV_IN_CTXSW  (1 << 30)
> > diff --git a/kernel/kcov.c b/kernel/kcov.c
> > index c3124f6d5536..5371d3f7b5c3 100644
> > --- a/kernel/kcov.c
> > +++ b/kernel/kcov.c
> > @@ -632,6 +632,7 @@ static int kcov_ioctl_locked(struct kcov *kcov, uns=
igned int cmd,
> >                         return -EINVAL;
> >                 kcov->mode =3D mode;
> >                 t->kcov =3D kcov;
> > +               WRITE_ONCE(t->kcov_mode, KCOV_MODE_REMOTE);
>
> Looking at this again, I don't think we need this WRITE_ONCE here, as
> we have interrupts disabled. But if we do, perhaps it makes sense to
> add a comment explaining why.

Thank you!
I've sent a v2:
https://lore.kernel.org/all/20240614171221.2837584-1-nogikh@google.com/

>
> >                 kcov->t =3D t;
> >                 kcov->remote =3D true;
> >                 kcov->remote_size =3D remote_arg->area_size;
> > --
> > 2.45.2.505.gda0bf45e8d-goog
> >
>