free_task_struct() called too early?

free_task_struct() called too early?

[Chen, Kenneth W]

When a process terminates, it appears that the task structure is freed too
early.  There are memory references to the kernel task area (task_struct and
stack space) after free_task_struct(p) is called.

If I modify the following line in include/asm-i386/processor.h

#define free_task_struct(p)   free_pages((unsigned long) (p), 1) to
#define free_task_struct(p)   memset((void*) (p), 0xf, PAGE_SIZE*2);
free_pages((unsigned long) (p), 1)
then kernel will boot to init and lockup on when first task terminates.

Has anyone looked into or aware of this issue?

Re: free_task_struct() called too early?

[Andrew Morton]

"Chen, Kenneth W" wrote:
> 
> When a process terminates, it appears that the task structure is freed too
> early.  There are memory references to the kernel task area (task_struct and
> stack space) after free_task_struct(p) is called.
> 
> If I modify the following line in include/asm-i386/processor.h
> 
> #define free_task_struct(p)   free_pages((unsigned long) (p), 1) to
> #define free_task_struct(p)   memset((void*) (p), 0xf, PAGE_SIZE*2);
> free_pages((unsigned long) (p), 1)
> then kernel will boot to init and lockup on when first task terminates.

free_pages will not actually free the page if its reference count
is greater than one - you're poisoning the page before it has actually
ceased to be used.

You should zap the page in __free_pages(), after the put_page_testzero()
call has succeeded.

Re: free_task_struct() called too early?

[Linus Torvalds]

On Fri, 10 Aug 2001, Alan Cox wrote:
> >
> > If I modify the following line in include/asm-i386/processor.h
> >
> > #define free_task_struct(p)   free_pages((unsigned long) (p), 1) to
> > #define free_task_struct(p)   memset((void*) (p), 0xf, PAGE_SIZE*2);
> > free_pages((unsigned long) (p), 1)
> > then kernel will boot to init and lockup on when first task terminates.
> >
> > Has anyone looked into or aware of this issue?
>
> 2.4.8pre fixed a case with semaphores on the stack. It might not be the only
> one. Your #define is wrong though, if a single free_task_struct path is an
> if you will not do what you expect
>
> 	do { memset(), free_pages } while(0);
>
> would be safer.

No no no.

You can NOT do a "memset()" before you free the task-struct: it's can
still be actively used, and will be marked such by having an elevated page
count (ie the "free()" may not be doing anything but atomically decrement
the page count).

This is, in fact, a very very common thing - I bet that you will reboot
the first time you touch a /proc/<pid>/xx file or do a "ptrace()", because
that code will do the free_task_struct too after having incremented the
usage count while they held on to it.

If you really really want to, you can do the memset() at the top of
"free_pages_ok()" - it will slow down the system considerably (because
_every_ page will be memset after being free'd), but hey, it's going to be
an even better stress-tester.

Oh, if you do it there, do something like thisL

	memset(page_address(page), 0xf0, PAGE_SIZE << order);

and realize that the above will not work on HIGHMEM machines (and making
it work on highmem machines is simply not worth it).

		Linus

Re: free_task_struct() called too early?

[Alan Cox]

> When a process terminates, it appears that the task structure is freed too
> early.  There are memory references to the kernel task area (task_struct and
> stack space) after free_task_struct(p) is called.
> 
> If I modify the following line in include/asm-i386/processor.h
> 
> #define free_task_struct(p)   free_pages((unsigned long) (p), 1) to
> #define free_task_struct(p)   memset((void*) (p), 0xf, PAGE_SIZE*2);
> free_pages((unsigned long) (p), 1)
> then kernel will boot to init and lockup on when first task terminates.
> 
> Has anyone looked into or aware of this issue?

2.4.8pre fixed a case with semaphores on the stack. It might not be the only
one. Your #define is wrong though, if a single free_task_struct path is an
if you will not do what you expect

	do { memset(), free_pages } while(0);

would be safer. 

I'd like to know if 2.4.8pre8 does it except on module unload (where it is
still buggy)

Alan