gmane.linux.scsi

Re: [PATCH 4/4] scsi_transport_fc: FC timeout handler

Christoph Hellwig — 2013-05-25T05:08:42

This looks like a good start, but why would we make this FC specific?

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo< at >vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: PING^7 (was Re: [PATCH v2 00/14] Corrections and customization of the SG_IO command whitelist (CVE-2012-4542))

Vladislav Bolkhovitin — 2013-05-25T03:54:34

Well, any abstraction is good only if it isn't artificial, so solving more problems
than creating.

Reality is that de facto in the industry _SCSI_ is the abstraction for block/direct
access to data. Look around. How many of systems around you after all layers end up to
SCSI commands in their storage devices?

Linux block layer is purely artificial creature slowly reinventing wheel creating more
problems, than solving. It enforces approach, where often "impossible" means
"impossible in this interface". For instance, how about copy offload? How about
reservations? How about atomic writes? Look at history of barriers and compare then
with what can be done in SCSI. It's still worse, because doesn't allow usage of all
devices capabilities. Why was it needed to create special blk integrity interface with
the only end user - SCSI? Artificial task created - then well solved. Etc, etc.

The block layer keeps repeating SCSI. So, maybe, after all, it's better to acknowledge
that direct usage of SCSI without any inter

Re: Question: eh_abort_handler() and terminate commands

Jeremy Linton — 2013-05-24T22:26:11

Well, eventually it ends up on the done_q and gets returned up the stack via
flush_done_q(). But that wasn't what you were asking?

Uh, well you don't get a "proper" SCSI status on a TMF or a ABTS/ABTX. So
basically, the abort just kills processing of the commands.



