[PATCH][resend] small binfmt_elf warning fix (copy_from_user return value checking) (fwd)

[PATCH][resend] small binfmt_elf warning fix (copy_from_user return value checking) (fwd)

[Jesper Juhl]

Andrew, I'm sending this to you since I can't find any maintainer for 
binfmt_elf, and mails to the list about this seem to hit some black hole.
Care to take a look and consider for inclusion (it's really small and 
simple)?
If there's something obviously stupid about this patch, then please let me 
know - I don't know if I should interpret the silence on the list as 
"noone can be bothered to look at it" or "insanely stupid, ignored". 
I'd like to get the warnings in binfmt_elf cleaned up, and I'm now trying 
to do it piece by piece (more pieces will follow when I get some sort of 
response to what I've already posted) instead of just one big chunk.

---
Jesper Juhl


---------- Forwarded message ----------
Date: Wed, 6 Oct 2004 23:39:06 +0200 (CEST)
From: Jesper Juhl <juhl-lkml@dif.dk>
To: linux-kernel <linux-kernel@vger.kernel.org>
Subject: [PATCH] small binfmt_elf warning fix (copy_from_user return value
    checking)


Since my patch to try and clean up all the warnings generated by 
fs/binfmt_elf.c all at once seems to be ignored I'll try a different 
approach; smaller patches fixing just a little bit at a time. Here's one 
such tiny fix.

This is the warning that triggered this patch :
fs/binfmt_elf.c:1226: warning: ignoring return value of `copy_from_user', declared with attribute warn_unused_result

The patch silences the warning by actually using the return value, but in 
this case there's no great harm done if the copy fails, psinfo has already 
been zeroed, so a failed copy will just result in a few psinfo->pr_psargs 
being zero (which as I read it is suboptimal but not fatal).
Even though we could safely ignore the return value we can gain a tiny 
bennefit from it by using it to decrease the 'len' variable that is used 
in the for() loop below - if copy to user failed to copy all data, then 
there's no need for the loop to iterate over any more than what was 
actually copied (and we know the rest is zeroed).

So, we get two small bennefits from this patch:
- Silence the warning by actually using the return value of copy_from_user
- If copy_from_user fails save a few loop iterations (at least then some 
good came from a bad situation).

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

diff -up linux-2.6.9-rc3-bk5-orig/fs/binfmt_elf.c linux-2.6.9-rc3-bk5/fs/binfmt_elf.c
--- linux-2.6.9-rc3-bk5-orig/fs/binfmt_elf.c	2004-09-30 05:04:32.000000000 +0200
+++ linux-2.6.9-rc3-bk5/fs/binfmt_elf.c	2004-10-06 23:21:22.000000000 +0200
@@ -1223,7 +1223,7 @@ static void fill_psinfo(struct elf_prpsi
 	len = mm->arg_end - mm->arg_start;
 	if (len >= ELF_PRARGSZ)
 		len = ELF_PRARGSZ-1;
-	copy_from_user(&psinfo->pr_psargs,
+	len -= copy_from_user(&psinfo->pr_psargs,
 		       (const char __user *)mm->arg_start, len);
 	for(i = 0; i < len; i++)
 		if (psinfo->pr_psargs[i] == 0)

Re: [PATCH][resend] small binfmt_elf warning fix (copy_from_user return value checking) (fwd)

[Andrew Morton]

Jesper Juhl <juhl-lkml@dif.dk> wrote:
>
>  diff -up linux-2.6.9-rc3-bk5-orig/fs/binfmt_elf.c linux-2.6.9-rc3-bk5/fs/binfmt_elf.c
>  --- linux-2.6.9-rc3-bk5-orig/fs/binfmt_elf.c	2004-09-30 05:04:32.000000000 +0200
>  +++ linux-2.6.9-rc3-bk5/fs/binfmt_elf.c	2004-10-06 23:21:22.000000000 +0200
>  @@ -1223,7 +1223,7 @@ static void fill_psinfo(struct elf_prpsi
>   	len = mm->arg_end - mm->arg_start;
>   	if (len >= ELF_PRARGSZ)
>   		len = ELF_PRARGSZ-1;
>  -	copy_from_user(&psinfo->pr_psargs,
>  +	len -= copy_from_user(&psinfo->pr_psargs,
>   		       (const char __user *)mm->arg_start, len);
>   	for(i = 0; i < len; i++)
>   		if (psinfo->pr_psargs[i] == 0)

It doesn't matter, really - we've already zeroed out the memory and will
correctly handle any uncopied data.

Maybe sticking a (void) in front of the copy_from_user() call will shut the
warning up.  Although it could possibly break the build, depending on how
the architecture implements copy_from_user().

Re: [PATCH][resend] small binfmt_elf warning fix (copy_from_user return value checking) (fwd)

[Jesper Juhl]

On Sun, 24 Oct 2004, Andrew Morton wrote:

> Jesper Juhl <juhl-lkml@dif.dk> wrote:
> >
> >  diff -up linux-2.6.9-rc3-bk5-orig/fs/binfmt_elf.c linux-2.6.9-rc3-bk5/fs/binfmt_elf.c
> >  --- linux-2.6.9-rc3-bk5-orig/fs/binfmt_elf.c	2004-09-30 05:04:32.000000000 +0200
> >  +++ linux-2.6.9-rc3-bk5/fs/binfmt_elf.c	2004-10-06 23:21:22.000000000 +0200
> >  @@ -1223,7 +1223,7 @@ static void fill_psinfo(struct elf_prpsi
> >   	len = mm->arg_end - mm->arg_start;
> >   	if (len >= ELF_PRARGSZ)
> >   		len = ELF_PRARGSZ-1;
> >  -	copy_from_user(&psinfo->pr_psargs,
> >  +	len -= copy_from_user(&psinfo->pr_psargs,
> >   		       (const char __user *)mm->arg_start, len);
> >   	for(i = 0; i < len; i++)
> >   		if (psinfo->pr_psargs[i] == 0)
> 
> It doesn't matter, really - we've already zeroed out the memory and will
> correctly handle any uncopied data.
> 
Yes, I know, it's mainly to shut up the warning in some resonable way. We 
may be saving a few loop iterations, but we'll be adding a subtraction, so 
performance wise it probably won't make any difference - but it's a very 
cold code path as far as I can see, so it matters little.

> Maybe sticking a (void) in front of the copy_from_user() call will shut the
> warning up.  Although it could possibly break the build, depending on how
> the architecture implements copy_from_user().
> 
Then isn't my way of shutting up gcc more sensible? little to no impact on 
the code and we get rid of the annoying warning in a safe way that won't 
break anything.

I'll prepare patches against a recent Linus kernel for the other things in 
binfmt_elf I have lying here and submit those shortly, those should fix 
more real issues - I hope you won't mind if I also send those your way.

Thank you very much for taking a look.


--
Jesper Juhl