[PATCH] SCSI host ID assigment overoptimization removal in 2.4.18

[PATCH] SCSI host ID assigment overoptimization removal in 2.4.18

[Vladislav Bolkhovitine]

There is an overoptimization in SCSI host ID assigment algorithm in 2.4.18 
and possibly others, which lead to appearance of SCSI hosts with the same IDs. 

Simple scenario:
1. Add one adapter, host_id=0
2. Remove it
3. Add another adapter, its host_id=0
4. Add the adapter 1 again, it reuses its original scsi_host_no_list entry
	and gets host_id=0 as well. Oops.

When the adapter was being unregistered on step 2, max_scsi_hosts gets 
decremented to 0. During registration of the new host, it received 
host_id=max_scsi_hosts. On the step 4, the old entry in scsi_host_no_list 
was found and reused, thus we have two hosts with the same host_id 0.

So, it is impossible to use simultaneously scsi_host_no_list and host IDs 
reusing (i.e. max_scsi_hosts decrementing). I chose to remove the last one. 
Here is the patch against 2.4.18.

I was not able to find who is the maintainer of SCSI subsistem at the moment. 
Who is doing so, please consider the idea of this patch to include in the 
mainstream kernel.

Vlad

diff -urdN linux-2.4.18.orig/drivers/scsi/hosts.c linux-2.4.18.host_fix/drivers/scsi/hosts.c
--- linux-2.4.18.orig/drivers/scsi/hosts.c      Mon Feb 25 22:38:04 2002
+++ linux-2.4.18.host_fix/drivers/scsi/hosts.c  Wed Jul 17 17:08:47 2002
@@ -107,6 +107,17 @@
     if (shn) shn->host_registered = 0;
     /* else {} : This should not happen, we should panic here... */

