[stgt] tgtd segfault with software RAID, hard resetting link

[Chris Webb]

Chris Webb <chris at arachsys.com> writes:

> > I see a null pointer dereference in bs_rdwr_request():
[...]
> > What's odd is that switch(cmd->scb[0]) didn't fail back on line 70, but was
> > valid and equal to WRITE_* or we wouldn't have got there. length and ret are
> > both 524288 here for what it's worth. I tried using a device mapper zero target
> > becoming error target, but couldn't reproduce the segfault with this.
[...]
> I've now also seen a segfault from a similar null pointer dereference at line
> 125, in the dprintf, following a read from a hanging md device:

Given that both of these look like the cmd struct being cleared up (client
disconnect?) while a read/write is hanging waiting for a backing device, and
then the cleared struct being used when the io returns, I've papered over
the cracks with the following hideous hack to avoid the null pointer
dereference when printing messages and so on. It's clearly nothing like the
right fix, though.

Cheers,

Chris.

diff --git a/usr/bs_rdwr.c b/usr/bs_rdwr.c
--- a/usr/bs_rdwr.c
+++ b/usr/bs_rdwr.c
@@ -66,14 +66,19 @@
 	ret = length = 0;
 	key = asc = 0;
 
-	switch (cmd->scb[0])
+	uint8_t cmd_scb0, cmd_scb1;
+	uint64_t cmd_offset;
+	cmd_scb0 = cmd->scb[0];
+	cmd_scb1 = cmd->scb[1];
+	cmd_offset = cmd->offset;
+	switch (cmd_scb0)
 	{
 	case SYNCHRONIZE_CACHE:
 	case SYNCHRONIZE_CACHE_16:
 		/* TODO */
-		length = (cmd->scb[0] == SYNCHRONIZE_CACHE) ? 0 : 0;
+		length = (cmd_scb0 == SYNCHRONIZE_CACHE) ? 0 : 0;
 
-		if (cmd->scb[1] & 0x2) {
+		if (cmd_scb1 & 0x2) {
 			result = SAM_STAT_CHECK_CONDITION;
 			key = ILLEGAL_REQUEST;
 			asc = ASC_INVALID_FIELD_IN_CDB;
@@ -86,15 +91,15 @@
 	case WRITE_16:
 		length = scsi_get_out_length(cmd);
 		ret = pwrite64(fd, scsi_get_out_buffer(cmd), length,
-			       cmd->offset);
-		if (ret == length) {
+			       cmd_offset);
+		if (cmd->scb && ret == length) {
 			/*
 			 * it would be better not to access to pg
 			 * directy.
 			 */
 			struct mode_pg *pg = cmd->dev->mode_pgs[0x8];
 
-			if (((cmd->scb[0] != WRITE_6) && (cmd->scb[1] & 0x8)) ||
+			if (((cmd_scb0 != WRITE_6) && (cmd_scb1 & 0x8)) ||
 			    !(pg->mode_data[0] & 0x04))
 				bs_sync_sync_range(cmd, length, &result, &key,
 						   &asc);
@@ -108,22 +113,25 @@
 	case READ_16:
 		length = scsi_get_in_length(cmd);
 		ret = pread64(fd, scsi_get_in_buffer(cmd), length,
-			      cmd->offset);
+			      cmd_offset);
 
-		if (ret != length)
+		if (!cmd->scb || ret != length)
 			set_medium_error(&result, &key, &asc);
 		break;
 	default:
 		break;
 	}
 
-	dprintf("io done %p %x %d %u\n", cmd, cmd->scb[0], ret, length);
+	dprintf("io done %p %x %d %u\n", cmd, cmd_scb0, ret, length);
 
 	scsi_set_result(cmd, result);
-
-	if (result != SAM_STAT_GOOD) {
+	if (!cmd->scb) {
+		eprintf("weird cmd->scb error %p %x %d %d %" PRIu64 ", %m\n",
+			cmd, cmd_scb0, ret, length, cmd_offset);
+		sense_data_build(cmd, key, asc);
+	} else if (result != SAM_STAT_GOOD) {
 		eprintf("io error %p %x %d %d %" PRIu64 ", %m\n",
-			cmd, cmd->scb[0], ret, length, cmd->offset);
+			cmd, cmd_scb0, ret, length, cmd_offset);
 		sense_data_build(cmd, key, asc);
 	}
 }
--
To unsubscribe from this list: send the line "unsubscribe stgt" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html