From: Arnd Bergmann <arnd@arndb.de>
To: scameron@beardog.cce.hp.com
Subject: Re: Question about driver ioctl, big kernel lock, mutexes
Date: Wed, 16 Mar 2011 20:36:56 +0100
User-Agent: KMail/1.13.5 (Linux/2.6.38-rc8+; KDE/4.5.1; x86_64; ; )
Cc: linux-kernel@vger.kernel.org, axboe@kernel.dk, dab@hp.com,
        mike.miller@hp.com
References: <20110316155033.GQ12760@beardog.cce.hp.com>
In-Reply-To: <20110316155033.GQ12760@beardog.cce.hp.com>
MIME-Version: 1.0
Content-Type: Text/Plain;
  charset="iso-8859-1"
Content-Transfer-Encoding: 7bit
Message-Id: <201103162036.56474.arnd@arndb.de>
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2111
Lines: 42

On Wednesday 16 March 2011 16:50:33 scameron@beardog.cce.hp.com wrote:
> That's a pretty big change in behavior.  Some commands sent through
> the passthrough ioctls may take awhile, and they're going to block
> any other ioctls from getting started while they're going.  Previously,
> it was possible to saturate the controller using multiple threads or
> processes hammering on the ioctl path.  This would appear to no longer
> be the case.

I just looked a bit closer at the driver, this is what I noticed:

* The mutex currently protects CCISS_GETNODENAME against CCISS_SETNODENAME
and CCISS_GETINTINFO against CCISS_SETINTINFO. You can easily change
that by taking &h->lock in the CCISS_GET* functions.

* The scsi ioctls don't need the mutex, we've checked that elsewhere.

* I can see no reason why any of the CCISS specific ioctls would
require a lock. There is no global data they touch, and very little
host specific data, most of which is accessed under a host spinlock.

* When you clean up the ioctl handling to remove the mutex, it would
be a good idea to clean up the compat_ioctl handling at the same
time, which is another artifact of a legacy implementation. Just
change cciss_ioctl_big_passthru to take a BIG_IOCTL_Command_struct
kernel pointer argument, and then it directly from
cciss_ioctl32_big_passthru without the extra compat_alloc_user_space
and copy_in_user. Same for cciss_ioctl32_passthru. This will
simplify the compat code path and make it faster.

* Allocating consistent memory on behalf of the user indeed looks
like a potential DoS attack, but it's only possible if you have
read permissions on the block device. To improve this, you should
track the total amount of memory allocated to the device, since
even a single ioctl apparently could be used to pin down all memory
otherwise, independent of the number of commands.

	Arnd
--
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/