+    /*
+     * With this algorithm together with scsi_host_no_list entry reusage
+     * from scsi_register() it is possible really easy to get two
+     * hosts with the same host_id. Simple scenario:
+     *  - add one adapter, host_id=0
+     *  - remove it
+     *  - add another adapter, its host_id=0
+     *  - add the adapter 1 again, it reuses its scsi_host_no_list entry
+     *     and gets host_id=0 as well. Oops.
+     */
+#if 0
     /* If we are removing the last host registered, it is safe to reuse
      * its host number (this avoids "holes" at boot time) (DB)
      * It is also safe to reuse those of numbers directly below which have
@@ -121,6 +132,7 @@
                break;
        }
     }
+#endif

Re: [PATCH] SCSI host ID assigment overoptimization removal in 2.4.18

[Luben Tuikov]

Vladislav Bolkhovitine wrote:
> 
> There is an overoptimization in SCSI host ID assigment algorithm in 2.4.18
> and possibly others, which lead to appearance of SCSI hosts with the same IDs.
> 
> Simple scenario:
> 1. Add one adapter, host_id=0
> 2. Remove it
> 3. Add another adapter, its host_id=0
> 4. Add the adapter 1 again, it reuses its original scsi_host_no_list entry
>         and gets host_id=0 as well. Oops.
> 
> When the adapter was being unregistered on step 2, max_scsi_hosts gets
> decremented to 0. During registration of the new host, it received
> host_id=max_scsi_hosts. On the step 4, the old entry in scsi_host_no_list
> was found and reused, thus we have two hosts with the same host_id 0.
> 
> So, it is impossible to use simultaneously scsi_host_no_list and host IDs
> reusing (i.e. max_scsi_hosts decrementing). I chose to remove the last one.
> Here is the patch against 2.4.18.
> 
> I was not able to find who is the maintainer of SCSI subsistem at the moment.
> Who is doing so, please consider the idea of this patch to include in the
> mainstream kernel.


Yep, I concur -- this patch works.

I've wanted a patch for this for sometime and brought this same
issue on 02/05/29, but was too busy with my own project to unscramble
the infamous SCSI host registration.

AFAIK, Doug is working on this now and it will be a whole different
and better story for latter 2.5/6 (struct list_head, etc.).

Anyways, this works for 2.4 and I'd be happy to see it in.

Thanks,
-- 
Luben
 
> Vlad
> 
> diff -urdN linux-2.4.18.orig/drivers/scsi/hosts.c linux-2.4.18.host_fix/drivers/scsi/hosts.c
> --- linux-2.4.18.orig/drivers/scsi/hosts.c      Mon Feb 25 22:38:04 2002
> +++ linux-2.4.18.host_fix/drivers/scsi/hosts.c  Wed Jul 17 17:08:47 2002
> @@ -107,6 +107,17 @@
>      if (shn) shn->host_registered = 0;
>      /* else {} : This should not happen, we should panic here... */
> 
> +    /*
> +     * With this algorithm together with scsi_host_no_list entry reusage
> +     * from scsi_register() it is possible really easy to get two
> +     * hosts with the same host_id. Simple scenario:
> +     *  - add one adapter, host_id=0
> +     *  - remove it
> +     *  - add another adapter, its host_id=0
> +     *  - add the adapter 1 again, it reuses its scsi_host_no_list entry
> +     *     and gets host_id=0 as well. Oops.
> +     */
> +#if 0
>      /* If we are removing the last host registered, it is safe to reuse
>       * its host number (this avoids "holes" at boot time) (DB)
>       * It is also safe to reuse those of numbers directly below which have
> @@ -121,6 +132,7 @@
>                 break;
>         }
>      }
> +#endif
> -
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] SCSI host ID assigment overoptimization removal in 2.4.18

[Aron Zeh]

My blessings go with the patch, too. I checked and tested the fix on its
prior submission and it worked fine.

Aron

Vladislav Bolkhovitine wrote:
>
> There is an overoptimization in SCSI host ID assigment algorithm in
2.4.18
> and possibly others, which lead to appearance of SCSI hosts with the same
IDs.
>
> Simple scenario:
> 1. Add one adapter, host_id=0
> 2. Remove it
> 3. Add another adapter, its host_id=0
> 4. Add the adapter 1 again, it reuses its original scsi_host_no_list
entry
>         and gets host_id=0 as well. Oops.
>
> When the adapter was being unregistered on step 2, max_scsi_hosts gets
> decremented to 0. During registration of the new host, it received
> host_id=max_scsi_hosts. On the step 4, the old entry in scsi_host_no_list
> was found and reused, thus we have two hosts with the same host_id 0.
>
> So, it is impossible to use simultaneously scsi_host_no_list and host IDs
> reusing (i.e. max_scsi_hosts decrementing). I chose to remove the last
one.
> Here is the patch against 2.4.18.
>
> I was not able to find who is the maintainer of SCSI subsistem at the
moment.
> Who is doing so, please consider the idea of this patch to include in the
> mainstream kernel.


Yep, I concur -- this patch works.

I've wanted a patch for this for sometime and brought this same
issue on 02/05/29, but was too busy with my own project to unscramble
the infamous SCSI host registration.

AFAIK, Doug is working on this now and it will be a whole different
and better story for latter 2.5/6 (struct list_head, etc.).

Anyways, this works for 2.4 and I'd be happy to see it in.

Thanks,
--
Luben

> Vlad
>
> diff -urdN linux-2.4.18.orig/drivers/scsi/hosts.c
linux-2.4.18.host_fix/drivers/scsi/hosts.c
> --- linux-2.4.18.orig/drivers/scsi/hosts.c      Mon Feb 25 22:38:04 2002
> +++ linux-2.4.18.host_fix/drivers/scsi/hosts.c  Wed Jul 17 17:08:47 2002
> @@ -107,6 +107,17 @@
>      if (shn) shn->host_registered = 0;
>      /* else {} : This should not happen, we should panic here... */
>
> +    /*
> +     * With this algorithm together with scsi_host_no_list entry reusage
> +     * from scsi_register() it is possible really easy to get two
> +     * hosts with the same host_id. Simple scenario:
> +     *  - add one adapter, host_id=0
> +     *  - remove it
> +     *  - add another adapter, its host_id=0
> +     *  - add the adapter 1 again, it reuses its scsi_host_no_list entry
> +     *     and gets host_id=0 as well. Oops.
> +     */
> +#if 0
>      /* If we are removing the last host registered, it is safe to reuse
>       * its host number (this avoids "holes" at boot time) (DB)
>       * It is also safe to reuse those of numbers directly below which
have
> @@ -121,6 +132,7 @@
>                 break;
>         }
>      }
> +#endif
> -
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] SCSI host ID assigment overoptimization removal in 2.4.18

[Vladislav Bolkhovitine]

So, should I send this patch directly to Linus Torvalds then to see it in?

BTW, looking at the sources, I have noticed kmalloc() with unchecked return
value in scsi_register(). Here is the patch.

Vlad

diff -urdN linux-2.4.18-enc.orig/drivers/scsi/hosts.c linux-2.4.18-enc.register_kmalloc/drivers/scsi/hosts.c
--- linux-2.4.18-enc.orig/drivers/scsi/hosts.c  Wed Jul 17 12:49:51 2002
+++ linux-2.4.18-enc.register_kmalloc/drivers/scsi/hosts.c      Thu Jul 18 15:47:51 2002
@@ -174,6 +186,12 @@
                 return NULL;
         }
        shn->name = kmalloc(hname_len + 1, GFP_ATOMIC);
+       if (!shn->name) {
+               kfree(retval);
+               kfree(shn);
+               printk(KERN_ERR "scsi: out of memory(3) in scsi_register.\n");
+               return NULL;
+        }
        if (hname_len > 0)
            strncpy(shn->name, hname, hname_len);
        shn->name[hname_len] = 0;


On Fri, Jul 19, 2002 at 09:09:05AM +0200, Aron Zeh wrote:
>
> My blessings go with the patch, too. I checked and tested the fix on its
> prior submission and it worked fine.
>
> Aron
>
> Vladislav Bolkhovitine wrote:
> >
> > There is an overoptimization in SCSI host ID assigment algorithm in
> 2.4.18
> > and possibly others, which lead to appearance of SCSI hosts with the same
> IDs.
> >
> > Simple scenario:
> > 1. Add one adapter, host_id=0
> > 2. Remove it
> > 3. Add another adapter, its host_id=0
> > 4. Add the adapter 1 again, it reuses its original scsi_host_no_list
> entry
> >         and gets host_id=0 as well. Oops.
> >
> > When the adapter was being unregistered on step 2, max_scsi_hosts gets
> > decremented to 0. During registration of the new host, it received
> > host_id=max_scsi_hosts. On the step 4, the old entry in scsi_host_no_list
> > was found and reused, thus we have two hosts with the same host_id 0.
> >
> > So, it is impossible to use simultaneously scsi_host_no_list and host IDs
> > reusing (i.e. max_scsi_hosts decrementing). I chose to remove the last
> one.
> > Here is the patch against 2.4.18.
> >
> > I was not able to find who is the maintainer of SCSI subsistem at the
> moment.
> > Who is doing so, please consider the idea of this patch to include in the
> > mainstream kernel.
>
>
> Yep, I concur -- this patch works.
>
> I've wanted a patch for this for sometime and brought this same
> issue on 02/05/29, but was too busy with my own project to unscramble
> the infamous SCSI host registration.
>
> AFAIK, Doug is working on this now and it will be a whole different
> and better story for latter 2.5/6 (struct list_head, etc.).
>
> Anyways, this works for 2.4 and I'd be happy to see it in.
>
> Thanks,
> --
> Luben
>
> > Vlad

Re: [PATCH] SCSI host ID assigment overoptimization removal in 2.4.18

[Pete Zaitcev]

> --- linux-2.4.18-enc.orig/drivers/scsi/hosts.c  Wed Jul 17 12:49:51 2002
> +++ linux-2.4.18-enc.register_kmalloc/drivers/scsi/hosts.c      Thu Jul 18 15:47:51 \
> 2002 @@ -174,6 +186,12 @@
>                  return NULL;
>          }
>         shn->name = kmalloc(hname_len + 1, GFP_ATOMIC);
> +       if (!shn->name) {
> +               kfree(retval);
> +               kfree(shn);
> +               printk(KERN_ERR "scsi: out of memory(3) in scsi_register.\n");
> +               return NULL;
> +        }
>         if (hname_len > 0)
>             strncpy(shn->name, hname, hname_len);
>         shn->name[hname_len] = 0;

Does it solve the same problem as the following? It looks like
it does, only in a different place. Which one is better?

--- drivers/scsi/hosts.c.orig   Mon Feb 25 21:38:04 2002
+++ drivers/scsi/hosts.c        Wed Apr 17 01:42:47 2002
@@ -81,8 +81,8 @@
 struct Scsi_Host * scsi_hostlist;
 struct Scsi_Device_Template * scsi_devicelist;

-int max_scsi_hosts;
-int next_scsi_host;
+int max_scsi_hosts;    /* host_no for next new host */
+int next_scsi_host;    /* count of registered scsi hosts */

 void
 scsi_unregister(struct Scsi_Host * sh){
@@ -107,21 +107,8 @@
     if (shn) shn->host_registered = 0;
     /* else {} : This should not happen, we should panic here... */

-    /* If we are removing the last host registered, it is safe to reuse
-     * its host number (this avoids "holes" at boot time) (DB)
-     * It is also safe to reuse those of numbers directly below which have
-     * been released earlier (to avoid some holes in numbering).
-     */
-    if(sh->host_no == max_scsi_hosts - 1) {
-       while(--max_scsi_hosts >= next_scsi_host) {
-           shpnt = scsi_hostlist;
-           while(shpnt && shpnt->host_no != max_scsi_hosts - 1)
-               shpnt = shpnt->next;
-           if(shpnt)
-               break;
-       }
-    }
     next_scsi_host--;
+
     kfree((char *) sh);
 }


