BUG: cat /proc/partitions endless loop

BUG: cat /proc/partitions endless loop

[Joachim Weller]

After upgrading from 2.4.9 to 2.4.10, I discovered that
a "cat /proc/partitions" resulted in an infinite  repetition
of the partition list.
This resulted in a complete hang of SuSE's sys admin tool yast.
A more detailed report was sent to Jens Axboe (axboe@suse.de)

The affected machine is a dual pentium pro:
 Linux version 2.4.10 (root@bmdipc2c) (gcc version 2.95.3 20010315 (SuSE)) #1 
SMP Fri Sep 28 13:10:18 CEST 2001

 I traced the problem down to drivers/block/genhd.c, 
where the function get_partition_list() outer loop does not 
terminate due to the last element in the structured list starting 
with gendisk_head is not initialized to NULL, by whatever reason.
My fix does not cure the pointered endless loop, but prevents
from looping when stepping thru the pointered list.
--------------------------------------------------------------------------------------
diff -u --recursive --new-file 2.4.10/linux/drivers/block/genhd.c 
linux/drivers/block/genhd.c
--- 2.4.10/linux/drivers/block/genhd.c  Sun Sep  9 21:00:55 2001
+++ linux/drivers/block/genhd.c Fri Sep 28 14:45:13 2001
@@ -14,6 +14,15 @@
  * TODO:  rip out the remaining init crap from this file  --hch
  */

+/*
+ * Change: Blocked potential endless loop within get_partition_list()
+ *         which occured for "cat /proc/partitions"
+ *         because gendisk_head->next is not initialized correctly to NULL
+ *         before add_gendisk() is called the first time.
+ *         Joachim Weller 28sep2001 (JoachimWeller@web.de)
+ *
+ */
+
 #include <linux/config.h>
 #include <linux/module.h>
 #include <linux/fs.h>
@@ -115,7 +124,7 @@

        len = sprintf(page, "major minor  #blocks  name\n\n");
        read_lock(&gendisk_lock);
