* [Bridge] Bridge sysfs port_no overflow
@ 2008-03-30 15:09 Osama Abu Elsorour
0 siblings, 0 replies; 4+ messages in thread
From: Osama Abu Elsorour @ 2008-03-30 15:09 UTC (permalink / raw)
To: bridge
All
We are running a setup with a large number of bridge ports that
reaches the 900 ports. After switching to recent kernel and brctl-
utils that uses the sysfs interface, we started noticing that the port
numbers are mis-reported when issues the command:
brctl showmacs br1
After tracing the code, we found that the problem lies in the sysfs
structure called __fdb_entry. The port_no is declared as a u8 while it
is u16 in the rest of the bridge structure. This causes the port_no to
overflow when the bridge port number exceeds 255.
Even if it is unusual to have this number of ports on a single bridge
it should be changed to the sake of consistency.
This patch changes the structure to __u16:
@@ -94,7 +94,7 @@ struct __port_info
struct __fdb_entry
{
__u8 mac_addr[6];
- __u8 port_no;
+ __u16 port_no;
__u8 is_local;
__u32 ageing_timer_value;
__u32 unused;
^ permalink raw reply [flat|nested] 4+ messages in thread
* [Bridge] Bridge sysfs port_no overflow
@ 2008-03-31 7:11 Osama Abu Elsorour
2008-03-31 15:42 ` Stephen Hemminger
0 siblings, 1 reply; 4+ messages in thread
From: Osama Abu Elsorour @ 2008-03-31 7:11 UTC (permalink / raw)
To: bridge
All
We are running a setup with a large number of bridge ports that
reaches the 900 ports. After switching to recent kernel and brctl-
utils that uses the sysfs interface, we started noticing that the port
numbers are mis-reported when issues the command:
brctl showmacs br1
After tracing the code, we found that the problem lies in the sysfs
structure called __fdb_entry. The port_no is declared as a u8 while it
is u16 in the rest of the bridge structure. This causes the port_no to
overflow when the bridge port number exceeds 255.
The overflow line is in file br_fdb.c function br_fdb_fillbuf:
fe->port_no = f->dst->port_no;
where left hand port_no is _u8 and right hand is _u16.
Even if it is unusual to have this number of ports on a single bridge
it should be changed to the sake of consistency.
This patch shows the change:
@@ -94,7 +94,7 @@ struct __port_info
struct __fdb_entry
{
__u8 mac_addr[6];
- __u8 port_no;
+ __u16 port_no;
__u8 is_local;
__u32 ageing_timer_value;
__u32 unused;
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Bridge] Bridge sysfs port_no overflow
2008-03-31 7:11 [Bridge] Bridge sysfs port_no overflow Osama Abu Elsorour
@ 2008-03-31 15:42 ` Stephen Hemminger
2008-04-01 7:30 ` Osama Abu Elsorour
0 siblings, 1 reply; 4+ messages in thread
From: Stephen Hemminger @ 2008-03-31 15:42 UTC (permalink / raw)
To: Osama Abu Elsorour; +Cc: bridge
On Mon, 31 Mar 2008 09:11:31 +0200
Osama Abu Elsorour <fobowise@gmail.com> wrote:
> All
>
> We are running a setup with a large number of bridge ports that
> reaches the 900 ports. After switching to recent kernel and brctl-
> utils that uses the sysfs interface, we started noticing that the port
> numbers are mis-reported when issues the command:
> brctl showmacs br1
> After tracing the code, we found that the problem lies in the sysfs
> structure called __fdb_entry. The port_no is declared as a u8 while it
> is u16 in the rest of the bridge structure. This causes the port_no to
> overflow when the bridge port number exceeds 255.
>
> The overflow line is in file br_fdb.c function br_fdb_fillbuf:
> fe->port_no = f->dst->port_no;
> where left hand port_no is _u8 and right hand is _u16.
>
> Even if it is unusual to have this number of ports on a single bridge
> it should be changed to the sake of consistency.
>
> This patch shows the change:
>
> @@ -94,7 +94,7 @@ struct __port_info
> struct __fdb_entry
> {
> __u8 mac_addr[6];
> - __u8 port_no;
> + __u16 port_no;
> __u8 is_local;
> __u32 ageing_timer_value;
> __u32 unused;
The problem is that this changes the size of the binary data structure
and therefore changes the API. Better to do something with the unused
field and maintain binary compatibility.
Like this:
--- a/include/linux/if_bridge.h 2008-03-31 08:37:57.000000000 -0700
+++ b/include/linux/if_bridge.h 2008-03-31 08:39:02.000000000 -0700
@@ -94,10 +94,11 @@ struct __port_info
struct __fdb_entry
{
__u8 mac_addr[6];
- __u8 port_no;
+ __u8 old_port_no;
__u8 is_local;
__u32 ageing_timer_value;
- __u32 unused;
+ __u16 port_no;
+ __u16 unused;
};
#ifdef __KERNEL__
--- a/net/bridge/br_fdb.c 2008-03-31 08:39:23.000000000 -0700
+++ b/net/bridge/br_fdb.c 2008-03-31 08:41:32.000000000 -0700
@@ -285,6 +285,7 @@ int br_fdb_fillbuf(struct net_bridge *br
/* convert from internal format to API */
memcpy(fe->mac_addr, f->addr.addr, ETH_ALEN);
+ fe->old_port_no = f->dst->port_no;
fe->port_no = f->dst->port_no;
fe->is_local = f->is_local;
if (!f->is_static)
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Bridge] Bridge sysfs port_no overflow
2008-03-31 15:42 ` Stephen Hemminger
@ 2008-04-01 7:30 ` Osama Abu Elsorour
0 siblings, 0 replies; 4+ messages in thread
From: Osama Abu Elsorour @ 2008-04-01 7:30 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: bridge
Yes. I guess the unused field came in handy. Thanks!
On Mar 31, 2008, at 5:42 PM, Stephen Hemminger wrote:
> On Mon, 31 Mar 2008 09:11:31 +0200
> Osama Abu Elsorour <fobowise@gmail.com> wrote:
>
>> All
>>
>> We are running a setup with a large number of bridge ports that
>> reaches the 900 ports. After switching to recent kernel and brctl-
>> utils that uses the sysfs interface, we started noticing that the
>> port
>> numbers are mis-reported when issues the command:
>> brctl showmacs br1
>> After tracing the code, we found that the problem lies in the sysfs
>> structure called __fdb_entry. The port_no is declared as a u8 while
>> it
>> is u16 in the rest of the bridge structure. This causes the port_no
>> to
>> overflow when the bridge port number exceeds 255.
>>
>> The overflow line is in file br_fdb.c function br_fdb_fillbuf:
>> fe->port_no = f->dst->port_no;
>> where left hand port_no is _u8 and right hand is _u16.
>>
>> Even if it is unusual to have this number of ports on a single bridge
>> it should be changed to the sake of consistency.
>>
>> This patch shows the change:
>>
>> @@ -94,7 +94,7 @@ struct __port_info
>> struct __fdb_entry
>> {
>> __u8 mac_addr[6];
>> - __u8 port_no;
>> + __u16 port_no;
>> __u8 is_local;
>> __u32 ageing_timer_value;
>> __u32 unused;
>
> The problem is that this changes the size of the binary data structure
> and therefore changes the API. Better to do something with the unused
> field and maintain binary compatibility.
>
> Like this:
>
> --- a/include/linux/if_bridge.h 2008-03-31 08:37:57.000000000 -0700
> +++ b/include/linux/if_bridge.h 2008-03-31 08:39:02.000000000 -0700
> @@ -94,10 +94,11 @@ struct __port_info
> struct __fdb_entry
> {
> __u8 mac_addr[6];
> - __u8 port_no;
> + __u8 old_port_no;
> __u8 is_local;
> __u32 ageing_timer_value;
> - __u32 unused;
> + __u16 port_no;
> + __u16 unused;
> };
>
> #ifdef __KERNEL__
> --- a/net/bridge/br_fdb.c 2008-03-31 08:39:23.000000000 -0700
> +++ b/net/bridge/br_fdb.c 2008-03-31 08:41:32.000000000 -0700
> @@ -285,6 +285,7 @@ int br_fdb_fillbuf(struct net_bridge *br
>
> /* convert from internal format to API */
> memcpy(fe->mac_addr, f->addr.addr, ETH_ALEN);
> + fe->old_port_no = f->dst->port_no;
> fe->port_no = f->dst->port_no;
> fe->is_local = f->is_local;
> if (!f->is_static)
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2008-04-01 7:30 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-03-31 7:11 [Bridge] Bridge sysfs port_no overflow Osama Abu Elsorour
2008-03-31 15:42 ` Stephen Hemminger
2008-04-01 7:30 ` Osama Abu Elsorour
-- strict thread matches above, loose matches on Subject: below --
2008-03-30 15:09 Osama Abu Elsorour
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox