[PATCH] fs/devfs/base.c

[PATCH] fs/devfs/base.c

[Akash Jain]

hello,

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

-aki-


--- fs/devfs/base.c.orig	Tue Apr 17 15:04:10 2001
+++ fs/devfs/base.c	Tue May 22 23:59:54 2001
@@ -3153,7 +3153,7 @@
     int done = FALSE;
     int ival;
     loff_t pos, devname_offset, tlen, rpos;
-    struct devfsd_notify_struct info;
+    struct devfsd_notify_struct* info;
     struct devfsd_buf_entry *entry;
     struct fs_info *fs_info = file->f_dentry->d_inode->i_sb->u.generic_sbp;
     DECLARE_WAITQUEUE (wait, current);
@@ -3162,8 +3162,14 @@
     if (ppos != &file->f_pos) return -ESPIPE;
     /*  Verify the task has grabbed the queue  */
     if (fs_info->devfsd_task != current) return -EPERM;
-    info.major = 0;
-    info.minor = 0;
+
+    /* must alloc the info struct */
+    info = kmalloc(sizeof(struct devfsd_notify_struct), GFP_KERNEL);
+    if(info == NULL)
+        return -ENOMEM;
+
+    info->major = 0;
+    info->minor = 0;
     /*  Block for a new entry  */
     add_wait_queue (&fs_info->devfsd_wait_queue, &wait);
     current->state = TASK_INTERRUPTIBLE;
@@ -3177,6 +3183,7 @@
 	{
 	    remove_wait_queue (&fs_info->devfsd_wait_queue, &wait);
 	    current->state = TASK_RUNNING;
+	    kfree(info);
 	    return -EINTR;
 	}
 	set_current_state(TASK_INTERRUPTIBLE);
@@ -3186,18 +3193,18 @@
     /*  Now play with the data  */
     ival = atomic_read (&fs_info->devfsd_overrun_count);
     if (ival > 0) atomic_sub (ival, &fs_info->devfsd_overrun_count);
-    info.overrun_count = ival;
+    info->overrun_count = ival;
     entry = (struct devfsd_buf_entry *) fs_info->devfsd_buffer +
 	fs_info->devfsd_buf_out;
-    info.type = entry->type;
-    info.mode = entry->mode;
-    info.uid = entry->uid;
-    info.gid = entry->gid;
+    info->type = entry->type;
+    info->mode = entry->mode;
+    info->uid = entry->uid;
+    info->gid = entry->gid;
     if (entry->type == DEVFSD_NOTIFY_LOOKUP)
     {
-	info.namelen = strlen (entry->data);
+	info->namelen = strlen (entry->data);
 	pos = 0;
-	memcpy (info.devname, entry->data, info.namelen + 1);
+	memcpy (info->devname, entry->data, info->namelen + 1);
     }
     else
     {
@@ -3205,23 +3212,27 @@

 	if ( S_ISCHR (de->mode) || S_ISBLK (de->mode) || S_ISREG (de->mode) )
 	{
-	    info.major = de->u.fcb.u.device.major;
-	    info.minor = de->u.fcb.u.device.minor;
+	    info->major = de->u.fcb.u.device.major;
+	    info->minor = de->u.fcb.u.device.minor;
+	}
+	pos = devfs_generate_path (de, info->devname, DEVFS_PATHLEN);
+	if (pos < 0) {
+	  kfree(info);
+	  return pos;
 	}
-	pos = devfs_generate_path (de, info.devname, DEVFS_PATHLEN);
-	if (pos < 0) return pos;
-	info.namelen = DEVFS_PATHLEN - pos - 1;
-	if (info.mode == 0) info.mode = de->mode;
+	info->namelen = DEVFS_PATHLEN - pos - 1;
+	if (info->mode == 0) info->mode = de->mode;
     }
-    devname_offset = info.devname - (char *) &info;
+    devname_offset = info->devname - (char *) info;
     rpos = *ppos;
     if (rpos < devname_offset)
     {
 	/*  Copy parts of the header  */
 	tlen = devname_offset - rpos;
 	if (tlen > len) tlen = len;
-	if ( copy_to_user (buf, (char *) &info + rpos, tlen) )
+	if ( copy_to_user (buf, (char *) info + rpos, tlen) )
 	{
+	    kfree(info);
 	    return -EFAULT;
 	}
 	rpos += tlen;
@@ -3231,12 +3242,13 @@
     if ( (rpos >= devname_offset) && (len > 0) )
     {
 	/*  Copy the name  */
-	tlen = info.namelen + 1;
+	tlen = info->namelen + 1;
 	if (tlen > len) tlen = len;
 	else done = TRUE;
-	if ( copy_to_user (buf, info.devname + pos + rpos - devname_offset,
+	if ( copy_to_user (buf, info->devname + pos + rpos - devname_offset,
 			   tlen) )
 	{
+	    kfree(info);
 	    return -EFAULT;
 	}
 	rpos += tlen;
@@ -3251,6 +3263,7 @@
 	*ppos = 0;
     }
     else *ppos = rpos;
+    kfree(info);
     return tlen;
 }   /*  End Function devfsd_read  */

Re: [PATCH] fs/devfs/base.c

[Richard Gooch]

Akash Jain writes:
> hello,
> 
> 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.

				Regards,

					Richard....
Permanent: rgooch@atnf.csiro.au
Current:   rgooch@ras.ucalgary.ca

Maximum size of automatic allocation? (was: [PATCH] fs/devfs/base.c)

[Daniel Phillips]

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

Re: [PATCH] fs/devfs/base.c

[Linus Torvalds]

On Sun, 27 May 2001, Richard Gooch wrote:
> 
> 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.

IT IS NEVER EVER SAFE TO ALLOCATE 1kB OF STACK!

Why?
 - automatic checkers are wonderful, and we do not want to have "oh, in
   this case it is magically ok" kinds of things.
 - the kernel stack is 4kB, and _nobody_ has the right to eat up a
   noticeable portion of it. It doesn't matter if you "know" your caller
   or not: you do not know what interrupts happen during this time, and
   how much stack they want.

Ergo: the simple rule of "don't allocate big structures of the stack" is
always a good rule, and making excuses for it is bad.

		Linus

Re: [PATCH] fs/devfs/base.c

[Richard Gooch]

Linus Torvalds writes:
> 
> On Sun, 27 May 2001, Richard Gooch wrote:
> > 
> > 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.
> 
> IT IS NEVER EVER SAFE TO ALLOCATE 1kB OF STACK!

I can see you care about this ;-)

> Why?
>  - the kernel stack is 4kB, and _nobody_ has the right to eat up a
>    noticeable portion of it. It doesn't matter if you "know" your caller
>    or not: you do not know what interrupts happen during this time, and
>    how much stack they want.

OK, that's a good point. Easy solution: disable interrupts in that
function.

Only kidding.

> Ergo: the simple rule of "don't allocate big structures of the
> stack" is always a good rule, and making excuses for it is bad.

OK, well, I can make the structure static instead, since the function
is single-threaded anyway.

				Regards,

					Richard....
Permanent: rgooch@atnf.csiro.au
Current:   rgooch@ras.ucalgary.ca

Re: [PATCH] fs/devfs/base.c

[Alan Cox]

>  - the kernel stack is 4kB, and _nobody_ has the right to eat up a
>    noticeable portion of it. It doesn't matter if you "know" your caller

Umm Linus on what platform - its 8K or more on all that I can think of

> Ergo: the simple rule of "don't allocate big structures of the stack" is
> always a good rule, and making excuses for it is bad.

We have a very large number of violators of 1K of stack, and very few of 2K 
right now.

Re: [PATCH] fs/devfs/base.c

[Richard Gooch]

Alan Cox writes:
> >  - the kernel stack is 4kB, and _nobody_ has the right to eat up a
> >    noticeable portion of it. It doesn't matter if you "know" your caller
> 
> Umm Linus on what platform - its 8K or more on all that I can think of

I assume he's referring to what's left after you take out the task
struct. More than 4 kiB, so 4 kiB is a conservative estimate.

> > Ergo: the simple rule of "don't allocate big structures of the stack" is
> > always a good rule, and making excuses for it is bad.
> 
> We have a very large number of violators of 1K of stack, and very
> few of 2K right now.

I guess we should ask the question as to what's an acceptable usage.
Theoretically, any amount could pose a problem, but that's hardly a
useful position to work from. Some decision or guideline about a
practical limit would be helpful. My gut feeling is that 1 kiB is
around the limit for functions with a well defined call path. The
limit should probably be less for generic functions and interrupt
handlers.

				Regards,

					Richard....
Permanent: rgooch@atnf.csiro.au
Current:   rgooch@ras.ucalgary.ca

Re: [PATCH] fs/devfs/base.c

[Linus Torvalds]

On Mon, 4 Jun 2001, Alan Cox wrote:
>
> >  - the kernel stack is 4kB, and _nobody_ has the right to eat up a
> >    noticeable portion of it. It doesn't matter if you "know" your caller
>
> Umm Linus on what platform - its 8K or more on all that I can think of

Ehh.. It's _noticeably_ less than 8kB at least on i386.

> We have a very large number of violators of 1K of stack, and very few of 2K
> right now.

"Two wrongs do not make a right".

		Linus

Re: [PATCH] fs/devfs/base.c

[Bill Pringlemeir]

>>>>> "Richard" == Richard Gooch <rgooch@ras.ucalgary.ca> writes:
[snip]
 > I guess we should ask the question as to what's an
 > acceptable usage.  Theoretically, any amount could pose a
 > problem, but that's hardly a useful position to work

There was a discussion on comp.arch.embedded about bounded stack use.
It is fairly easy to calculate the stack usage for call trees, but
much more difficult for `DAGs'.  Ie, a recursive functions etc.  I
don't know about the policy on recursion in the kernel, but I think it
would be bad.

Perhaps the checker could be modified to keep track of the call tree
and find the largest value used in the tree.  Each function will have
a maximum, to which you should add the interrupt handling overhead,
which would be calculated in a similar way.  This will work if you do
not allow re-entrant interrupts and you do not have any `cycles' in the
function call hierarchies.

hth,
Bill Pringlemeir.

Kernel Stack usage [was: [PATCH] fs/devfs/base.c]

[Bill Pringlemeir]

>>>>> Bill Pringlemeir <bpringle@sympatico.ca> writes:

  > There was a discussion on comp.arch.embedded about bounded stack
  > use.  It is fairly easy to calculate the stack usage for call
  > trees, but much more difficult for `DAGs'.  Ie, a recursive
  > functions etc.  I don't know about the policy on recursion in the
  > kernel, but I think it would be bad.

  > Perhaps the checker could be modified to keep track of the call
  > tree and find the largest value used in the tree.  Each function
  > will have a maximum, to which you should add the interrupt
  > handling overhead, which would be calculated in a similar way.
  > This will work if you do not allow re-entrant interrupts and you
  > do not have any `cycles' in the function call hierarchies.

Sorry, I neglected the important case of `alloca', and other variable
length stack allocation functions/constructs.  Maybe this becomes too
restrictive to be useful.

regards,
Bill Pringlemeir

Re: [PATCH] fs/devfs/base.c

[Daniel Phillips]

On Monday 04 June 2001 01:55, Linus Torvalds wrote:
>  - the kernel stack is 4kB, and _nobody_ has the right to eat up a
>    noticeable portion of it. It doesn't matter if you "know" your
> caller or not: you do not know what interrupts happen during this
> time, and how much stack they want.

We'd better know the upper bound of interrupt allocations or we have an 
accident waiting to happen.  How much of the kernel stack is reserved 
for interrupts?

--
Daniel

Re: [PATCH] fs/devfs/base.c

[Dawson R Engler]

> We have a very large number of violators of 1K of stack, and very few of 2K 
> right now.

Yeah, there's 25 that we know in 2.4.5-ac4 of that allocate a single
large var -- one of 4k, a few of 3k, looks like 8 of 2k, and then a tail
of 1Ks.  This isn't counting the functions that in aggregate have large
allocations, or have deep call chains.

Dawson

# BUGs	|	File Name
5	|	drivers/message/i2o/i2o_proc.c
4	|	drivers/cdrom/cdrom.c
2	|	net/irda/af_irda.c
1	|	fs/nfs/nfsroot.c
1	|	drivers/sound/emu10k1/audio.c
1	|	drivers/scsi/qlogicfc.c
1	|	arch/i386/kernel/nmi.c
1	|	drivers/net/wan/cycx_x25.c
1	|	drivers/block/cpqarray.c
1	|	drivers/media/video/w9966.c
1	|	arch/i386/kernel/setup.c
1	|	fs/devfs/base.c
1	|	drivers/net/ewrk3.c
1	|	net/wanrouter/wanmain.c
1	|	net/bridge/br_ioctl.c
1	|	drivers/cdrom/optcd.c
1	|	drivers/atm/iphase.c

#
---------------------------------------------------------
[BUG]
/u2/engler/mc/oses/linux/2.4.5-ac4/drivers/sound/emu10k1/audio.c:906:emu10k1_audio_ioctl: ERROR:VAR:906:906: suspicious var 'buf' = 4016 bytes:906 [nbytes=4016]

		break;

	case SNDCTL_COPR_LOAD:
		{

Error --->
			copr_buffer buf;
---------------------------------------------------------
[BUG]
/u2/engler/mc/oses/linux/2.4.5-ac4/drivers/message/i2o/i2o_proc.c:955:i2o_proc_read_drivers_stored: ERROR:VAR:955:955: suspicious var 'result' = 3596 bytes:955 [nbytes=3596]
		u8  block_status;
		u8  error_info_size;
		u16 row_count;
		u16 more_flag;
		i2o_driver_store_table dst[MAX_I2O_MODULES];

Error --->
	} result;
---------------------------------------------------------
[BUG]
/u2/engler/mc/oses/linux/2.4.5-ac4/drivers/net/ewrk3.c:1639:ewrk3_ioctl: ERROR:VAR:1639:1639: suspicious var 'tmp' = 3072 bytes:1639 [nbytes=3072]
	int i, j, status = 0;
	u_char csr;
	union {
		u_char addr[HASH_TABLE_LEN * ETH_ALEN];
		u_short val[(HASH_TABLE_LEN * ETH_ALEN) >> 1];

Error --->
	} tmp;
---------------------------------------------------------
[BUG]
/u2/engler/mc/oses/linux/2.4.5-ac4/drivers/scsi/qlogicfc.c:867:isp2x00_make_portdb: ERROR:VAR:867:867: suspicious var 'temp' = 3072 bytes:867 [nbytes=3072]
static int isp2x00_make_portdb(struct Scsi_Host *host)
{

	short param[8];
	int i, j;

Error --->
	struct id_name_map temp[QLOGICFC_MAX_ID + 1];
---------------------------------------------------------
[BUG]
/u2/engler/mc/oses/linux/2.4.5-ac4/drivers/message/i2o/i2o_proc.c:840:i2o_proc_read_ddm_table: ERROR:VAR:840:840: suspicious var 'result' = 2828 bytes:840 [nbytes=2828]
		u8  block_status;
		u8  error_info_size;
		u16 row_count;
		u16 more_flag;
		i2o_exec_execute_ddm_table ddm_table[MAX_I2O_MODULES];

Error --->
	} result;
---------------------------------------------------------
[BUG]
/u2/engler/mc/oses/linux/2.4.5-ac4/drivers/cdrom/optcd.c:1625:cdromread: ERROR:VAR:1625:1625: suspicious var 'buf' = 2646 bytes:1625 [nbytes=2646]

static int cdromread(unsigned long arg, int blocksize, int cmd)
{
	int status;
	struct cdrom_msf msf;

Error --->
	char buf[CD_FRAMESIZE_RAWER];
---------------------------------------------------------
[BUG]
/u2/engler/mc/oses/linux/2.4.5-ac4/drivers/message/i2o/i2o_proc.c:2261:i2o_proc_read_lan_mcast_addr: ERROR:VAR:2261:2261: suspicious var 'result' = 2060 bytes:2261 [nbytes=2060]
		u8  block_status;
		u8  error_info_size;
		u16 row_count;
		u16 more_flag;
		u8  mc_addr[256][8];

Error --->
	} result;	
---------------------------------------------------------
[BUG]
/u2/engler/mc/oses/linux/2.4.5-ac4/drivers/message/i2o/i2o_proc.c:1044:i2o_proc_read_groups: ERROR:VAR:1044:1044: suspicious var 'result' = 2060 bytes:1044 [nbytes=2060]
		u8  block_status;
		u8  error_info_size;
		u16 row_count;
		u16 more_flag;
		i2o_group_info group[256];

Error --->
	} result;
---------------------------------------------------------
[BUG]
/u2/engler/mc/oses/linux/2.4.5-ac4/drivers/message/i2o/i2o_proc.c:2492:i2o_proc_read_lan_alt_addr: ERROR:VAR:2492:2492: suspicious var 'result' = 2060 bytes:2492 [nbytes=2060]
		u8  block_status;
		u8  error_info_size;
		u16 row_count;
		u16 more_flag;
		u8  alt_addr[256][8];

Error --->
	} result;	
---------------------------------------------------------
[BUG]
/u2/engler/mc/oses/linux/2.4.5-ac4/drivers/media/video/w9966.c:921:w9966_v4l_read: ERROR:VAR:921:921: suspicious var 'tbuf' = 2048 bytes:921 [nbytes=2048]
	}
	
	while(dleft > 0)
	{
		unsigned long tsize = (dleft > W9966_RBUFFER) ? W9966_RBUFFER : dleft;

Error --->
		unsigned char tbuf[W9966_RBUFFER];
---------------------------------------------------------
[BUG]
/u2/engler/mc/oses/linux/2.4.5-ac4/drivers/atm/iphase.c:2768:ia_ioctl: ERROR:VAR:2768:2768: suspicious var 'regs_local' = 2048 bytes:2768 [nbytes=2048]
             ia_cmds.status = 0;
             ia_cmds.len = 0x80;
             break;
          case MEMDUMP_FFL:
          {  

Error --->
             ia_regs_t       regs_local;
---------------------------------------------------------
[BUG]
/u2/engler/mc/oses/linux/2.4.5-ac4/arch/i386/kernel/setup.c:579:sanitize_e820_map: ERROR:VAR:579:579: function stack consumes 1840 bytes:579 [nbytes=1840]
			{
				if (overlap_list[i] == change_point[chgidx]->pbios)
					overlap_list[i] = overlap_list[overlap_entries-1];
			}
			overlap_entries--;

Error --->
		}
---------------------------------------------------------
[BUG]
/u2/engler/mc/oses/linux/2.4.5-ac4/net/irda/af_irda.c:1743:irda_setsockopt: ERROR:VAR:1743:1743: suspicious var 'ias_opt' = 1356 bytes:1743 [nbytes=1356]
static int irda_setsockopt(struct socket *sock, int level, int optname, 
			   char *optval, int optlen)
{
 	struct sock *sk = sock->sk;
	struct irda_sock *self;

Error --->
	struct irda_ias_set	ias_opt;
---------------------------------------------------------
[BUG]
/u2/engler/mc/oses/linux/2.4.5-ac4/net/irda/af_irda.c:1981:irda_getsockopt: ERROR:VAR:1981:1981: suspicious var 'ias_opt' = 1356 bytes:1981 [nbytes=1356]
{
	struct sock *sk = sock->sk;
	struct irda_sock *self;
	struct irda_device_list list;
	struct irda_device_info *discoveries;

Error --->
	struct irda_ias_set	ias_opt;	/* IAS get/query params */
---------------------------------------------------------
[BUG]
/u2/engler/mc/oses/linux/2.4.5-ac4/drivers/block/cpqarray.c:1196:ida_ioctl: ERROR:VAR:1196:1196: suspicious var 'my_io' = 1296 bytes:1196 [nbytes=1296]
	int dsk  = MINOR(inode->i_rdev) >> NWD_SHIFT;
	int error;
	int diskinfo[4];
	struct hd_geometry *geo = (struct hd_geometry *)arg;
	ida_ioctl_t *io = (ida_ioctl_t*)arg;

Error --->
	ida_ioctl_t my_io;
---------------------------------------------------------
[BUG]
/u2/engler/mc/oses/linux/2.4.5-ac4/net/wanrouter/wanmain.c:765:device_new_if: ERROR:VAR:765:765: suspicious var 'conf' = 1272 bytes:765 [nbytes=1272]
 *	o register network interface
 */

static int device_new_if (wan_device_t *wandev, wanif_conf_t *u_conf)
{

Error --->
	wanif_conf_t conf;
---------------------------------------------------------
[BUG]
/u2/engler/mc/oses/linux/2.4.5-ac4/fs/devfs/base.c:3155:devfsd_read: ERROR:VAR:3155:3155: suspicious var 'info' = 1056 bytes:3155 [nbytes=1056]
			    loff_t *ppos)
{
    int done = FALSE;
    int ival;
    loff_t pos, devname_offset, tlen, rpos;

Error --->
    struct devfsd_notify_struct info;
---------------------------------------------------------
[BUG]
/u2/engler/mc/oses/linux/2.4.5-ac4/drivers/cdrom/cdrom.c:815:cdrom_number_of_slots: ERROR:VAR:815:815: suspicious var 'info' = 1032 bytes:815 [nbytes=1032]
 */
int cdrom_number_of_slots(struct cdrom_device_info *cdi) 
{
	int status;
	int nslots = 1;

Error --->
	struct cdrom_changer_info info;
---------------------------------------------------------
[BUG]
/u2/engler/mc/oses/linux/2.4.5-ac4/drivers/cdrom/cdrom.c:1612:cdrom_ioctl: ERROR:VAR:1612:1612: suspicious var 'info' = 1032 bytes:1612 [nbytes=1032]
			cdi->options |= CDO_AUTO_CLOSE | CDO_AUTO_EJECT;
		return 0;
		}

	case CDROM_MEDIA_CHANGED: {

Error --->
		struct cdrom_changer_info info;
---------------------------------------------------------
[BUG]
/u2/engler/mc/oses/linux/2.4.5-ac4/drivers/cdrom/cdrom.c:791:cdrom_slot_status: ERROR:VAR:791:791: suspicious var 'info' = 1032 bytes:791 [nbytes=1032]
	return cdo->generic_packet(cdi, &cgc);
}

static int cdrom_slot_status(struct cdrom_device_info *cdi, int slot)
{

Error --->
	struct cdrom_changer_info info;
---------------------------------------------------------
[BUG]
/u2/engler/mc/oses/linux/2.4.5-ac4/drivers/cdrom/cdrom.c:857:cdrom_select_disc: ERROR:VAR:857:857: suspicious var 'info' = 1032 bytes:857 [nbytes=1032]
	return cdi->ops->generic_packet(cdi, &cgc);
}

int cdrom_select_disc(struct cdrom_device_info *cdi, int slot)
{

Error --->
	struct cdrom_changer_info info;
---------------------------------------------------------
[BUG]
/u2/engler/mc/oses/linux/2.4.5-ac4/arch/i386/kernel/nmi.c:47:check_nmi_watchdog: ERROR:VAR:47:47: suspicious var 'tmp' = 1024 bytes:47 [nbytes=1024]
#define P6_EVENT_CPU_CLOCKS_NOT_HALTED	0x79
#define P6_NMI_EVENT		P6_EVENT_CPU_CLOCKS_NOT_HALTED

int __init check_nmi_watchdog (void)
{

Error --->
	irq_cpustat_t tmp[NR_CPUS];
---------------------------------------------------------
[BUG]
/u2/engler/mc/oses/linux/2.4.5-ac4/net/bridge/br_ioctl.c:86:br_ioctl_device: ERROR:VAR:86:86: suspicious var 'indices' = 1024 bytes:86 [nbytes=1024]
	}

	case BRCTL_GET_PORT_LIST:
	{
		int i;

Error --->
		int indices[256];
---------------------------------------------------------
[BUG]
/u2/engler/mc/oses/linux/2.4.5-ac4/fs/nfs/nfsroot.c:238:root_nfs_name: ERROR:VAR:238:238: suspicious var 'buf' = 1024 bytes:238 [nbytes=1024]
/*
 *  Prepare the NFS data structure and parse all options.
 */
static int __init root_nfs_name(char *name)
{

Error --->
	char buf[NFS_MAXPATHLEN];
---------------------------------------------------------
[BUG]
/u2/engler/mc/oses/linux/2.4.5-ac4/drivers/net/wan/cycx_x25.c:983:hex_dump: ERROR:VAR:983:983: suspicious var 'hex' = 1024 bytes:983 [nbytes=1024]
			 card->devname, cmd->command);
}
#ifdef CYCLOMX_X25_DEBUG
static void hex_dump(char *msg, unsigned char *p, int len)
{

Error --->
	unsigned char hex[1024],

Re: [PATCH] fs/devfs/base.c

[Paul Mackerras]

Daniel Phillips writes:

> We'd better know the upper bound of interrupt allocations or we have an 
> accident waiting to happen.  How much of the kernel stack is reserved 
> for interrupts?

Since interrupt handlers generally run with other interrupts enabled,
and only their own interrupt disabled, it seems to me that the bound
on how much stack space you need to leave for interrupt handlers
depends on how many different interrupts you have in the system.  On a
large system there could easily be tens or even hundreds of active
devices, all with different IRQs.  It would be possible (although
unlikely) for them to all to interrupt at just the right time to get
all their handlers stacked, and that could easily overflow the stack.

One solution would be to start running interrupt handlers with
interrupts disabled (__cli) when they are getting close to being too
deeply nested - it would not be hard to check the stack pointer and if
there is less than some defined amount of stack space left then we
don't do the __sti before calling the handler.

Paul.

Re: [PATCH] fs/devfs/base.c

[H. Peter Anvin]

Followup to:  <m2elt011y6.fsf@sympatico.ca>
By author:    Bill Pringlemeir <bpringle@sympatico.ca>
In newsgroup: linux.dev.kernel
> 
> There was a discussion on comp.arch.embedded about bounded stack use.
> It is fairly easy to calculate the stack usage for call trees, but
> much more difficult for `DAGs'.  Ie, a recursive functions etc.
> 

It's trivial to calculate for DAGs -- directed acyclic graphs.  It's
when the "acyclic" constraint is violated that you have problems!

	-hpa
-- 
<hpa@transmeta.com> at work, <hpa@zytor.com> in private!
"Unix gives you enough rope to shoot yourself in the foot."
http://www.zytor.com/~hpa/puzzle.txt

Re: [PATCH] fs/devfs/base.c

[Alan Cox]

> It's trivial to calculate for DAGs -- directed acyclic graphs.  It's
> when the "acyclic" constraint is violated that you have problems!

It may well be that interrupt stacks are a win anyway. If we can get the kernel
struct out of the stack pages (which would fix some very unpleasant cache
colour problems) and take the non irq stack down to 4K then irq stacks would
pay off once you had 25 or so processes on a system

Re: [PATCH] fs/devfs/base.c

[Ingo Molnar]

On Mon, 4 Jun 2001, Alan Cox wrote:

> >  - the kernel stack is 4kB, and _nobody_ has the right to eat up a
> >    noticeable portion of it. It doesn't matter if you "know" your caller
>
> Umm Linus on what platform - its 8K or more on all that I can think of

it's 8K-sizeof(struct task_struct). On x86, the size of task_struct is
getting near to 2K already, and it's not getting smaller in the future, so
4K is a safe thing.

	Ingo

Re: [PATCH] fs/devfs/base.c

[Andrew Morton]

Alan Cox wrote:
> 
> > It's trivial to calculate for DAGs -- directed acyclic graphs.  It's
> > when the "acyclic" constraint is violated that you have problems!
> 
> It may well be that interrupt stacks are a win anyway. If we can get the kernel
> struct out of the stack pages (which would fix some very unpleasant cache
> colour problems) and take the non irq stack down to 4K then irq stacks would
> pay off once you had 25 or so processes on a system

All this talk about stack utilisation, we may as well fix 
up the current (ly broken) instrumentation.

The patch enables the display of least-ever stack headroom
in the sysrq-T handler.  Also prints a little summary at
the end of the SYSRQ-T output which shows:

a) which currently-running pid has used the most stack and

