[PATCH] DAC960 open with O_NONBLOCK

[PATCH] DAC960 open with O_NONBLOCK

[Dave Olien]

This patch fixes a problem with the DAC960 driver that occurs when the
block special file for controller 0, disk 0 is opened with the O_NONBLOCK
flag.

The DAC960 is a RAID controller, and under startup or error conditions,
it's possible for all devices under DAC960 control to be "offline".  
The driver supports "pass through" commands to the controller that can be
used to diagnose and modify the state of disk devices. A RAID management
tool uses ioctl() on a "special" file descriptor to send these "pass through"
commands to a controller. The first argument of these ioctls is the
controller number that is the target of the pass through command. So, this
one special file descriptor can operate on any DAC960 controller.

This "special" file descriptor is created when the driver's open method
is called with for block special file for controller 0, disk 0,
with the O_NONBLOCK flag set.  In thsi case, the driver's open method
bypasses all checks for a disk associated with that file, and doesn't
increment any reference counts.

The problem comes at close() time.  This driver code apparently originated
with linux 2.2, where driver operations were all "file operations".
In linux 2.2, the driver release method was passed the file descriptor
that has the O_NONBLOCK flag set.  The release method knows then to not
decrement reference counts, etc.

In linux 2.4, driver release methods still have a file descriptor argument.
But, the kernel NEVER passes a non-NULL file argument to the release method.
So, the DAC960 driver doesn't know that the release method is being called
for this "special" file descriptor, and improperly decrements reference
counts.  This makes subsequent opens now work right.

I don't like this "special file descriptor" method.  I would have much
rather created an entry in /proc or something to support these pass through
commands.  But I imagine there are applications out there that expect
these special file descriptors.  On the other hand, this HAS been
broken throught the life on linux 2.4.

The patch below does a "best try" fix to this problem.  It's not
a 100% reliable fix.

I'll be submitting a similar patch to linux 2.5 shortly.

Comments?



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

diff -ur linux-2.4.18_original/drivers/block/DAC960.c linux-2.4.18_DAC/drivers/block/DAC960.c
--- linux-2.4.18_original/drivers/block/DAC960.c	2001-10-25 13:58:35.000000000 -0700
+++ linux-2.4.18_DAC/drivers/block/DAC960.c	2003-04-17 13:11:15.000000000 -0700
@@ -48,6 +48,31 @@
 #include <asm/uaccess.h>
 #include "DAC960.h"
 
+/*
+ * DAC960_SpecialInode is used to indicate that the DAC960_Open
+ * method was called with the file descriptor that has its O_NONBLOCK
+ * flag set, and was with an inode for controller 0, logical device 0.
+ *
+ * Under this case, DAC960_Open enables a "special" file descriptor that
+ * does not reference a real disk device.  This file descriptor can
+ * be used ONLY for calls to DAC960_UserIOCTL(), which is able to
+ * operate on DAC960 controllers to do "pass through" commands.  These
+ * "pass through" commands can be used to query the state of the devices
+ * on the controller, and modify the state of those devices.
+ *
+ * This "special" file descriptor implementation is a bad idea.  But,
+ * we're stuck with it because there applications that rely on it.
+ * (although this has been broken throughout the entire linux 2.4
+ * release.  So, maybe this could in fact be removed.)
+ *
+ * This fix isn't completely reliable.  But, it should handle  the
+ * common cases.  Almost certainly, there is only one inode for
+ * the (controller 0, disk 0) device in any system.  So, the
+ * SpecialInode pointer is really just a flag in this case.  But,
+ * we'll save the inode pointer as well, just in case.
+ */
+static Inode_T		*DAC960_SpecialInode = NULL;
+static spinlock_t	DAC960_SpecialInodeLock;
 
 /*
   DAC960_ControllerCount is the number of DAC960 Controllers detected.
@@ -5340,9 +5365,29 @@
   int ControllerNumber = DAC960_ControllerNumber(Inode->i_rdev);
   int LogicalDriveNumber = DAC960_LogicalDriveNumber(Inode->i_rdev);
   DAC960_Controller_T *Controller;
+
+  /*
+   * Open a "Special" file descriptor that can be used
+   * to operate on any DAC960 controller, even if there are
+   * no logical devices online.  This hooks into code
+   * in DAC960_IOCTL and DAC960_Close.
+   *
+   * This "Special" file descriptor is a bad idea, but
+   * we're probably stuck with it because of existing 
+   * applications that use it.
+   */
   if (ControllerNumber == 0 && LogicalDriveNumber == 0 &&
-      (File->f_flags & O_NONBLOCK))
-    goto ModuleOnly;
+		(File->f_flags & O_NONBLOCK) && capable(CAP_SYS_ADMIN)) {
+      spin_lock_irq(&DAC960_SpecialInodeLock);
+      if (DAC960_SpecialInode != NULL) {
+      	spin_unlock_irq(&DAC960_SpecialInodeLock);
+	return -ENXIO;
+      }
+      DAC960_SpecialInode = Inode;
+      spin_unlock_irq(&DAC960_SpecialInodeLock);
+      goto ModuleOnly;
+  }
+
   if (ControllerNumber < 0 || ControllerNumber > DAC960_ControllerCount - 1)
     return -ENXIO;
   Controller = DAC960_Controllers[ControllerNumber];
