Re: [PATCH RFC 1/9] [SCSI] Detect overflow of sense data buffer
On 01/22/2013 04:08 PM, Ewan Milne wrote:
On Fri, 2013-01-18 at 16:46 +, James Bottomley wrote:
On Fri, 2013-01-18 at 11:27 -0500, Ewan D. Milne wrote:
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -241,6 +241,9 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
if (! scsi_command_normalize_sense(scmd, sshdr))
return FAILED; /* no valid sense data */
+ if (sshdr.overflow)
+ scmd_printk(KERN_WARNING, scmd, Sense data overflow);
+
if (scsi_sense_is_deferred(sshdr))
return NEEDS_RETRY;
@@ -2059,14 +2062,18 @@ int scsi_normalize_sense(const u8 *sense_buffer, int
sb_len,
sshdr-asc = sense_buffer[2];
if (sb_len 3)
sshdr-ascq = sense_buffer[3];
+ if (sb_len 4)
+ sshdr-overflow = ((sense_buffer[4] 0x80) != 0);
if (sb_len 7)
sshdr-additional_length = sense_buffer[7];
} else {
/*
* fixed format
*/
- if (sb_len 2)
+ if (sb_len 2) {
+ sshdr-overflow = ((sense_buffer[2] 0x10) != 0);
sshdr-sense_key = (sense_buffer[2] 0xf);
+ }
if (sb_len 7) {
sb_len = (sb_len (sense_buffer[7] + 8)) ?
sb_len : (sense_buffer[7] + 8);
This isn't the right way to do it: The overflow bit is a recent
introduction in SPC-4. The correct way to tell if we have an overflow
or not is to look at the additional sense length and compare it to the
allocation length; this will work for everything.
Unfortunately, I am not sure that the allocation length that was sent
to the device is always available. I will look into this more closely
but it appeared to me that e.g. FC drivers like qla2xxx get the sense
data automatically from the HBA firmware. In the case of that driver
the host sense buffer size looks like it is hard-coded to 32 bytes,
for all I know the firmware might only asking for 18 bytes.
Hmm.
Maybe we should be adding a 'max_sense_len' field to the SCSI host;
that way we could check against this.
Nevertheless, that information would be lost to us in either case.
I wonder how these vendors propose to handle long sense code data;
silently ignoring it doesn't seem to be the correct way.
Of course, for a normal REQUEST SENSE command where the allocation
length is in the CDB, it would indeed be easy to add a check against
the additional sense length.
I'm not even convinced that overflow is important: for a lot of the
sense probes, we deliberately induce overflows by giving the request
sense command a short buffer. Printing a warning in scsi_check_sense
will get very noisy very fast.
That would indeed be a problem. I didn't see this behavior when testing
the changes but I'll need to investigate this further.
The purpose of detecting the sense data overflow was to provide some
visibility that a device is returning a large amount of sense data that
is currently being silently ignored. In the case of descriptor format
sense data, it is possible that a descriptor we want to examine is
located after one or more other descriptors, and we might not get it
at all if the buffer isn't large enough.
Agreed. but the newest standard won't be adopted to existing
devices, so we should figure out a way of dealing with those, too.
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries Storage
h...@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 1/9] [SCSI] Detect overflow of sense data buffer
On Tue, 2013-01-22 at 10:08 -0500, Ewan Milne wrote:
On Fri, 2013-01-18 at 16:46 +, James Bottomley wrote:
On Fri, 2013-01-18 at 11:27 -0500, Ewan D. Milne wrote:
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -241,6 +241,9 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
if (! scsi_command_normalize_sense(scmd, sshdr))
return FAILED; /* no valid sense data */
+ if (sshdr.overflow)
+ scmd_printk(KERN_WARNING, scmd, Sense data overflow);
+
if (scsi_sense_is_deferred(sshdr))
return NEEDS_RETRY;
@@ -2059,14 +2062,18 @@ int scsi_normalize_sense(const u8 *sense_buffer,
int sb_len,
sshdr-asc = sense_buffer[2];
if (sb_len 3)
sshdr-ascq = sense_buffer[3];
+ if (sb_len 4)
+ sshdr-overflow = ((sense_buffer[4] 0x80) != 0);
if (sb_len 7)
sshdr-additional_length = sense_buffer[7];
} else {
/*
* fixed format
*/
- if (sb_len 2)
+ if (sb_len 2) {
+ sshdr-overflow = ((sense_buffer[2] 0x10) != 0);
sshdr-sense_key = (sense_buffer[2] 0xf);
+ }
if (sb_len 7) {
sb_len = (sb_len (sense_buffer[7] + 8)) ?
sb_len : (sense_buffer[7] + 8);
This isn't the right way to do it: The overflow bit is a recent
introduction in SPC-4. The correct way to tell if we have an overflow
or not is to look at the additional sense length and compare it to the
allocation length; this will work for everything.
Unfortunately, I am not sure that the allocation length that was sent
to the device is always available.
Well, yes it is, we just don't store it. scsi_normalize_sense() takes
as input the length of the sense buffer we gave it. If we want an
overflow indication, we can certainly compare that against the
additional length (assuming we have enough bytes to get the additional
length).
I will look into this more closely
but it appeared to me that e.g. FC drivers like qla2xxx get the sense
data automatically from the HBA firmware. In the case of that driver
the host sense buffer size looks like it is hard-coded to 32 bytes,
for all I know the firmware might only asking for 18 bytes.
You mean for their ACA emulation in the transport? They have to be
picking up at least the standard minimum in order not to be in
violation, surely?
Of course, for a normal REQUEST SENSE command where the allocation
length is in the CDB, it would indeed be easy to add a check against
the additional sense length.
I'm not even convinced that overflow is important: for a lot of the
sense probes, we deliberately induce overflows by giving the request
sense command a short buffer. Printing a warning in scsi_check_sense
will get very noisy very fast.
That would indeed be a problem. I didn't see this behavior when testing
the changes but I'll need to investigate this further.
The purpose of detecting the sense data overflow was to provide some
visibility that a device is returning a large amount of sense data that
is currently being silently ignored. In the case of descriptor format
sense data, it is possible that a descriptor we want to examine is
located after one or more other descriptors, and we might not get it
at all if the buffer isn't large enough.
But why should we care? A lot of the time it will be spewing
descriptors with irrelevant FRU information. I really think printing
there's been an overflow isn't a good idea. I'm not sure there's much
use in the sshdr indicating there's been one. It *may* be useful to
indicate how big the allocation length should have been, but I'm not
even convinced of that, since the data is now lost.
James
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
linux-3.7.4: BUG: unable to handle kernel NULL pointer dereference at target_fabric_port_link
Hi, after upgrade from 3.7.3 to 3.7.4, I got NULL pointer dereference at target_fabric_port_link(). Jan 22 23:58:52 kernel: [ 89.333115] BUG: unable to handle kernel NULL pointer dereference at (null) Jan 22 23:58:52 kernel: [ 89.333251] IP: [a049d988] target_fabric_port_link+0x18/0x100 [target_core_mod] Jan 22 23:58:52 kernel: [ 89.82] PGD 40b94c067 PUD 40b8de067 PMD 0 Jan 22 23:58:52 kernel: [ 89.333564] Oops: [#1] PREEMPT SMP Jan 22 23:58:52 kernel: [ 89.333756] Modules linked in: iscsi_target_mod target_core_pscsi target_core_file target_core_iblock binfmt_misc target _core_mod iscsi_tcp libiscsi_tcp libiscsi scsi_transport_iscsi twofish_generic twofish_avx_x86_64 twofish_x86_64_3way twofish_x86_64 twofish_common camellia _generic camellia_x86_64 serpent_avx_x86_64 serpent_sse2_x86_64 serpent_generic glue_helper blowfish_generic blowfish_x86_64 blowfish_common dlm cast5_avx_x 86_64 cast5_generic sctp sha512_generic sha256_generic crypto_null bridge stp llc nf_conntrack_ipv6 nf_defrag_ipv6 xt_LOG nf_conntrack_ipv4 nf_defrag_ipv4 k vm_intel kvm crc32c_intel aesni_intel ablk_helper cryptd xts lrw gf128mul iTCO_wdt snd_hda_codec_hdmi microcode psmouse serio_raw pcspkr snd_hda_codec_realt ek lpc_ich mfd_core snd_ice1724 snd_ak4113 snd_hda_intel snd_pt2258 snd_hda_codec snd_i2c snd_ak4114 snd_hwdep snd_ice17xx_ak4xxx snd_ak4xxx_adda snd_pcm_os s snd_mixer_oss snd_ac97_codec ac97_bus snd_pcm snd_seq_dummy snd_page_alloc snd_seq_oss snd_ Jan 22 23:58:52 kernel: seq_midi snd_rawmidi snd_seq_midi_event snd_seq rtc_cmos snd_seq_device snd_timer snd mei soundcore button vhost_net tun w8 3627ehf hwmon_vid coretemp hwmon acpi_cpufreq mperf processor firewire_sbp2 firewire_core crc_itu_t evdev fuse ocfs2_dlmfs ocfs2_stack_o2cb ocfs2_dlm ocfs2_ nodemanager ocfs2_stackglue configfs autofs4 usb_storage sg ata_generic pata_acpi sr_mod cdrom pata_marvell xhci_hcd e1000e ehci_hcd Jan 22 23:58:52 kernel: [ 89.339089] CPU 7 Jan 22 23:58:52 kernel: [ 89.339132] Pid: 4050, comm: ln Tainted: GW 3.7.4-dirty #1 /DP67BG Jan 22 23:58:52 kernel: [ 89.339277] RIP: 0010:[a049d988] [a049d988] target_fabric_port_link+0x18/0x100 [target_core_mod] Jan 22 23:58:52 kernel: [ 89.339413] RSP: 0018:88040d45de78 EFLAGS: 00010286 Jan 22 23:58:52 kernel: [ 89.339476] RAX: a04bc840 RBX: 880404d499dc RCX: Jan 22 23:58:52 kernel: [ 89.339538] RDX: 0007 RSI: 88040c70a7b0 RDI: 88040e4d2070 Jan 22 23:58:52 kernel: [ 89.339609] RBP: 88040d45de98 R08: 0002 R09: fefefefefefefeff Jan 22 23:58:52 kernel: [ 89.339677] R10: 2f2f2f2f2f2f2f2f R11: R12: 88040b189440 Jan 22 23:58:52 kernel: [ 89.339742] R13: 88040e4d2070 R14: 88040c70a7b0 R15: 88040c5dfb58 Jan 22 23:58:52 kernel: [ 89.339807] FS: 7f3edb7ef700() GS:88041f5c() knlGS: Jan 22 23:58:52 kernel: [ 89.339893] CS: 0010 DS: ES: CR0: 80050033 Jan 22 23:58:52 kernel: [ 89.339960] CR2: CR3: 00040cc92000 CR4: 000407e0 Jan 22 23:58:52 kernel: [ 89.340030] DR0: DR1: DR2: Jan 22 23:58:52 kernel: [ 89.340098] DR3: DR6: 0ff0 DR7: 0400 Jan 22 23:58:52 kernel: [ 89.340161] Process ln (pid: 4050, threadinfo 88040d45c000, task 88040d91c840) Jan 22 23:58:52 kernel: [ 89.340243] Stack: Jan 22 23:58:52 kernel: [ 89.340302] 880404d499dc 88040b189440 88040e4d2070 88040c70a7b0 Jan 22 23:58:52 kernel: [ 89.340559] 88040d45def8 a005f02d 88040d45deb8 88040ba1e800 Jan 22 23:58:52 kernel: [ 89.340802] 88040ebcabe0 880404d49980 88040d45dee8 Jan 22 23:58:52 kernel: [ 89.341057] Call Trace: Jan 22 23:58:52 kernel: [ 89.341125] [a005f02d] configfs_symlink+0x12d/0x340 [configfs] Jan 22 23:58:52 kernel: [ 89.341189] [8113aa6d] vfs_symlink+0x8d/0xf0 Jan 22 23:58:52 kernel: [ 89.341250] [8113e5b9] sys_symlinkat+0x59/0x90 Jan 22 23:58:52 kernel: [ 89.341322] [8113e601] sys_symlink+0x11/0x20 Jan 22 23:58:52 kernel: [ 89.341391] [816c1892] system_call_fastpath+0x16/0x1b Jan 22 23:58:52 kernel: [ 89.341456] Code: 8b 65 f8 c9 c3 66 66 66 66 66 2e 0f 1f 84 00 00 00 00 00 55 48 89 e5 48 83 ec 20 48 89 5d e0 4c 89 65 e8 4c 89 6d f0 4c 89 75 f8 81 3c 25 00 00 00 00 ef de ed fe 75 76 48 8b 47 30 48 83 7f f0 Jan 22 23:58:52 kernel: [ 89.344386] RIP [a049d988] target_fabric_port_link+0x18/0x100 [target_core_mod] Jan 22 23:58:52 kernel: [ 89.344526] RSP 88040d45de78 Jan 22 23:58:52 kernel: [ 89.344592] CR2: Jan 22 23:58:52 kernel: [ 89.344673] ---[ end trace 60b2bd0028e165d0 ]--- At target_fabric_port_link(),
[RESEND PATCH] libiscsi: avoid unnecessary multiple NULL assignments
In iscsi_free_task, NULL is assigned to task-sc twice: before and
after kfifo_in invocatoin. Allocating and freeing iscsi_task are guarded
with session-lock, so multiple NULL assignments cause no trouble. But
people reading the source code may be confused.
The second NULL assignment comes from commit:
3e5c28ad0391389959ccae81c938c7533efb3490
It seems that the line after kfifo_in invocation was introduced
accidentally.
Signed-off-by: Masatake YAMATO yam...@redhat.com
Reviewed-by: Mike Christie micha...@cs.wisc.edu
---
drivers/scsi/libiscsi.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 82c3fd4..7aacf3a 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -507,7 +507,6 @@ static void iscsi_free_task(struct iscsi_task *task)
kfifo_in(session-cmdpool.queue, (void*)task, sizeof(void*));
if (sc) {
- task-sc = NULL;
/* SCSI eh reuses commands to verify us */
sc-SCp.ptr = NULL;
/*
--
1.7.11.7
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 4/9] [SCSI] Rename scsi_evt_xxx to sdev_evt_xxx and scsi_event to sdev_event
On Tue, 2013-01-22 at 10:38 -0700, Bart Van Assche wrote:
On Fri, Jan 18, 2013 at 9:27 AM, Ewan D. Milne emi...@redhat.com wrote:
@@ -2206,7 +2206,7 @@ static void scsi_evt_emit(struct scsi_device *sdev,
struct scsi_event *evt)
* Dispatch queued events to their associated scsi_device kobjects
* as uevents.
*/
-void scsi_evt_thread(struct work_struct *work)
+void sdev_evt_thread(struct work_struct *work)
{
struct scsi_device *sdev;
LIST_HEAD(event_list);
@@ -2214,7 +2214,7 @@ void scsi_evt_thread(struct work_struct *work)
sdev = container_of(work, struct scsi_device, event_work);
while (1) {
- struct scsi_event *evt;
+ struct sdev_event *evt;
struct list_head *this, *tmp;
unsigned long flags;
@@ -2226,9 +2226,9 @@ void scsi_evt_thread(struct work_struct *work)
break;
list_for_each_safe(this, tmp, event_list) {
- evt = list_entry(this, struct scsi_event, node);
+ evt = list_entry(this, struct sdev_event, node);
list_del(evt-node);
- scsi_evt_emit(sdev, evt);
+ sdev_evt_emit(sdev, evt);
kfree(evt);
}
}
If schedule_work() gets invoked if work is already on a workqueue then
it will return immediately. Does that mean that the above approach for
processing the event list is racy and that new events will not get
processed if schedule_work() is invoked after the while loop finished
but before the above function returns ?
I thought that the way that the workqueue mechanism did this was that
the work struct was removed from the list and WORK_STRUCT_PENDING_BIT
was cleared in the work_struct before the work function was invoked.
In that case I the code should be OK since the work function will have
to take the lock which is held around the code to add the event to the
list and the call to schedule_work().
But I see your point, and I since I just added more SDEV_EVT_xxx codes
(and an STARGET_EVT_xxx code, which uses the same mechanism), I didn't
inspect this code carefully. I'll give it some more thought.
It seems to me that the workqueue mechanism would be hard to use if
there weren't some guarantee against the race condition you mention.
Bart.
Thank you for your comments.
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 4/9] [SCSI] Rename scsi_evt_xxx to sdev_evt_xxx and scsi_event to sdev_event
On Tue, 2013-01-22 at 10:33 -0700, Bart Van Assche wrote:
On Fri, Jan 18, 2013 at 9:27 AM, Ewan D. Milne emi...@redhat.com wrote:
@@ -2206,7 +2206,7 @@ static void scsi_evt_emit(struct scsi_device *sdev,
struct scsi_event *evt)
* Dispatch queued events to their associated scsi_device kobjects
* as uevents.
*/
-void scsi_evt_thread(struct work_struct *work)
+void sdev_evt_thread(struct work_struct *work)
{
struct scsi_device *sdev;
LIST_HEAD(event_list);
This code does not run as a long-living thread so please consider
using the name sdev_evt_work instead.
Good idea. I'll do that.
Bart.
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 1/9] [SCSI] Detect overflow of sense data buffer
On Wed, 2013-01-23 at 13:06 +, James Bottomley wrote:
On Tue, 2013-01-22 at 10:08 -0500, Ewan Milne wrote:
On Fri, 2013-01-18 at 16:46 +, James Bottomley wrote:
On Fri, 2013-01-18 at 11:27 -0500, Ewan D. Milne wrote:
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -241,6 +241,9 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
if (! scsi_command_normalize_sense(scmd, sshdr))
return FAILED; /* no valid sense data */
+ if (sshdr.overflow)
+ scmd_printk(KERN_WARNING, scmd, Sense data overflow);
+
if (scsi_sense_is_deferred(sshdr))
return NEEDS_RETRY;
@@ -2059,14 +2062,18 @@ int scsi_normalize_sense(const u8
*sense_buffer, int sb_len,
sshdr-asc = sense_buffer[2];
if (sb_len 3)
sshdr-ascq = sense_buffer[3];
+ if (sb_len 4)
+ sshdr-overflow = ((sense_buffer[4] 0x80) !=
0);
if (sb_len 7)
sshdr-additional_length = sense_buffer[7];
} else {
/*
* fixed format
*/
- if (sb_len 2)
+ if (sb_len 2) {
+ sshdr-overflow = ((sense_buffer[2] 0x10) !=
0);
sshdr-sense_key = (sense_buffer[2] 0xf);
+ }
if (sb_len 7) {
sb_len = (sb_len (sense_buffer[7] + 8)) ?
sb_len : (sense_buffer[7] + 8);
This isn't the right way to do it: The overflow bit is a recent
introduction in SPC-4. The correct way to tell if we have an overflow
or not is to look at the additional sense length and compare it to the
allocation length; this will work for everything.
Unfortunately, I am not sure that the allocation length that was sent
to the device is always available.
Well, yes it is, we just don't store it. scsi_normalize_sense() takes
as input the length of the sense buffer we gave it. If we want an
overflow indication, we can certainly compare that against the
additional length (assuming we have enough bytes to get the additional
length).
I will look into this more closely
but it appeared to me that e.g. FC drivers like qla2xxx get the sense
data automatically from the HBA firmware. In the case of that driver
the host sense buffer size looks like it is hard-coded to 32 bytes,
for all I know the firmware might only asking for 18 bytes.
You mean for their ACA emulation in the transport? They have to be
picking up at least the standard minimum in order not to be in
violation, surely?
I'm sure that's the case, I just don't know if the minimum is big
enough. See below.
Of course, for a normal REQUEST SENSE command where the allocation
length is in the CDB, it would indeed be easy to add a check against
the additional sense length.
I'm not even convinced that overflow is important: for a lot of the
sense probes, we deliberately induce overflows by giving the request
sense command a short buffer. Printing a warning in scsi_check_sense
will get very noisy very fast.
That would indeed be a problem. I didn't see this behavior when testing
the changes but I'll need to investigate this further.
The purpose of detecting the sense data overflow was to provide some
visibility that a device is returning a large amount of sense data that
is currently being silently ignored. In the case of descriptor format
sense data, it is possible that a descriptor we want to examine is
located after one or more other descriptors, and we might not get it
at all if the buffer isn't large enough.
But why should we care? A lot of the time it will be spewing
descriptors with irrelevant FRU information. I really think printing
there's been an overflow isn't a good idea. I'm not sure there's much
use in the sshdr indicating there's been one. It *may* be useful to
indicate how big the allocation length should have been, but I'm not
even convinced of that, since the data is now lost.
My thinking was that it was important to log the fact that the device
had reported a Unit Attention queue overflow, which is reported in a
sense key specific descriptor, so I was concerned that if the sense
buffer wasn't big enough, the host would never see that there had
been a UA queue overflow. That was why I had added the logging of
a sense buffer overflow. I wasn't so much interested in reporting
sense buffer overflow for its own sake.
So, as it stands right now, I think I'll remove the 1/9 component
of the patch set, unless there is consensus that it is really needed.
James
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the
[PATCH] mpt2sas: prevent double free on error path
I noticed this one when list_del was called with poisoned list
pointers, but the real problem is a double-free (and a use-after-free
just before that).
Both _scsih_probe_boot_devices() and _scsih_sas_device_add() put the
sas_device onto a list, thereby giving up control. Next they call
mpt2sas_transport_port_add() and will list_del and free the object on
errors. If some other function already did the list_del and free, it
will happen again.
This patch adds reference counting to prevent the double free. One
reference count goes to the caller of mpt2sas_transport_port_add(), the
second to the list. Whoever removes the object from the list gets to
drop one reference count. _scsih_probe_boot_devices() and
_scsih_sas_device_add() get a second reference count to ensure the
object is not freed while they are still accessing it.
To prevent the double list_del(), I changed the code to list_del_init()
and added a list_empty() check before that. Since the
list_empty/list_del_init is always called under a lock, this should be
safe.
I hate the complexity this patch adds, but see no alternative to it.
mpt2sas0: failure at
drivers/scsi/mpt2sas/mpt2sas_transport.c:708/mpt2sas_transport_port_add()!
general protection fault: [#1] SMP
CPU 9
Pid: 3097, comm: kworker/u:11 Tainted: GW O 3.6.10+ #31392.trunk
/0JP31P
RIP: 0010:[a0309744] [a0309744]
_scsih_sas_device_remove+0x54/0x90 [mpt2sas]
RSP: 0018:881fed4d7ab0 EFLAGS: 00010046
RAX: dead00200200 RBX: 881ff6a5cd88 RCX: 10e8
RDX: 881ff7dab800 RSI: 881ff7daba00 RDI: dead00100100
RBP: 881fed4d7ad0 R08: dead00200200 R09: 880fff802200
R10: a0317980 R11: R12: 881ff7daba00
R13: 0286 R14: 500605ba006c9d09 R15: 881ff7daba00
FS: () GS:88203fc8() knlGS:
CS: 0010 DS: ES: CR0: 8005003b
CR2: 7f8ac89ec458 CR3: 001ff4c5c000 CR4: 000407e0
DR0: DR1: DR2:
DR3: DR6: 0ff0 DR7: 0400
Process kworker/u:11 (pid: 3097, threadinfo 881fed4d6000, task
881402f3d9c0)
Stack:
0401 881ff6a5c6b0 0401 0016
881fed4d7bb0 a030f93e 881ff6a5cd88
0012000e0f08 006c9d090002000b 00180009500605ba 04010016
Call Trace:
[a030f93e] _scsih_add_device.clone.32+0x2fe/0x420 [mpt2sas]
[a03126e5] _scsih_sas_topology_change_event.clone.38+0x285/0x620
[mpt2sas]
[81078c90] ? load_balance+0x100/0x7a0
[a0312a80] ? _scsih_sas_topology_change_event.clone.38+0x620/0x620
[mpt2sas]
[a0312d8a] _firmware_event_work+0x30a/0xfc0 [mpt2sas]
[810015cc] ? __switch_to+0x14c/0x410
[8106dfdb] ? finish_task_switch+0x4b/0xf0
[a0312a80] ? _scsih_sas_topology_change_event.clone.38+0x620/0x620
[mpt2sas]
[8105bf40] process_one_work+0x140/0x500
[8105d354] worker_thread+0x194/0x510
[8106dfdb] ? finish_task_switch+0x4b/0xf0
[8105d1c0] ? manage_workers+0x320/0x320
[8106282e] kthread+0x9e/0xb0
[815bef44] kernel_thread_helper+0x4/0x10
[815b5e5d] ? retint_restore_args+0x13/0x13
[81062790] ? kthread_freezable_should_stop+0x70/0x70
[815bef40] ? gs_change+0x13/0x13
Signed-off-by: Joern Engel jo...@logfs.org
---
drivers/scsi/mpt2sas/mpt2sas_base.h |1 +
drivers/scsi/mpt2sas/mpt2sas_scsih.c | 55 --
2 files changed, 47 insertions(+), 9 deletions(-)
diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.h
b/drivers/scsi/mpt2sas/mpt2sas_base.h
index 543d8d6..ceb7d41 100644
--- a/drivers/scsi/mpt2sas/mpt2sas_base.h
+++ b/drivers/scsi/mpt2sas/mpt2sas_base.h
@@ -367,6 +367,7 @@ struct _sas_device {
u16 slot;
u8 phy;
u8 responding;
+ struct kref kref;
};
/**
diff --git a/drivers/scsi/mpt2sas/mpt2sas_scsih.c
b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
index c6bdc92..43b3a98 100644
--- a/drivers/scsi/mpt2sas/mpt2sas_scsih.c
+++ b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
@@ -570,6 +570,18 @@ _scsih_sas_device_find_by_handle(struct MPT2SAS_ADAPTER
*ioc, u16 handle)
return NULL;
}
+static void free_sas_device(struct kref *kref)
+{
+ struct _sas_device *sas_device = container_of(kref, struct _sas_device,
+ kref);
+ kfree(sas_device);
+}
+
+static void put_sas_device(struct _sas_device *sas_device)
+{
+ kref_put(sas_device-kref, free_sas_device);
+}
+
/**
* _scsih_sas_device_remove - remove sas_device from list.
* @ioc: per adapter object
@@ -583,14 +595,19 @@ _scsih_sas_device_remove(struct MPT2SAS_ADAPTER *ioc,
struct _sas_device *sas_device)
{
unsigned long flags;
+ int was_on_list = 0;
if (!sas_device)
return;
Re: [PATCH RFC 0/9] [SCSI] Enhanced sense and Unit Attention handling
On Fri, Jan 18, 2013 at 9:27 AM, Ewan D. Milne emi...@redhat.com wrote: This patch set adds changes to the SCSI mid-layer, sysfs and scsi_debug to provide enhanced support for Unit Attention conditions, as well as detection of reported sense data overflow conditions and some changes to sense data processing. It also adds a uevent when the reported capacity changes on an sd device. There was some discussion about this a couple of years ago on the linux-scsi mailing list: http://marc.info/?l=linux-scsim=129702506514742w=2 Although one approach is to send all SCSI sense data to a userspace daemon for processing, this patch set does not take that approach due to the difficulty in reliably delivering all of the data. An interesting UA condition might not be delivered due to a flood of media errors, for example. The mechanism used is to flag when certain UA ASC/ASCQ codes are received that report asynchronous changes to the storage device configuration. An appropriate uevent is then generated for the scsi_device or scsi_target object. An aggregation mechanism is used to avoid generating uevents at too high a rate, and to coalesce multiple UAs reported by LUNs on the same target for a REPORTED LUNS DATA HAS CHANGED sense code. Does this patch series add a function that allows SCSI LLDs to report AEN data to the SCSI core ? What if a SCSI target reports a LUN inventory change via AER to e.g. the iSCSI initiator and that initiator ignores the AEN data ? Will that result in AEN data being ignored and no automatic LUN rescanning ? Bart. -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mpt2sas: prevent double free on error path
Jörn Engel jo...@logfs.org writes:
diff --git a/drivers/scsi/mpt2sas/mpt2sas_scsih.c
b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
index c6bdc92..43b3a98 100644
--- a/drivers/scsi/mpt2sas/mpt2sas_scsih.c
+++ b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
@@ -570,6 +570,18 @@ _scsih_sas_device_find_by_handle(struct MPT2SAS_ADAPTER
*ioc, u16 handle)
return NULL;
}
+static void free_sas_device(struct kref *kref)
+{
+ struct _sas_device *sas_device = container_of(kref, struct _sas_device,
+ kref);
+ kfree(sas_device);
+}
+
+static void put_sas_device(struct _sas_device *sas_device)
+{
+ kref_put(sas_device-kref, free_sas_device);
+}
+
/**
* _scsih_sas_device_remove - remove sas_device from list.
* @ioc: per adapter object
@@ -583,14 +595,19 @@ _scsih_sas_device_remove(struct MPT2SAS_ADAPTER *ioc,
struct _sas_device *sas_device)
{
unsigned long flags;
+ int was_on_list = 0;
if (!sas_device)
return;
spin_lock_irqsave(ioc-sas_device_lock, flags);
- list_del(sas_device-list);
- kfree(sas_device);
+ if (!list_empty(sas_device-list)) {
+ list_del_init(sas_device-list);
+ was_on_list = 1;
+ }
spin_unlock_irqrestore(ioc-sas_device_lock, flags);
+ if (was_on_list)
+ put_sas_device(sas_device);
}
How about the copy of this code in drivers/scsi/mpt3sas/mpt3sas_scsih.c?
Is that safe, or does it need fixing as well?
Bjørn
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
