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.
Prasanna Meda <[email protected]> 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];
}
_
Andrew Morton wrote:
> Prasanna Meda <[email protected]> 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...
Andrew Morton wrote:
> Prasanna Meda <[email protected]> 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;
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