lkml.org 
[lkml]   [2004]   [Jan]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH(s)][RFC] variable size and signedness issues in ldt.c - potential problem?

Found some errors in my earlier reply. I've added additional
comments below.
I've not quoted the long style cleanup patch in this reply as I trust you
can find it in the previous mail in the thread.
I've also not quoted the bits where I have no corrections - please see the
previous email for those.

On Sun, 11 Jan 2004, Jesper Juhl wrote:

>
> One additional thing I noticed in the ldt code is that it seems to me that
> there could be some bennefit to moving the declaration of 'bytes' in the
> for() loop in read_ldt() outside the loop. It gets initialized by
> 'bytes = size - i;' every time through the loop before it's used, so I se
> no reason to re-create the variable every time through the loop.
> If that makes sense, then here's a patch to make this change (it's against
> 2.6.1-mm1) :
>
> --- linux-2.6.1-mm1-orig/arch/i386/kernel/ldt.c 2004-01-09 19:04:23.000000000 +0100
> +++ linux-2.6.1-mm1-juhl/arch/i386/kernel/ldt.c 2004-01-11 03:36:02.000000000 +0100
> @@ -118,10 +118,11 @@ void destroy_context(struct mm_struct *m
> mm->context.size = 0;
> }
>
> -static int read_ldt(void __user * ptr, unsigned long bytecount)
> +static int read_ldt(void __user *ptr, unsigned long bytecount)
> {
> int err, i;
> unsigned long size;
> + unsigned long bytes;
> struct mm_struct * mm = current->mm;
>
> if (!mm->context.size)
> @@ -144,7 +145,7 @@ static int read_ldt(void __user * ptr, u
> __flush_tlb_global();
>
> for (i = 0; i < size; i += PAGE_SIZE) {
> - int nr = i / PAGE_SIZE, bytes;
> + int nr = i / PAGE_SIZE;
> char *kaddr = kmap(mm->context.ldt_pages[nr]);
>
> bytes = size - i;
>
>
In the above patch 'unsigned long bytes;' should ofcourse have been
'int bytes' - I should have read the patch more closely after
running diff on my own tree.
As I've been unable to find any scenario where an int can actually
overflow there is ofcourse no need to change the type - my bad.


>
> > > and finally a purely style related thing (sure, call me pedantic); in both
> > > read_ldt() and write_ldt() 'mm' is declared as
> > >
> > > struct mm_struct * mm = current->mm;
> >
> > yep, you are right, this is the wrong style.
> >
>
> Ok, thank you for confirming that.
>
> Since this /is/ incorrect style I've created a patch to clean it up, as
> well as a bunch of other instances of the same style issue I found nearby.
> I certainly haven't cleaned up *all* instances of this inccorect style,
> but I have nailed 26 files with such instances, and I believe I've nailed
> all occourances of this style issue in those files.
>

After creating the initial cleanup patch I've noticed several more
instances of this 'bad style'. If there's any interrest in cleaning them
up I'll be happy to create a patch. Is this wanted?


-- Jesper Juhl

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2005-03-22 14:00    [W:1.436 / U:0.004 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site