Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   =?ISO-8859-1?Q?R=E9mi?= Denis-Courmont <remi@remlab.net>
To:     Atish Patra <atishp@atishpatra.org>,
        linux-riscv@lists.infradead.org
Cc:     linux-kernel@vger.kernel.org,
        Aurelien Jarno <aurelien@aurel32.net>,
        Mathieu Malaterre <malat@debian.org>,
        Jan Newger <jannewger@google.com>
Subject: Re: [PATCH v4 00/10] riscv: Allow userspace to directly access perf counters
Date:   Tue, 18 Jul 2023 20:06:36 +0300
Message-ID: <4761144.fSbbhVQpq0@basile.remlab.net>
Organization: Remlab
In-Reply-To: <CAOnJCU+ee=FXWp7mz_Tn07TbRBb3eXLFBqyRBAJbhQTO-Jxvrw@mail.gmail.com>
References: <20230703124647.215952-1-alexghiti@rivosinc.com>
 <DDB4EEB8-12A0-4E66-8E96-808E84A82078@remlab.net>
 <CAOnJCU+ee=FXWp7mz_Tn07TbRBb3eXLFBqyRBAJbhQTO-Jxvrw@mail.gmail.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8BIT
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

	Hi,

Le tiistaina 18. heinäkuuta 2023, 2.22.54 EEST Atish Patra a écrit :
> > AFAIK, if the default settings breaks user space, the patchset is
> > considered to break user space. That being the case, either this case is
> > deemed special enough that breaking user space is OK, or it is not.

> This case is a special case as the usage was incorrect in the first
> place.

I agree that it's not only insecure but also incorrect. However it mostly 
works. In fact I don't disagree with the change as such, but I think that the 
commit messages are misleading and confusing. For a start, in one place it 
says that it is not breaking user space and in another it says basically the 
opposite.

(Unfortunately, not everybody agrees with the change. I can't seem to get 
FFmpeg's checkasm tool fixed:
http://ffmpeg.org/pipermail/ffmpeg-devel/2023-July/312245.html )

Also this is not the first time somebody argues that an API should be removed 
because it's broken. That's not special.

> Any application that genuinely requires rdcycle can always get
> it now via the perf interface.

Sure. But the question is whether it breaks user space and if so, whether 
that's acceptable. Existing apps that call RDCYCLE will now fail, presumbly 
receive SIGILL(?).

> If the insecure and incorrect behavior is allowed, we suspect the user
> space behavior will never be fixed as it is hard to put a future flag
> date in these cases.

For better or worse, I can only agree there. But then adding an option to 
preserve the broken behaviour is begging for people to (ab)use it, and indeed 
never fix the problem, and never be able to remove the option.

> > If it is not OK, then the only way out that I can think of, consists of
> > trapping and emulating the counters, returning the same sanitised values
> > that Linux perf would return. Then you can add a kernel config option to
> > disable that trap-and-emulation code in the future.
> What do you mean by "sanitised" value ?

I mean whatever avoids creating a security issue. Presumably report the number 
of cycles spent in the calling thread and in user mode since the first time 
that the process called RDCYCLE?

Maybe it's not reasonable for complexity or performance reasons, but then IMO, 
it deserves a little bit better explaining in the commit message.

-- 
雷米‧德尼-库尔蒙
http://www.remlab.net/