-       for (gp = gendisk_head; gp; gp = gp->next) {
+       for (gp = gendisk_head; gp; gp = (gp->next!=gendisk_head ?  gp->next 
: NULL)) {
                for (n = 0; n < (gp->nr_real << gp->minor_shift); n++) {
                        if (gp->part[n].nr_sects == 0)
                                continue;

--------------------------------------------------------------------------------------

-- 
Joachim Weller

Philips Medizinsysteme Boeblingen GmbH		Mail:  joachim_weller@hsgmed.com
Cardiac and Monitoring Systems (CMS)		Phone: {+49|0}-7031-463-1891
New Product Engineering				Fax:   {+49|0}-7031-463-2112

Hewlett-Packard Str. 2,	D 71034 Boeblingen	-GERMANY-

Re: BUG: cat /proc/partitions endless loop

[Christoph Hellwig]

In article <200109281315.f8SDFpA01669@bmdipc2c.germany.agilent.com> you wrote:
>  I traced the problem down to drivers/block/genhd.c, 
> where the function get_partition_list() outer loop does not 
> terminate due to the last element in the structured list starting 
> with gendisk_head is not initialized to NULL, by whatever reason.
> My fix does not cure the pointered endless loop, but prevents
> from looping when stepping thru the pointered list.

I think the fix could be simpler.  What about:

--- ../master/linux-2.4.10/drivers/block/genhd.c	Sun Sep 23 21:20:52 2001
+++ linux-2.4.10/drivers/block/genhd.c	Fri Sep 28 15:34:14 2001
@@ -115,7 +115,7 @@
 
 	len = sprintf(page, "major minor  #blocks  name\n\n");
 	read_lock(&gendisk_lock);
-	for (gp = gendisk_head; gp; gp = gp->next) {
+	for (gp = gendisk_head; gp != gendisk_head; gp = gp->next) {
 		for (n = 0; n < (gp->nr_real << gp->minor_shift); n++) {
 			if (gp->part[n].nr_sects == 0)
 				continue;

Re: Re: BUG: cat /proc/partitions endless loop

[Joachim Weller]

Christoph Hellwig <hch@ns.caldera.de> schrieb am 28.09.01:
> In article <200109281315.f8SDFpA01669@bmdipc2c.germany.agilent.com> you wrote:
> >  I traced the problem down to drivers/block/genhd.c, 
> > where the function get_partition_list() outer loop does not 
> > terminate due to the last element in the structured list starting 
> > with gendisk_head is not initialized to NULL, by whatever reason.
> > My fix does not cure the pointered endless loop, but prevents
> > from looping when stepping thru the pointered list.
> 
> I think the fix could be simpler.  What about:
[...] 

> + for (gp = gendisk_head; gp != gendisk_head; gp = gp->next) {

This will break your for loop immedeately, because the loop criteria
is already violated by the initialization !

But I tried another solution:
  for (gp = gendisk_head; gp && (gp->next != gendisk_head); gp = gp->next) {

with no success - the cat /proc/partition only printed the heading line.
This proofed to me, that the pointered list is created the wrong way,
in other words, gendisk_head->next is a pointer to itself.
It looks to me, that the "next" field in 
/*static*/ struct gendisk *gendisk_head;
is not initialized (by the compiler ?) correctly to NULL. 




>     if (gp->part[n].nr_sects == 0)
>      continue;
> 
>  


_______________________________________________________________________
1.000.000 DM gewinnen - kostenlos tippen - http://millionenklick.web.de
IhrName@web.de, 8MB Speicher, Verschluesselung - http://freemail.web.de

Re: BUG: cat /proc/partitions endless loop

[Christoph Hellwig]

On Fri, Sep 28, 2001 at 04:48:57PM +0200, Joachim Weller wrote:
> Christoph Hellwig <hch@ns.caldera.de> schrieb am 28.09.01:
> > In article <200109281315.f8SDFpA01669@bmdipc2c.germany.agilent.com> you wrote:
> > >  I traced the problem down to drivers/block/genhd.c, 
> > > where the function get_partition_list() outer loop does not 
> > > terminate due to the last element in the structured list starting 
> > > with gendisk_head is not initialized to NULL, by whatever reason.
> > > My fix does not cure the pointered endless loop, but prevents
> > > from looping when stepping thru the pointered list.
> > 
> > I think the fix could be simpler.  What about:
> [...] 
> 
> > + for (gp = gendisk_head; gp != gendisk_head; gp = gp->next) {
> 
> This will break your for loop immedeately, because the loop criteria
> is already violated by the initialization !
> 
> But I tried another solution:
>   for (gp = gendisk_head; gp && (gp->next != gendisk_head); gp = gp->next) {
> 
> with no success - the cat /proc/partition only printed the heading line.
> This proofed to me, that the pointered list is created the wrong way,
> in other words, gendisk_head->next is a pointer to itself.
> It looks to me, that the "next" field in 
> /*static*/ struct gendisk *gendisk_head;
> is not initialized (by the compiler ?) correctly to NULL. 

That's odd, could you try initilazing gendisk_head to NULL explicitly and try
my proposed fix?  The gendisk list handling starts to smell _really_ funny.

	Christoph

-- 
Of course it doesn't work. We've performed a software upgrade.

Re: BUG: cat /proc/partitions endless loop

[Joachim Weller]

Christoph Hellwig wrote:
> 
> On Fri, Sep 28, 2001 at 04:48:57PM +0200, Joachim Weller wrote:
> > Christoph Hellwig <hch@ns.caldera.de> schrieb am 28.09.01:
> > > I think the fix could be simpler.  What about:
> > [...]
> >
> > > + for (gp = gendisk_head; gp != gendisk_head; gp = gp->next) {
> >
> > This will break your for loop immedeately, because the loop criteria
> > is already violated by the initialization !
> >
> > But I tried another solution:
> >   for (gp = gendisk_head; gp && (gp->next != gendisk_head); gp = gp->next) {
> >
> > with no success - the cat /proc/partition only printed the heading line.
> > This proofed to me, that the pointered list is created the wrong way,
> > in other words, gendisk_head->next is a pointer to itself.
> > It looks to me, that the "next" field in
> > /*static*/ struct gendisk *gendisk_head;
> > is not initialized (by the compiler ?) correctly to NULL.
>
> That's odd, could you try initilazing gendisk_head to NULL explicitly and try
> my proposed fix?  The gendisk list handling starts to smell _really_ funny.
You would need to initialize gendisk_head->next to NULL *before* any call to 
add_gendisk(struct gendisk *gp) takes place. The latter is used in many other 
sources, to grow up the pointered list with additional partition info. 
gendisk_head always points to the most recently added item. 
When you step through the list, you reach the very first entry, which existed 
before any call to add_gendisk() was made. And the "next" field of that should 
point to NULL. 
But instead of this, it points to itself, thus forming a ring pointered loop.
I don't know in which part of the kernel sources, that pre-initialization 
of gendisk_head->next to NULL should be made. Maybe it is expected to be done
by the compiler.

But my fix works perfectly for me, resulting in exactly the same behaviour
as 2.4.9.

I should point out, that my 2.4.9 was compiled under gcc 2.95.2 (SuSE-6.4) , whereas
2.4.10 was compiled under 2.95.3 delivered by SuSE-7.2 ! Maybe that's the point.

-- 
Joachim Weller

Philips Medizinsysteme Boeblingen GmbH          Mail:  joachim_weller@hsgmed.com
Cardiac and Monitoring Systems (CMS)            Phone: {+49|0}-7031-463-1891
New Product Engineering                         Fax:   {+49|0}-7031-463-2112

Hewlett-Packard Str. 2, D 71034 Boeblingen      -GERMANY-