See:
http://www.uwsg.indiana.edu/hypermail/linux/kernel/0206.0/1220.html

I poked Marcelo with it for Itai, and he said to wait for 2.4.20
to open. Currently we ship Itai's variant.

-- Pete

Re: [PATCH] SCSI host ID assigment overoptimization removal in 2.4.18

[Itai Nahshon]

This patch is similar to a patch that I sumbitted some time ago.
It is already in the latest ac tree (I think 2.4.19-rc1-ac7 and above).

Marcelo answered that it is too late to get it in 2.4.19 final, and
I should resubmit it for 2.4.20.

For 2.5 - the same bug is stil in 2.5.26. I understand that Doug
is rewriting the scsi mid-level (which is definitely the right thing to do).

-- Itai

On Thursday 18 July 2002 14:22 pm, Vladislav Bolkhovitine wrote:
> There is an overoptimization in SCSI host ID assigment algorithm in 2.4.18
> and possibly others, which lead to appearance of SCSI hosts with the same
> IDs.
>
> Simple scenario:
> 1. Add one adapter, host_id=0
> 2. Remove it
> 3. Add another adapter, its host_id=0
> 4. Add the adapter 1 again, it reuses its original scsi_host_no_list entry
> 	and gets host_id=0 as well. Oops.
>
> When the adapter was being unregistered on step 2, max_scsi_hosts gets
> decremented to 0. During registration of the new host, it received
> host_id=max_scsi_hosts. On the step 4, the old entry in scsi_host_no_list
> was found and reused, thus we have two hosts with the same host_id 0.
>
> So, it is impossible to use simultaneously scsi_host_no_list and host IDs
> reusing (i.e. max_scsi_hosts decrementing). I chose to remove the last one.
> Here is the patch against 2.4.18.
>
> I was not able to find who is the maintainer of SCSI subsistem at the
> moment. Who is doing so, please consider the idea of this patch to include
> in the mainstream kernel.
>
> Vlad
>
> diff -urdN linux-2.4.18.orig/drivers/scsi/hosts.c
> linux-2.4.18.host_fix/drivers/scsi/hosts.c ---
> linux-2.4.18.orig/drivers/scsi/hosts.c      Mon Feb 25 22:38:04 2002 +++
> linux-2.4.18.host_fix/drivers/scsi/hosts.c  Wed Jul 17 17:08:47 2002 @@
> -107,6 +107,17 @@
>      if (shn) shn->host_registered = 0;
>      /* else {} : This should not happen, we should panic here... */
>
> +    /*
> +     * With this algorithm together with scsi_host_no_list entry reusage
> +     * from scsi_register() it is possible really easy to get two
> +     * hosts with the same host_id. Simple scenario:
> +     *  - add one adapter, host_id=0
> +     *  - remove it
> +     *  - add another adapter, its host_id=0
> +     *  - add the adapter 1 again, it reuses its scsi_host_no_list entry
> +     *     and gets host_id=0 as well. Oops.
> +     */
> +#if 0
>      /* If we are removing the last host registered, it is safe to reuse
>       * its host number (this avoids "holes" at boot time) (DB)
>       * It is also safe to reuse those of numbers directly below which have
> @@ -121,6 +132,7 @@
>                 break;
>         }
>      }
> +#endif
> -
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] SCSI host ID assigment overoptimization removal in 2.4.18

[Aron Zeh]

Hi Pete,

as far as I can see these are two completely distinct problem fixes.
The first one addresses the fact that allocation success has to be checked
for shn->name in scsi_register.
The second one deals with the problem of leaving the scsi host numbering in
a weird state upon scsi unregister. (Fixed by simply removing the
interconnected series of bizarre loops).
They should therefore both be included in the kernel as fixes.

Cheers,
Aron

> --- linux-2.4.18-enc.orig/drivers/scsi/hosts.c  Wed Jul 17 12:49:51 2002
> +++ linux-2.4.18-enc.register_kmalloc/drivers/scsi/hosts.c      Thu Jul
18 15:47:51 \
> 2002 @@ -174,6 +186,12 @@
>                  return NULL;
>          }
>         shn->name = kmalloc(hname_len + 1, GFP_ATOMIC);
> +       if (!shn->name) {
> +               kfree(retval);
> +               kfree(shn);
> +               printk(KERN_ERR "scsi: out of memory(3) in scsi_register.
\n");
> +               return NULL;
> +        }
>         if (hname_len > 0)
>             strncpy(shn->name, hname, hname_len);
>         shn->name[hname_len] = 0;

Does it solve the same problem as the following? It looks like
it does, only in a different place. Which one is better?

--- drivers/scsi/hosts.c.orig   Mon Feb 25 21:38:04 2002
+++ drivers/scsi/hosts.c        Wed Apr 17 01:42:47 2002
@@ -81,8 +81,8 @@
 struct Scsi_Host * scsi_hostlist;
 struct Scsi_Device_Template * scsi_devicelist;

-int max_scsi_hosts;
-int next_scsi_host;
+int max_scsi_hosts;    /* host_no for next new host */
+int next_scsi_host;    /* count of registered scsi hosts */

 void
 scsi_unregister(struct Scsi_Host * sh){
@@ -107,21 +107,8 @@
     if (shn) shn->host_registered = 0;
     /* else {} : This should not happen, we should panic here... */

-    /* If we are removing the last host registered, it is safe to reuse
-     * its host number (this avoids "holes" at boot time) (DB)
-     * It is also safe to reuse those of numbers directly below which have
-     * been released earlier (to avoid some holes in numbering).
-     */
-    if(sh->host_no == max_scsi_hosts - 1) {
-       while(--max_scsi_hosts >= next_scsi_host) {
-           shpnt = scsi_hostlist;
-           while(shpnt && shpnt->host_no != max_scsi_hosts - 1)
-               shpnt = shpnt->next;
-           if(shpnt)
-               break;
-       }
-    }
     next_scsi_host--;
+
     kfree((char *) sh);
 }


See:
http://www.uwsg.indiana.edu/hypermail/linux/kernel/0206.0/1220.html

I poked Marcelo with it for Itai, and he said to wait for 2.4.20
to open. Currently we ship Itai's variant.

-- Pete
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html