b) the name of the process which used the most stack since
   the machine booted.

On my super-stripped-down development box, the answer is

	Minimum ever: `zsh' with 3908 bytes

Against -ac7, it's all dependent upon CONFIG_DEBUG_SLAB.





--- linux-2.4.5-ac7/include/asm-i386/processor.h	Tue Jun  5 17:46:37 2001
+++ ac/include/asm-i386/processor.h	Tue Jun  5 21:20:47 2001
@@ -446,7 +446,21 @@
 #define KSTK_ESP(tsk)	(((unsigned long *)(4096+(unsigned long)(tsk)))[1022])
 
 #define THREAD_SIZE (2*PAGE_SIZE)
+#ifdef CONFIG_DEBUG_SLAB	/* For tracking max stack utilisation */
+#define HAVE_STACK_PREFILL
+unsigned min_stack_headroom(struct task_struct *tsk);
+#define alloc_task_struct()								\
+	({										\
+		struct task_struct *p =							\
+			((struct task_struct *) __get_free_pages(GFP_KERNEL,1));	\
+		if (p)									\
+			memset(p+1, 0xa5, PAGE_SIZE*2 - sizeof(*p));			\
+		p;									\
+	})
+#else
 #define alloc_task_struct() ((struct task_struct *) __get_free_pages(GFP_KERNEL,1))
+#endif
+
 #define free_task_struct(p) free_pages((unsigned long) (p), 1)
 #define get_task_struct(tsk)      atomic_inc(&virt_to_page(tsk)->count)
 
--- linux-2.4.5-ac7/kernel/sched.c	Tue Jun  5 17:46:37 2001
+++ ac/kernel/sched.c	Tue Jun  5 21:30:02 2001
@@ -1101,9 +1101,31 @@
 	return retval;
 }
 
-static void show_task(struct task_struct * p)
+#ifdef HAVE_STACK_PREFILL
+
+static unsigned min_ever_stack_headroom = ~0U;
+static char min_stack_name[16];
+
+/* tasklist_lock needed if tsk != current */
+unsigned min_stack_headroom(struct task_struct *tsk)
+{
+	unsigned long *p = (unsigned long *)(tsk + 1);
+	unsigned ret;
+	while (*p == 0xa5a5a5a5)
+		p++;
+	ret = (unsigned long)p - (unsigned long)(tsk + 1);
+	if (ret < min_ever_stack_headroom) {
+		min_ever_stack_headroom = ret;
+		strncpy(min_stack_name, tsk->comm, sizeof(min_stack_name));
+	}
+	return ret;
+}
+
+#endif
+
+static void show_task(struct task_struct * p,
+		unsigned *least_spare_stack, pid_t *pid)
 {
-	unsigned long free = 0;
 	int state;
 	static const char * stat_nam[] = { "R", "S", "D", "Z", "T", "W" };
 
@@ -1124,13 +1146,19 @@
 	else
 		printk(" %016lx ", thread_saved_pc(&p->thread));
 #endif
+#ifdef HAVE_STACK_PREFILL
 	{
-		unsigned long * n = (unsigned long *) (p+1);
-		while (!*n)
-			n++;
-		free = (unsigned long) n - (unsigned long)(p+1);
+		unsigned long free = min_stack_headroom(p);
+
+		printk("%5lu %5d %6d ", free, p->pid, p->p_pptr->pid);
+		if (free < *least_spare_stack) {
+			*least_spare_stack = free;
+			*pid = p->pid;
+		}
 	}
-	printk("%5lu %5d %6d ", free, p->pid, p->p_pptr->pid);
+#else
+	printk("%5d ????? %6d ", p->pid, p->p_pptr->pid);
+#endif
 	if (p->p_cptr)
 		printk("%5d ", p->p_cptr->pid);
 	else
@@ -1175,6 +1203,8 @@
 void show_state(void)
 {
 	struct task_struct *p;
+	unsigned least_spare_stack;
+	pid_t pid;
 
 #if (BITS_PER_LONG == 32)
 	printk("\n"
@@ -1185,6 +1215,8 @@
 	       "                                 free                        sibling\n");
 	printk("  task                 PC        stack   pid father child younger older\n");
 #endif
+	least_spare_stack = ~0U;
+	pid = -1;
 	read_lock(&tasklist_lock);
 	for_each_task(p) {
 		/*
@@ -1192,9 +1224,17 @@
 		 * console might take alot of time:
 		 */
 		touch_nmi_watchdog();
-		show_task(p);
+		show_task(p, &least_spare_stack, &pid);
 	}
 	read_unlock(&tasklist_lock);
+#ifdef HAVE_STACK_PREFILL
+	if (pid != -1) {
+		printk("Minimum stack headroom: pid %d with %u bytes\n",
+			pid, least_spare_stack);
+		printk("Minimum ever: `%s' with %u bytes\n",
+				min_stack_name, min_ever_stack_headroom);
+	}
+#endif
 }
 
 /*
--- linux-2.4.5-ac7/kernel/exit.c	Mon May 28 13:31:49 2001
+++ ac/kernel/exit.c	Tue Jun  5 21:07:34 2001
@@ -455,6 +455,9 @@
 
 	tsk->exit_code = code;
 	exit_notify();
+#ifdef HAVE_STACK_PREFILL
+	min_stack_headroom(current);
+#endif
 	schedule();
 	BUG();
 /*

Re: [PATCH] fs/devfs/base.c

[Pavel Machek]

Hi!

> > It's trivial to calculate for DAGs -- directed acyclic graphs.  It's
> > when the "acyclic" constraint is violated that you have problems!
> 
> It may well be that interrupt stacks are a win anyway. If we can get the kernel
> struct out of the stack pages (which would fix some very unpleasant cache
> colour problems) and take the non irq stack down to 4K then irq stacks would
> pay off once you had 25 or so processes on a system

For what it is worth, we are using interrupt stack on x86-64. And it
was not *that* painfull.
								Pavel
-- 
I'm pavel@ucw.cz. "In my country we have almost anarchy and I don't care."
Panos Katsaloulis describing me w.r.t. patents at discuss@linmodems.org