Re: [PATCH] Prevent crash on missing sysfs attribute group

[ebiederm@xmission.com (Eric W. Biederman)]

Ingo Molnar <mingo@kernel.org> writes:

> * Eric W. Biederman <ebiederm@xmission.com> wrote:
>
>> > Huh, so put repeated, duplicated, inconsistently applied sanity 
>> > checks into dozens of sysfs attribute using kernel subsystems?
>>
>> [...]
>>
>> No.  I was not talking about every usage site.
>
> Note, I'm not arguing that this isn't a bug in the P4 PMU driver 
> - it is clearly a bug and I've applied the fix for it. I'm 
> arguing about the escallation vector that this bug takes - that 
> is unnecessarily disruptive:
>
> You were talking about:
>
>> >> FIX perf to include sanity checks.
>
> and what the PMU drivers do here is not uncommon at all, and the 
> bug (for which I applied the fix and will push to Linus ASAP) is 
> not uncommon either:

> Bugs happen and indirections happen too. perf uses a generic PMU 
> driver layer where the lower level layers register themselves. 
> There's at least a dozen similar constructs in the kernel and 
> you suggest that the right solution is to put checks in every 
> one of them, while the nice patch from Bruno could catch it too, 
> in one central place?

What is uncommon is that perf_pmu_register is called from
an early initcall, and then later a device_init call
is used to register the pmu subsystem with sysfs.

That extra delay step is weird.  That registering extra early
is weird.

How we get from x86_pmu to the variable simply named pmu
that is registered I am still in the dark about.  There is a lot
of weird magic going on in that registration path before
these things get to sysfs.

And those extra steps are what make Bruno's patch largely
useless for this case.  

> If the PMU code used those attributes directly and could 
> crash/misbehave then you'd have a point. But the first thing 
> that makes real use of these objects is sysfs - so it's 
> trivially useful to at minimum have a sanity check there...

If the pmu subsystem is doing odd and peculiar things it should strive
to not impose debugging burden on others.

>> [...]  I was talking about the sites that are don't have a 
>> direct call chain to the sysfs methods and instead do 
>> something clever that makes backtraces worthless.
>> 
>> In the normal case sysfs registration problems are simple to 
>> trace back to their source because the backtrace points a 
>> finger at the piece of code that when registering had a 
>> problem.
>
> You mean the crash backtrace?

I mean a backtrace when people try and abuse sysfs by accident.
Typically they are backtraces from WARN_ON that I look at.  It is common
enough and I look a reasonable number of them.  Usually they point back
to the subsystem and the borked piece of code.  In this case the
backtrace barely hit the broad side of the barn.

It really irritates me that a stack backtrace is useless for figuring
out that this was something in under arch/x86 let alone for figuring
out that this was p4 related.

So since the perf pmu event subsystem is extremely atypical I am asking
that it be looked at to see if it's structure or sanity checks can
be improved so it is more debuggable when people make stupid mistakes.

Eric