@@ -5376,7 +5421,6 @@
   /*
     Increment Controller and Logical Drive Usage Counts.
   */
-  Controller->ControllerUsageCount++;
   Controller->LogicalDriveUsageCount[LogicalDriveNumber]++;
  ModuleOnly:
   return 0;
@@ -5392,15 +5436,12 @@
   int ControllerNumber = DAC960_ControllerNumber(Inode->i_rdev);
   int LogicalDriveNumber = DAC960_LogicalDriveNumber(Inode->i_rdev);
   DAC960_Controller_T *Controller = DAC960_Controllers[ControllerNumber];
-  if (ControllerNumber == 0 && LogicalDriveNumber == 0 &&
-      File != NULL && (File->f_flags & O_NONBLOCK))
-    goto ModuleOnly;
-  /*
-    Decrement the Logical Drive and Controller Usage Counts.
-  */
-  Controller->LogicalDriveUsageCount[LogicalDriveNumber]--;
-  Controller->ControllerUsageCount--;
- ModuleOnly:
+
+  if ((Inode == DAC960_SpecialInode) && capable(CAP_SYS_ADMIN))
+    DAC960_SpecialInode = NULL;
+  else
+     Controller->LogicalDriveUsageCount[LogicalDriveNumber]--;
+
   return 0;
 }
 
diff -ur linux-2.4.18_original/drivers/block/DAC960.h linux-2.4.18_DAC/drivers/block/DAC960.h
--- linux-2.4.18_original/drivers/block/DAC960.h	2001-10-17 14:46:29.000000000 -0700
+++ linux-2.4.18_DAC/drivers/block/DAC960.h	2003-04-17 11:17:45.000000000 -0700
@@ -2341,7 +2341,6 @@
   unsigned short MaxBlocksPerCommand;
   unsigned short ControllerScatterGatherLimit;
   unsigned short DriverScatterGatherLimit;
-  unsigned int ControllerUsageCount;
   unsigned int CombinedStatusBufferLength;
   unsigned int InitialStatusLength;
   unsigned int CurrentStatusLength;

Re: [PATCH] DAC960 open with O_NONBLOCK

[Christoph Hellwig]

On Mon, Apr 21, 2003 at 10:24:04AM -0700, Dave Olien wrote:
> This "special" file descriptor is created when the driver's open method
> is called with for block special file for controller 0, disk 0,
> with the O_NONBLOCK flag set.  In thsi case, the driver's open method
> bypasses all checks for a disk associated with that file, and doesn't
> increment any reference counts.
> 
> The problem comes at close() time.  This driver code apparently originated
> with linux 2.2, where driver operations were all "file operations".
> In linux 2.2, the driver release method was passed the file descriptor
> that has the O_NONBLOCK flag set.  The release method knows then to not
> decrement reference counts, etc.
>
> In linux 2.4, driver release methods still have a file descriptor argument.
> But, the kernel NEVER passes a non-NULL file argument to the release method.
> So, the DAC960 driver doesn't know that the release method is being called
> for this "special" file descriptor, and improperly decrements reference
> counts.  This makes subsequent opens now work right.

I just went over the same code in 2.5 - the reference counts are
entirely superflous, you can just nuke them.

> I don't like this "special file descriptor" method.  I would have much
> rather created an entry in /proc or something to support these pass through
> commands.  But I imagine there are applications out there that expect
> these special file descriptors.  On the other hand, this HAS been
> broken throught the life on linux 2.4.

What applications?

> I'll be submitting a similar patch to linux 2.5 shortly.

Don't even bother.  Linux 2.5 is the place to fix this issue
correctly.

Re: [PATCH] DAC960 open with O_NONBLOCK

[Dave Olien]

On Mon, Apr 21, 2003 at 06:37:52PM +0100, Christoph Hellwig wrote:
> 
> I just went over the same code in 2.5 - the reference counts are
> entirely superflous, you can just nuke them.

Yup, that was the plan.

> 
> > I don't like this "special file descriptor" method.  I would have much
> > rather created an entry in /proc or something to support these pass through
> > commands.  But I imagine there are applications out there that expect
> > these special file descriptors.  On the other hand, this HAS been
> > broken throught the life on linux 2.4.
> 
> What applications?

John Kamp has run across a libhd applcation from Suse that hit this bug.
It's some kind of hardware detection application.  It opens devices with
O_NONBLOCK.  But, it doesn't in fact use the DAC960 pass-through commands.

The Mylex web page has a RAID management application for DAC960 on Linux that
is available only in BINARY form.  Unfortunately, it requires
a Windows front-end to provide a GUI.  So, I haven't actually experimented
with it. If any application uses the pass-through commands, this would likely
be it.  But since no one has complained about this being broken, it may
indicate no one is using this application.

Or, maybe the application is just not being used in a way that triggers
this bug.

