Skip to contentSkip to conversation

panic in scsi_free/sr_scatter_pad

PATCH In review Paul Gortmaker 2 participants 3 messages May 29, 2001 SCSI

lore
Share & tools
lore

Paul Gortmaker

author
#0 lore raw

PATCH 1/1 panic in scsi_free/sr_scatter_pad

I think I recall seeing something reported like this on the list(?):

sr: ran out of mem for scatter pad

  Kernel panic: scsi_free: bad offset

Regardless, I've seen this on 2.4.5, aha1542, 40MB, mount /dev/scd0 after a fresh reboot and spark, pop, fizz, plop...

Seems there is a bug in sr_scatter_pad() associated with ENOMEM handling. AFAICT it goes something like this:

- sr_scatter_pad increases use_sg (and sglist_len)
- scsi_malloc(sglist_len) returns NULL (hence message 1)
- sr_scatter_pad bails out but leaves increased values
- scsi_release_buffers loops on use_sg, calls scsi_free each time.
- scsi_free gets called with random garbage - hence message 2. 8-)

Restoring the old info back into SCpnt fixes the panic - patch follows. I'll have to read some more to determine why scsi_malloc is having trouble in handing out ISA DMA mem and causing the 1st message...

Paul.

scsi/sr.c renamed from scsi/sr.c~ +7 -0
@@ -31,6 +31,8 @@
  *	 Modified by Arnaldo Carvalho de Melo <[email protected]>
  *	 check resource allocation in sr_init and some cleanups
  *
+ *	 Restore SCpnt state if scsi_malloc fails in sr_scatter_pad - Paul G.
+ *
  */
 
 #include <linux/module.h>

@@ -263,10 +265,13 @@
 {
 	struct scatterlist *sg, *old_sg = NULL;
 	int i, fsize, bsize, sg_ent;
+	unsigned short old_sglist_len;
 	char *front, *back;
 
 	back = front = NULL;
 	sg_ent = SCpnt->use_sg;
+	old_sglist_len = SCpnt->sglist_len;
+	SCpnt->old_use_sg = SCpnt->use_sg;
 	bsize = 0; /* gcc... */
 
 	/*

@@ -332,6 +337,8 @@
 
 no_mem:
 	printk("sr: ran out of mem for scatter pad\n");
+	SCpnt->use_sg = SCpnt->old_use_sg;
+	SCpnt->sglist_len = old_sglist_len;
 	if (front)
 		scsi_free(front, fsize);
 	if (back)
#1 lore raw
Quoted 4 lines · Paul Gortmaker

I think I recall seeing something reported like this on the list(?):

sr: ran out of mem for scatter pad

  Kernel panic: scsi_free: bad offset

Here's a better patch, it also gets the freeing right. It's been fixed for long, just haven't been sent to Linus yet. It's in Alan's tree, and in fact I think I've send it to this list more than once :)

Signature · 1 line
Jens Axboe
drivers/scsi/sr.c renamed from opt/kernel/linux-2.4.4-pre2/drivers/scsi/sr.c +12 -5
diff -urN --exclude-from /home/axboe/cdrom/exclude /opt/kernel/linux-2.4.4-pre2/drivers/scsi/sr.c linux/drivers/scsi/sr.c
@@ -262,7 +262,7 @@
 static int sr_scatter_pad(Scsi_Cmnd *SCpnt, int s_size)
 {
 	struct scatterlist *sg, *old_sg = NULL;
-	int i, fsize, bsize, sg_ent;
+	int i, fsize, bsize, sg_ent, sg_count;
 	char *front, *back;
 
 	back = front = NULL;

@@ -290,17 +290,24 @@
 	/*
 	 * extend or allocate new scatter-gather table
 	 */
-	if (SCpnt->use_sg)
+	sg_count = SCpnt->use_sg;
+	if (sg_count)
 		old_sg = (struct scatterlist *) SCpnt->request_buffer;
 	else {
-		SCpnt->use_sg = 1;
+		sg_count = 1;
 		sg_ent++;
 	}
 
-	SCpnt->sglist_len = ((sg_ent * sizeof(struct scatterlist)) + 511) & ~511;
-	if ((sg = scsi_malloc(SCpnt->sglist_len)) == NULL)
+	i = ((sg_ent * sizeof(struct scatterlist)) + 511) & ~511;
+	if ((sg = scsi_malloc(i)) == NULL)
 		goto no_mem;
 
+	/*
+	 * no more failing memory allocs possible, we can safely assign
+	 * SCpnt values now
+	 */
+	SCpnt->sglist_len = i;
+	SCpnt->use_sg = sg_count;
 	memset(sg, 0, SCpnt->sglist_len);
 
 	i = 0;
author
#2 lore raw
In reply to Paul Gortmaker
Quoted 10 lines · Jens Axboe
Quoted 4 lines · Paul Gortmaker

I think I recall seeing something reported like this on the list(?):

sr: ran out of mem for scatter pad

  Kernel panic: scsi_free: bad offset

Here's a better patch, it also gets the freeing right. It's been fixed for long, just haven't been sent to Linus yet. It's in Alan's tree, and in fact I think I've send it to this list more than once :)

Ok, essentially same patch - good to see. Seems old rule of thumb[1] for
linux patches still applies :) I see you opted for a new var. to store
old use_sg value, rather than make use of SCpnt->old_use_sg like I did. Was curious as to your reasoning for that - maybe I'm overlooking something.

Other thing that crossed my mind was the appropriateness of scsi_free() doing a panic. For this particular case, a BUG() would have been more informative if we were relying on info in a bug report from Joe Average to solve the problem. (Also, panic is a bit harsh if say CD is only SCSI device and everything else is EIDE, but scsi_free has no way of knowing...)

Paul.

[1] Odds are somebody has already posted the patch you just finished...