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@xxxxxxxx wrote:
> > From: Juergen Gross <jgross@xxxxxxxx>
> > 
> > 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@xxxxxxxx>
> 
> 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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/