Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933648AbYBMX0E (ORCPT ); Wed, 13 Feb 2008 18:26:04 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1758310AbYBMXZm (ORCPT ); Wed, 13 Feb 2008 18:25:42 -0500 Received: from accolon.hansenpartnership.com ([76.243.235.52]:36326 "EHLO accolon.hansenpartnership.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1760343AbYBMXZh (ORCPT ); Wed, 13 Feb 2008 18:25:37 -0500 Subject: Re: [PATCH] SCSI: fix data corruption caused by ses From: James Bottomley To: Yinghai Lu Cc: Andrew Morton , Linux Kernel Mailing List , linux-scsi@vger.kernel.org, linux-ide@vger.kernel.org, kristen.c.accardi@intel.com, Ingo Molnar , "H. Peter Anvin" In-Reply-To: <200802122310.23050.yinghai.lu@sun.com> References: <200802090413.53275.yinghai.lu@sun.com> <1202749326.3122.39.camel@localhost.localdomain> <200802111225.04021.yinghai.lu@sun.com> <200802122310.23050.yinghai.lu@sun.com> Content-Type: text/plain Date: Wed, 13 Feb 2008 17:25:27 -0600 Message-Id: <1202945127.3109.89.camel@localhost.localdomain> Mime-Version: 1.0 X-Mailer: Evolution 2.12.3 (2.12.3-1.fc8) Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 9362 Lines: 312 On Tue, 2008-02-12 at 23:10 -0800, Yinghai Lu wrote: > if (type_ptr[0] != ENCLOSURE_COMPONENT_DEVICE && > type_ptr[0] != ENCLOSURE_COMPONENT_ARRAY_DEVICE) > - continue; > + goto next; > + > ecomp = enclosure_component_register(edev, > components++, > type_ptr[0], > name); > + > + if (desc_ptr && !IS_ERR(ecomp) && addl_desc_ptr) > + ses_process_descriptor(ecomp, > + addl_desc_ptr); > + next: > if (desc_ptr) { > desc_ptr += len; > - if (!IS_ERR(ecomp)) > - ses_process_descriptor(ecomp, > - addl_desc_ptr); > > if (addl_desc_ptr) > addl_desc_ptr += addl_desc_ptr[1] + 2; Everything looks fine, thanks, except this piece. That if (x) goto next; ... next: Needs to be if (!x) { ... } I've fixed it up below. (I suppose the same thing goes for the no_page10: label as well). James --- From: Yinghai Lu Date: Tue, 12 Feb 2008 23:10:22 -0800 Subject: [SCSI] ses: fix data corruption one system: initrd get courrupted: RAMDISK: Compressed image found at block 0 RAMDISK: incomplete write (-28 != 2048) 134217728 crc error VFS: Mounted root (ext2 filesystem). Freeing unused kernel memory: 388k freed init_special_inode: bogus i_mode (177777) Warning: unable to open an initial console. init_special_inode: bogus i_mode (177777) init_special_inode: bogus i_mode (177777) Kernel panic - not syncing: No init found. Try passing init= option to kernel. bisected to commit 9927c68864e9c39cc317b4f559309ba29e642168 Author: James Bottomley Date: Sun Feb 3 15:48:56 2008 -0600 [SCSI] ses: add new Enclosure ULD changes: 1. change char to unsigned char to avoid type change later. 2. preserve len for page1 3. need to move desc_ptr even the entry is not enclosure_component_device/raid. so keep desc_ptr on right position 4. also record page7 len, and double check if desc_ptr out of boundary before touch. Signed-off-by: Yinghai Lu Signed-off-by: James Bottomley --- drivers/scsi/ses.c | 75 ++++++++++++++++++++++++++++----------------------- 1 files changed, 41 insertions(+), 34 deletions(-) diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c index a57fed4..614879e 100644 --- a/drivers/scsi/ses.c +++ b/drivers/scsi/ses.c @@ -33,9 +33,9 @@ #include struct ses_device { - char *page1; - char *page2; - char *page10; + unsigned char *page1; + unsigned char *page2; + unsigned char *page10; short page1_len; short page2_len; short page10_len; @@ -67,7 +67,7 @@ static int ses_probe(struct device *dev) static int ses_recv_diag(struct scsi_device *sdev, int page_code, void *buf, int bufflen) { - char cmd[] = { + unsigned char cmd[] = { RECEIVE_DIAGNOSTIC, 1, /* Set PCV bit */ page_code, @@ -85,7 +85,7 @@ static int ses_send_diag(struct scsi_device *sdev, int page_code, { u32 result; - char cmd[] = { + unsigned char cmd[] = { SEND_DIAGNOSTIC, 0x10, /* Set PF bit */ 0, @@ -104,13 +104,13 @@ static int ses_send_diag(struct scsi_device *sdev, int page_code, static int ses_set_page2_descriptor(struct enclosure_device *edev, struct enclosure_component *ecomp, - char *desc) + unsigned char *desc) { int i, j, count = 0, descriptor = ecomp->number; struct scsi_device *sdev = to_scsi_device(edev->cdev.dev); struct ses_device *ses_dev = edev->scratch; - char *type_ptr = ses_dev->page1 + 12 + ses_dev->page1[11]; - char *desc_ptr = ses_dev->page2 + 8; + unsigned char *type_ptr = ses_dev->page1 + 12 + ses_dev->page1[11]; + unsigned char *desc_ptr = ses_dev->page2 + 8; /* Clear everything */ memset(desc_ptr, 0, ses_dev->page2_len - 8); @@ -133,14 +133,14 @@ static int ses_set_page2_descriptor(struct enclosure_device *edev, return ses_send_diag(sdev, 2, ses_dev->page2, ses_dev->page2_len); } -static char *ses_get_page2_descriptor(struct enclosure_device *edev, +static unsigned char *ses_get_page2_descriptor(struct enclosure_device *edev, struct enclosure_component *ecomp) { int i, j, count = 0, descriptor = ecomp->number; struct scsi_device *sdev = to_scsi_device(edev->cdev.dev); struct ses_device *ses_dev = edev->scratch; - char *type_ptr = ses_dev->page1 + 12 + ses_dev->page1[11]; - char *desc_ptr = ses_dev->page2 + 8; + unsigned char *type_ptr = ses_dev->page1 + 12 + ses_dev->page1[11]; + unsigned char *desc_ptr = ses_dev->page2 + 8; ses_recv_diag(sdev, 2, ses_dev->page2, ses_dev->page2_len); @@ -160,17 +160,18 @@ static char *ses_get_page2_descriptor(struct enclosure_device *edev, static void ses_get_fault(struct enclosure_device *edev, struct enclosure_component *ecomp) { - char *desc; + unsigned char *desc; desc = ses_get_page2_descriptor(edev, ecomp); - ecomp->fault = (desc[3] & 0x60) >> 4; + if (desc) + ecomp->fault = (desc[3] & 0x60) >> 4; } static int ses_set_fault(struct enclosure_device *edev, struct enclosure_component *ecomp, enum enclosure_component_setting val) { - char desc[4] = {0 }; + unsigned char desc[4] = {0 }; switch (val) { case ENCLOSURE_SETTING_DISABLED: @@ -190,26 +191,28 @@ static int ses_set_fault(struct enclosure_device *edev, static void ses_get_status(struct enclosure_device *edev, struct enclosure_component *ecomp) { - char *desc; + unsigned char *desc; desc = ses_get_page2_descriptor(edev, ecomp); - ecomp->status = (desc[0] & 0x0f); + if (desc) + ecomp->status = (desc[0] & 0x0f); } static void ses_get_locate(struct enclosure_device *edev, struct enclosure_component *ecomp) { - char *desc; + unsigned char *desc; desc = ses_get_page2_descriptor(edev, ecomp); - ecomp->locate = (desc[2] & 0x02) ? 1 : 0; + if (desc) + ecomp->locate = (desc[2] & 0x02) ? 1 : 0; } static int ses_set_locate(struct enclosure_device *edev, struct enclosure_component *ecomp, enum enclosure_component_setting val) { - char desc[4] = {0 }; + unsigned char desc[4] = {0 }; switch (val) { case ENCLOSURE_SETTING_DISABLED: @@ -229,7 +232,7 @@ static int ses_set_active(struct enclosure_device *edev, struct enclosure_component *ecomp, enum enclosure_component_setting val) { - char desc[4] = {0 }; + unsigned char desc[4] = {0 }; switch (val) { case ENCLOSURE_SETTING_DISABLED: @@ -409,11 +412,11 @@ static int ses_intf_add(struct class_device *cdev, { struct scsi_device *sdev = to_scsi_device(cdev->dev); struct scsi_device *tmp_sdev; - unsigned char *buf = NULL, *hdr_buf, *type_ptr, *desc_ptr, - *addl_desc_ptr; + unsigned char *buf = NULL, *hdr_buf, *type_ptr, *desc_ptr = NULL, + *addl_desc_ptr = NULL; struct ses_device *ses_dev; u32 result; - int i, j, types, len, components = 0; + int i, j, types, len, page7_len = 0, components = 0; int err = -ENOMEM; struct enclosure_device *edev; struct ses_component *scomp = NULL; @@ -461,9 +464,8 @@ static int ses_intf_add(struct class_device *cdev, goto recv_failed; types = buf[10]; - len = buf[11]; - type_ptr = buf + 12 + len; + type_ptr = buf + 12 + buf[11]; for (i = 0; i < types; i++, type_ptr += 4) { if (type_ptr[0] == ENCLOSURE_COMPONENT_DEVICE || @@ -530,7 +532,7 @@ static int ses_intf_add(struct class_device *cdev, if (result) goto simple_populate; - len = (hdr_buf[2] << 8) + hdr_buf[3] + 4; + page7_len = len = (hdr_buf[2] << 8) + hdr_buf[3] + 4; /* add 1 for trailing '\0' we'll use */ buf = kzalloc(len + 1, GFP_KERNEL); if (!buf) @@ -547,7 +549,8 @@ static int ses_intf_add(struct class_device *cdev, len = (desc_ptr[2] << 8) + desc_ptr[3]; /* skip past overall descriptor */ desc_ptr += len + 4; - addl_desc_ptr = ses_dev->page10 + 8; + if (ses_dev->page10) + addl_desc_ptr = ses_dev->page10 + 8; } type_ptr = ses_dev->page1 + 12 + ses_dev->page1[11]; components = 0; @@ -557,6 +560,8 @@ static int ses_intf_add(struct class_device *cdev, struct enclosure_component *ecomp; if (desc_ptr) { + if (desc_ptr >= buf + page7_len) + break; len = (desc_ptr[2] << 8) + desc_ptr[3]; desc_ptr += 4; /* Add trailing zero - pushes into @@ -564,18 +569,20 @@ static int ses_intf_add(struct class_device *cdev, desc_ptr[len] = '\0'; name = desc_ptr; } - if (type_ptr[0] != ENCLOSURE_COMPONENT_DEVICE && - type_ptr[0] != ENCLOSURE_COMPONENT_ARRAY_DEVICE) - continue; - ecomp = enclosure_component_register(edev, + if (type_ptr[0] == ENCLOSURE_COMPONENT_DEVICE || + type_ptr[0] == ENCLOSURE_COMPONENT_ARRAY_DEVICE) { + + ecomp = enclosure_component_register(edev, components++, type_ptr[0], name); - if (desc_ptr) { - desc_ptr += len; - if (!IS_ERR(ecomp)) + + if (desc_ptr && !IS_ERR(ecomp) && addl_desc_ptr) ses_process_descriptor(ecomp, addl_desc_ptr); + } + if (desc_ptr) { + desc_ptr += len; if (addl_desc_ptr) addl_desc_ptr += addl_desc_ptr[1] + 2; -- 1.5.3.8 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/