* [PATCH] megaraid_sas: fix physical disk handling
@ 2006-02-06 14:05 Christoph Hellwig
2006-02-09 18:30 ` Sumant Patro
2006-02-17 11:13 ` Christoph Hellwig
0 siblings, 2 replies; 6+ messages in thread
From: Christoph Hellwig @ 2006-02-06 14:05 UTC (permalink / raw)
To: Sreenivas.Bagalkote, Sumant.Patro; +Cc: Chandra_Nelogal, linux-scsi
Chandra_Nelogal noticed that megaraid_sas currently exports all physical
disks normally to the disk layer, which is obviously quite bad.
The problems is that megaraid_sas is doing inquiry sniffing, and since
2.6.15 inquiry commands are sent down as one-element scatterlists on
which the code in the driver doesn't work anymore. The right place to
keep the scsi midlayer from attaching to a device is the slave_alloc
method in the host template. To completely prevent attaching the method
needs to return -ENXIO, but the patch below sets the no_uld_attach flag
instead which prevents upper level drivers from attaching while still
allowing scsi generic access to it, as in other raid HBA drivers.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Index: linux-2.6/drivers/scsi/megaraid/megaraid_sas.c
===================================================================
--- linux-2.6.orig/drivers/scsi/megaraid/megaraid_sas.c 2006-02-04 13:35:00.000000000 +0100
+++ linux-2.6/drivers/scsi/megaraid/megaraid_sas.c 2006-02-06 14:59:13.000000000 +0100
@@ -707,6 +707,16 @@
return 0;
}
+static int megasas_slave_configure(struct scsi_device *sdev)
+{
+ /*
+ * Don't export physical disk devices to the disk driver.
+ */
+ if (sdev->channel < MEGASAS_MAX_PD_CHANNELS && sdev->type == TYPE_DISK)
+ sdev->no_uld_attach = 1;
+ return 0;
+}
+
/**
* megasas_wait_for_outstanding - Wait for all outstanding cmds
* @instance: Adapter soft state
@@ -857,6 +867,7 @@
.module = THIS_MODULE,
.name = "LSI Logic SAS based MegaRAID driver",
.proc_name = "megaraid_sas",
+ .slave_configure = megasas_slave_configure,
.queuecommand = megasas_queue_command,
.eh_device_reset_handler = megasas_reset_device,
.eh_bus_reset_handler = megasas_reset_bus_host,
@@ -985,20 +996,6 @@
break;
}
- /*
- * Don't export physical disk devices to mid-layer.
- */
- if (!MEGASAS_IS_LOGICAL(cmd->scmd) &&
- (hdr->cmd_status == MFI_STAT_OK) &&
- (cmd->scmd->cmnd[0] == INQUIRY)) {
-
- if (((*(u8 *) cmd->scmd->request_buffer) & 0x1F) ==
- TYPE_DISK) {
- cmd->scmd->result = DID_BAD_TARGET << 16;
- exception = 1;
- }
- }
-
case MFI_CMD_LD_READ:
case MFI_CMD_LD_WRITE:
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] megaraid_sas: fix physical disk handling
2006-02-06 14:05 [PATCH] megaraid_sas: fix physical disk handling Christoph Hellwig
@ 2006-02-09 18:30 ` Sumant Patro
2006-02-09 22:55 ` Douglas Gilbert
2006-02-17 11:13 ` Christoph Hellwig
1 sibling, 1 reply; 6+ messages in thread
From: Sumant Patro @ 2006-02-09 18:30 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Sreenivas.Bagalkote, Chandra_Nelogal, linux-scsi
Hello Christoph,
Thank you for the patch.
As you mentioned this patch allows scsi generic access to the
physical disks. I see this as potential data integrity issue if someone
does a write operation on a disk that belongs to a Logical RAID volume.
I would rather prevent all access to the disks using slave_alloc(). I
will be sending a patch shortly for review that implements
megasas_slave_alloc to block all direct access to the physical disks.
Regards,
Sumant
On Mon, 2006-02-06 at 15:05 +0100, Christoph Hellwig wrote:
> Chandra_Nelogal noticed that megaraid_sas currently exports all physical
> disks normally to the disk layer, which is obviously quite bad.
>
> The problems is that megaraid_sas is doing inquiry sniffing, and since
> 2.6.15 inquiry commands are sent down as one-element scatterlists on
> which the code in the driver doesn't work anymore. The right place to
> keep the scsi midlayer from attaching to a device is the slave_alloc
> method in the host template. To completely prevent attaching the method
> needs to return -ENXIO, but the patch below sets the no_uld_attach flag
> instead which prevents upper level drivers from attaching while still
> allowing scsi generic access to it, as in other raid HBA drivers.
>
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
>
> Index: linux-2.6/drivers/scsi/megaraid/megaraid_sas.c
> ===================================================================
> --- linux-2.6.orig/drivers/scsi/megaraid/megaraid_sas.c 2006-02-04 13:35:00.000000000 +0100
> +++ linux-2.6/drivers/scsi/megaraid/megaraid_sas.c 2006-02-06 14:59:13.000000000 +0100
> @@ -707,6 +707,16 @@
> return 0;
> }
>
> +static int megasas_slave_configure(struct scsi_device *sdev)
> +{
> + /*
> + * Don't export physical disk devices to the disk driver.
> + */
> + if (sdev->channel < MEGASAS_MAX_PD_CHANNELS && sdev->type == TYPE_DISK)
> + sdev->no_uld_attach = 1;
> + return 0;
> +}
> +
> /**
> * megasas_wait_for_outstanding - Wait for all outstanding cmds
> * @instance: Adapter soft state
> @@ -857,6 +867,7 @@
> .module = THIS_MODULE,
> .name = "LSI Logic SAS based MegaRAID driver",
> .proc_name = "megaraid_sas",
> + .slave_configure = megasas_slave_configure,
> .queuecommand = megasas_queue_command,
> .eh_device_reset_handler = megasas_reset_device,
> .eh_bus_reset_handler = megasas_reset_bus_host,
> @@ -985,20 +996,6 @@
> break;
> }
>
> - /*
> - * Don't export physical disk devices to mid-layer.
> - */
> - if (!MEGASAS_IS_LOGICAL(cmd->scmd) &&
> - (hdr->cmd_status == MFI_STAT_OK) &&
> - (cmd->scmd->cmnd[0] == INQUIRY)) {
> -
> - if (((*(u8 *) cmd->scmd->request_buffer) & 0x1F) ==
> - TYPE_DISK) {
> - cmd->scmd->result = DID_BAD_TARGET << 16;
> - exception = 1;
> - }
> - }
> -
> case MFI_CMD_LD_READ:
> case MFI_CMD_LD_WRITE:
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] megaraid_sas: fix physical disk handling
2006-02-09 18:30 ` Sumant Patro
@ 2006-02-09 22:55 ` Douglas Gilbert
0 siblings, 0 replies; 6+ messages in thread
From: Douglas Gilbert @ 2006-02-09 22:55 UTC (permalink / raw)
To: Sumant Patro
Cc: Christoph Hellwig, Sreenivas.Bagalkote, Chandra_Nelogal,
linux-scsi
Sumant Patro wrote:
> Hello Christoph,
>
> Thank you for the patch.
>
> As you mentioned this patch allows scsi generic access to the
> physical disks. I see this as potential data integrity issue if someone
> does a write operation on a disk that belongs to a Logical RAID volume.
> I would rather prevent all access to the disks using slave_alloc(). I
> will be sending a patch shortly for review that implements
> megasas_slave_alloc to block all direct access to the physical disks.
Sumant,
Typically a non-root user would not have write (or read)
permissions on a sg device node. A root user (or perhaps
a user with CAP_SYS_RAWIO) could send a SCSI WRITE command
to a physical disk, but that is no different from the
current situation.
IMO The main reason to allow access to the physical "logical"
units is for tools like smartmontools and access for commands
like LOG SENSE and MODE SENSE (and perhaps the SELECT variants
of those commands used carefully). Add INQUIRY to that list ...
With FC and SAS dual ported disks one could also think about
taking a persistent reservation on one I_T nexus (e.g. the
active port) and only allow access to the physical disk via
the secondary port. That way the finer graded command
filtering of PERSISTENT RESERVATION could be used to guard
the integrity of the disk.
Doug Gilbert
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] megaraid_sas: fix physical disk handling
2006-02-06 14:05 [PATCH] megaraid_sas: fix physical disk handling Christoph Hellwig
2006-02-09 18:30 ` Sumant Patro
@ 2006-02-17 11:13 ` Christoph Hellwig
2006-02-18 3:02 ` Sumant Patro
1 sibling, 1 reply; 6+ messages in thread
From: Christoph Hellwig @ 2006-02-17 11:13 UTC (permalink / raw)
To: Sreenivas.Bagalkote, Sumant.Patro; +Cc: Chandra_Nelogal, linux-scsi
On Mon, Feb 06, 2006 at 03:05:19PM +0100, Christoph Hellwig wrote:
> Chandra_Nelogal noticed that megaraid_sas currently exports all physical
> disks normally to the disk layer, which is obviously quite bad.
>
> The problems is that megaraid_sas is doing inquiry sniffing, and since
> 2.6.15 inquiry commands are sent down as one-element scatterlists on
> which the code in the driver doesn't work anymore. The right place to
> keep the scsi midlayer from attaching to a device is the slave_alloc
> method in the host template. To completely prevent attaching the method
> needs to return -ENXIO, but the patch below sets the no_uld_attach flag
> instead which prevents upper level drivers from attaching while still
> allowing scsi generic access to it, as in other raid HBA drivers.
I got feedback from LSI that they're very unhappy to allow generic
access to the physical devices at this point because they haven't
audited the firmware yet about what could happen if someone actually
accesses the physical devices.
This patch hides the devices completely from the midlayer instead.
It requires the patch to handle the slave_configure failure I posted
earlier.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Index: linux-2.6/drivers/scsi/megaraid/megaraid_sas.c
===================================================================
--- linux-2.6.orig/drivers/scsi/megaraid/megaraid_sas.c 2006-02-14 16:23:29.000000000 +0100
+++ linux-2.6/drivers/scsi/megaraid/megaraid_sas.c 2006-02-14 20:38:06.000000000 +0100
@@ -707,6 +707,20 @@
return 0;
}
+static int megasas_slave_configure(struct scsi_device *sdev)
+{
+ /*
+ * Don't export physical disk devices to the disk driver.
+ *
+ * FIXME: Currently we don't export them to the midlayer at all.
+ * That will be fixed once LSI engineers have audited the
+ * firmware for possible issues.
+ */
+ if (sdev->channel < MEGASAS_MAX_PD_CHANNELS && sdev->type == TYPE_DISK)
+ return -ENXIO;
+ return 0;
+}
+
/**
* megasas_wait_for_outstanding - Wait for all outstanding cmds
* @instance: Adapter soft state
@@ -857,6 +871,7 @@
.module = THIS_MODULE,
.name = "LSI Logic SAS based MegaRAID driver",
.proc_name = "megaraid_sas",
+ .slave_configure = megasas_slave_configure,
.queuecommand = megasas_queue_command,
.eh_device_reset_handler = megasas_reset_device,
.eh_bus_reset_handler = megasas_reset_bus_host,
@@ -985,20 +1000,6 @@
break;
}
- /*
- * Don't export physical disk devices to mid-layer.
- */
- if (!MEGASAS_IS_LOGICAL(cmd->scmd) &&
- (hdr->cmd_status == MFI_STAT_OK) &&
- (cmd->scmd->cmnd[0] == INQUIRY)) {
-
- if (((*(u8 *) cmd->scmd->request_buffer) & 0x1F) ==
- TYPE_DISK) {
- cmd->scmd->result = DID_BAD_TARGET << 16;
- exception = 1;
- }
- }
-
case MFI_CMD_LD_READ:
case MFI_CMD_LD_WRITE:
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] megaraid_sas: fix physical disk handling
2006-02-17 11:13 ` Christoph Hellwig
@ 2006-02-18 3:02 ` Sumant Patro
0 siblings, 0 replies; 6+ messages in thread
From: Sumant Patro @ 2006-02-18 3:02 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Sreenivas.Bagalkote, Sumant.Patro, Chandra_Nelogal, linux-scsi
On Fri, 2006-02-17 at 12:13 +0100, Christoph Hellwig wrote:
> On Mon, Feb 06, 2006 at 03:05:19PM +0100, Christoph Hellwig wrote:
> > Chandra_Nelogal noticed that megaraid_sas currently exports all physical
> > disks normally to the disk layer, which is obviously quite bad.
> >
> > The problems is that megaraid_sas is doing inquiry sniffing, and since
> > 2.6.15 inquiry commands are sent down as one-element scatterlists on
> > which the code in the driver doesn't work anymore. The right place to
> > keep the scsi midlayer from attaching to a device is the slave_alloc
> > method in the host template. To completely prevent attaching the method
> > needs to return -ENXIO, but the patch below sets the no_uld_attach flag
> > instead which prevents upper level drivers from attaching while still
> > allowing scsi generic access to it, as in other raid HBA drivers.
>
> I got feedback from LSI that they're very unhappy to allow generic
> access to the physical devices at this point because they haven't
> audited the firmware yet about what could happen if someone actually
> accesses the physical devices.
>
> This patch hides the devices completely from the midlayer instead.
> It requires the patch to handle the slave_configure failure I posted
> earlier.
>
Thank you.
--Sumant
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
>
> Index: linux-2.6/drivers/scsi/megaraid/megaraid_sas.c
> ===================================================================
> --- linux-2.6.orig/drivers/scsi/megaraid/megaraid_sas.c 2006-02-14 16:23:29.000000000 +0100
> +++ linux-2.6/drivers/scsi/megaraid/megaraid_sas.c 2006-02-14 20:38:06.000000000 +0100
> @@ -707,6 +707,20 @@
> return 0;
> }
>
> +static int megasas_slave_configure(struct scsi_device *sdev)
> +{
> + /*
> + * Don't export physical disk devices to the disk driver.
> + *
> + * FIXME: Currently we don't export them to the midlayer at all.
> + * That will be fixed once LSI engineers have audited the
> + * firmware for possible issues.
> + */
> + if (sdev->channel < MEGASAS_MAX_PD_CHANNELS && sdev->type == TYPE_DISK)
> + return -ENXIO;
> + return 0;
> +}
> +
> /**
> * megasas_wait_for_outstanding - Wait for all outstanding cmds
> * @instance: Adapter soft state
> @@ -857,6 +871,7 @@
> .module = THIS_MODULE,
> .name = "LSI Logic SAS based MegaRAID driver",
> .proc_name = "megaraid_sas",
> + .slave_configure = megasas_slave_configure,
> .queuecommand = megasas_queue_command,
> .eh_device_reset_handler = megasas_reset_device,
> .eh_bus_reset_handler = megasas_reset_bus_host,
> @@ -985,20 +1000,6 @@
> break;
> }
>
> - /*
> - * Don't export physical disk devices to mid-layer.
> - */
> - if (!MEGASAS_IS_LOGICAL(cmd->scmd) &&
> - (hdr->cmd_status == MFI_STAT_OK) &&
> - (cmd->scmd->cmnd[0] == INQUIRY)) {
> -
> - if (((*(u8 *) cmd->scmd->request_buffer) & 0x1F) ==
> - TYPE_DISK) {
> - cmd->scmd->result = DID_BAD_TARGET << 16;
> - exception = 1;
> - }
> - }
> -
> case MFI_CMD_LD_READ:
> case MFI_CMD_LD_WRITE:
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] megaraid_sas: fix physical disk handling
@ 2006-02-09 20:44 Salyzyn, Mark
0 siblings, 0 replies; 6+ messages in thread
From: Salyzyn, Mark @ 2006-02-09 20:44 UTC (permalink / raw)
To: Sumant Patro, Christoph Hellwig
Cc: Sreenivas.Bagalkote, Chandra_Nelogal, linux-scsi
[-- Attachment #1: Type: text/plain, Size: 4010 bytes --]
What about considering the enclosed patch (submitted 2/2/2006 under the
title "RE: [PATCH 1/5] aacraid: simplify non-dasd support") and clearing
the scsi device writeable flag along with the no_uld_attach flag?
Sincerely -- Mark Salyzyn
> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org
> [mailto:linux-scsi-owner@vger.kernel.org] On Behalf Of Sumant Patro
> Sent: Thursday, February 09, 2006 1:30 PM
> To: Christoph Hellwig
> Cc: Sreenivas.Bagalkote@lsil.com; Chandra_Nelogal@Dell.com;
> linux-scsi@vger.kernel.org
> Subject: Re: [PATCH] megaraid_sas: fix physical disk handling
>
>
> Hello Christoph,
>
> Thank you for the patch.
>
> As you mentioned this patch allows scsi generic access to the
> physical disks. I see this as potential data integrity issue
> if someone
> does a write operation on a disk that belongs to a Logical
> RAID volume.
> I would rather prevent all access to the disks using slave_alloc(). I
> will be sending a patch shortly for review that implements
> megasas_slave_alloc to block all direct access to the physical disks.
>
> Regards,
>
> Sumant
>
> On Mon, 2006-02-06 at 15:05 +0100, Christoph Hellwig wrote:
> > Chandra_Nelogal noticed that megaraid_sas currently exports
> all physical
> > disks normally to the disk layer, which is obviously quite bad.
> >
> > The problems is that megaraid_sas is doing inquiry
> sniffing, and since
> > 2.6.15 inquiry commands are sent down as one-element scatterlists on
> > which the code in the driver doesn't work anymore. The
> right place to
> > keep the scsi midlayer from attaching to a device is the slave_alloc
> > method in the host template. To completely prevent
> attaching the method
> > needs to return -ENXIO, but the patch below sets the
> no_uld_attach flag
> > instead which prevents upper level drivers from attaching
> while still
> > allowing scsi generic access to it, as in other raid HBA drivers.
> >
> >
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> >
> > Index: linux-2.6/drivers/scsi/megaraid/megaraid_sas.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/scsi/megaraid/megaraid_sas.c
> 2006-02-04 13:35:00.000000000 +0100
> > +++ linux-2.6/drivers/scsi/megaraid/megaraid_sas.c
> 2006-02-06 14:59:13.000000000 +0100
> > @@ -707,6 +707,16 @@
> > return 0;
> > }
> >
> > +static int megasas_slave_configure(struct scsi_device *sdev)
> > +{
> > + /*
> > + * Don't export physical disk devices to the disk driver.
> > + */
> > + if (sdev->channel < MEGASAS_MAX_PD_CHANNELS &&
> sdev->type == TYPE_DISK)
> > + sdev->no_uld_attach = 1;
> > + return 0;
> > +}
> > +
> > /**
> > * megasas_wait_for_outstanding - Wait for all outstanding cmds
> > * @instance: Adapter soft state
> > @@ -857,6 +867,7 @@
> > .module = THIS_MODULE,
> > .name = "LSI Logic SAS based MegaRAID driver",
> > .proc_name = "megaraid_sas",
> > + .slave_configure = megasas_slave_configure,
> > .queuecommand = megasas_queue_command,
> > .eh_device_reset_handler = megasas_reset_device,
> > .eh_bus_reset_handler = megasas_reset_bus_host,
> > @@ -985,20 +996,6 @@
> > break;
> > }
> >
> > - /*
> > - * Don't export physical disk devices to mid-layer.
> > - */
> > - if (!MEGASAS_IS_LOGICAL(cmd->scmd) &&
> > - (hdr->cmd_status == MFI_STAT_OK) &&
> > - (cmd->scmd->cmnd[0] == INQUIRY)) {
> > -
> > - if (((*(u8 *)
> cmd->scmd->request_buffer) & 0x1F) ==
> > - TYPE_DISK) {
> > - cmd->scmd->result =
> DID_BAD_TARGET << 16;
> > - exception = 1;
> > - }
> > - }
> > -
> > case MFI_CMD_LD_READ:
> > case MFI_CMD_LD_WRITE:
> >
>
> -
> To unsubscribe from this list: send the line "unsubscribe
> linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
[-- Attachment #2: wprotect2.patch --]
[-- Type: application/octet-stream, Size: 780 bytes --]
diff -ru a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
--- a/drivers/scsi/scsi.c 2006-01-16 07:56:08.000000000 -0500
+++ b/drivers/scsi/scsi.c 2006-02-02 08:40:36.522927910 -0500
@@ -621,6 +621,23 @@
goto out;
}
+ /*
+ * Device or LLD write protect enforcement.
+ */
+ if (unlikely(!cmd->device->writeable) &&
+ (cmd->sc_data_direction == DMA_TO_DEVICE)) {
+ cmd->result = DID_OK << 16
+ | COMMAND_COMPLETE << 8
+ | SAM_STAT_CHECK_CONDITION;
+ cmd->sense_buffer[0] = 0xF0;
+ cmd->sense_buffer[2] = DATA_PROTECT;
+ cmd->sense_buffer[7] = 6;
+ cmd->sense_buffer[12] = DATA_PROTECT << 1;
+ cmd->sense_buffer[13] = 0x05;
+ scsi_done(cmd);
+ goto out;
+ }
+
spin_lock_irqsave(host->host_lock, flags);
scsi_cmd_get_serial(host, cmd);
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2006-02-18 2:59 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2006-02-06 14:05 [PATCH] megaraid_sas: fix physical disk handling Christoph Hellwig
2006-02-09 18:30 ` Sumant Patro
2006-02-09 22:55 ` Douglas Gilbert
2006-02-17 11:13 ` Christoph Hellwig
2006-02-18 3:02 ` Sumant Patro
-- strict thread matches above, loose matches on Subject: below --
2006-02-09 20:44 Salyzyn, Mark
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).