Re: [PATCH (resend)]: SCSI: fix /proc memory leak in the SCSI core

Re: [PATCH (resend)]: SCSI: fix /proc memory leak in the SCSI core

[Tony Battersby]

I tested Alan's original patch with sym53c8xx back in January:

http://marc.info/?l=linux-scsi&m=123143710010939&w=4

Tested-by: Tony Battersby <tonyb@cybernetics.com>

Re: [PATCH (resend)]: SCSI: fix /proc memory leak in the SCSI core

[Alan Stern]

On Fri, 27 Feb 2009, Tony Battersby wrote:

> I tested Alan's original patch with sym53c8xx back in January:
> 
> http://marc.info/?l=linux-scsi&m=123143710010939&w=4
> 
> Tested-by: Tony Battersby <tonyb@cybernetics.com>

Thanks for the support.  I must have missed your post when it appeared
last month.

In fact, I don't care which approach gets accepted: James's or my own.  
His has the advantage of being somewhat shorter and not changing the
order of events during host creation.

On the other hand, James, your suggested patch is definitely wrong,
for a trivial reason.  You moved the scsi_proc_hostdir_rm() call into
scsi_host_put() instead of scsi_host_dev_release(), where it belongs.
I'll post a corrected version shortly.

Alan Stern

[PATCH (resend)]: SCSI: fix /proc memory leak in the SCSI core

[Alan Stern]

The SCSI core calls scsi_proc_hostdir_add() from within
scsi_host_alloc(), but the corresponding scsi_proc_hostdir_rm()
routine is called from within scsi_remove_host().  As a result, if a
host is allocated and then deallocated without ever being registered,
the host's directory in /proc is leaked.

This patch (as1181) fixes this bug in the SCSI core by moving
scsi_proc_hostdir_add() into scsi_host_add().

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

---

Note: This is a repost of a bug-fix patch that was first submitted on 
December 1.

After looking through the documentation and code for the SCSI legacy 
API, I can't find anything incompatible with this change.  However I 
don't have any hardware with a legacy driver so I can't actually make 
the test.


