Re: scsi_host_alloc does not check for used shost->host_no

[Brian King <brking@linux.vnet.ibm.com>]

Daniel Debonzi wrote:
> Hi everyone,
> 
> First of all, it is the first time I am sending something to one of the
> kernel mail lists. So, if it is not the right place for that, if it is
> not the only place for that, or I am doing something wrong, or wherever,
> please, just let me know.

A couple of comments regarding patch submission..

1. Read Documentation/SubmittingPatches and follow the guidelines contained
   within regarding patch format.
2. Subject should be something like:
	[PATCH] SCSI: scsi_host_alloc does not check for used shost->host_no

> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> index c6457bf..2e191f4 100644
> --- a/drivers/scsi/hosts.c
> +++ b/drivers/scsi/hosts.c
> @@ -310,7 +310,7 @@ struct device_type scsi_host_type = {
>   **/
>  struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
>  {
> -	struct Scsi_Host *shost;
> +	struct Scsi_Host *shost, *tmp_shost;
>  	gfp_t gfp_mask = GFP_KERNEL;
>  	int rval;
> 
> @@ -332,7 +332,18 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
> 
>  	mutex_init(&shost->scan_mutex);
> 
> +	/* 
> +	 * Look if host_no is not been used somewhere else. Is is used to
> +	 * happen when scsi_host_next_hn overflows and goes back to 0.
> +	 */
> + host_no_already_exists:
>  	shost->host_no = scsi_host_next_hn++; /* XXX(hch): still racy */
> +        if(!IS_ERR(tmp_shost = scsi_host_lookup(shost->host_no)))
> +          {

This needs to follow Documentation/CodingStyle, in this particular case, K&R braces.

> +            scsi_host_put(tmp_shost);
> +            goto host_no_already_exists;
> +          }
> +

Do we need to worry about a host in the SHOST_DEL state? In that case, it will still
exist to some degree, but scsi_host_get will fail. For example, what happens if a
shell is in /sys/class/scsi_host/host5/ and you delete host 5 and try to add another.
Couldn't you run into the same problem? In that case the scsi_host_get will fail.
I suppose you could check specifically for -ENXIO getting returned...

-Brian

-- 
Brian King
Linux on Power Virtualization
IBM Linux Technology Center