Please revert 21564fd2a3deb48200b595332f9ed4c9f311f2a7

Please revert 21564fd2a3deb48200b595332f9ed4c9f311f2a7

[Adrian Bunk]

Linus, please revert commit 21564fd2a3deb48200b595332f9ed4c9f311f2a7

It's not acceptable since illegal modules should definitely not get 
write access to paravirt_ops.

Andi forwarded it although the following people had already NAK'ed it:
- Christoph Hellwig [1]
- Peter Zijlstra [2]
- Alan Cox [3]

Considering that Andi forwarded it 2 days after he himself said a 
different solution was pending [4] I assume he mistakenly sent it for 
inclusion in your tree.

Reverting is safe since it simply re-establishes the 2.6.21 status quo.

cu
Adrian

[1] http://lkml.org/lkml/2007/4/30/132
[2] http://lkml.org/lkml/2007/4/30/197
[3] http://lkml.org/lkml/2007/4/30/237
[4] http://lkml.org/lkml/2007/4/30/335

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: Please revert 21564fd2a3deb48200b595332f9ed4c9f311f2a7

[Jeremy Fitzhardinge]

Adrian Bunk wrote:
> Linus, please revert commit 21564fd2a3deb48200b595332f9ed4c9f311f2a7
>
> It's not acceptable since illegal modules should definitely not get 
> write access to paravirt_ops.
>   

What's the concern?  That a module will update the pv_ops structure to
repoint the function?  In practice that won't work because inline
patching will have already happened, so all the callsites will have
direct calls to the appropriate function.

> Andi forwarded it although the following people had already NAK'ed it:
> - Christoph Hellwig [1]
> - Peter Zijlstra [2]
> - Alan Cox [3]
>
> Considering that Andi forwarded it 2 days after he himself said a 
> different solution was pending [4] I assume he mistakenly sent it for 
> inclusion in your tree.
>   

We played with some ideas, but they all turned out way too ugly to live. 

> Reverting is safe since it simply re-establishes the 2.6.21 status quo.
>   

Well, not really.  It breaks any non-GPL module when CONFIG_PARAVIRT is
enabled, even though the same module would work fine otherwise.  That's
a pretty large regression.

I'm not really sure what the concerns are with exporting paravirt_ops as
a non-GPL symbol.

I think the basic arguments are:

    security - it makes an obvious place to hook things in.  Perhaps,
    but in practice the patching code will replace all the indirect
    calls with either inline code or a direct call to the proper
    function.  Once that's happened, changing the structure will have no
    effect.

    license - it makes GPL-only functions visible to non-GPL modules. 
    This is largely moot, since in the non-PARAVIRT case most of the
    functions in question are exported from the kernel as inline code in
    headers anyway, and so are available to all modules anyway.  In
    either case (ie PARAVIRT or no), non-inlined GPL functions will
    still be unavailable to non-GPL modules, and will still cause errors
    at insmod time.  If a module decides to directly access paravirt_ops
    (which is never a supported API, for anyone), then they could get
    access to GPL code without raising a complaint - but it would be
    very fragile piece of code, and definitely easily broken (it would
    be not much different from  jumping into the kernel at a fixed
    address because it "knows" a particular function is there).

