[PATCH] Save command pool address of Scsi_Host

[PATCH] Save command pool address of Scsi_Host

[jgross]

From: Juergen Gross <jgross@suse.com>

If a scsi host driver specifies .cmd_len in it's scsi_host_template, a driver's
private command pool is needed. scsi_find_host_cmd_pool() will locate it, but
scsi_alloc_host_cmd_pool() isn't saving the pool address in the host template.

This will result in an access error when the host is removed.

Avoid the problem by saving the address of a new allocated command pool where
it is expected.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 drivers/scsi/scsi.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 88d46fe..da769f9 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -380,6 +380,10 @@ scsi_alloc_host_cmd_pool(struct Scsi_Host *shost)
 		pool->slab_flags |= SLAB_CACHE_DMA;
 		pool->gfp_mask = __GFP_DMA;
 	}
+
+	if (shost->hostt->cmd_size)
+		shost->hostt->cmd_pool = pool;
+
 	return pool;
 }
 
-- 
1.8.4.5

Re: [PATCH] Save command pool address of Scsi_Host

[Christoph Hellwig]

On Fri, Aug 01, 2014 at 08:27:05AM +0200, jgross@suse.com wrote:
> From: Juergen Gross <jgross@suse.com>
> 
> If a scsi host driver specifies .cmd_len in it's scsi_host_template, a driver's
> private command pool is needed. scsi_find_host_cmd_pool() will locate it, but
> scsi_alloc_host_cmd_pool() isn't saving the pool address in the host template.
> 
> This will result in an access error when the host is removed.
> 
> Avoid the problem by saving the address of a new allocated command pool where
> it is expected.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Looks good, but minor nitpick below:

> +	if (shost->hostt->cmd_size)
> +		shost->hostt->cmd_pool = pool;
> +


We already have a local hostt variable for the host template in this
function, please use it.

Re: [PATCH] Save command pool address of Scsi_Host

[James Bottomley]

On Fri, 2014-08-01 at 05:03 -0700, Christoph Hellwig wrote:
> On Fri, Aug 01, 2014 at 08:27:05AM +0200, jgross@suse.com wrote:
> > From: Juergen Gross <jgross@suse.com>
> > 
> > If a scsi host driver specifies .cmd_len in it's scsi_host_template, a driver's
> > private command pool is needed. scsi_find_host_cmd_pool() will locate it, but
> > scsi_alloc_host_cmd_pool() isn't saving the pool address in the host template.
> > 
> > This will result in an access error when the host is removed.
> > 
> > Avoid the problem by saving the address of a new allocated command pool where
> > it is expected.
> > 
> > Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> Looks good, but minor nitpick below:
> 
> > +	if (shost->hostt->cmd_size)
> > +		shost->hostt->cmd_pool = pool;
> > +
> 
> 
> We already have a local hostt variable for the host template in this
> function, please use it.

Wait, that's not right at all.  There looks to be a thinko in the
command pool handling code.  We have both a cmd_pool in the host
structure and in the host template structure, but there's confusion
about which one we're supposed to be using.

The origin of confusion seems to be the reference counting in the pool
itself ... you want the same pool for all hosts, since they can only
have one cmd_size, but you want it created on first host use and
destroyed again on the last one.

If you take this patch, a host that attached, detaches and then attaches
a host will panic because it will use a freed pool structure.

This whole mess is created by the attempt to refcount the pools.  What's
wrong with simply creating the pool at init time and deleting it again
at module removal ... that way no refcounting and no bogus problems like
this (and we can delete the cmd_pool from the host).  The restriction
this would give is that cmd_size can only be set in the template, but
that seems to be the only safe use anyway, since any driver trying to
vary this in its host add routines will get unexpected results.

James

Re: [PATCH] Save command pool address of Scsi_Host

[Juergen Gross]