I'm reluctant to just eliminate the behavior because of that.

> 
> > I'll be submitting a similar patch to linux 2.5 shortly.
> 
> Don't even bother.  Linux 2.5 is the place to fix this issue
> correctly.

It would be nice to just eliminate the O_NONBLOCK sillines from the
open(), ioctl(), and release() methods.

The pass-through behavior could be made available either through
a /proc or a sysfs file.

The difficulty is that the Mylex application is available only in binary
form.  Mylex is very secretive about its controller commands.
It would be nice to be able to create a library that an application
could call to perform high-level operations, and the library would
construct the pass-through commands and pass them to the driver.
Then, anyone could write their own GUI.

A related question, why does linux 2.5 continue to have a "struct file *"
argument to driver release methods? As far as I can tell, that argument
is always NULL?

Re: [PATCH] DAC960 open with O_NONBLOCK

[Christoph Hellwig]

On Mon, Apr 21, 2003 at 12:01:11PM -0700, Dave Olien wrote:
> > What applications?
> 
> John Kamp has run across a libhd applcation from Suse that hit this bug.
> It's some kind of hardware detection application.  It opens devices with
> O_NONBLOCK.  But, it doesn't in fact use the DAC960 pass-through commands.

Do you have source to it?

> The Mylex web page has a RAID management application for DAC960 on Linux that
> is available only in BINARY form.  Unfortunately, it requires
> a Windows front-end to provide a GUI.  So, I haven't actually experimented
> with it. If any application uses the pass-through commands, this would likely
> be it.  But since no one has complained about this being broken, it may
> indicate no one is using this application.

Hmm, breaking it wouldn't be nice, but if they're not willing to
release an updated version we'll just need a LD_PRELOAD wrapper
that maps the open to a new mangment device.

> The pass-through behavior could be made available either through
> a /proc or a sysfs file.

My preference would be a char device (miscdev)

> A related question, why does linux 2.5 continue to have a "struct file *"
> argument to driver release methods? As far as I can tell, that argument
> is always NULL?

->release will change to struct gendisk * at some point.  Touching
it before to just remove the struct file * sounds like a bad idea.

Re: [PATCH] DAC960 open with O_NONBLOCK

[Alan Cox]

> John Kamp has run across a libhd applcation from Suse that hit this bug.
> It's some kind of hardware detection application.  It opens devices with
> O_NONBLOCK.  But, it doesn't in fact use the DAC960 pass-through commands.

It should be checking major/minor first - rightfully or wrongly (I'd 
favour wrongly) open has side effects on multiple devices.

> The difficulty is that the Mylex application is available only in binary
> form.  Mylex is very secretive about its controller commands.
> It would be nice to be able to create a library that an application
> could call to perform high-level operations, and the library would
> construct the pass-through commands and pass them to the driver.
> Then, anyone could write their own GUI.

Or someone could just stick a command dumper in the driver. Thats how I
wrote ps and a few other utils for my aacraid 8)

Re: [PATCH] DAC960 open with O_NONBLOCK

[Dave Olien]

I looked briefly at the source code for libhd.  It consists of two
applications: hwinfo and hwscan, and a supporting library.  It has
compiled into it knowledge of all "possible" device drivers, including
the DAC960.  It has a unique of probe routine for each "possible"
device driver.  It collects information about the devices, such as disk
geometry and partition sizes.

Some of this information is collected using ioctl().  John indicated my
patch to linux 2.4's DAC driver worked.  But I think my patch makes it
only "mostly" worked, and is better because it doesn't leave the driver
in a broken state.

I think this problem needs to be fixed correctly.  I'll try to
put together a fix first for linux 2.5, and then backport it to linux 2.4.

I'll first try to get the Mylex "Global Array Manager" (GAM) software working
on a system around here.  I'll make certain it is using the pass-through
commands.  Then, I'll either try to reverse-engineer the pass-through
commands, or I'll try Christoph's suggestion of moving the
pass-through ioctl's to a new char device, and try writing a LD_PRELOAD
wrapper.

I've looked at Christoph's DAC960 patch fixing up the open method, removing
the release method and adding a media change method.  It looks OK.
I'll apply it to my local source, and begin changes from there.

I'll see if I can get something in about a week.

On Mon, Apr 21, 2003 at 04:14:44PM -0400, Alan Cox wrote:
> 
> > John Kamp has run across a libhd applcation from Suse that hit this bug.
> > It's some kind of hardware detection application.  It opens devices with
> > O_NONBLOCK.  But, it doesn't in fact use the DAC960 pass-through commands.
> 
> It should be checking major/minor first - rightfully or wrongly (I'd 
> favour wrongly) open has side effects on multiple devices.
> 
> > The difficulty is that the Mylex application is available only in binary
> > form.  Mylex is very secretive about its controller commands.
> > It would be nice to be able to create a library that an application
> > could call to perform high-level operations, and the library would
> > construct the pass-through commands and pass them to the driver.
> > Then, anyone could write their own GUI.
> 
> Or someone could just stick a command dumper in the driver. Thats how I
> wrote ps and a few other utils for my aacraid 8)