[PATCH] kill access_ok() call from copy_siginfo_to_user() that we might as well avoid.

[PATCH] kill access_ok() call from copy_siginfo_to_user() that we might as well avoid.

[Jesper Juhl]

Hi,

In kernel/signal.c::copy_siginfo_to_user() we are calling access_ok() 
unconditionally. As I see it there's no need for this since we might as 
well just call copy_to_user() instead of __copy_to_user() later on and 
then only get the overhead of the access_ok() check (inside 
copy_to_user()) when the  if (from->si_code < 0)  branch is actually taken.
In the case where the branch is taken the cost is the same, but when the 
branch is not taken this patch will save us an access_ok() call.

Comments are always welcome.

Signed-off-by: Jesper Juhl <juhl-lkml@dif.dk>

diff -up linux-2.6.10-rc3-bk13-orig/kernel/signal.c linux-2.6.10-rc3-bk13/kernel/signal.c
--- linux-2.6.10-rc3-bk13-orig/kernel/signal.c	2004-12-06 22:24:56.000000000 +0100
+++ linux-2.6.10-rc3-bk13/kernel/signal.c	2004-12-21 00:23:46.000000000 +0100
@@ -2070,10 +2070,8 @@ int copy_siginfo_to_user(siginfo_t __use
 {
 	int err;
 
-	if (!access_ok (VERIFY_WRITE, to, sizeof(siginfo_t)))
-		return -EFAULT;
 	if (from->si_code < 0)
-		return __copy_to_user(to, from, sizeof(siginfo_t))
+		return copy_to_user(to, from, sizeof(siginfo_t))
 			? -EFAULT : 0;
 	/*
 	 * If you change siginfo_t structure, please be sure

Re: [PATCH] kill access_ok() call from copy_siginfo_to_user() that we might as well avoid.

[Linus Torvalds]

On Tue, 21 Dec 2004, Jesper Juhl wrote:
> 
> In kernel/signal.c::copy_siginfo_to_user() we are calling access_ok()  
> unconditionally. As I see it there's no need for this since we might as
> well just call copy_to_user() instead of __copy_to_user() later on and
> then only get the overhead of the access_ok() check (inside
> copy_to_user())

No, the "access_ok()" is to protect the other side too, ie all the 
"__put_user()" calls.

If you remove the access_ok(), you need to also change all the 
__put_user() calls to "put_user()". And then the end result will be much 
worse code than it is right now.

		Linus

Re: [PATCH] kill access_ok() call from copy_siginfo_to_user() that we might as well avoid.

[Jesper Juhl]

On Mon, 20 Dec 2004, Linus Torvalds wrote:

> 
> 
> On Tue, 21 Dec 2004, Jesper Juhl wrote:
> > 
> > In kernel/signal.c::copy_siginfo_to_user() we are calling access_ok()  
> > unconditionally. As I see it there's no need for this since we might as
> > well just call copy_to_user() instead of __copy_to_user() later on and
> > then only get the overhead of the access_ok() check (inside
> > copy_to_user())
> 
> No, the "access_ok()" is to protect the other side too, ie all the 
> "__put_user()" calls.
> 
> If you remove the access_ok(), you need to also change all the 
> __put_user() calls to "put_user()". And then the end result will be much 
> worse code than it is right now.
> 
Hmm, right, now that you point it out that's glaringly obvious - that 
slipped by me - sorry for the noice.

-- 
Jesper Juhl

Re: [PATCH] kill access_ok() call from copy_siginfo_to_user() that we might as well avoid.

[Linus Torvalds]

[ Linux-kernel added back into the cc, because I actually think this is 
  important. ]

On Tue, 21 Dec 2004, Jesper Juhl wrote:
> 
> Should I just stop attemting to make these trivial cleanups/fixes/whatever 
> patches? are they more noice than gain? am I being a pain to more skilled 
> people on lkml or can you all live with my, sometimes quite ignorant, 
> patches?
> I do try to learn from the feedback I get, and I like to think that my 
> patches are gradually getting a bit better, but if I'm more of a bother 
> than a help I might as well stop.

To me, the biggest thing with small patches is not necessarily the patch 
itself. I think that much more important than the patch is the fact that 
people get used to the notion that they can change the kernel - not just 
on an intellectual level ("I understand that the GPL means that I have the 
right to change my kernel"), but on a more practical level ("Hey, I did 
that small change").

And whether it ends up being the right thing or not, that's how everybody
starts out. It's simply not possible to "get into" the kernel without
starting out small, and making mistakes. So I very much encourage it, even
if I often don't have the time to actually worry about small patches, and 
I try to get suckers^H^H^H^H^H^H^Hother developers like Rusty to try to 
acts as quality control and a "gathering place".

Btw, this is why even "trivial patches" really do take time - they often 
have trivial mistakes in them, and it's not just because there are more 
inexperienced people doing them - most of _my_ mistakes tend to be at the 
truly idiotic level, just because it "looked obvious", and then there's 
something that I miss.

So at one level I absolutely _hate_ trivial patches: they take time and 
effort to merge, and individually the patch itself is often not really 
obviously "worth it". But at the same time, I think the trivial patches 
are among the most important ones - exactly because they are the "entry" 
patches for every new developer.

I just try really hard to find somebody else to worry about them ;)

(It's not a thankful job, btw, exactly because it _looks_ so trivial. It's
easy to point to 99 patches that are absolutely obvious, and complain
about the fact that they haven't been merged. But they take time to merge
exactly because of that one patch that _did_ look obvious, but wasn't.  
And actually, it's usually not 99:1, it's usually more like 10:1 or
something).

So please don't stop. Yes, those trivial patches _are_ a bother. Damn, 
they are _horrible_. But at the same time, the devil is in the detail, and 
they are needed in the long run. Both the patches themselves, and the 
people that grew up on them.

		Linus

Re: [PATCH] kill access_ok() call from copy_siginfo_to_user() that we might as well avoid.

[Jesper Juhl]

On Mon, 20 Dec 2004, Linus Torvalds wrote:

> 
> [ Linux-kernel added back into the cc, because I actually think this is 
>   important. ]
> 
> On Tue, 21 Dec 2004, Jesper Juhl wrote:
> > 
> > Should I just stop attemting to make these trivial cleanups/fixes/whatever 
> > patches? are they more noice than gain? am I being a pain to more skilled 
> > people on lkml or can you all live with my, sometimes quite ignorant, 
> > patches?
> > I do try to learn from the feedback I get, and I like to think that my 
> > patches are gradually getting a bit better, but if I'm more of a bother 
> > than a help I might as well stop.
> 
> To me, the biggest thing with small patches is not necessarily the patch 
> itself. I think that much more important than the patch is the fact that 
> people get used to the notion that they can change the kernel - not just 
> on an intellectual level ("I understand that the GPL means that I have the 
> right to change my kernel"), but on a more practical level ("Hey, I did 
> that small change").
>
Right, that actually does give one a warm fuzzy feeling inside once in a 
while and encourages you to carry on.
 

> And whether it ends up being the right thing or not, that's how everybody
> starts out. It's simply not possible to "get into" the kernel without
> starting out small, and making mistakes. So I very much encourage it, even
> if I often don't have the time to actually worry about small patches, and 
> I try to get suckers^H^H^H^H^H^H^Hother developers like Rusty to try to 
> acts as quality control and a "gathering place".
> 
> Btw, this is why even "trivial patches" really do take time - they often 
> have trivial mistakes in them, and it's not just because there are more 
> inexperienced people doing them - most of _my_ mistakes tend to be at the 
> truly idiotic level, just because it "looked obvious", and then there's 
> something that I miss.
> 
> So at one level I absolutely _hate_ trivial patches: they take time and 
> effort to merge, and individually the patch itself is often not really 
> obviously "worth it". But at the same time, I think the trivial patches 
> are among the most important ones - exactly because they are the "entry" 
> patches for every new developer.
> 
> I just try really hard to find somebody else to worry about them ;)
> 
Heh, quite understandable - I wouldn't want to be at the other end of your 
mailbox.

> (It's not a thankful job, btw, exactly because it _looks_ so trivial. It's
> easy to point to 99 patches that are absolutely obvious, and complain
> about the fact that they haven't been merged. But they take time to merge
> exactly because of that one patch that _did_ look obvious, but wasn't.  
> And actually, it's usually not 99:1, it's usually more like 10:1 or
> something).
> 
> So please don't stop. Yes, those trivial patches _are_ a bother. Damn, 
> they are _horrible_. But at the same time, the devil is in the detail, and 
> they are needed in the long run. Both the patches themselves, and the 
> people that grew up on them.
> 
Ok, thank you so much for that reply. I'll try to keep the pain to a 
minimum, but I'll keep on doing the trivial (and hopefully in the 
long run the not-so trivial) patches :)
You just brought some courage/spirit/enthusiasm back to a disheartened 
kernel-hacker-wannabe ;-)  Thank you!


-- 
Jesper