A loop mount/umounting a pcdrw or iso9660 (through the pktcdvd device)
sees a stack overflow in four or five tries. Doing the same thing with
the same CD in a normal non-pktcdvd-mounted drive doesn't cause a crash.

Here's a couple of oopses. config follows.

(There are a wide variety. Some I couldn't collect because they appeared
to recurse, blurring past much too fast to read. Some simply didn't go
out of the netconsole logger at all for no obvious reason.)

(This may or may not be PREEMPT+PREEMPT_BKL-specific: I'll try turning
them off tomorrow and repeating.)

(The presence of dm in the first oops below must surely be attributed to
preempt: certainly my CD isn't managed by dm :) )

pktcdvd: Fixed packets, 32 blocks, Mode-2 disc
pktcdvd: Max. media speed: 4
pktcdvd: write speed 2x
pktcdvd: 551232kB available on disc
UDF-fs INFO UDF 0.9.8.1 (2004/29/09) Mounting volume 'LinuxUDF', timestamp 2004/02/09 07:10 (1000)
do_IRQ: stack overflow: 480
Pid: 4645, comm: mount Not tainted 2.6.24.2-dirty #4
 [<c0104171>] do_IRQ+0x66/0xc5
 [<c0102f8b>] common_interrupt+0x23/0x28
 [<c027b5da>] ide_outsl+0x5/0x9
 [<c027c540>] ata_output_data+0x4d/0x64
 [<c027b8a6>] atapi_output_bytes+0x19/0x3f
 [<c0285377>] cdrom_transfer_packet_command+0xb5/0xde
 [<c0282607>] cdrom_timer_expiry+0x0/0x51
 [<c02840c3>] cdrom_start_packet_command+0x14f/0x157
 [<c02853e9>] cdrom_do_pc_continuation+0x0/0x2c
 [<c027aa33>] ide_do_request+0x70a/0x943
 [<c0363ef3>] schedule_timeout+0x13/0x8b
 [<c021faeb>] elv_drain_elevator+0x15/0x58
 [<c0220277>] elv_insert+0xf6/0x1d9
 [<c0285377>] cdrom_transfer_packet_command+0xb5/0xde
 [<c0282607>] cdrom_timer_expiry+0x0/0x51
 [<c027b038>] ide_do_drive_cmd+0x99/0xe9
 [<c0282abe>] cdrom_queue_packet_command+0x35/0xa9
 [<c0363b2b>] schedule+0x321/0x33e
 [<c0363ef3>] schedule_timeout+0x13/0x8b
 [<c0282d11>] cdrom_read_tocentry+0x96/0xa1
 [<c02220d3>] b...
[ message continues ]

It is not preempt-specific, nor dm-specific. Nor it is very easy to
capture tracebacks of: even netconsole generally gives up when faced
with a string of recursive tracebacks blurring past forever at blinding
speed.

But while I'd normally blame pktcdvd there's only one pktcdvd function
in these tracebacks (pkt_open) and it's not got a significant stack
footprint.

More notable is a great stack of mutual recursion between
dm_bio_destructor() and the CDROM code: it seems to burn most of the
stack on this sort of thrashing. Here's one of those tracebacks again:

do_IRQ: stack overflow: 480
id: 4645, comm: mount Not tainted 2.6.24.2-dirty #4
 [<c0104171>] do_IRQ+0x66/0xc5
 [<c0102f8b>] common_interrupt+0x23/0x28
 [<c027b5da>] ide_outsl+0x5/0x9
 [<c027c540>] ata_output_data+0x4d/0x64
 [<c027b8a6>] atapi_output_bytes+0x19/0x3f
 [<c0285377>] cdrom_transfer_packet_command+0xb5/0xde
 [<c0282607>] cdrom_timer_expiry+0x0/0x51
 [<c02840c3>] cdrom_start_packet_command+0x14f/0x157
 [<c02853e9>] cdrom_do_pc_continuation+0x0/0x2c
 [<c027aa33>] ide_do_request+0x70a/0x943
 [<c0363ef3>] schedule_timeout+0x13/0x8b
 [<c021faeb>] elv_drain_elevator+0x15/0x58
 [<c0220277>] elv_insert+0xf6/0x1d9
 [<c0285377>] cdrom_transfer_packet_command+0xb5/0xde
 [<c0282607>] cdrom_timer_expiry+0x0/0x51
 [<c027b038>] ide_do_drive_cmd+0x99/0xe9
 [<c0282abe>] cdrom_queue_packet_command+0x35/0xa9
 [<c0363b2b>] schedule+0x321/0x33e
 [<c0363ef3>] schedule_timeout+0x13/0x8b
 [<c0282d11>] cdrom_read_tocentry+0x96/0xa1
 [<c02220d3>] blk_end_sync_rq+0x0/0x23
 [<c028315b>] cdrom_read_toc+0x14b/0x42e
 [<c02220d3>] blk_end_sync_rq+0x0/0x23
 [<c027b07e>] ide_do_drive_cmd+0xdf/0xe9
 [<c0283ed2>] ide_cdrom_audio_ioctl+0x13c/0x1de
 [<c02af8fe>] dm_bio_destructor+0x0/0x8
 [<c017162c>] end_bio_bh_io_sync+0x0/0x27
 [<c0282f42>] cdrom_check_status+0x55/0x60
 [...
[ message continues ]

Did you verify that with "make checkstack" or just by looking at the
source code? On my system, pkt_open() consumes 584 bytes because the
compiler decides to inline lots of functions that would not normally
be part of long call chains. The following patch fixes that problem on

Maybe dm_bio_destructor() is just old cruft left on the stack from
previous function calls?


From: Peter Osterlund <petero2@telia.com>

Signed-off-by: Peter Osterlund <petero2@telia.com>
---

 drivers/block/pktcdvd.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/block/pktcdvd.c b/drivers/block/pktcdvd.c
index 674cd66..f2510e7 100644
--- a/drivers/block/pktcdvd.c
+++ b/drivers/block/pktcdvd.c
@@ -849,7 +849,7 @@ static int pkt_flush_cache(struct pktcdvd_device *pd)
 /*
  * speed is given as the normal factor, e.g. 4 for 4x
  */
-static int pkt_set_speed(struct pktcdvd_device *pd, unsigned write_speed, unsigned read_speed)
+static noinline int pkt_set_speed(struct pktcdvd_device *pd, unsigned write_speed, unsigned read_speed)
 {
 	struct packet_command cgc;
 	struct request_sense sense;
@@ -1776,7 +1776,7 @@ static int pkt_get_track_info(struct pktcdvd_device *pd, __u16 track, __u8 type,
 	return pkt_generic_packet(pd, &cgc);
 }
 
-static int pkt_get_last_written(struct pktcdvd_device *pd, long *last_written)
+static noinline int pkt_get_last_written(struct pktcdvd_device *pd, long *last_written)
 {
 	disc_information di;
 	track_information ti;
@@ -1813,7 +1813,7 @@ static int pkt_get_last_written(struct pktcdvd_device *pd, long *last_written)
 /*
  * write mode select package based on pd->settings
  */
-static int pkt_set_write_settings(struct pktcdvd_device *pd)
+static noinline int pkt_set_write_settings(struct pktcdvd_device *pd)
 {
 	struct packet_command cgc;
 	struct request_sense sense;
@@ -1972,7 +1972,7 @@ static int pkt_writable_disc(struct pktcdvd_device *pd, disc_information *di)
 	return 1;
 }
 
-static i...
[ message continues ]

yup, I'll grab that.  I'll even write your changelog for you (grr).

But first, let's do this:


From: Andrew Morton <akpm@linux-foundation.org>

People are adding `noinline' in various places to prevent excess stack
consumption due to gcc inlining.  But once this is done, it is quite unobvious
why the `noinline' is present in the code.  We can comment each and every
site, or we can use noinline_for_stack.


Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 include/linux/compiler.h |    6 ++++++
 1 file changed, 6 insertions(+)

diff -puN include/linux/compiler.h~add-noinline_for_stack include/linux/compiler.h
--- a/include/linux/compiler.h~add-noinline_for_stack
+++ a/include/linux/compiler.h
@@ -138,6 +138,12 @@ extern void __chk_io_ptr(const volatile 
 #define noinline
 #endif
 
+/*
+ * Rather then using noinline to prevent stack consumption, use
+ * noinline_for_stack instead.  For documentaiton reasons.
+ */
+#define noinline_for_stack noinline
+
 #ifndef __always_inline
 #define __always_inline inline
 #endif
_

(Note that these changes don't let DM off the hook!)

--

I just looked at the source; I forgot `make checkstack' existed.

On this system:

0xc0263e0f pkt_open [vmlinux]:                          556

which is nearly as bad.

(As an aside, I'm surprised I didn't oops when packet-writing as well:

0xc021270d udf_process_sequence [vmlinux]:              692
0xc020f43d udf_add_entry [vmlinux]:                     628

owch. I guess that's called via a shorter call chain...)


I'll try the patch after this series of backups is done :)

-- 
`The rest is a tale of post and counter-post.' --- Ian Rawlings
                                                   describes USENET
--

If you are interested, linux-next or -mm tree should contain a patch now
that fixes the problem...

								Honza
-- 
Jan Kara <jack@suse.cz>
SuSE CR Labs
--

As soon as I've figured out why dhclient is failing to establish DHCP
connections from 2.6.14.3 (the DHCPOFFERs seem to get ignored, it's
weird and the kernel shouldn't have anything to do with it, but it does)
so I can actually compile something that works and doesn't knock me off
the net every two minutes, I'll try it :)

(yes, I could try -mm directly, but it's been some weeks since I backed
up, thanks to an abruptly chocolate-filled CD-RW drive, so I'm being a
bit cautious.)

-- 
`The rest is a tale of post and counter-post.' --- Ian Rawlings
                                                   describes USENET
--

udf_process_sequence() seems to be another victim of gcc inlining.

udf_add_entry() defines a couple of 256-byte local arrays.
--

Yes, exactly two of them. One is  non-trivial to get rid of - it's
used for encoding of filename before we write it, but one is used during
scanning of the directory whether the entry doesn't already exists (oh,
my!) and we can just rip that off..

								Honza
-- 
Jan Kara <jack@suse.cz>
SuSE CR Labs
--

Why can't we do just



UDF: Optimize stack usage

Signed-off-by: Jiri Kosina <jkosina@suse.cz>

diff --git a/fs/udf/namei.c b/fs/udf/namei.c
index 112a5fb..706a2b5 100644
--- a/fs/udf/namei.c
+++ b/fs/udf/namei.c
@@ -336,7 +336,7 @@ static struct fileIdentDesc *udf_add_entry(struct inode *dir,
 {
 	struct super_block *sb = dir->i_sb;
 	struct fileIdentDesc *fi = NULL;
-	char name[UDF_NAME_LEN], fname[UDF_NAME_LEN];
+	char *name, *fname;
 	int namelen;
 	loff_t f_pos;
 	int flen;
@@ -352,6 +352,14 @@ static struct fileIdentDesc *udf_add_entry(struct inode *dir,
 	struct extent_position epos = {};
 	struct udf_inode_info *dinfo;
 
+	name = kmalloc(sizeof(char) * UDF_NAME_LEN, GFP_KERNEL);
+	fname = kmalloc(sizeof(char) * UDF_NAME_LEN, GFP_KERNEL);
+
+	if (!name || !fname) {
+		*err = -ENOMEM;
+		return NULL;
+	}
+
 	if (dentry) {
 		if (!dentry->d_name.len) {
 			*err = -EINVAL;
diff --git a/fs/udf/super.c b/fs/udf/super.c
index f3ac4ab..42e3ba8 100644
--- a/fs/udf/super.c
+++ b/fs/udf/super.c
@@ -1345,7 +1345,7 @@ static void udf_load_logicalvolint(struct super_block *sb, kernel_extent_ad loc)
  *	July 1, 1997 - Andrew E. Mileski
  *	Written, tested, and released.
  */
-static int udf_process_sequence(struct super_block *sb, long block,
+static int noinline udf_process_sequence(struct super_block *sb, long block,
 				long lastblock, kernel_lb_addr *fileset)
 {
 	struct buffer_head *bh = NULL;
--

Wouldn't it be better to check each individually, so you do wind up leaking a 

DRH

-- 
Dialup is like pissing through a pipette. Slow and excruciatingly painful.
--

this bit is missing i think:

	if (name)
		kfree(name);
	if (fname)
		kfree(fname);

	Ingo
--

Ergh, of course, stupid me, sorry, it should be freed on all exit paths. I 
am not sending updated patch, as Jan is probably working on complete 
removal of one of those fields ... ?

Thanks,

-- 
Jiri Kosina
--

Yes, I'll convert one variable to kmalloc and the other one remove
completely. Stay tuned ;).

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR
--

kmalloc is quite fast ;)
--