[Bridge] Bridge sysfs port_no overflow

[Bridge] Bridge sysfs port_no overflow

[Osama Abu Elsorour]

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;

Re: [Bridge] Bridge sysfs port_no overflow

[Stephen Hemminger]

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)

Re: [Bridge] Bridge sysfs port_no overflow

[Osama Abu Elsorour]

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)

[Bridge] Bridge sysfs port_no overflow

[Osama Abu Elsorour]

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;