Well, I think there is some rule here (scsi_eh.txt, "everyone forgets about
the command") that by the time the eh_abort_handler() completes you won't get
any new scsi_done()s. This doesn't appear to mean that you won't get them
while the abort_handler is running. Hence if you look at send_eh_cmnd() you
see that the done completion being triggered at any time after the
wait_for_completion_timeout() doesn't really do anything useful. The normal
abort path completion doesn't appear to care either. Abort success/failure
doesn't appear to fundamentally change the eventual return status of the
commands.



Yes, if for some reason a status comes in for an aborted exchange the HBA
firmware rejects it because its against an invalid exchange (or sho

Re: [PATCH 1/4] scsi: move initialization of scmd->eh_entry

Jörn Engel — 2013-05-24T16:57:40

I always suspect something subtle going on when a variable is
initialized to 0 three lines below a memset.  Can line this go away?

Jörn

--
Those who come seeking peace without a treaty are plotting.

[PATCH v5 2/2] block,scsi: convert and handle ERR_PTR from blk_get_request

Joe Lawrence — 2013-05-24T18:17:28

From 5b26d593807b30f60ed41f6fd5a16a56c3c9a43c Mon Sep 17 00:00:00 2001
From: Joe Lawrence <joe.lawrence< at >stratus.com>
Date: Fri, 24 May 2013 13:05:09 -0400
Subject: [PATCH v5 2/2] block,scsi: convert and handle ERR_PTR from blk_get_request

The blk_get_request function may fail in low-memory conditions or during
device removal (even if __GFP_WAIT is set). To distinguish between these
errors, modify the blk_get_request call stack to return the appropriate
ERR_PTR. Verify that all callers check the return status and consider
IS_ERR instead of a simple NULL pointer check.

Signed-off-by: Joe Lawrence <joe.lawrence< at >stratus.com>
Cc: Jens Axboe <axboe< at >kernel.dk>
Cc: Jiri Kosina <jkosina< at >suse.cz>
Cc: "James E.J. Bottomley" <JBottomley< at >parallels.com>
Cc: Bart Van Assche <bvanassche< at >acm.org>
Cc: linux-scsi< at >vger.kernel.org
---
 block/blk-core.c                            | 34 ++++++++++++++---------------
 block/bsg.c                                 |  8 +++----
 block/scsi_ioctl.c                          | 12 +++++--

[PATCH v5 1/2] block,scsi: verify return pointer from blk_get_request

Joe Lawrence — 2013-05-24T18:16:37

From 22307be1bc6e404622b1f074094902e385a1bd30 Mon Sep 17 00:00:00 2001
From: Joe Lawrence <joe.lawrence< at >stratus.com>
Date: Fri, 24 May 2013 12:39:04 -0400
Subject: [PATCH v5 1/2] block,scsi: verify return pointer from blk_get_request

The blk-core dead queue checks introduced in commit 70460571 added an
error scenario to blk_get_request that returns NULL if the request queue
has been shutdown. This changed the behavior for __GFP_WAIT callers, who
should now verify the return value before dereferencing.

Signed-off-by: Joe Lawrence <joe.lawrence< at >stratus.com>
Cc: Jens Axboe <axboe< at >kernel.dk>
Cc: Jiri Kosina <jkosina< at >suse.cz>
Cc: "James E.J. Bottomley" <JBottomley< at >parallels.com>
Cc: Bart Van Assche <bvanassche< at >acm.org>
Cc: linux-scsi< at >vger.kernel.org
---
 block/scsi_ioctl.c        | 9 ++++++++-
 drivers/block/paride/pd.c | 2 ++
 drivers/block/pktcdvd.c   | 2 ++
 drivers/scsi/scsi_error.c | 2 ++
 4 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index 9a87daa..

[PATCH v5 0/2] block,scsi: fixup blk_get_request dead queue scenarios

Joe Lawrence — 2013-05-24T18:15:31

[PATCH v5 0/2] block,scsi: fixup blk_get_request dead queue scenarios

Changes from v4:

 - As per James' suggestion, split into two patches: the first adds
   blk_get_request return value checking to avoid potential oops, the
   second converts callers and friends to handle ERR_PTR differentiation
   of -ENOMEM and -ENODEV.

Regards,

[PATCH 1/1] ipr: possible irq lock inversion dependency detected

2013-05-24T14:59:13

When enable lockdep, seeing "possible irq lock inversion dependency detected"
error. This patch fixes the issue.

Signed-off-by: Wen Xiong <wenxiong< at >linux.vnet.ibm.com>
---
 drivers/scsi/ipr.c |   17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

Index: b/drivers/scsi/ipr.c
===================================================================
--- a/drivers/scsi/ipr.c2013-05-20 09:55:23.000000000 -0500
+++ b/drivers/scsi/ipr.c2013-05-20 10:00:34.548380858 -0500
< at >< at > -9408,7 +9408,7 < at >< at > static int ipr_probe_ioa(struct pci_dev 
 void __iomem *ipr_regs;
 int rc = PCIBIOS_SUCCESSFUL;
 volatile u32 mask, uproc, interrupts;
-unsigned long lock_flags;
+unsigned long lock_flags, driver_lock_flags;
 
 ENTER;
 
< at >< at > -9631,9 +9631,9 < at >< at > static int ipr_probe_ioa(struct pci_dev 
 } else
 ioa_cfg->reset = ipr_reset_start_bist;
 
-spin_lock(&ipr_driver_lock);
+spin_lock_irqsave(&ipr_driver_lock, driver_lock_flags);
 list_add_tail(&ioa_cfg->queue, &ipr_ioa_head);
-spin_unlock(&ipr_driver_lock);
+

[PATCH 0/1] ipr: possible irq lock inversion dependency detected

2013-05-24T14:59:12

Hi All,

When enable lockdep, seeing "possible irq lock inversion dependency detected"
error. This patch fixed the issue.

Thanks,
Wendy

Question: eh_abort_handler() and terminate commands

Hannes Reinecke — 2013-05-24T10:57:59

Hi all,

after having posted the first attempt for an updated FC error
handler I found that the 'eh_abort_handler()' semantics are a bit odd.

Obviously, the 'eh_abort_handler' is called to abort a command.
But what _exactly_ is supposed to happen if it returns 'SUCCESS'?
Initially one does expect that the command has been aborted.
But then the callback itself does _not_ terminate the command,
it's rather expected that the caller of 'eh_abort_command' does it.

Which leads to the interesting question:
What happens with the actual command once eh_abort_handler returns?

As normally 'eh_abort_handler' is implemented as a TMF, one does
assume that the command itself will be returned by the target with
an appropriate status.
However, as the upper layer is expected to terminate the command
itself, we will never see this status, right?
OTOH it also means that the HBA firmware might receive a completion
for a command which the upper layer has already completed.
Will this completion ever being mirrored to the LLDD?

[PATCH 3/4] scsi: export functions for new fc timeout handler

Hannes Reinecke — 2013-05-24T09:50:49

Export some more functions which are used by the new FC timeout
handler.

Signed-off-by: Hannes Reinecke <hare< at >suse.de>
---
 drivers/scsi/scsi_error.c | 5 ++++-
 drivers/scsi/scsi_lib.c   | 2 ++
 drivers/scsi/scsi_priv.h  | 2 ++
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index bef6799e..f21ad7c 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
< at >< at > -119,6 +119,7 < at >< at > int scsi_eh_scmd_add(struct scsi_cmnd *scmd, int eh_flag)
 spin_unlock_irqrestore(shost->host_lock, flags);
 return ret;
 }
+EXPORT_SYMBOL_GPL(scsi_eh_scmd_add);
 
 /**
  * scsi_times_out - Timeout function for normal scsi commands.
< at >< at > -659,13 +660,14 < at >< at > static int scsi_try_bus_device_reset(struct scsi_cmnd *scmd)
 return rtn;
 }
 
-static int scsi_try_to_abort_cmd(struct scsi_host_template *hostt, struct scsi_cmnd *scmd)
+int scsi_try_to_abort_cmd(struct scsi_host_template *hostt, struct scsi_cmnd *scmd)
 {
 if (!hostt->eh_abort_handler)
 return FAI

[PATCH 4/4] scsi_transport_fc: FC timeout handler

Hannes Reinecke — 2013-05-24T09:50:50

When a command runs into a timeout we need to send an 'ABORT TASK'
TMF. This is typically done by the 'eh_abort_handler' LLDD callback.

Conceptually, however, this function is a normal SCSI command, so
there is no need to enter the error handler.

This patch implements a new FC timeout handler which invokes
the 'eh_abort_handler' directly when the timeout occurs without
entering the error handler.

If the 'eh_abort_handler' returns SUCCESS or FAST_IO_FAIL the
command will be retried if possible. If no retries are allowed
the command will be returned immediately, as we have to assume
the TMF succeeded and the command is completed with the LLDD.
For any other return code from 'eh_abort_handler' the command
will be pushed onto the existing SCSI EH handler, or aborted
with an error if that fails.

Signed-off-by: Hannes Reinecke <hare< at >suse.de>
---
 drivers/scsi/scsi_transport_fc.c | 63 +++++++++++++++++++++++++++++++++++++++-
 include/scsi/scsi_transport_fc.h |  2 ++
 2 files changed, 64 insertions(+), 1 deletio

[PATCH 1/4] scsi: move initialization of scmd->eh_entry

Hannes Reinecke — 2013-05-24T09:50:47

The 'eh_entry' list might be used even before scsi_softirq_done()
is called. Hence we should rather initialize it together with
the other eh-related variables.

Signed-off-by: Hannes Reinecke <hare< at >suse.de>
---
 drivers/scsi/scsi_lib.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index c31187d..a5515ec 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
< at >< at > -318,6 +318,8 < at >< at > static void scsi_init_cmd_errh(struct scsi_cmnd *cmd)
 memset(cmd->sense_buffer, 0, SCSI_SENSE_BUFFERSIZE);
 if (cmd->cmd_len == 0)
 cmd->cmd_len = scsi_command_size(cmd->cmnd);
+INIT_LIST_HEAD(&cmd->eh_entry);
+cmd->eh_eflags = 0;
 }
 
 void scsi_device_unbusy(struct scsi_device *sdev)
< at >< at > -1483,8 +1485,6 < at >< at > static void scsi_softirq_done(struct request *rq)
 unsigned long wait_for = (cmd->allowed + 1) * rq->timeout;
 int disposition;
 
-INIT_LIST_HEAD(&cmd->eh_entry);
-
 atomic_inc(&cmd->device->iodone_cnt);
 if (cmd->result)
 atomic_in

[PATCH 2/4] blk-timeout: add BLK_EH_SCHEDULED return code

Hannes Reinecke — 2013-05-24T09:50:48

Add a 'BLK_EH_SCHEDULED' return code for blk-timeout to indicate
that a delayed error recovery has been initiated.

Signed-off-by: Hannes Reinecke <hare< at >suse.de>
---
 drivers/scsi/scsi_error.c | 3 +++
 include/linux/blkdev.h    | 1 +
 2 files changed, 4 insertions(+)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index c1b05a8..bef6799e 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
< at >< at > -145,6 +145,9 < at >< at > enum blk_eh_timer_return scsi_times_out(struct request *req)
 rtn = host->hostt->eh_timed_out(scmd);
 
 scmd->result |= DID_TIME_OUT << 16;
+/* Check for delayed EH scheduling */
+if (rtn == BLK_EH_SCHEDULED)
+return BLK_EH_NOT_HANDLED;
 
 if (unlikely(rtn == BLK_EH_NOT_HANDLED &&
      !scsi_eh_scmd_add(scmd, SCSI_EH_CANCEL_CMD)))
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 78feda9..0a38211 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
< at >< at > -238,6 +238,7 < at >< at > enum blk_eh_timer_return {
 BLK_EH_NOT_HANDLED,
 BLK_EH_H

[PATCH 0/4] New FC timeout handler

Hannes Reinecke — 2013-05-24T09:50:46

Hi all,

this is the first step towards a new FC error handler.
This patch implements a new FC command timeout handler
which will be sending command aborts inline without
engaging SCSI EH.

In addition the commands will be returned directly
if the command abort succeeded, cutting down recovery
times dramatically.

With the original scsi error recovery I got:
# time dd if=/dev/zero of=/mnt/test.blk bs=512 count=2048 oflag=sync
2048+0 records in
2048+0 records out
1048576 bytes (1.0 MB) copied, 3.72732 s, 281 kB/s

real2m14.475s
user0m0.000s
sys0m0.104s

with this patchset I got:
# time dd if=/dev/zero of=/mnt/test.blk bs=512 count=2048 oflag=sync
2048+0 records in
2048+0 records out
1048576 bytes (1.0 MB) copied, 79.6376 s, 13.2 kB/s

real1m19.642s
user0m0.000s
sys0m0.100s

Test was to disable RSCN on the target port, disable the
target port, and then start the 'dd' command as indicated.

Comments etc are welcome.

Hannes Reinecke (4):
  scsi: move initialization of scmd->eh_entry
  blk-timeout: add BLK

Re: PING^7 (was Re: [PATCH v2 00/14] Corrections and customization of the SG_IO command whitelist (CVE-2012-4542))

Tejun Heo — 2013-05-24T09:07:11

It's not just unimplemented commands. Exposing any new command exposes
its borderline problems together with it.


Yes and that's why the whitelist is generally frowned upon. It's
inherently fragile. These devices simply aren't designed and
implemented to be exposed to lesser security domains directly. It's
true that it's already kinda broken that way but as I wrote before
it's a vicious cycle and we don't wanna keep building on top of it.
This expansion is gonna increase the usage of whitelisting which will
in turn attract further use cases which are likely to call for even
more expansion.


If the bulk of filtering can be solved with userland whitelisting as a
confined user, it should be possible to resolve peripheral problems
like log messages in simpler way, no? Can you please elaborate on the
log message problem? Who's spewing those messages?

--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo< at >vger.kernel.org
More majordomo info at  h

Re: PING^7 (was Re: [PATCH v2 00/14] Corrections and customization of the SG_IO command whitelist (CVE-2012-4542))

Tejun Heo — 2013-05-24T08:02:50

Well, I'd actually much prefer disabling CDB whitelisting for all !MMC
devices if at all possible. You're basically arguing that because what
we have is already broken, it should be okay to break it further.
Also, RW commands having more quirks doesn't necessarily indicate that
they tend to be more broken. They just get hammered on a lot in
various ways so problems on those commands tend to be more noticeable.


The thing is that both approaches aren't perfect here so you can make
similar type of argument from the other side. If the system wants to
give out raw hardware access to VMs, requiring it to delegate the
device fully could be reasonable. Not ideal but widening direct
command access by default is pretty nasty too.

--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo< at >vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 part1 1/4] sg_io: pass request_queue to blk_verify_command

James Bottomley — 2013-05-24T07:50:57

That's a circular response that doesn't answer the question.  The actual
question is: what is simple fix for the bug that isn't entangled with
enabling the SG_IO per device type whitelist feature.


Fine, so produce a simple fix for this bug which we can discuss that's
not tied to this feature.

James


--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo< at >vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] block,scsi: verify return pointer from blk_get_request

Joe Lawrence — 2013-05-23T21:37:58

On Fri, 24 May 2013 00:40:11 +0400
James Bottomley <James.Bottomley< at >hansenpartnership.com> wrote:

That would certainly be safer (apologies for not getting to this before
the merge window).

I can split these modifications from the return type changes tomorrow.
This would include fixes to address new and previous patch comments:

  - v1: If callers are passing in __GFP_WAIT, then blk_get_request
    should only fail if the device queue is dead, so it would be
    more appropriate to return -ENODEV.  (Jens)
  - v1: scsi_eh_lock_door is defined as void, don't return errno
    (Bart)
  - v1: drivers/block/paride/pd.c :: pd_special_command should check
    blk_get_request return value
  - v4: the error path introduced in sg_scsi_ioctl should free the
    buffer

Thanks,

Re: [PATCH] block,scsi: verify return pointer from blk_get_request

James Bottomley — 2013-05-23T20:40:11

Now that we see the size of the patch diff between fixing the bug and
doing proper error returns, I'm really not convinced this should be done
as a single bug fix patch.  The modify all error returns is big and if
we missed one, it will cause problems that will manifest as an oops, so
it makes a fairly compact fix a big error prone patch.

What about two patches: one to fix the actual bug (this patch), which
could go now and one to change the return type, which would go in the
normal merge window.

James


--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo< at >vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4] block: handle pointer error from blk_get_request

Boaz Harrosh — 2013-05-23T20:24:47

ACK-by: Boaz Harrosh <bharrosh< at >panasas.com>

<>
<>


--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo< at >vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html