On Wed, Aug 12, 2020 at 01:32:58PM +0200, Paolo Bonzini wrote: > On 12/08/20 13:11, peterz@xxxxxxxxxxxxx wrote: > > Right, but we want to tighten the permission checks and not excluding_hv > > is just sloppy. > > I would just document that it's ignored as it doesn't make sense. ARM64 > does that too, for new processors where the kernel is not itself split > between supervisor and hypervisor privilege levels. This isn't about x86, I want these checks in generic code. We have the flag, it needs checking. unpriv users have no busniess getting anything from a possible hv. > > The thing is, we very much do not want to allow unpriv user to be able > > to create: exclude_host=1, exclude_guest=0 counters (they currently > > can). > > That would be the case of an unprivileged user that wants to measure > performance of its guests. It's a scenario that makes a lot of sense, > are you worried about side channels? Can perf-events on guests leak > more about the host than perf-events on a random userspace program? An unpriv user can run guests? > > Also, exclude_host is really poorly defined: > > > > https://lkml.kernel.org/r/20200806091827.GY2674@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx > > > > "Suppose we have nested virt: > > > > L0-hv > > | > > G0/L1-hv > > | > > G1 > > > > And we're running in G0, then: > > > > - 'exclude_hv' would exclude L0 events > > - 'exclude_host' would ... exclude L1-hv events? > > - 'exclude_guest' would ... exclude G1 events? > > From the point of view of G0, L0 *does not exist at all*. You just > cannot see L0 events if you're running in G0. On x86, probably, in general, I'm not at all sure, we have that exclude_hv flag after all. > exclude_host/exclude_guest are the right definition. For what? I still think exclude_host is absolute shit. If you set it, you'll not get anything even without virt. Run a native linux kernel, no kvm loaded, create a counter with exclude_host=1 and you'll get nothing, that's just really confusing IMO. There is no host, so excluding it should not affect anything. > > Then the next question is, if G0 is a host, does the L1-hv run in > > G0 userspace or G0 kernel space? > > It's mostly kernel, but sometimes you're interested in events from QEMU > or whoever else has opened /dev/kvm. In that case you care about G0 > userspace too. I really don't think userspace helpers should be consideed part of the host, but whatever. > > The way it is implemented, you basically have to always set > > exclude_host=0, even if there is no virt at all and you want to measure > > your own userspace thing -- which is just weird. > > I understand regretting having exclude_guest that way; include_guest > (defaulting to 0!) would have made more sense. But defaulting to > exclude_host==0 makes sense: if there is no virt at all, memset(0) does > the right thing so it does not seem weird to me. Sure, but having exclude_host affect anything outside of kvm is still dodgy as heck. > > I suppose the 'best' option at this point is something like: > > > > /* > > * comment that explains the trainwreck. > > */ > > if (!exclude_host && !exclude_guest) > > exclude_guest = 1; > > > > if ((!exclude_hv || !exclude_guest) && !perf_allow_kernel()) > > return -EPERM; > > > > But that takes away the possibility of actually having: > > 'exclude_host=0, exclude_guest=0' to create an event that measures both, > > which also sucks. > > In fact both of the above "if"s suck. :( If, as you seem to imply above, that unpriv users can create guests, then maybe so, but if I look at /dev/kvm it seems to have 0660 permissions and thus really requires privileges.
