On Wed, Mar 12 2008 at 15:07 +0200, Boaz Harrosh <bharrosh@xxxxxxxxxxx> wrote: > On Tue, Mar 11 2008 at 21:18 +0200, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote: >> 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. >>> > > OK Now I see isd200_ata_command() is called from a usb.c internal thread. > > What I need to do is call scsi_get_command(scsi_device*) on first invocation. > Now for the call to scsi_put_command()? At the time of the call to > isd200_free_info_ptrs() do you think I still have a valid scsi_device at this point? > > What I will do is this. I will resend my original patch with your comments > fixed. This is for the 2.6.25-rc. And I will send another patch that uses > the proper scsi_get/put_command() for testing and inclusion into the 2.6.26 kernel. > Please ACK on the patch > >>>>> (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. > > How are they unnecessary? who will free them? other wise they will only be > freed at the very end. And that is only because usb_stor_transparent_scsi_command() > does not need any us->extra of it's own. But if ever it will, then this code > buried here will become a leak. > > And I disagree. with your solution. The module that did the allocation should > do the freeing. The above is just an example of what happens with bad programing > style. the core should not have attempted a free on a void pointer just so > protocols can get lazy and not register a destructor. Other wise we do not > learn from passed mistakes and keep doing the same errors. The free should > always be at same file right next to the alloc. (And don't get me started > on the flexibility that enables) > > I keep the patch as it is, I recommend to commit it for solving the leak. > rrr only I cannot do that because the destructor does not have access to the us_data that contains it. As I said, very ugly. New patch attached. Boaz --- From: Boaz Harrosh <bharrosh@xxxxxxxxxxx> Date: Tue, 11 Mar 2008 20:30:53 +0200 Subject: [PATCH] isd200: Fix memory leak in isd200_get_inquiry_data if the inquiry fails then the info structure on us->extra was not freed. Signed-off-by: Boaz Harrosh <bharrosh@xxxxxxxxxxx> --- drivers/usb/storage/isd200.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/drivers/usb/storage/isd200.c b/drivers/usb/storage/isd200.c index ac1764b..2de1f1e 100644 --- a/drivers/usb/storage/isd200.c +++ b/drivers/usb/storage/isd200.c @@ -1231,6 +1231,7 @@ static int isd200_get_inquiry_data( struct us_data *us ) /* Free driver structure */ us->extra_destructor(info); + kfree(info); us->extra = NULL; us->extra_destructor = NULL; } -- 1.5.3.3 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