It would be possible to arrange for paravirt_ops to be largely cleared
out once patching has happened, so that anyone looking at it wouldn't
get any useful information anyway (we'd need to keep a non-exported copy
around for patching modules, but that's OK).

Or, alternatively, we could wrap parts of struct paravirt_ops with
#ifdef MODULE to obscure the entrypoints from modules.  They would still
be able to get around this, of course, but it makes the intent clear
("thou shalt not play with these").  But I think its overkill, ugly, and
doesn't really achieve much in practice.

    J

Re: Please revert 21564fd2a3deb48200b595332f9ed4c9f311f2a7

[Adrian Bunk]

On Sun, Jun 17, 2007 at 09:43:51PM -0700, Jeremy Fitzhardinge wrote:
> Adrian Bunk wrote:
>...
> > Andi forwarded it although the following people had already NAK'ed it:
> > - Christoph Hellwig [1]
> > - Peter Zijlstra [2]
> > - Alan Cox [3]
> >
> > Considering that Andi forwarded it 2 days after he himself said a 
> > different solution was pending [4] I assume he mistakenly sent it for 
> > inclusion in your tree.
> >   
> 
> We played with some ideas, but they all turned out way too ugly to live. 

Andi got some NAK's, said himself it will be solved differently, and
two days later he submits the NAK'ed patch into Linus' tree.

Was this a mistake that should be reverted at least for now because of 
this, or is silently doing the opposite of what you said you'd do how
Linux development is expected to work today?

> > Reverting is safe since it simply re-establishes the 2.6.21 status quo.
> 
> Well, not really.  It breaks any non-GPL module when CONFIG_PARAVIRT is
> enabled, even though the same module would work fine otherwise.  That's
> a pretty large regression.
>...

The 2.6.21 status quo can by definition not be a regression compared
to 2.6.21.

>     J

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: Please revert 21564fd2a3deb48200b595332f9ed4c9f311f2a7

[Andi Kleen]

On Monday 09 July 2007 00:44:44 Adrian Bunk wrote:
> On Sun, Jun 17, 2007 at 09:43:51PM -0700, Jeremy Fitzhardinge wrote:
> > Adrian Bunk wrote:
> >...
> > > Andi forwarded it although the following people had already NAK'ed it:
> > > - Christoph Hellwig [1]
> > > - Peter Zijlstra [2]
> > > - Alan Cox [3]
> > >
> > > Considering that Andi forwarded it 2 days after he himself said a 
> > > different solution was pending [4] I assume he mistakenly sent it for 
> > > inclusion in your tree.
> > >   
> > 
> > We played with some ideas, but they all turned out way too ugly to live. 
> 
> Andi got some NAK's, said himself it will be solved differently, and
> two days later he submits the NAK'ed patch into Linus' tree.

It will be solved differently longer term, but short term the fix
was still needed. There are limits on what can be done late 
in the release cycle so simple patches win.

Besides none of the "NAK"s were particularly inspired in my opinion;
there were no clear technical objections brought forward.

-Andi
 

Re: Please revert 21564fd2a3deb48200b595332f9ed4c9f311f2a7

[Adrian Bunk]

On Mon, Jul 09, 2007 at 01:02:21AM +0200, Andi Kleen wrote:
> On Monday 09 July 2007 00:44:44 Adrian Bunk wrote:
> > On Sun, Jun 17, 2007 at 09:43:51PM -0700, Jeremy Fitzhardinge wrote:
> > > Adrian Bunk wrote:
> > >...
> > > > Andi forwarded it although the following people had already NAK'ed it:
> > > > - Christoph Hellwig [1]
> > > > - Peter Zijlstra [2]
> > > > - Alan Cox [3]
> > > >
> > > > Considering that Andi forwarded it 2 days after he himself said a 
> > > > different solution was pending [4] I assume he mistakenly sent it for 
> > > > inclusion in your tree.
> > > >   
> > > 
> > > We played with some ideas, but they all turned out way too ugly to live. 
> > 
> > Andi got some NAK's, said himself it will be solved differently, and
> > two days later he submits the NAK'ed patch into Linus' tree.
> 
> It will be solved differently longer term, but short term the fix
> was still needed. There are limits on what can be done late 
> in the release cycle so simple patches win.

Your patch got into Linus' tree in the middle of the merge window...

> Besides none of the "NAK"s were particularly inspired in my opinion;
> there were no clear technical objections brought forward.

Why didn't you say this?

Your answer in [1] didn't sound as if you would submit this patch, and 
even less that you would submit it just two days later.

> -Andi

cu
Adrian

[1] http://lkml.org/lkml/2007/4/30/335

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: Please revert 21564fd2a3deb48200b595332f9ed4c9f311f2a7

[Jeremy Fitzhardinge]

Adrian Bunk wrote:
>>> Reverting is safe since it simply re-establishes the 2.6.21 status quo.
>>>       
>> Well, not really.  It breaks any non-GPL module when CONFIG_PARAVIRT is
>> enabled, even though the same module would work fine otherwise.  That's
>> a pretty large regression.
>> ...
>>     
>
> The 2.6.21 status quo can by definition not be a regression compared
> to 2.6.21.
>   

2.6.21's behaviour was a bug.  CONFIG_PARAVIRT is not supposed to cause 
any behavioural changes.

    J

Re: Please revert 21564fd2a3deb48200b595332f9ed4c9f311f2a7

[Alan Cox]

> It will be solved differently longer term, but short term the fix
> was still needed. There are limits on what can be done late 
> in the release cycle so simple patches win.
> 
> Besides none of the "NAK"s were particularly inspired in my opinion;
> there were no clear technical objections brought forward.


There were a considerable number of sensible logical objections. You just
didn't agree with them. You've now effectively made .22 unsupportable
since we don't know if someone has binary virtualiser crap loaded.

Alan

Re: Please revert 21564fd2a3deb48200b595332f9ed4c9f311f2a7

[Andi Kleen]

> > It will be solved differently longer term, but short term the fix
> > was still needed. There are limits on what can be done late 
> > in the release cycle so simple patches win.
> 
> Your patch got into Linus' tree in the middle of the merge window...

Even in the middle of the merge window it might be too late
to do some large scale restructuring (as would have been needed
for this particular issue) 

Merge window is basically for about finished patches, not for development.

-Andi

Re: Please revert 21564fd2a3deb48200b595332f9ed4c9f311f2a7

[Andi Kleen]

On Monday 09 July 2007 11:32:23 Alan Cox wrote:
> > It will be solved differently longer term, but short term the fix
> > was still needed. There are limits on what can be done late 
> > in the release cycle so simple patches win.
> > 
> > Besides none of the "NAK"s were particularly inspired in my opinion;
> > there were no clear technical objections brought forward.
> 
> 
> There were a considerable number of sensible logical objections. You just
> didn't agree with them.

Well they didn't offer a workable alternative (that's my definition for "not 
particularly inspired")

> You've now effectively made .22 unsupportable 
> since we don't know if someone has binary virtualiser crap loaded.

What binary virtualiser crap? 

The most common external patchers are probably root kits and those
don't really care about GPL or non GPL exports.

-Andi

Re: Please revert 21564fd2a3deb48200b595332f9ed4c9f311f2a7

[Alan Cox]

On Sun, 08 Jul 2007 19:46:46 -0700
Jeremy Fitzhardinge <jeremy@goop.org> wrote:

> Adrian Bunk wrote:
> >>> Reverting is safe since it simply re-establishes the 2.6.21 status quo.
> >>>       
> >> Well, not really.  It breaks any non-GPL module when CONFIG_PARAVIRT is
> >> enabled, even though the same module would work fine otherwise.  That's
> >> a pretty large regression.
> >> ...
> >>     
> >
> > The 2.6.21 status quo can by definition not be a regression compared
> > to 2.6.21.
> >   
> 
> 2.6.21's behaviour was a bug.  CONFIG_PARAVIRT is not supposed to cause 
> any behavioural changes.

2.6.22's behaviour is the bug. 2.6.21 you couldn't load random binary
crap into the kernel without logging a taint. 2.6.22 you can. This means
every single 2.6.22 bug report has to be assumed to be caused by binary
module crap as a starting point which slows down debug immensely.

Alan

Re: Please revert 21564fd2a3deb48200b595332f9ed4c9f311f2a7

[Alan Cox]

On Mon, 9 Jul 2007 11:39:25 +0200
Andi Kleen <ak@suse.de> wrote:

> 
> > > It will be solved differently longer term, but short term the fix
> > > was still needed. There are limits on what can be done late 
> > > in the release cycle so simple patches win.
> > 
> > Your patch got into Linus' tree in the middle of the merge window...
> 
> Even in the middle of the merge window it might be too late
> to do some large scale restructuring (as would have been needed
> for this particular issue) 

You should have reverted the change and held it over for 2.6.23-rc1 then
as other developers do with problem code.

Alan

Re: Please revert 21564fd2a3deb48200b595332f9ed4c9f311f2a7

[Andi Kleen]

On Monday 09 July 2007 12:06:05 Alan Cox wrote:
> On Mon, 9 Jul 2007 11:39:25 +0200
> Andi Kleen <ak@suse.de> wrote:
> 
> > 
> > > > It will be solved differently longer term, but short term the fix
> > > > was still needed. There are limits on what can be done late 
> > > > in the release cycle so simple patches win.
> > > 
> > > Your patch got into Linus' tree in the middle of the merge window...
> > 
> > Even in the middle of the merge window it might be too late
> > to do some large scale restructuring (as would have been needed
> > for this particular issue) 
> 
> You should have reverted the change and held it over for 2.6.23-rc1 then
> as other developers do with problem code.

Didn't seem justified. 

If someone really wanted to do evil binary only virtualization there
are lots of other ways to do this anyways (e.g. just patching the code
or looking for the paravirt_ops struct signature in memory) 

-Andi

Re: Please revert 21564fd2a3deb48200b595332f9ed4c9f311f2a7

[Jeremy Fitzhardinge]

Alan Cox wrote:
> 2.6.22's behaviour is the bug. 2.6.21 you couldn't load random binary
> crap into the kernel without logging a taint. 2.6.22 you can. This means
> every single 2.6.22 bug report has to be assumed to be caused by binary
> module crap as a starting point which slows down debug immensely.

Er, what?  What do you mean by "load random binary crap"?  Are you 
worried that a malicious kernel module might modify paravirt_ops?  How 
is that different from a malicious module doing any of the infinite 
other things a malicious module can do?  A module will only register a 
taint if its playing by the rules anyway, and a module playing by the 
rules won't touch paravirt_ops directly.

    J

Re: Please revert 21564fd2a3deb48200b595332f9ed4c9f311f2a7

[Jeremy Fitzhardinge]

Alan Cox wrote:
> There were a considerable number of sensible logical objections. You just
> didn't agree with them. You've now effectively made .22 unsupportable
> since we don't know if someone has binary virtualiser crap loaded.


The pv_ops infrastructure doesn't support modular pv_ops 
implementations, and if it did, there's certainly no intention of 
supporting non-GPL implementations.

At the same time that all the other altinstruction patching happens, it 
converts any indirect calls via paravirt_ops into direct calls to the 
target function, bypassing struct paravirt_ops altogether.  After that 
the structure is pretty much only used for patching modules.  Any 
attempt at writing a modular pv_ops implementation would fail as a result.

    J