public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
From: Daniel Phillips <phillips@bonn-fries.net>
To: Richard Gooch <rgooch@ras.ucalgary.ca>
Cc: <alan@lxorguk.ukuu.org.uk>, <torvalds@transmeta.com>,
	<linux-kernel@vger.kernel.org>
Subject: Maximum size of automatic allocation? (was: [PATCH] fs/devfs/base.c)
Date: Mon, 28 May 2001 16:43:06 +0200	[thread overview]
Message-ID: <01052816430616.06233@starship> (raw)
In-Reply-To: <GLEPIDKFGKPCBDLKDHEAAELGDDAA.aki.jain@stanford.edu> <200105271321.f4RDLoM00342@mobilix.ras.ucalgary.ca>
In-Reply-To: <200105271321.f4RDLoM00342@mobilix.ras.ucalgary.ca>

On Sunday 27 May 2001 15:21, Richard Gooch wrote:
> Akash Jain writes:
> > in fs/devfs/base.c,
> > the struct devfsd_notify_struct is approx 1056 bytes, allocating it
> > on a stack of 8k seems unreasonable.  here we simply move it to the
> > heap, i don't think it is a _must_ be on stack type thing
>
> I absolutely don't want this patch applied. It's bogus. It is
> entirely safe to alloc 1 kB on the stack in this code, since it has a
> short and well-controlled code path from syscall entry to the
> function. This is not some function that can be called from some
> random place in the kernel and thus has a random call path.
>
> Using the stack is much faster than calling kmalloc() and it also
> doesn't add to system memory pressure. That's why I did it this way
> in the first place. Further, it's much safer to use the stack, since
> the memory is freed automatically. Thus, there's less scope for
> introducing errors.
>
> Please fix your checker to deal with this class of functions which
> have a well-defined call path. I'd suggest looking at the total stack
> allocations from syscall entry point all the way to the end function.
> Ideally, you'd trace the call path to every function, but of course
> that may be computationally infeasible. Hopefully it's feasible to do
> this for any function which has a stack allocation which exceeds some
> threshold.

I did the same thing in my directory indexing patch, but with a much 
larger buffer: 2728 bytes.  I traced the path from the syscall and all 
the paths out as well.  I did cause a stack overflow with this because 
gcc took the union of two such allocations in different control blocks 
instead of the intersection, much to my surprise.  This was cured by 
using fancier code to eliminate one of the allocations.  Al since broke 
this out into a separate function, making it more obviously safe, but 
note: it has to be broken out further so that there are no complex 
trees of calls descending from the stack storage pig.

This call appends a new block to a file then splits the contents of 
some other block into the new block using some workspace on the stack. 
The block has to be appended outside the function, otherwise the big 
stack allocation gets carried arbitrarily far through the kernel (think 
recursive allocation).  Similarly for mark_buffer_dirty and just to be 
safe, brelse as well.  Mark_buffer_dirty didn't use to have a big hairy 
call chain attached to it but it does now.   Even lowly brelse might 
evolve this way without warning.  The only safe thing to do is avoid 
all calls outside the subystem and comment the others.

My question: assuming the entire call chain is documented, exactly how 
much of the 8K kernel stack is safe to use?

--
Daniel

  reply	other threads:[~2001-05-28 14:41 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2001-05-27 10:12 [PATCH] fs/devfs/base.c Akash Jain
2001-05-27 13:21 ` Richard Gooch
2001-05-28 14:43   ` Daniel Phillips [this message]
2001-06-03 23:55   ` Linus Torvalds
2001-06-04  0:03     ` Richard Gooch
2001-06-04  6:44     ` Alan Cox
2001-06-04  7:07       ` Richard Gooch
2001-06-04 19:26         ` Bill Pringlemeir
2001-06-04 19:39           ` Kernel Stack usage [was: [PATCH] fs/devfs/base.c] Bill Pringlemeir
2001-06-05  6:10           ` [PATCH] fs/devfs/base.c H. Peter Anvin
2001-06-05  6:56             ` Alan Cox
2001-06-05 11:37               ` Andrew Morton
2001-06-05 21:38               ` Pavel Machek
2001-06-04 10:09       ` Linus Torvalds
2001-06-05  1:41       ` Dawson R Engler
2001-06-05  8:49       ` Ingo Molnar
2001-06-04 20:15     ` Daniel Phillips
2001-06-05  4:24       ` Paul Mackerras

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=01052816430616.06233@starship \
    --to=phillips@bonn-fries.net \
    --cc=alan@lxorguk.ukuu.org.uk \
    --cc=linux-kernel@vger.kernel.org \
    --cc=rgooch@ras.ucalgary.ca \
    --cc=torvalds@transmeta.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox