Poss. bug in tulip driver since 2.4.7

Poss. bug in tulip driver since 2.4.7

[Prasanna Meda]

The inner for loop shown below was not
supposed to be  inside  the  outside  loop.
They  also use  the  same index i.
Due to  this, when mc_count is more than
14,  with non ASIX chips,  panics, corruptions
and  denial of services to multicast addresses
can  result!

http://lxr.linux.no/source/drivers/net/tulip/tulip_core.c#L1055

static void build_setup_frame_hash(u16 *setup_frm, struct net_device
*dev)
{
        struct tulip_private *tp = (struct tulip_private *)dev->priv;
        u16 hash_table[32];
        struct dev_mc_list *mclist;
        int i;
        u16 *eaddrs;

        memset(hash_table, 0, sizeof(hash_table));
        set_bit_le(255, hash_table);                    /* Broadcast
entry */
        /* This should work on big-endian machines as well. */
        for (i = 0, mclist = dev->mc_list; mclist && i < dev->mc_count;
             i++, mclist = mclist->next) {
                int index = ether_crc_le(ETH_ALEN, mclist->dmi_addr) &
0x1ff;

                set_bit_le(index, hash_table);

                for (i = 0; i < 32; i++) {
                        *setup_frm++ = hash_table[i];
                        *setup_frm++ = hash_table[i];
                }
                setup_frm = &tp->setup_frame[13*6];
        }

        /* Fill the final entry with our physical address. */
        eaddrs = (u16 *)dev->dev_addr;
        *setup_frm++ = eaddrs[0]; *setup_frm++ = eaddrs[0];
        *setup_frm++ = eaddrs[1]; *setup_frm++ = eaddrs[1];
        *setup_frm++ = eaddrs[2]; *setup_frm++ = eaddrs[2];
}



Thanks,
Prasanna.

Re: Poss. bug in tulip driver since 2.4.7

[Andrew Morton]

Prasanna Meda <pmeda@akamai.com> wrote:
>
> The inner for loop shown below was not
>  supposed to be  inside  the  outside  loop.
>  They  also use  the  same index i.
>  Due to  this, when mc_count is more than
>  14,  with non ASIX chips,  panics, corruptions
>  and  denial of services to multicast addresses
>  can  result!
> 
>  http://lxr.linux.no/source/drivers/net/tulip/tulip_core.c#L1055

So can you confirm that the driver works correctly with this change?

--- 25/drivers/net/tulip/tulip_core.c~tulip-hash-fix	2003-11-11 18:51:52.000000000 -0800
+++ 25-akpm/drivers/net/tulip/tulip_core.c	2003-11-11 18:52:31.000000000 -0800
@@ -979,12 +979,13 @@ static void build_setup_frame_hash(u16 *
 	for (i = 0, mclist = dev->mc_list; mclist && i < dev->mc_count;
 	     i++, mclist = mclist->next) {
 		int index = ether_crc_le(ETH_ALEN, mclist->dmi_addr) & 0x1ff;
+		int j;
 
 		set_bit_le(index, hash_table);
 
-		for (i = 0; i < 32; i++) {
-			*setup_frm++ = hash_table[i];
-			*setup_frm++ = hash_table[i];
+		for (j = 0; j < 32; j++) {
+			*setup_frm++ = hash_table[j];
+			*setup_frm++ = hash_table[j];
 		}
 		setup_frm = &tp->setup_frame[13*6];
 	}

_

Re: Poss. bug in tulip driver since 2.4.7

[Jeff Garzik]

Andrew Morton wrote:
> Prasanna Meda <pmeda@akamai.com> wrote:
> 
>>The inner for loop shown below was not
>> supposed to be  inside  the  outside  loop.
>> They  also use  the  same index i.
>> Due to  this, when mc_count is more than
>> 14,  with non ASIX chips,  panics, corruptions
>> and  denial of services to multicast addresses
>> can  result!
>>
>> http://lxr.linux.no/source/drivers/net/tulip/tulip_core.c#L1055
> 
> 
> So can you confirm that the driver works correctly with this change?
> 
> --- 25/drivers/net/tulip/tulip_core.c~tulip-hash-fix	2003-11-11 18:51:52.000000000 -0800
> +++ 25-akpm/drivers/net/tulip/tulip_core.c	2003-11-11 18:52:31.000000000 -0800


Patch looks sane to me...

Re: Poss. bug in tulip driver since 2.4.7

[Prasanna Meda]

Andrew Morton wrote:

> Prasanna Meda <pmeda@akamai.com> wrote:
> >
> > The inner for loop shown below was not
> >  supposed to be  inside  the  outside  loop.
> >  They  also use  the  same index i.
> >  Due to  this, when mc_count is more than
> >  14,  with non ASIX chips,  panics, corruptions
> >  and  denial of services to multicast addresses
> >  can  result!
> >
> >  http://lxr.linux.no/source/drivers/net/tulip/tulip_core.c#L1055
>
> So can you confirm that the driver works correctly with this change?
>
> --- 25/drivers/net/tulip/tulip_core.c~tulip-hash-fix    2003-11-11 18:51:52.000000000 -0800
> +++ 25-akpm/drivers/net/tulip/tulip_core.c      2003-11-11 18:52:31.000000000 -0800
> @@ -979,12 +979,13 @@ static void build_setup_frame_hash(u16 *
>         for (i = 0, mclist = dev->mc_list; mclist && i < dev->mc_count;
>              i++, mclist = mclist->next) {
>                 int index = ether_crc_le(ETH_ALEN, mclist->dmi_addr) & 0x1ff;
> +               int j;
>
>                 set_bit_le(index, hash_table);
>
> -               for (i = 0; i < 32; i++) {
> -                       *setup_frm++ = hash_table[i];
> -                       *setup_frm++ = hash_table[i];
> +               for (j = 0; j < 32; j++) {
> +                       *setup_frm++ = hash_table[j];
> +                       *setup_frm++ = hash_table[j];
>                 }
>                 setup_frm = &tp->setup_frame[13*6];
>         }

No,  you need to bring the for loop outside the loop.
 - Otherwise we need to reset the setup_frame to
tp->setup_frame after every loop.
 - You do not need to set the setup_frm for every
mc address, we can set once after the complete
has_table is ready.
And also the  2.4 code missed a bit in tx_flags.

We did the following change that makes it identical
to 2.2 tulip driver, and it worked.

--- Linux/drivers/net/tulip/tulip_core.c  Fri Oct 10 20:22:29 2003
+++ linux/drivers/net/tulip/tulip_core.c        Fri Oct 10 20:28:19 2003
@@ -1054,19 +1054,19 @@

        memset(hash_table, 0, sizeof(hash_table));
        set_bit_le(255, hash_table);                    /* Broadcast entry */

        /* This should work on big-endian machines as well. */
       for (i = 0, mclist = dev->mc_list; mclist && i < dev->mc_count;
             i++, mclist = mclist->next) {
                int index = ether_crc_le(ETH_ALEN, mclist->dmi_addr) & 0x1ff;

                set_bit_le(index, hash_table);
+       }

-               for (i = 0; i < 32; i++) {
-                       *setup_frm++ = hash_table[i];
-                       *setup_frm++ = hash_table[i];
-               }
-               setup_frm = &tp->setup_frame[13*6];
+       for (i = 0; i < 32; i++) {
+               *setup_frm++ = hash_table[i];
+               *setup_frm++ = hash_table[i];
        }
+       setup_frm = &tp->setup_frame[13*6];

        /* Fill the final entry with our physical address. */
        eaddrs = (u16 *)dev->dev_addr;
@@ -1166,11 +1168,13 @@
                }
        } else {
                unsigned long flags;
+               u32 tx_flags = 0x08000000 | 192;

                /* Note that only the low-address shortword of setup_frame is valid!
                   The values are doubled for big-endian architectures. */
                if (dev->mc_count > 14) { /* Must use a multicast hash table. */
                        build_setup_frame_hash(tp->setup_frame, dev);
+                       tx_flags = 0x08400000 | 192;
                } else {
                        build_setup_frame_perfect(tp->setup_frame, dev);
                }
@@ -1180,7 +1184,6 @@
                if (tp->cur_tx - tp->dirty_tx > TX_RING_SIZE - 2) {
                        /* Same setup recently queued, we need not add it. */
                } else {
-                       u32 tx_flags = 0x08000000 | 192;
                        unsigned int entry;
                        int dummy = -1;

Re: Poss. bug in tulip driver since 2.4.7

[Jeff Garzik]

Prasanna Meda wrote:
> No,  you need to bring the for loop outside the loop.
>  - Otherwise we need to reset the setup_frame to
> tp->setup_frame after every loop.
>  - You do not need to set the setup_frm for every
> mc address, we can set once after the complete
> has_table is ready.
> And also the  2.4 code missed a bit in tx_flags.
> 
> We did the following change that makes it identical
> to 2.2 tulip driver, and it worked.
> 
> --- Linux/drivers/net/tulip/tulip_core.c  Fri Oct 10 20:22:29 2003
> +++ linux/drivers/net/tulip/tulip_core.c        Fri Oct 10 20:28:19 2003


Yeah, that looks better...  I'll give it a quick test locally, then push 
it to Linus.

	Jeff