Index: usb-2.6/drivers/scsi/hosts.c
===================================================================
--- usb-2.6.orig/drivers/scsi/hosts.c
+++ usb-2.6/drivers/scsi/hosts.c
@@ -206,9 +206,10 @@ int scsi_add_host(struct Scsi_Host *shos
 	if (error)
 		goto fail;
 
+	scsi_proc_hostdir_add(shost->hostt);
+
 	if (!shost->shost_gendev.parent)
 		shost->shost_gendev.parent = dev ? dev : &platform_bus;
-
 	error = device_add(&shost->shost_gendev);
 	if (error)
 		goto out;
@@ -259,6 +260,7 @@ int scsi_add_host(struct Scsi_Host *shos
  out_del_gendev:
 	device_del(&shost->shost_gendev);
  out:
+	scsi_proc_hostdir_rm(shost->hostt);
 	scsi_destroy_command_freelist(shost);
  fail:
 	return error;
@@ -407,7 +409,6 @@ struct Scsi_Host *scsi_host_alloc(struct
 		goto fail_kfree;
 	}
 
-	scsi_proc_hostdir_add(shost->hostt);
 	return shost;
 
  fail_kfree:

Re: [PATCH (resend)]: SCSI: fix /proc memory leak in the SCSI core

[James Bottomley]

On Wed, 2009-02-18 at 10:52 -0500, Alan Stern wrote:
> The SCSI core calls scsi_proc_hostdir_add() from within
> scsi_host_alloc(), but the corresponding scsi_proc_hostdir_rm()
> routine is called from within scsi_remove_host().  As a result, if a
> host is allocated and then deallocated without ever being registered,
> the host's directory in /proc is leaked.
> 
> This patch (as1181) fixes this bug in the SCSI core by moving
> scsi_proc_hostdir_add() into scsi_host_add().
> 
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> 
> ---
> 
> Note: This is a repost of a bug-fix patch that was first submitted on 
> December 1.
> 
> After looking through the documentation and code for the SCSI legacy 
> API, I can't find anything incompatible with this change.  However I 
> don't have any hardware with a legacy driver so I can't actually make 
> the test.

OK, but resending a patch you expressed reservations about putting in
without testing doesn't really help me.  I need a way to get comfortable
with its safety.

So, what about this alternative fix instead: if the removal were moved
to scsi_host_put(), that would address all the problems and have the
advantage that everyone will test it ...

James

---

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index aa670a1..f4240a8 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -176,7 +176,6 @@ void scsi_remove_host(struct Scsi_Host *shost)
 	transport_unregister_device(&shost->shost_gendev);
 	device_unregister(&shost->shost_dev);
 	device_del(&shost->shost_gendev);
-	scsi_proc_hostdir_rm(shost->hostt);
 }
 EXPORT_SYMBOL(scsi_remove_host);
 
@@ -493,6 +492,7 @@ EXPORT_SYMBOL(scsi_host_get);
  **/
 void scsi_host_put(struct Scsi_Host *shost)
 {
+	scsi_proc_hostdir_rm(shost->hostt);
 	put_device(&shost->shost_gendev);
 }
 EXPORT_SYMBOL(scsi_host_put);

Re: [PATCH (resend)]: SCSI: fix /proc memory leak in the SCSI core

[Alan Stern]

On Sun, 22 Feb 2009, James Bottomley wrote:

> OK, but resending a patch you expressed reservations about putting in
> without testing doesn't really help me.  I need a way to get comfortable
> with its safety.
> 
> So, what about this alternative fix instead: if the removal were moved
> to scsi_host_put(), that would address all the problems and have the
> advantage that everyone will test it ...

I thought of doing it that way too.  It has the disadvantage of
exposing part of the proc interface to userspace before the host is
registered.  Now, since all we're adding is the host's directory, maybe 
this doesn't matter.  But it didn't seem like a good idea.

Alan Stern

Re: [PATCH (resend)]: SCSI: fix /proc memory leak in the SCSI core

[James Bottomley]

On Sun, 2009-02-22 at 14:34 -0500, Alan Stern wrote:
> On Sun, 22 Feb 2009, James Bottomley wrote:
> 
> > OK, but resending a patch you expressed reservations about putting in
> > without testing doesn't really help me.  I need a way to get comfortable
> > with its safety.
> > 
> > So, what about this alternative fix instead: if the removal were moved
> > to scsi_host_put(), that would address all the problems and have the
> > advantage that everyone will test it ...
> 
> I thought of doing it that way too.  It has the disadvantage of
> exposing part of the proc interface to userspace before the host is
> registered.  Now, since all we're adding is the host's directory, maybe 
> this doesn't matter.  But it didn't seem like a good idea.

It's current behaviour (and has been so for all of git history) with no
reported bugs.

All it's doing is creating a proc dir ... it's not exposing any
interfaces within, so from a theoretical standpoint it's perfectly OK.
The necessity for finding a legacy system to test was precisely because
you moved it.

James

Re: [PATCH (resend)]: SCSI: fix /proc memory leak in the SCSI core

[Alan Stern]

On Sun, 22 Feb 2009, James Bottomley wrote:

> On Sun, 2009-02-22 at 14:34 -0500, Alan Stern wrote:
> > On Sun, 22 Feb 2009, James Bottomley wrote:
> > 
> > > OK, but resending a patch you expressed reservations about putting in
> > > without testing doesn't really help me.  I need a way to get comfortable
> > > with its safety.
> > > 
> > > So, what about this alternative fix instead: if the removal were moved
> > > to scsi_host_put(), that would address all the problems and have the
> > > advantage that everyone will test it ...
> > 
> > I thought of doing it that way too.  It has the disadvantage of
> > exposing part of the proc interface to userspace before the host is
> > registered.  Now, since all we're adding is the host's directory, maybe 
> > this doesn't matter.  But it didn't seem like a good idea.
> 
> It's current behaviour (and has been so for all of git history) with no
> reported bugs.
> 
> All it's doing is creating a proc dir ... it's not exposing any
> interfaces within, so from a theoretical standpoint it's perfectly OK.
> The necessity for finding a legacy system to test was precisely because
> you moved it.

Okay.  Then either patch is fine with me.

Alan Stern