[9fans] kernel bug

[9fans] kernel bug

[Yoann Padioleau]

Hi,

in the newseg() function there is:

    mapsize = ROUND(size, PTEPERTAB)/PTEPERTAB;
    if(mapsize > nelem(s->ssegmap)){
        mapsize *= 2;
        if(mapsize > (SEGMAPSIZE*PTEPERTAB))
            mapsize = (SEGMAPSIZE*PTEPERTAB);
        s->map = smalloc(mapsize*sizeof(Pte*));
        s->mapsize = mapsize;
    }
    else{
        s->map = s->ssegmap;
        s->mapsize = nelem(s->ssegmap);
    }

I think it should be
if(mapsize > (SEGMAPSIZE))
            mapsize = SEGMAPSIZE;

Also why in the kernel they use 'struct Pte' instead of the better name Pagetable.
In many places this is very confusing because when I see Pte I think of a Pagetable Entry
where really they are speaking about a Pagetable.

Re: [9fans] kernel bug

[cinap_lenrek]

good catch. thank you.

--
cinap

Re: [9fans] kernel bug

[Anthony Martin]

Yoann Padioleau <pad@fb.com> once said:
> in the newseg() function there is:
>
> [...]
>
> I think it should be
> if(mapsize > (SEGMAPSIZE))
>             mapsize = SEGMAPSIZE;

Yes, you're correct.

The code allows the creation of a segment VM map with more
than SEGMAPSIZE Ptes.

Personally, I'd remove the check entirely along with the
optimization of doubling the segment size, perhaps moving
it to ibrk. I think it's more likely that the segment will
grow if brk is called at least once.

  Anthony

P.S. It looks like this is a 15 year old bug:

1998/0916/sys/src/9/port/segment.c:62,67 - 1998/0919/sys/src/9/port/segment.c:62,70

    mapsize = ROUND(size, PTEPERTAB)/PTEPERTAB;
    if(mapsize > nelem(s->ssegmap)){
+       mapsize *= 2;
+       if(mapsize > (SEGMAPSIZE*PTEPERTAB))
+           mapsize = (SEGMAPSIZE*PTEPERTAB);
        s->map = smalloc(mapsize*sizeof(Pte*));
        s->mapsize = mapsize;
    }

Re: [9fans] kernel bug

[erik quanstrom]

> I think it should be
> if(mapsize > (SEGMAPSIZE))
>             mapsize = SEGMAPSIZE;

hmm.  i think this code is correct.  ssegmap is a static map
to handle small segments.  small segments fit in ssegmap.
the point must have been to avoid malloc.

this test is a little more questionable

>	if(mapsize > (SEGMAPSIZE*PTEPERTAB))
>		mapsize = (SEGMAPSIZE*PTEPERTAB);

cf. the check in ibrk

	if(newsize > (SEGMAPSIZE*PTEPERTAB)) {
		qunlock(&s->lk);
		error(Enovmem);
	}

i think this check is either not wrong, or more extensive rework
is necessary.

@anthony, do you know if this code or similar occurred in even older kernels?
if there was a cap also in ibrk() then i would suspect this code was originally
correct.

i don't know where a history of stuff older than sources (2002) is.

> Also why in the kernel they use 'struct Pte' instead of the better name Pagetable.
> In many places this is very confusing because when I see Pte I think of a Pagetable Entry
> where really they are speaking about a Pagetable.

i would naturally think a Pte* would be an array of Pte's, i.e. a Pte table,
just like an array of uchar* could be used as a table.

- erik

Re: [9fans] kernel bug

[andrey mirtchovski]

> i don't know where a history of stuff older than sources (2002) is.

http://swtch.com/plan9history/

Re: [9fans] kernel bug

[erik quanstrom]

too bad, i don't see anthony's diff, and i get this error
(perhaps unrelated)

	Too many diffs (26 > 25). Stopping.

- erik

Re: [9fans] kernel bug

[Charles Forsyth]

[-- Attachment #1: Type: text/plain, Size: 436 bytes --]

On 3 June 2014 02:14, Yoann Padioleau <pad@fb.com> wrote:

>         if(mapsize > (SEGMAPSIZE*PTEPERTAB))
>             mapsize = (SEGMAPSIZE*PTEPERTAB);
>
...

>
>

I made the change you suggest in the PAE kernel but perhaps Erik missed it
during his merge:
if(mapsize > nelem(s->ssegmap)){
mapsize *= 2;
if(mapsize > SEGMAPSIZE)
mapsize = SEGMAPSIZE;
s->map = smalloc(mapsize*sizeof(Pte*));
s->mapsize = mapsize;
}

[-- Attachment #2: Type: text/html, Size: 1689 bytes --]

Re: [9fans] kernel bug

[erik quanstrom]

> I made the change you suggest in the PAE kernel but perhaps Erik missed it
> during his merge:
> if(mapsize > nelem(s->ssegmap)){
> mapsize *= 2;
> if(mapsize > SEGMAPSIZE)
> mapsize = SEGMAPSIZE;
> s->map = smalloc(mapsize*sizeof(Pte*));
> s->mapsize = mapsize;
> }

ok.  good.  that's what happened.  perhaps some change needs
to be made to segattach to mirror this change.

- erik

Re: [9fans] kernel bug

[Charles Forsyth]

[-- Attachment #1: Type: text/plain, Size: 438 bytes --]

On 3 June 2014 18:08, erik quanstrom <quanstro@quanstro.net> wrote:

> ok.  good.  that's what happened.  perhaps some change needs
> to be made to segattach to mirror this change.
>

I think this is independent. In ibrk I changed the test from using newsize
to calculating mapsize first and
checking mapsize in the same way as above, since mapsize had to be
calculated anyway, and it seemed
better to keep the tests the same.

[-- Attachment #2: Type: text/html, Size: 848 bytes --]

Re: [9fans] kernel bug

[Charles Forsyth]

[-- Attachment #1: Type: text/plain, Size: 328 bytes --]

Looking at the diffs I can see how it might easily be missed, since there
are many other changes to allow for different page sizes.
It's different again in the 64-bit version, where I really went over the
top (nice 1914 centenary reference there) on changes.
I'd better put that code somewhere before someone shouts at me.

[-- Attachment #2: Type: text/html, Size: 481 bytes --]

Re: [9fans] kernel bug

[Anthony Martin]

erik quanstrom <quanstro@quanstro.net> once said:
> i don't know where a history of stuff older than sources (2002) is.

/n/sources/extra/9hist

  Anthony