On 08/01/2014 10:24 PM, James Bottomley wrote:
> On Fri, 2014-08-01 at 05:03 -0700, Christoph Hellwig wrote:
>> On Fri, Aug 01, 2014 at 08:27:05AM +0200, jgross@suse.com wrote:
>>> From: Juergen Gross <jgross@suse.com>
>>>
>>> If a scsi host driver specifies .cmd_len in it's scsi_host_template, a driver's
>>> private command pool is needed. scsi_find_host_cmd_pool() will locate it, but
>>> scsi_alloc_host_cmd_pool() isn't saving the pool address in the host template.
>>>
>>> This will result in an access error when the host is removed.
>>>
>>> Avoid the problem by saving the address of a new allocated command pool where
>>> it is expected.
>>>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>
>> Looks good, but minor nitpick below:
>>
>>> +	if (shost->hostt->cmd_size)
>>> +		shost->hostt->cmd_pool = pool;
>>> +
>>
>>
>> We already have a local hostt variable for the host template in this
>> function, please use it.
>
> Wait, that's not right at all.  There looks to be a thinko in the
> command pool handling code.  We have both a cmd_pool in the host
> structure and in the host template structure, but there's confusion
> about which one we're supposed to be using.
>
> The origin of confusion seems to be the reference counting in the pool
> itself ... you want the same pool for all hosts, since they can only
> have one cmd_size, but you want it created on first host use and
> destroyed again on the last one.
>
> If you take this patch, a host that attached, detaches and then attaches
> a host will panic because it will use a freed pool structure.

Indeed.

> This whole mess is created by the attempt to refcount the pools.  What's
> wrong with simply creating the pool at init time and deleting it again
> at module removal ... that way no refcounting and no bogus problems like
> this (and we can delete the cmd_pool from the host).  The restriction
> this would give is that cmd_size can only be set in the template, but
> that seems to be the only safe use anyway, since any driver trying to
> vary this in its host add routines will get unexpected results.

OTOH it would be possible to just delete .cmd_pool in the template when
deleting the pool. I'll send a patch doing this and you can decide
whether to take it or to use the other solution.

I'm not sure which to prefer: the init/remove version is simple, while
the dynamic version requires no changes in the driver's source and the
pool's resources are allocated only when really needed.


Juergen

Re: [PATCH] Save command pool address of Scsi_Host

[Christoph Hellwig]

On Mon, Aug 04, 2014 at 06:22:59AM +0200, Juergen Gross wrote:
> OTOH it would be possible to just delete .cmd_pool in the template when
> deleting the pool. I'll send a patch doing this and you can decide
> whether to take it or to use the other solution.
> 
> I'm not sure which to prefer: the init/remove version is simple, while
> the dynamic version requires no changes in the driver's source and the
> pool's resources are allocated only when really needed.

the init/remove version needs the driver to call a per-host template
function/method from module_init/exit, something we've avoided
successfully for the last 10 years.

Re: [PATCH] Save command pool address of Scsi_Host

[Christoph Hellwig]

On Sat, Aug 02, 2014 at 12:24:34AM +0400, James Bottomley wrote:
> Wait, that's not right at all.  There looks to be a thinko in the
> command pool handling code.  We have both a cmd_pool in the host
> structure and in the host template structure, but there's confusion
> about which one we're supposed to be using.

For anything in the I/O path: shost->cmd_pool, for finding a pool
created for a specific host template: template->cmd_pool.

> The origin of confusion seems to be the reference counting in the pool
> itself ... you want the same pool for all hosts, since they can only
> have one cmd_size, but you want it created on first host use and
> destroyed again on the last one.
> 
> If you take this patch, a host that attached, detaches and then attaches
> a host will panic because it will use a freed pool structure.

Yes, it also needs to set hostt->cmd_pool to NULL when the last
reference goes away.

Re: [PATCH] Save command pool address of Scsi_Host

[James Bottomley]

On Mon, 2014-08-04 at 04:03 -0700, Christoph Hellwig wrote:
> On Mon, Aug 04, 2014 at 06:22:59AM +0200, Juergen Gross wrote:
> > OTOH it would be possible to just delete .cmd_pool in the template when
> > deleting the pool. I'll send a patch doing this and you can decide
> > whether to take it or to use the other solution.
> > 
> > I'm not sure which to prefer: the init/remove version is simple, while
> > the dynamic version requires no changes in the driver's source and the
> > pool's resources are allocated only when really needed.
> 
> the init/remove version needs the driver to call a per-host template
> function/method from module_init/exit, something we've avoided
> successfully for the last 10 years.

Well, not exactly.

You're correct we eliminated the scsi module initialisatio which served
as per-template initialisation.  However, when we eliminated this, there
was one thing: the proc directory, that was still per-template not per
host.  If you look, it already has a ref counting mechanism similar to
the one you reinvented for the pools.  Since it's easy to get this
wrong, we should just generalise the existing mechanism for anything
today that wants to be per-host-template so people have more difficulty
getting it wrong and so we consolidate code that's the same.

However, this is way beyond a simple bug fix.  Do the bug fix first,
then we can consolidate the code as an enhancement.

James