Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755218AbYCKTSm (ORCPT ); Tue, 11 Mar 2008 15:18:42 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1753266AbYCKTSd (ORCPT ); Tue, 11 Mar 2008 15:18:33 -0400 Received: from iolanthe.rowland.org ([192.131.102.54]:59384 "HELO iolanthe.rowland.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1751282AbYCKTSc (ORCPT ); Tue, 11 Mar 2008 15:18:32 -0400 Date: Tue, 11 Mar 2008 15:18:31 -0400 (EDT) From: Alan Stern X-X-Sender: stern@iolanthe.rowland.org To: Boaz Harrosh cc: James Bottomley , Matthew Dharm , Sven Schnelle , , linux-scsi , FUJITA Tomonori , Andrew Morton Subject: Re: [PATCH] [SCSI] gdth: Allocate sense_buffer to prevent NULL pointer dereference In-Reply-To: <47D6D148.1090304@panasas.com> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2426 Lines: 76 On Tue, 11 Mar 2008, Boaz Harrosh wrote: > > You can first get to the scsi_device in isd200_ata_command(). > > I was afraid of that. I don't think I want to call scsi_get_command > from within .queuecommand. I will leave the code hacked as today. What are you talking about? isd200_ata_command isn't called by queuecommand. > > The last > > place you can get to the scsi_device (if one exists!) is > > quiesce_and_remove_host() -- but that's part of the core, not specific > > to isd200. > > > > Here two, it looks like I need to introduce a new function pointer for isd200 Why? And why do you need to get to the scsi_device in the first place? > I'll leave it for now. Though I know this is not the last I'll see of this driver. > > >> (And one more stupid question. Why does isd200_init_info allocates the info > >> structure but the isd200_free_info_ptrs does not free it, it kind of > >> limits the way it can be allocated, no?) > > > > Not at all. isd200_free_info_ptrs() frees everything pointed to by the > > info structure, and the info structure itself is freed later on by the > > usb-storage core in usb_stor_release_resources(). > > > > OK so in isd200_get_inquiry_data() at the end near the call to: > us->extra_destructor(info); > us->extra = NULL; > > It leaks the info. Yes. The three lines of code there are unnecessary. You should remove them (and the comment) instead of adding more somewhere else. Or if you want to keep them, just add a line to kfree(us->extra) before us->extra is set to NULL. > Please ACK the first patch sent, so James can put it in scsi-rc-fixes as part > of the sense_buffer effort for 2.6.25-rc The first patch is okay except for style violations: > @@ -294,6 +294,7 @@ struct isd200_info { > unsigned char MaxLUNs; > struct scsi_cmnd srb; > struct scatterlist sg; > + u8* sense_buffer; This should be u8 *sense_buffer; And > isd200_free_info_ptrs(info); > kfree(info); > retStatus = ISD200_ERROR; > } > + else > + info->srb.sense_buffer = info->sense_buffer; The "else" should go on the same line as the closing brace, and it should have its own opening brace. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/