[patch 1/1] Switch ioctl functions of drivers/scsi/sg.c to unlocked_ioctl

[patch 1/1] Switch ioctl functions of drivers/scsi/sg.c to unlocked_ioctl

[Andre Noll]

[-- Attachment #1: Type: text/plain, Size: 12540 bytes --]

[Resent with proper subject and to additional recipients]

This patch against linus-current is compile-tested on x86 and x86-64.

Please review
Andre
---

Change sg.c to use the unlocked_ioctl instead of the ioctl handler.
The patch moves the BKL into sg.c and is thus a first step to get
rid of the BKL in the scsi code.

Signed-off-by: Andre Noll <maan@systemlinux.org>

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index f1871ea..3063307 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -48,6 +48,7 @@ static int sg_version_num = 30534;	/* 2 digits for each component */
 #include <linux/blkdev.h>
 #include <linux/delay.h>
 #include <linux/scatterlist.h>
+#include <linux/smp_lock.h>
 
 #include "scsi.h"
 #include <scsi/scsi_dbg.h>
@@ -764,20 +765,22 @@ sg_srp_done(Sg_request *srp, Sg_fd *sfp)
 	return done;
 }
 
-static int
-sg_ioctl(struct inode *inode, struct file *filp,
-	 unsigned int cmd_in, unsigned long arg)
+static long
+sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned long arg)
 {
 	void __user *p = (void __user *)arg;
 	int __user *ip = p;
-	int result, val, read_only;
+	int val, read_only;
 	Sg_device *sdp;
 	Sg_fd *sfp;
 	Sg_request *srp;
 	unsigned long iflags;
+	long result;
 
+	lock_kernel();
+	result = -ENXIO;
 	if ((!(sfp = (Sg_fd *) filp->private_data)) || (!(sdp = sfp->parentdp)))
-		return -ENXIO;
+		goto out;
 	SCSI_LOG_TIMEOUT(3, printk("sg_ioctl: %s, cmd=0x%x\n",
 				   sdp->disk->disk_name, (int) cmd_in));
 	read_only = (O_RDWR != (filp->f_flags & O_ACCMODE));
@@ -787,57 +790,66 @@ sg_ioctl(struct inode *inode, struct file *filp,
 		{
 			int blocking = 1;	/* ignore O_NONBLOCK flag */
 
+			result = -ENODEV;
 			if (sdp->detached)
-				return -ENODEV;
+				goto out;
+			result = -ENXIO;
 			if (!scsi_block_when_processing_errors(sdp->device))
-				return -ENXIO;
+				goto out;
+			result = -EFAULT;
 			if (!access_ok(VERIFY_WRITE, p, SZ_SG_IO_HDR))
-				return -EFAULT;
-			result =
-			    sg_new_write(sfp, p, SZ_SG_IO_HDR,
-					 blocking, read_only, &srp);
+				goto out;
+			result = sg_new_write(sfp, p, SZ_SG_IO_HDR, blocking,
+				read_only, &srp);
 			if (result < 0)
-				return result;
+				goto out;
 			srp->sg_io_owned = 1;
 			while (1) {
 				result = 0;	/* following macro to beat race condition */
 				__wait_event_interruptible(sfp->read_wait,
 					(sdp->detached || sfp->closed || sg_srp_done(srp, sfp)),
 							   result);
-				if (sdp->detached)
-					return -ENODEV;
+				if (sdp->detached) {
+					result = -ENODEV;
+					goto out;
+				}
 				if (sfp->closed)
-					return 0;	/* request packet dropped already */
+					goto out;	/* request packet dropped already */
 				if (0 == result)
 					break;
 				srp->orphan = 1;
-				return result;	/* -ERESTARTSYS because signal hit process */
+				goto out;	/* -ERESTARTSYS because signal hit process */
 			}
 			write_lock_irqsave(&sfp->rq_list_lock, iflags);
 			srp->done = 2;
 			write_unlock_irqrestore(&sfp->rq_list_lock, iflags);
 			result = sg_new_read(sfp, p, SZ_SG_IO_HDR, srp);
-			return (result < 0) ? result : 0;
+			if (result > 0)
+				result = 0;
+			goto out;
 		}
 	case SG_SET_TIMEOUT:
 		result = get_user(val, ip);
 		if (result)
-			return result;
+			goto out;
+		result = -EIO;
 		if (val < 0)
-			return -EIO;
-		if (val >= MULDIV (INT_MAX, USER_HZ, HZ))
-		    val = MULDIV (INT_MAX, USER_HZ, HZ);
+			goto out;
+		if (val >= MULDIV(INT_MAX, USER_HZ, HZ))
+			val = MULDIV(INT_MAX, USER_HZ, HZ);
 		sfp->timeout_user = val;
 		sfp->timeout = MULDIV (val, HZ, USER_HZ);
 
-		return 0;
+		result = 0;
+		goto out;
 	case SG_GET_TIMEOUT:	/* N.B. User receives timeout as return value */
 				/* strange ..., for backward compatibility */
-		return sfp->timeout_user;
+		result = sfp->timeout_user;
+		goto out;
 	case SG_SET_FORCE_LOW_DMA:
 		result = get_user(val, ip);
 		if (result)
-			return result;
+			goto out;
 		if (val) {
 			sfp->low_dma = 1;
 			if ((0 == sfp->low_dma) && (0 == sg_res_in_use(sfp))) {
@@ -846,21 +858,26 @@ sg_ioctl(struct inode *inode, struct file *filp,
 				sg_build_reserve(sfp, val);
 			}
 		} else {
+			result = -ENODEV;
 			if (sdp->detached)
-				return -ENODEV;
+				goto out;
 			sfp->low_dma = sdp->device->host->unchecked_isa_dma;
 		}
-		return 0;
+		result = 0;
+		goto out;
 	case SG_GET_LOW_DMA:
-		return put_user((int) sfp->low_dma, ip);
+		result = put_user((int) sfp->low_dma, ip);
+		goto out;
 	case SG_GET_SCSI_ID:
+		result = -EFAULT;
 		if (!access_ok(VERIFY_WRITE, p, sizeof (sg_scsi_id_t)))
-			return -EFAULT;
-		else {
+			goto out;
+		{
 			sg_scsi_id_t __user *sg_idp = p;
 
+			result = -ENODEV;
 			if (sdp->detached)
-				return -ENODEV;
+				goto out;
 			__put_user((int) sdp->device->host->host_no,
 				   &sg_idp->host_no);
 			__put_user((int) sdp->device->channel,
@@ -874,29 +891,33 @@ sg_ioctl(struct inode *inode, struct file *filp,
 				   &sg_idp->d_queue_depth);
 			__put_user(0, &sg_idp->unused[0]);
 			__put_user(0, &sg_idp->unused[1]);
-			return 0;
+			result = 0;
+			goto out;
 		}
 	case SG_SET_FORCE_PACK_ID:
 		result = get_user(val, ip);
 		if (result)
-			return result;
+			goto out;
 		sfp->force_packid = val ? 1 : 0;
-		return 0;
+		result = 0;
+		goto out;
 	case SG_GET_PACK_ID:
+		result = -EFAULT;
 		if (!access_ok(VERIFY_WRITE, ip, sizeof (int)))
-			return -EFAULT;
+			goto out;
 		read_lock_irqsave(&sfp->rq_list_lock, iflags);
+		result = 0;
 		for (srp = sfp->headrp; srp; srp = srp->nextrp) {
 			if ((1 == srp->done) && (!srp->sg_io_owned)) {
 				read_unlock_irqrestore(&sfp->rq_list_lock,
 						       iflags);
 				__put_user(srp->header.pack_id, ip);
-				return 0;
+				goto out;
 			}
 		}
 		read_unlock_irqrestore(&sfp->rq_list_lock, iflags);
 		__put_user(-1, ip);
-		return 0;
+		goto out;
 	case SG_GET_NUM_WAITING:
 		read_lock_irqsave(&sfp->rq_list_lock, iflags);
 		for (val = 0, srp = sfp->headrp; srp; srp = srp->nextrp) {
@@ -904,67 +925,76 @@ sg_ioctl(struct inode *inode, struct file *filp,
 				++val;
 		}
 		read_unlock_irqrestore(&sfp->rq_list_lock, iflags);
-		return put_user(val, ip);
+		result = put_user(val, ip);
+		goto out;
 	case SG_GET_SG_TABLESIZE:
-		return put_user(sdp->sg_tablesize, ip);
+		result = put_user(sdp->sg_tablesize, ip);
+		goto out;
 	case SG_SET_RESERVED_SIZE:
 		result = get_user(val, ip);
 		if (result)
-			return result;
-                if (val < 0)
-                        return -EINVAL;
+			goto out;
+		result = -EINVAL;
+		if (val < 0)
+			goto out;
 		val = min_t(int, val,
 				sdp->device->request_queue->max_sectors * 512);
 		if (val != sfp->reserve.bufflen) {
+			result = -EBUSY;
 			if (sg_res_in_use(sfp) || sfp->mmap_called)
-				return -EBUSY;
+				goto out;
 			sg_remove_scat(&sfp->reserve);
 			sg_build_reserve(sfp, val);
 		}
-		return 0;
+		result = 0;
+		goto out;
 	case SG_GET_RESERVED_SIZE:
 		val = min_t(int, sfp->reserve.bufflen,
 				sdp->device->request_queue->max_sectors * 512);
-		return put_user(val, ip);
+		result = put_user(val, ip);
+		goto out;
 	case SG_SET_COMMAND_Q:
 		result = get_user(val, ip);
-		if (result)
-			return result;
-		sfp->cmd_q = val ? 1 : 0;
-		return 0;
+		if (!result)
+			sfp->cmd_q = val ? 1 : 0;
+		goto out;
 	case SG_GET_COMMAND_Q:
-		return put_user((int) sfp->cmd_q, ip);
+		result = put_user((int) sfp->cmd_q, ip);
+		goto out;
 	case SG_SET_KEEP_ORPHAN:
 		result = get_user(val, ip);
-		if (result)
-			return result;
-		sfp->keep_orphan = val;
-		return 0;
+		if (!result)
+			sfp->keep_orphan = val;
+		goto out;
 	case SG_GET_KEEP_ORPHAN:
-		return put_user((int) sfp->keep_orphan, ip);
+		result = put_user((int) sfp->keep_orphan, ip);
+		goto out;
 	case SG_NEXT_CMD_LEN:
 		result = get_user(val, ip);
-		if (result)
-			return result;
-		sfp->next_cmd_len = (val > 0) ? val : 0;
-		return 0;
+		if (!result)
+			sfp->next_cmd_len = (val > 0) ? val : 0;
+		goto out;
 	case SG_GET_VERSION_NUM:
-		return put_user(sg_version_num, ip);
+		result = put_user(sg_version_num, ip);
+		goto out;
 	case SG_GET_ACCESS_COUNT:
 		/* faked - we don't have a real access count anymore */
 		val = (sdp->device ? 1 : 0);
-		return put_user(val, ip);
+		result = put_user(val, ip);
+		goto out;
 	case SG_GET_REQUEST_TABLE:
+		result = -EFAULT;
 		if (!access_ok(VERIFY_WRITE, p, SZ_SG_REQ_INFO * SG_MAX_QUEUE))
-			return -EFAULT;
-		else {
+			goto out;
+		{
 			sg_req_info_t *rinfo;
 			unsigned int ms;
 
 			rinfo = kmalloc(SZ_SG_REQ_INFO * SG_MAX_QUEUE,
 								GFP_KERNEL);
+			result = -ENOMEM;
 			if (!rinfo)
-				return -ENOMEM;
+				goto out;
 			read_lock_irqsave(&sfp->rq_list_lock, iflags);
 			for (srp = sfp->headrp, val = 0; val < SG_MAX_QUEUE;
 			     ++val, srp = srp ? srp->nextrp : srp) {
@@ -998,25 +1028,29 @@ sg_ioctl(struct inode *inode, struct file *filp,
 						SZ_SG_REQ_INFO * SG_MAX_QUEUE);
 			result = result ? -EFAULT : 0;
 			kfree(rinfo);
-			return result;
 		}
+		goto out;
 	case SG_EMULATED_HOST:
 		if (sdp->detached)
-			return -ENODEV;
-		return put_user(sdp->device->host->hostt->emulated, ip);
+			result = -ENODEV;
+		else
+			result = put_user(sdp->device->host->hostt->emulated, ip);
+		goto out;
 	case SG_SCSI_RESET:
+		result = -ENODEV;
 		if (sdp->detached)
-			return -ENODEV;
+			goto out;
+		result = -EBUSY;
 		if (filp->f_flags & O_NONBLOCK) {
 			if (scsi_host_in_recovery(sdp->device->host))
-				return -EBUSY;
+				goto out;
 		} else if (!scsi_block_when_processing_errors(sdp->device))
-			return -EBUSY;
+			goto out;
 		result = get_user(val, ip);
 		if (result)
-			return result;
+			goto out;
 		if (SG_SCSI_RESET_NOTHING == val)
-			return 0;
+			goto out;
 		switch (val) {
 		case SG_SCSI_RESET_DEVICE:
 			val = SCSI_TRY_RESET_DEVICE;
@@ -1028,46 +1062,60 @@ sg_ioctl(struct inode *inode, struct file *filp,
 			val = SCSI_TRY_RESET_HOST;
 			break;
 		default:
-			return -EINVAL;
+			result = -EINVAL;
+			goto out;
 		}
+		result = -EACCES;
 		if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO))
-			return -EACCES;
-		return (scsi_reset_provider(sdp->device, val) ==
-			SUCCESS) ? 0 : -EIO;
+			goto out;
+		result = scsi_reset_provider(sdp->device, val) == SUCCESS?
+			0 : -EIO;
+		goto out;
 	case SCSI_IOCTL_SEND_COMMAND:
+		result = -ENODEV;
 		if (sdp->detached)
-			return -ENODEV;
+			goto out;
 		if (read_only) {
 			unsigned char opcode = WRITE_6;
 			Scsi_Ioctl_Command __user *siocp = p;
 
+			result = -EFAULT;
 			if (copy_from_user(&opcode, siocp->data, 1))
-				return -EFAULT;
+				goto out;
+			result = -EPERM;
 			if (!sg_allow_access(opcode, sdp->device->type))
-				return -EPERM;
+				goto out;
 		}
-		return sg_scsi_ioctl(filp, sdp->device->request_queue, NULL, p);
+		result = sg_scsi_ioctl(filp, sdp->device->request_queue, NULL, p);
+		goto out;
 	case SG_SET_DEBUG:
 		result = get_user(val, ip);
-		if (result)
-			return result;
-		sdp->sgdebug = (char) val;
-		return 0;
+		if (!result)
+			sdp->sgdebug = (char) val;
+		goto out;
 	case SCSI_IOCTL_GET_IDLUN:
 	case SCSI_IOCTL_GET_BUS_NUMBER:
 	case SCSI_IOCTL_PROBE_HOST:
 	case SG_GET_TRANSFORM:
+		result = -ENODEV;
 		if (sdp->detached)
-			return -ENODEV;
-		return scsi_ioctl(sdp->device, cmd_in, p);
+			goto out;
+		result = scsi_ioctl(sdp->device, cmd_in, p);
+		goto out;
 	case BLKSECTGET:
-		return put_user(sdp->device->request_queue->max_sectors * 512,
+		result = put_user(sdp->device->request_queue->max_sectors * 512,
 				ip);
+		goto out;
 	default:
+		result = -EPERM;	/* don't know so take safe approach */
 		if (read_only)
-			return -EPERM;	/* don't know so take safe approach */
-		return scsi_ioctl(sdp->device, cmd_in, p);
+			goto out;
+		result = scsi_ioctl(sdp->device, cmd_in, p);
+		goto out;
 	}
+out:
+	unlock_kernel();
+	return result;
 }
 
 #ifdef CONFIG_COMPAT
@@ -1314,7 +1362,7 @@ static struct file_operations sg_fops = {
 	.read = sg_read,
 	.write = sg_write,
 	.poll = sg_poll,
-	.ioctl = sg_ioctl,
+	.unlocked_ioctl = sg_ioctl,
 #ifdef CONFIG_COMPAT
 	.compat_ioctl = sg_compat_ioctl,
 #endif
-- 
The only person who always got his work done by Friday was Robinson Crusoe

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [patch 1/1] Switch ioctl functions of drivers/scsi/sg.c to unlocked_ioctl

[James Bottomley]

On Thu, 2008-01-10 at 19:05 +0100, Andre Noll wrote:
> [Resent with proper subject and to additional recipients]
> 
> This patch against linus-current is compile-tested on x86 and x86-64.
> 
> Please review

This is rather long.  For the utility of what you've just done, what's
wrong with just making the .unlocked_ioctl point to sg_unlocked_ioctl()
and doing:

sg_unlocked_ioctl()
	int rc;

	lock_kernel();
	rc = sg_ioctl();
	unlock_kernel();

	return rc;
}

Really, all this is doing is open coding what the ioctl handler is doing
anyway, isn't it? in which case, why bother to change it at all?

James

Re: [patch 1/1] Switch ioctl functions of drivers/scsi/sg.c to unlocked_ioctl

[Andi Kleen]

> Really, all this is doing is open coding what the ioctl handler is doing
> anyway, isn't it? in which case, why bother to change it at all?

Because once it's open coded it is visible and can then be eliminated.
Does SCSI need the BKL at all?

But perhaps for such a long ioctl handler it would be better to move
the lock/unlock_kernel()s into the individual case ...: statements;
then it could be eliminated step by step.

-Andi

Re: [patch 1/1] Switch ioctl functions of drivers/scsi/sg.c to unlocked_ioctl

[Matthew Wilcox]

On Thu, Jan 10, 2008 at 07:59:44PM +0100, Andi Kleen wrote:
> > Really, all this is doing is open coding what the ioctl handler is doing
> > anyway, isn't it? in which case, why bother to change it at all?
> 
> Because once it's open coded it is visible and can then be eliminated.
> Does SCSI need the BKL at all?
> 
> But perhaps for such a long ioctl handler it would be better to move
> the lock/unlock_kernel()s into the individual case ...: statements;
> then it could be eliminated step by step.

This style of conversion is going to cause a lot of churn --
re-architecting this function to be single-exit, then presumably when
the lock_kernel calls are pushed further down or eliminated, turning it
back into a multiple-exit function.

I suggest that for complex ioctl handlers, it be left to the maintainers
to handle, and handle it properly all at once, rather than a gradual
pushdown.

You could argue that unlocked_ioctl has been around for a long time and
people haven't made that move yet.  But there's been no pressure before
now to do so, and I think people would rather convert their own code
than have somebody else do it.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [patch 1/1] Switch ioctl functions of drivers/scsi/sg.c to unlocked_ioctl

[Andi Kleen]

On Thu, Jan 10, 2008 at 12:03:24PM -0700, Matthew Wilcox wrote:
> On Thu, Jan 10, 2008 at 07:59:44PM +0100, Andi Kleen wrote:
> > > Really, all this is doing is open coding what the ioctl handler is doing
> > > anyway, isn't it? in which case, why bother to change it at all?
> > 
> > Because once it's open coded it is visible and can then be eliminated.
> > Does SCSI need the BKL at all?
> > 
> > But perhaps for such a long ioctl handler it would be better to move
> > the lock/unlock_kernel()s into the individual case ...: statements;
> > then it could be eliminated step by step.
> 
> This style of conversion is going to cause a lot of churn --

If he pushes the lock_kernels() into the cases then not.

But even a few refactorings are not that bad if they result in
a BKL less SCSI.

> re-architecting this function to be single-exit, then presumably when
> the lock_kernel calls are pushed further down or eliminated, turning it
> back into a multiple-exit function.
> 
> I suggest that for complex ioctl handlers, it be left to the maintainers

Which have had years to convert their drivers, but they mostly didn't
do it. They need more incentive.

> to handle, and handle it properly all at once, rather than a gradual
> pushdown.
> 
> You could argue that unlocked_ioctl has been around for a long time and
> people haven't made that move yet.  But there's been no pressure before

Yes I argue exactly that.

> now to do so, and I think people would rather convert their own code
> than have somebody else do it.

Has been proven to not work.

I don't really blame the maintainers too much. The implicit BKL in ioctl
is actually pretty obscure. But making it explicit will force their
hand.

So in short I'm sorry, but you're wrong on that.

-Andi

Re: [patch 1/1] Switch ioctl functions of drivers/scsi/sg.c to unlocked_ioctl

[James Bottomley]

On Thu, 2008-01-10 at 19:59 +0100, Andi Kleen wrote:
> > Really, all this is doing is open coding what the ioctl handler is doing
> > anyway, isn't it? in which case, why bother to change it at all?
> 
> Because once it's open coded it is visible and can then be eliminated.
> Does SCSI need the BKL at all?

No, of course not ... it hasn't for ages, otherwise linux performance
would be in the tank.  If we require the BKL at all in the ioctl path it
will be to protect the non-SCSI structures we have to use.  Is there any
guide to which structures in the kernel still require the BKL?

> But perhaps for such a long ioctl handler it would be better to move
> the lock/unlock_kernel()s into the individual case ...: statements;
> then it could be eliminated step by step.

James

Re: [patch 1/1] Switch ioctl functions of drivers/scsi/sg.c to unlocked_ioctl

[Andi Kleen]

On Thu, Jan 10, 2008 at 01:03:48PM -0600, James Bottomley wrote:
> 
> On Thu, 2008-01-10 at 19:59 +0100, Andi Kleen wrote:
> > > Really, all this is doing is open coding what the ioctl handler is doing
> > > anyway, isn't it? in which case, why bother to change it at all?
> > 
> > Because once it's open coded it is visible and can then be eliminated.
> > Does SCSI need the BKL at all?
> 
> No, of course not ... it hasn't for ages, otherwise linux performance
> would be in the tank.  If we require the BKL at all in the ioctl path it
> will be to protect the non-SCSI structures we have to use.  Is there any
> guide to which structures in the kernel still require the BKL?

Not many really in the core kernel. Hardly any. Grep for 
lock_kernel to be sure, but there is not much.

It's mostly drivers that still need it.

How about the low level SCSI drivers that might called from the high
level SCSI code? 

Anyways starting these kinds of discussions was the goal of the proposal.
Even if Andre's patch ends up not being merged it would have reached
its goal.

-Andi 

Re: [patch 1/1] Switch ioctl functions of drivers/scsi/sg.c to unlocked_ioctl

[Matthew Wilcox]

On Thu, Jan 10, 2008 at 08:32:27PM +0100, Andi Kleen wrote:
> Not many really in the core kernel. Hardly any. Grep for 
> lock_kernel to be sure, but there is not much.
> 
> It's mostly drivers that still need it.
> 
> How about the low level SCSI drivers that might called from the high
> level SCSI code? 
> 
> Anyways starting these kinds of discussions was the goal of the proposal.
> Even if Andre's patch ends up not being merged it would have reached
> its goal.

If your goal is to get rid of the BKL everywhere, sure.  It's not clear
to me this is the most productive way of spending our time though.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [patch 1/1] Switch ioctl functions of drivers/scsi/sg.c to unlocked_ioctl

[Andi Kleen]

> If your goal is to get rid of the BKL everywhere, sure.  It's not clear
> to me this is the most productive way of spending our time though.

I believe eventually eliminating ->ioctl (as opposed to ->unlocked_ioctl)
would be a productive thing to do. Still having implicit BKL semantics in 
such an important interface is just wrong.

-Andi

Re: [patch 1/1] Switch ioctl functions of drivers/scsi/sg.c to unlocked_ioctl

[Andre Noll]

[-- Attachment #1: Type: text/plain, Size: 528 bytes --]

On 19:59, Andi Kleen wrote:

> But perhaps for such a long ioctl handler it would be better to move
> the lock/unlock_kernel()s into the individual case ...: statements;
> then it could be eliminated step by step.

Sure, I can do that if James likes the idea. Since not all case
statements need the BKL, we could add it only to those for which it
isn't clear that it is unnecessary.

And this would actually improve something.

Andre
-- 
The only person who always got his work done by Friday was Robinson Crusoe

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [patch 1/1] Switch ioctl functions of drivers/scsi/sg.c to unlocked_ioctl

[Andi Kleen]

On Thu, Jan 10, 2008 at 08:07:48PM +0100, Andre Noll wrote:
> On 19:59, Andi Kleen wrote:
> 
> > But perhaps for such a long ioctl handler it would be better to move
> > the lock/unlock_kernel()s into the individual case ...: statements;
> > then it could be eliminated step by step.
> 
> Sure, I can do that if James likes the idea. Since not all case
> statements need the BKL, we could add it only to those for which it
> isn't clear that it is unnecessary.
> 
> And this would actually improve something.

I still think it would be a good strategy to first add it to all
(in a essentially nop semantics patch) and then later eliminate
it from the cases that obviously don't need it. 

But yes eliminating it from all is the long term goal.

-Andi

Re: [patch 1/1] Switch ioctl functions of drivers/scsi/sg.c to unlocked_ioctl

[Andre Noll]

[-- Attachment #1: Type: text/plain, Size: 590 bytes --]

On 20:29, Andi Kleen wrote:

> > Sure, I can do that if James likes the idea. Since not all case
> > statements need the BKL, we could add it only to those for which it
> > isn't clear that it is unnecessary.
> > 
> > And this would actually improve something.
> 
> I still think it would be a good strategy to first add it to all
> (in a essentially nop semantics patch) and then later eliminate
> it from the cases that obviously don't need it. 

James, would you accept such a patch?

Andre
-- 
The only person who always got his work done by Friday was Robinson Crusoe

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [patch 1/1] Switch ioctl functions of drivers/scsi/sg.c to unlocked_ioctl

[James Bottomley]

On Thu, 2008-01-10 at 20:45 +0100, Andre Noll wrote:
> On 20:29, Andi Kleen wrote:
> 
> > > Sure, I can do that if James likes the idea. Since not all case
> > > statements need the BKL, we could add it only to those for which it
> > > isn't clear that it is unnecessary.
> > > 
> > > And this would actually improve something.
> > 
> > I still think it would be a good strategy to first add it to all
> > (in a essentially nop semantics patch) and then later eliminate
> > it from the cases that obviously don't need it. 
> 
> James, would you accept such a patch?

Our current suspicion is that none of the SCSI ioctl paths require the
BKL.  Al Viro said he'd try to look into this, so currently I think the
conversion path is just to move .ioctl -> .unlocked_ioctl

However, more eyes on the problem would be most welcome.

James

Re: [patch 1/1] Switch ioctl functions of drivers/scsi/sg.c to unlocked_ioctl

[Boaz Harrosh]

On Thu, Jan 10 2008 at 21:45 +0200, Andre Noll <maan@systemlinux.org> wrote:
> On 20:29, Andi Kleen wrote:
> 
>>> Sure, I can do that if James likes the idea. Since not all case
>>> statements need the BKL, we could add it only to those for which it
>>> isn't clear that it is unnecessary.
>>>
>>> And this would actually improve something.
>> I still think it would be a good strategy to first add it to all
>> (in a essentially nop semantics patch) and then later eliminate
>> it from the cases that obviously don't need it. 
> 
> James, would you accept such a patch?
> 
> Andre

Andre hi.

All the scsi calls do not need any locks. The scsi LLDS never
see these threads since commands are queued through the block
layer.

What's left is what you see, here in sg.c. you must have the best 
knowledge about the possible races between ioctl and open/release
and probe/remove. And all these put_user() copy_user() etc...
Why don't you have a hard look and fix them properly.

please don't *lock_kernel();* for scsi's sake. Also note that
sg is not a scsi device it is a block device. It used to Q commands
directly to scsi but it does not do that any more.

Again I think SCSI neto is safe and tested. My $0.02.

Boaz

Re: [patch 1/1] Switch ioctl functions of drivers/scsi/sg.c to unlocked_ioctl

[Andre Noll]

[-- Attachment #1: Type: text/plain, Size: 1167 bytes --]

On 22:13, Boaz Harrosh wrote:

> All the scsi calls do not need any locks. The scsi LLDS never
> see these threads since commands are queued through the block
> layer.

That's what everybody believes, but nobody seems to know for sure.
Therefore I did what Andi suggested: Make a zero-semantics change
that moves the lock_kernel() to sg_ioctl() to make people aware of
the fact that this function runs under the BKL. At least the latter
has already succeeded.

> What's left is what you see, here in sg.c. you must have the best 
> knowledge about the possible races between ioctl and open/release
> and probe/remove. And all these put_user() copy_user() etc...
> Why don't you have a hard look and fix them properly.

Because that requires much more knowledge. Al is looking into this
which indicates that it is non-trivial issue. I am clearly not the
right person to decide this question.

> please don't *lock_kernel();* for scsi's sake.

The BKL was there all the time. My patch just made it more visable
to the scsi people by moving it into sg.c.

Andre
-- 
The only person who always got his work done by Friday was Robinson Crusoe

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]