[S390] cio: kernel stack overflow.

[S390] cio: kernel stack overflow.

[Martin Schwidefsky]

From: Heiko Carstens <heiko.carstens@de.ibm.com>

[S390] cio: kernel stack overflow.

Use different kind of assignment to make sure gcc doesn't create code
that creates temp variables on the stack, assigns values to it and
copies the content of the whole temp variable to the destination.
This reduces stack usage of e.g. ccwgroup_driver_register from 976
to 48 bytes instead.

Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>

---

 drivers/s390/cio/ccwgroup.c   |   14 +++++---------
 drivers/s390/cio/chsc.c       |    6 ++----
 drivers/s390/cio/device.c     |   19 +++++++------------
 drivers/s390/cio/device_fsm.c |   20 ++++++++------------
 4 files changed, 22 insertions(+), 37 deletions(-)

diff -urpN linux-2.6/drivers/s390/cio/ccwgroup.c linux-2.6-patched/drivers/s390/cio/ccwgroup.c
--- linux-2.6/drivers/s390/cio/ccwgroup.c	2006-08-30 14:24:13.000000000 +0200
+++ linux-2.6-patched/drivers/s390/cio/ccwgroup.c	2006-08-30 14:24:33.000000000 +0200
@@ -183,11 +183,9 @@ ccwgroup_create(struct device *root,
 
 	gdev->creator_id = creator_id;
 	gdev->count = argc;
-	gdev->dev = (struct device ) {
-		.bus = &ccwgroup_bus_type,
-		.parent = root,
-		.release = ccwgroup_release,
-	};
+	gdev->dev.bus = &ccwgroup_bus_type;
+	gdev->dev.parent = root;
+	gdev->dev.release = ccwgroup_release;
 
 	snprintf (gdev->dev.bus_id, BUS_ID_SIZE, "%s",
 			gdev->cdev[0]->dev.bus_id);
@@ -391,10 +389,8 @@ int
 ccwgroup_driver_register (struct ccwgroup_driver *cdriver)
 {
 	/* register our new driver with the core */
-	cdriver->driver = (struct device_driver) {
-		.bus = &ccwgroup_bus_type,
-		.name = cdriver->name,
-	};
+	cdriver->driver.bus = &ccwgroup_bus_type;
+	cdriver->driver.name = cdriver->name;
 
 	return driver_register(&cdriver->driver);
 }
diff -urpN linux-2.6/drivers/s390/cio/chsc.c linux-2.6-patched/drivers/s390/cio/chsc.c
--- linux-2.6/drivers/s390/cio/chsc.c	2006-08-30 14:24:13.000000000 +0200
+++ linux-2.6-patched/drivers/s390/cio/chsc.c	2006-08-30 14:24:33.000000000 +0200
@@ -1391,10 +1391,8 @@ new_channel_path(int chpid)
 	/* fill in status, etc. */
 	chp->id = chpid;
 	chp->state = 1;
-	chp->dev = (struct device) {
-		.parent  = &css[0]->device,
-		.release = chp_release,
-	};
+	chp->dev.parent = &css[0]->device;
+	chp->dev.release = chp_release;
 	snprintf(chp->dev.bus_id, BUS_ID_SIZE, "chp0.%x", chpid);
 
 	/* Obtain channel path description and fill it in. */
diff -urpN linux-2.6/drivers/s390/cio/device.c linux-2.6-patched/drivers/s390/cio/device.c
--- linux-2.6/drivers/s390/cio/device.c	2006-08-30 14:24:13.000000000 +0200
+++ linux-2.6-patched/drivers/s390/cio/device.c	2006-08-30 14:24:33.000000000 +0200
@@ -556,12 +556,11 @@ get_disc_ccwdev_by_devno(unsigned int de
 			 struct ccw_device *sibling)
 {
 	struct device *dev;
-	struct match_data data = {
-		.devno   = devno,
-		.ssid    = ssid,
-		.sibling = sibling,
-	};
+	struct match_data data;
 
+	data.devno = devno;
+	data.ssid = ssid;
+	data.sibling = sibling;
 	dev = bus_find_device(&ccw_bus_type, NULL, &data, match_devno);
 
 	return dev ? to_ccwdev(dev) : NULL;
@@ -835,10 +834,8 @@ io_subchannel_probe (struct subchannel *
 		return -ENOMEM;
 	}
 	atomic_set(&cdev->private->onoff, 0);
-	cdev->dev = (struct device) {
-		.parent = &sch->dev,
-		.release = ccw_device_release,
-	};
+	cdev->dev.parent = &sch->dev;
+	cdev->dev.release = ccw_device_release;
 	INIT_LIST_HEAD(&cdev->private->kick_work.entry);
 	/* Do first half of device_register. */
 	device_initialize(&cdev->dev);
@@ -977,9 +974,7 @@ ccw_device_console_enable (struct ccw_de
 	int rc;
 
 	/* Initialize the ccw_device structure. */
-	cdev->dev = (struct device) {
-		.parent = &sch->dev,
-	};
+	cdev->dev.parent= &sch->dev;
 	rc = io_subchannel_recog(cdev, sch);
 	if (rc)
 		return rc;
diff -urpN linux-2.6/drivers/s390/cio/device_fsm.c linux-2.6-patched/drivers/s390/cio/device_fsm.c
--- linux-2.6/drivers/s390/cio/device_fsm.c	2006-08-30 14:24:13.000000000 +0200
+++ linux-2.6-patched/drivers/s390/cio/device_fsm.c	2006-08-30 14:24:33.000000000 +0200
@@ -267,12 +267,10 @@ ccw_device_recog_done(struct ccw_device 
 			notify = 1;
 		}
 		/* fill out sense information */
-		cdev->id = (struct ccw_device_id) {
-			.cu_type   = cdev->private->senseid.cu_type,
-			.cu_model  = cdev->private->senseid.cu_model,
-			.dev_type  = cdev->private->senseid.dev_type,
-			.dev_model = cdev->private->senseid.dev_model,
-		};
+		cdev->id.cu_type   = cdev->private->senseid.cu_type;
+		cdev->id.cu_model  = cdev->private->senseid.cu_model;
+		cdev->id.dev_type  = cdev->private->senseid.dev_type;
+		cdev->id.dev_model = cdev->private->senseid.dev_model;
 		if (notify) {
 			cdev->private->state = DEV_STATE_OFFLINE;
 			if (same_dev) {
@@ -566,12 +564,10 @@ ccw_device_verify_done(struct ccw_device
 		/* Deliver fake irb to device driver, if needed. */
 		if (cdev->private->flags.fake_irb) {
 			memset(&cdev->private->irb, 0, sizeof(struct irb));
-			cdev->private->irb.scsw = (struct scsw) {
-				.cc = 1,
-				.fctl = SCSW_FCTL_START_FUNC,
-				.actl = SCSW_ACTL_START_PEND,
-				.stctl = SCSW_STCTL_STATUS_PEND,
-			};
+			cdev->private->irb.scsw.cc = 1;
+			cdev->private->irb.scsw.fctl = SCSW_FCTL_START_FUNC;
+			cdev->private->irb.scsw.actl = SCSW_ACTL_START_PEND;
+			cdev->private->irb.scsw.stctl = SCSW_STCTL_STATUS_PEND;
 			cdev->private->flags.fake_irb = 0;
 			if (cdev->handler)
 				cdev->handler(cdev, cdev->private->intparm,

Re: [S390] cio: kernel stack overflow.

[David Wagner]

Have you checked that in all cases all fields of the struct have
been overwritten?  For instance, look at this:

Martin Schwidefsky  wrote:
>-	chp->dev = (struct device) {
>-		.parent  = &css[0]->device,
>-		.release = chp_release,
>-	};
>+	chp->dev.parent = &css[0]->device;
>+	chp->dev.release = chp_release;

Doesn't this leave chp->dev.bus still holding whatever old value it
had laying around before?  Unless I'm missing something, it looks to
me like this diff causes a change in the semantics of the code.

Perhaps it would be better to memset() the entire struct (chp->dev, in
this case) to zero, before assigning to individual fields, so there is
no possibility of old remnant data still being left laying around?

Re: [S390] cio: kernel stack overflow.

[Martin Schwidefsky]

On 8/30/06, David Wagner <daw@cs.berkeley.edu> wrote:
> Have you checked that in all cases all fields of the struct have
> been overwritten?  For instance, look at this:
>
> Martin Schwidefsky  wrote:
> >-      chp->dev = (struct device) {
> >-              .parent  = &css[0]->device,
> >-              .release = chp_release,
> >-      };
> >+      chp->dev.parent = &css[0]->device;
> >+      chp->dev.release = chp_release;
>
> Doesn't this leave chp->dev.bus still holding whatever old value it
> had laying around before?  Unless I'm missing something, it looks to
> me like this diff causes a change in the semantics of the code.
>
> Perhaps it would be better to memset() the entire struct (chp->dev, in
> this case) to zero, before assigning to individual fields, so there is
> no possibility of old remnant data still being left laying around?

The structure is allocated with kzalloc().

-- 
blue skies,
  Martin

Re: [S390] cio: kernel stack overflow.

[Julio Auto]

On 8/30/06, David Wagner <daw@cs.berkeley.edu> wrote:
> Have you checked that in all cases all fields of the struct have
> been overwritten?  For instance, look at this:
>
> Martin Schwidefsky  wrote:
> >-      chp->dev = (struct device) {
> >-              .parent  = &css[0]->device,
> >-              .release = chp_release,
> >-      };
> >+      chp->dev.parent = &css[0]->device;
> >+      chp->dev.release = chp_release;
>
> Doesn't this leave chp->dev.bus still holding whatever old value it
> had laying around before?

You're correct. While this eliminates the possibility of getting
random values from the stack, it still leaves space for letting
previous unwanted values unchanged.
Unless, of course, the structure in question is kcalloc()'d (which is
not the case of gdev in the beginning of the patch - I haven't had the
time to check the other cases). But we surely can't rely on that.

>Unless I'm missing something, it looks to
> me like this diff causes a change in the semantics of the code.

I can't see the semantic change.

>
> Perhaps it would be better to memset() the entire struct (chp->dev, in
> this case) to zero, before assigning to individual fields, so there is
> no possibility of old remnant data still being left laying around?

Yes. Since the code can't be depend on the caller passing a zeroed
structure, it's definitely more safe to memset to 0 (or use kcalloc(),
instead of kmalloc(), when the allocation is the routine's own
responsability).

Changing subjects a little bit: where's the stack overflow in the
code? Either I'm too stupid to find it myself by looking at the patch
or the e-mail is mistitled.

Cheers,

    Julio Auto

Re: [S390] cio: kernel stack overflow.

[Julio Auto]

On 8/30/06, Julio Auto <mindvortex@gmail.com> wrote:
> Unless, of course, the structure in question is kcalloc()'d (which is
> not the case of gdev in the beginning of the patch - I haven't had the
> time to check the other cases).

Sorry, actually gdev is kzalloc()'d (which works exactly kcalloc()
without the check for n*size integer overflows). I was looking at a
much older version of the code.

I still think, however, that memset()'ing to zero is still something
to consider, in the cases where the structure is passed to the routine
(as opposed to when it's _created by_ the routine). The code shouldn't
rely on the awareness of future developers when adding other calls to
these functions.

Re: [S390] cio: kernel stack overflow.

[David Wagner]

Martin Schwidefsky  wrote:
>-      chp->dev = (struct device) {
>-              .parent  = &css[0]->device,
>-              .release = chp_release,
>-      };
>+      chp->dev.parent = &css[0]->device;
>+      chp->dev.release = chp_release;

>On 8/30/06, David Wagner <daw@cs.berkeley.edu> wrote:
>Unless I'm missing something, it looks to
>me like this diff causes a change in the semantics of the code.

Julio Auto wrote:
>I can't see the semantic change.

Maybe "semantic change" isn't the right term.  I'm not sure how gcc
handles the case where some field (like .bus) is missing from the C99
initializer as the struct device is created -- but it seemed plausible to
me that gcc might guarantee that all missing fields are automatically
initialized to 0 when the struct is constructed.  If this is the
case, then the semantic change is that the original code guarantees
to initialize chp->dev.bus to 0, whereas the proposed replacement code
does not.  I don't know whether gcc actually promises to initialize to
0 all unmentioned fields, or whether the C standard requires that, but
you could imagine some compiler making that promise (it is a reasonable
thing to do).

Re: [S390] cio: kernel stack overflow.

[David Wagner]

Thanks for pointing out that in most cases there was immediately
preceding code that zeroes out the whole struct using kzalloc() or
memset(.., 0, ..).  Sorry that I overlooked that; my mistake.  That
takes care of all but one of these.  But in the interests of caution,
let me ask about the following one:

Martin Schwidefsky  wrote:
>-		cdev->id = (struct ccw_device_id) {
>-			.cu_type   = cdev->private->senseid.cu_type,
>-			.cu_model  = cdev->private->senseid.cu_model,
>-			.dev_type  = cdev->private->senseid.dev_type,
>-			.dev_model = cdev->private->senseid.dev_model,
>-		};
>+		cdev->id.cu_type   = cdev->private->senseid.cu_type;
>+		cdev->id.cu_model  = cdev->private->senseid.cu_model;
>+		cdev->id.dev_type  = cdev->private->senseid.dev_type;
>+		cdev->id.dev_model = cdev->private->senseid.dev_model;

I don't see any obvious place that zeroes out cdev->id.
In particular, it looks like cdev->id.match_flags and .driver_info
are never cleared (i.e., they retain whatever old garbage they had
before).  More importantly, if anyone ever adds any more fields to
struct ccw_device_id, then they will also be retain old garbage values,
which is a maintenance pitfall.  Is this right, or did I miss something
again?

Re: [S390] cio: kernel stack overflow.

[Heiko Carstens]

On Wed, Aug 30, 2006 at 07:05:54PM +0000, David Wagner wrote:
> Thanks for pointing out that in most cases there was immediately
> preceding code that zeroes out the whole struct using kzalloc() or
> memset(.., 0, ..).  Sorry that I overlooked that; my mistake.  That
> takes care of all but one of these.  But in the interests of caution,
> let me ask about the following one:
> 
> Martin Schwidefsky  wrote:
> >-		cdev->id = (struct ccw_device_id) {
> >-			.cu_type   = cdev->private->senseid.cu_type,
> >-			.cu_model  = cdev->private->senseid.cu_model,
> >-			.dev_type  = cdev->private->senseid.dev_type,
> >-			.dev_model = cdev->private->senseid.dev_model,
> >-		};
> >+		cdev->id.cu_type   = cdev->private->senseid.cu_type;
> >+		cdev->id.cu_model  = cdev->private->senseid.cu_model;
> >+		cdev->id.dev_type  = cdev->private->senseid.dev_type;
> >+		cdev->id.dev_model = cdev->private->senseid.dev_model;
> 
> I don't see any obvious place that zeroes out cdev->id.
> In particular, it looks like cdev->id.match_flags and .driver_info
> are never cleared (i.e., they retain whatever old garbage they had
> before).  More importantly, if anyone ever adds any more fields to
> struct ccw_device_id, then they will also be retain old garbage values,
> which is a maintenance pitfall.  Is this right, or did I miss something
> again?

You're right. Thanks for pointing this out! I will take care of it.

Re: [S390] cio: kernel stack overflow.

[Martin Schwidefsky]

On Wed, 2006-08-30 at 21:19 +0200, Heiko Carstens wrote:
> > >-            cdev->id = (struct ccw_device_id) {
> > >-                    .cu_type   = cdev->private->senseid.cu_type,
> > >-                    .cu_model  = cdev->private->senseid.cu_model,
> > >-                    .dev_type  = cdev->private->senseid.dev_type,
> > >-                    .dev_model = cdev->private->senseid.dev_model,
> > >-            };
> > >+            cdev->id.cu_type   = cdev->private->senseid.cu_type;
> > >+            cdev->id.cu_model  = cdev->private->senseid.cu_model;
> > >+            cdev->id.dev_type  = cdev->private->senseid.dev_type;
> > >+            cdev->id.dev_model = cdev->private->senseid.dev_model;
> > 
> > I don't see any obvious place that zeroes out cdev->id.
> > In particular, it looks like cdev->id.match_flags and .driver_info
> > are never cleared (i.e., they retain whatever old garbage they had
> > before).  More importantly, if anyone ever adds any more fields to
> > struct ccw_device_id, then they will also be retain old garbage values,
> > which is a maintenance pitfall.  Is this right, or did I miss something
> > again?
> 
> You're right. Thanks for pointing this out! I will take care of it.

The ccw_device_id structure contains two more fields in addition to the
field that are set up in ccw_device_recog_done, namely match_flags and
driver_info. driver_info is set later in ccw_bus_match, so that is fine.
match_flags of the device is never used, only the match_flags of the
drivers version of the ccw_device_id is used. So the code is correct
even without the memset. But your point about the maintenance pitfall is
valid, we will add a memset after 2.6.18. I don't want to push yet
another patch.

-- 
blue skies,
  Martin.

Martin Schwidefsky
Linux for zSeries Development & Services
IBM Deutschland Entwicklung GmbH

"Reality continues to ruin my life." - Calvin.

Re: [S390] cio: kernel stack overflow.

[Julio Auto]

First of all, about your previous e-mail: you're correct, the members
not explicitly initialized behave like when  a _static_ object is not
explicitly initialized (ie., zero'ing it), instead of behaving like an
_automatic_ object not being explicitly initialized (which was the
kind of behavior I was expecting). This is part of the C99
specification, indeed.

See section 6.7.8, constraints no. 10 and 19, for more info.

On 8/30/06, David Wagner <daw@cs.berkeley.edu> wrote:
> I don't see any obvious place that zeroes out cdev->id.
> In particular, it looks like cdev->id.match_flags and .driver_info
> are never cleared (i.e., they retain whatever old garbage they had
> before).  More importantly, if anyone ever adds any more fields to
> struct ccw_device_id, then they will also be retain old garbage values,
> which is a maintenance pitfall.  Is this right, or did I miss something
> again?

Nicely pointed out, I hadn't thought about this possible maintenance
issue. Looks like a nice place for a memset() to reside.

Cheers,

    Julio Auto