[ECOS] Possible fix for duplicated ARP entries in the FreeBSD stack

[ECOS] Possible fix for duplicated ARP entries in the FreeBSD stack

[Sturle Mastberg]

Hello,

For some time I've had problem with duplicated ARP entries that have 
caused all sorts of problems. I searched the archive and discovered that 
the problem had been reported before:
 

http://sourceware.org/ml/ecos-discuss/2004-11/msg00097.html
 

My proposal to a fix is to make the sockaddr_inarp struct 
(include/netinet/if_ether.h) equal in size to the sockaddr struct by 
padding it at the end. This is exactly what is done to the sockaddr_in 
struct (include/netinet/in.h) for different reasons.
 

I reached this conclusion after I discovered that two virutally 
identical calls to rtalloc1 (net/route.c) returned different results. 
The first instance appears in arplookup (netinet/if_ether.c) where the 
first parameter to rtalloc1 is a struct sockaddr_inarp cast to a struct 
sockaddr. The second instance appears in ip_output (netinet/ip_output) 
via rtalloc_ign (net/route.c) where the first parameter to rtalloc1 is 
an actual struct sockaddr. The rtalloc1 function does a radix tree 
search with a call to the rn_match function (net/radix.c). A closer look 
at this code reveals that it does indeed depend on the size of the 
supplied struct.
 

The only conclusion a can draw from this is that the three structs: 
sockaddr, sockaddr_in and sockaddr_inarp must all be of equal size. I 
have checked the FreeBSD source repository that this is the case for the 
original code.
 

While browsing the FreeBSD source repository I discovered that the 
sa_data character array member of the sockaddr struct was increased in 
size in the eCos FreeBSD stack. Does anyone know why this increase was 
introduced in eCos?

Regards,
SM

-- 
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss

Re: [ECOS] Possible fix for duplicated ARP entries in the FreeBSD stack

[Gary Thomas]

On Fri, 2005-06-17 at 10:52 +0200, Sturle Mastberg wrote:
> Hello,
> 
> For some time I've had problem with duplicated ARP entries that have 
> caused all sorts of problems. I searched the archive and discovered that 
> the problem had been reported before:
>  
> 
> http://sourceware.org/ml/ecos-discuss/2004-11/msg00097.html
>  
> 
> My proposal to a fix is to make the sockaddr_inarp struct 
> (include/netinet/if_ether.h) equal in size to the sockaddr struct by 
> padding it at the end. This is exactly what is done to the sockaddr_in 
> struct (include/netinet/in.h) for different reasons.
>  
> 
> I reached this conclusion after I discovered that two virutally 
> identical calls to rtalloc1 (net/route.c) returned different results. 
> The first instance appears in arplookup (netinet/if_ether.c) where the 
> first parameter to rtalloc1 is a struct sockaddr_inarp cast to a struct 
> sockaddr. The second instance appears in ip_output (netinet/ip_output) 
> via rtalloc_ign (net/route.c) where the first parameter to rtalloc1 is 
> an actual struct sockaddr. The rtalloc1 function does a radix tree 
> search with a call to the rn_match function (net/radix.c). A closer look 
> at this code reveals that it does indeed depend on the size of the 
> supplied struct.
>  
> 
> The only conclusion a can draw from this is that the three structs: 
> sockaddr, sockaddr_in and sockaddr_inarp must all be of equal size. I 
> have checked the FreeBSD source repository that this is the case for the 
> original code.
>  
> 
> While browsing the FreeBSD source repository I discovered that the 
> sa_data character array member of the sockaddr struct was increased in 
> size in the eCos FreeBSD stack. Does anyone know why this increase was 
> introduced in eCos?

Can you provide the details of what you found?  i.e. exactly how
these structures were modified during the port?  [more likely, you're
looking at a newer version of the FreeBSD code than I used and the 
changes happened in the BSD codebase]  In any case, then we can analyze
how things are different and what might need to be done.

Thanks


-- 
------------------------------------------------------------
Gary Thomas                 |  Consulting for the
MLB Associates              |    Embedded world
------------------------------------------------------------


-- 
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss

Re: [ECOS] Possible fix for duplicated ARP entries in the FreeBSD stack

[Sturle Mastberg]

Gary Thomas wrote:
> On Fri, 2005-06-17 at 10:52 +0200, Sturle Mastberg wrote:
> 
>>Hello,
>>
>>For some time I've had problem with duplicated ARP entries that have 
>>caused all sorts of problems. I searched the archive and discovered that 
>>the problem had been reported before:
>> 
>>
>>http://sourceware.org/ml/ecos-discuss/2004-11/msg00097.html
>> 
>>
>>My proposal to a fix is to make the sockaddr_inarp struct 
>>(include/netinet/if_ether.h) equal in size to the sockaddr struct by 
>>padding it at the end. This is exactly what is done to the sockaddr_in 
>>struct (include/netinet/in.h) for different reasons.
>> 
>>
>>I reached this conclusion after I discovered that two virutally 
>>identical calls to rtalloc1 (net/route.c) returned different results. 
>>The first instance appears in arplookup (netinet/if_ether.c) where the 
>>first parameter to rtalloc1 is a struct sockaddr_inarp cast to a struct 
>>sockaddr. The second instance appears in ip_output (netinet/ip_output) 
>>via rtalloc_ign (net/route.c) where the first parameter to rtalloc1 is 
>>an actual struct sockaddr. The rtalloc1 function does a radix tree 
>>search with a call to the rn_match function (net/radix.c). A closer look 
>>at this code reveals that it does indeed depend on the size of the 
>>supplied struct.
>> 
>>
>>The only conclusion a can draw from this is that the three structs: 
>>sockaddr, sockaddr_in and sockaddr_inarp must all be of equal size. I 
>>have checked the FreeBSD source repository that this is the case for the 
>>original code.
>> 
>>
>>While browsing the FreeBSD source repository I discovered that the 
>>sa_data character array member of the sockaddr struct was increased in 
>>size in the eCos FreeBSD stack. Does anyone know why this increase was 
>>introduced in eCos?
> 
> 
> Can you provide the details of what you found?  i.e. exactly how
> these structures were modified during the port?  [more likely, you're
> looking at a newer version of the FreeBSD code than I used and the 
> changes happened in the BSD codebase]  In any case, then we can analyze
> how things are different and what might need to be done.
> 
> Thanks
> 

The sockaddr struct in socket.h v1.39.2.7 in FreeBSD (which is the 
version the eCos port is based on) looks like this:

struct sockaddr {
   u_char       sa_len;      /* total length */
   sa_family_t  sa_family;   /* address family */
   char         sa_data[14]; /* actually longer; address value */
};

This structure has remained unchanged in FreeBSD from first to current 
version. I suspect the somehwat curious size of sa_data is due to the 
fact that the size of this struct must match that of the sockaddr_inarp 
struct. Here is the whole revision history:

http://www.freebsd.org/cgi/cvsweb.cgi/src/sys/sys/socket.h

The first version of socket.h in the eCos source repository looks like this:

struct sockaddr {
   u_char       sa_len;      /* total length */
   sa_family_t  sa_family;   /* address family */
   char         sa_data[30]; /* actually longer; address value */
};

The sa_data member has increased 16 bytes in size for no apparent reason 
and hench the reason for my question.

This size increase will also likely lead to a performance degradation, 
since the radix tree search algorithm does a byte by byte comparison for 
an exact match whenever an ARP cache entry is looked up (which is for 
every packet that is sent). This comparison, as mentioned earlier, 
depends on the size of the supplied struct (sockaddr[_in[arp]]) and now 
it has to deal with a struct that is twice the size is once was.

Regards,
SM

-- 
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss

Re: [ECOS] Possible fix for duplicated ARP entries in the FreeBSD stack

[Andrew Lunn]

On Fri, Jun 17, 2005 at 12:57:49PM +0200, Sturle Mastberg wrote:
> Gary Thomas wrote:
> >On Fri, 2005-06-17 at 10:52 +0200, Sturle Mastberg wrote:
> >
> >>Hello,
> >>
> >>For some time I've had problem with duplicated ARP entries that have 
> >>caused all sorts of problems. I searched the archive and discovered that 
> >>the problem had been reported before:
> >>
> >>
> >>http://sourceware.org/ml/ecos-discuss/2004-11/msg00097.html
> >>
> >>
> >>My proposal to a fix is to make the sockaddr_inarp struct 
> >>(include/netinet/if_ether.h) equal in size to the sockaddr struct by 
> >>padding it at the end. This is exactly what is done to the sockaddr_in 
> >>struct (include/netinet/in.h) for different reasons.
> >>
> >>
> >>I reached this conclusion after I discovered that two virutally 
> >>identical calls to rtalloc1 (net/route.c) returned different results. 
> >>The first instance appears in arplookup (netinet/if_ether.c) where the 
> >>first parameter to rtalloc1 is a struct sockaddr_inarp cast to a struct 
> >>sockaddr. The second instance appears in ip_output (netinet/ip_output) 
> >>via rtalloc_ign (net/route.c) where the first parameter to rtalloc1 is 
> >>an actual struct sockaddr. The rtalloc1 function does a radix tree 
> >>search with a call to the rn_match function (net/radix.c). A closer look 
> >>at this code reveals that it does indeed depend on the size of the 
> >>supplied struct.
> >>
> >>
> >>The only conclusion a can draw from this is that the three structs: 
> >>sockaddr, sockaddr_in and sockaddr_inarp must all be of equal size. I 
> >>have checked the FreeBSD source repository that this is the case for the 
> >>original code.
> >>
> >>
> >>While browsing the FreeBSD source repository I discovered that the 
> >>sa_data character array member of the sockaddr struct was increased in 
> >>size in the eCos FreeBSD stack. Does anyone know why this increase was 
> >>introduced in eCos?

I beleave Nick did this for IPv6. This bigger size allows an IPv6
address to be placed into a sockaddr which you cannot normally do.

        Andrew

-- 
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss

Re: [ECOS] Possible fix for duplicated ARP entries in the FreeBSD stack

[Nick Garnett]

Andrew Lunn <andrew@lunn.ch> writes:

> On Fri, Jun 17, 2005 at 12:57:49PM +0200, Sturle Mastberg wrote:
> > Gary Thomas wrote:
> > >On Fri, 2005-06-17 at 10:52 +0200, Sturle Mastberg wrote:
> > >
> > >>Hello,
> > >>
> > >>For some time I've had problem with duplicated ARP entries that have 
> > >>caused all sorts of problems. I searched the archive and discovered that 
> > >>the problem had been reported before:
> > >>
> > >>
> > >>http://sourceware.org/ml/ecos-discuss/2004-11/msg00097.html
> > >>
> > >>
> > >>My proposal to a fix is to make the sockaddr_inarp struct 
> > >>(include/netinet/if_ether.h) equal in size to the sockaddr struct by 
> > >>padding it at the end. This is exactly what is done to the sockaddr_in 
> > >>struct (include/netinet/in.h) for different reasons.
> > >>
> > >>
> > >>I reached this conclusion after I discovered that two virutally 
> > >>identical calls to rtalloc1 (net/route.c) returned different results. 
> > >>The first instance appears in arplookup (netinet/if_ether.c) where the 
> > >>first parameter to rtalloc1 is a struct sockaddr_inarp cast to a struct 
> > >>sockaddr. The second instance appears in ip_output (netinet/ip_output) 
> > >>via rtalloc_ign (net/route.c) where the first parameter to rtalloc1 is 
> > >>an actual struct sockaddr. The rtalloc1 function does a radix tree 
> > >>search with a call to the rn_match function (net/radix.c). A closer look 
> > >>at this code reveals that it does indeed depend on the size of the 
> > >>supplied struct.
> > >>
> > >>
> > >>The only conclusion a can draw from this is that the three structs: 
> > >>sockaddr, sockaddr_in and sockaddr_inarp must all be of equal size. I 
> > >>have checked the FreeBSD source repository that this is the case for the 
> > >>original code.
> > >>
> > >>
> > >>While browsing the FreeBSD source repository I discovered that the 
> > >>sa_data character array member of the sockaddr struct was increased in 
> > >>size in the eCos FreeBSD stack. Does anyone know why this increase was 
> > >>introduced in eCos?
> 
> I beleave Nick did this for IPv6. This bigger size allows an IPv6
> address to be placed into a sockaddr which you cannot normally do.

I thought that initially and had a look. The change I made was to
struct sockaddr_in to make it match the increased sockaddr size so
that some punned data structures would match. I think the change to
struct sockaddr was done by Gary during the initial port to make IPv6
addresses fit correctly. I'm sure it all has something to do with the
lack of user/kernel space and the fact that we don't copy things to
and fro as much as the BSD kernel does.


-- 
Nick Garnett                                     eCos Kernel Architect
http://www.ecoscentric.com                The eCos and RedBoot experts


-- 
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss

Re: [ECOS] Possible fix for duplicated ARP entries in the FreeBSD stack

[Gary Thomas]

On Fri, 2005-06-17 at 13:53 +0100, Nick Garnett wrote:
> Andrew Lunn <andrew@lunn.ch> writes:
> 
> > On Fri, Jun 17, 2005 at 12:57:49PM +0200, Sturle Mastberg wrote:
> > > Gary Thomas wrote:
> > > >On Fri, 2005-06-17 at 10:52 +0200, Sturle Mastberg wrote:
> > > >
> > > >>Hello,
> > > >>
> > > >>For some time I've had problem with duplicated ARP entries that have 
> > > >>caused all sorts of problems. I searched the archive and discovered that 
> > > >>the problem had been reported before:
> > > >>
> > > >>
> > > >>http://sourceware.org/ml/ecos-discuss/2004-11/msg00097.html
> > > >>
> > > >>
> > > >>My proposal to a fix is to make the sockaddr_inarp struct 
> > > >>(include/netinet/if_ether.h) equal in size to the sockaddr struct by 
> > > >>padding it at the end. This is exactly what is done to the sockaddr_in 
> > > >>struct (include/netinet/in.h) for different reasons.
> > > >>
> > > >>
> > > >>I reached this conclusion after I discovered that two virutally 
> > > >>identical calls to rtalloc1 (net/route.c) returned different results. 
> > > >>The first instance appears in arplookup (netinet/if_ether.c) where the 
> > > >>first parameter to rtalloc1 is a struct sockaddr_inarp cast to a struct 
> > > >>sockaddr. The second instance appears in ip_output (netinet/ip_output) 
> > > >>via rtalloc_ign (net/route.c) where the first parameter to rtalloc1 is 
> > > >>an actual struct sockaddr. The rtalloc1 function does a radix tree 
> > > >>search with a call to the rn_match function (net/radix.c). A closer look 
> > > >>at this code reveals that it does indeed depend on the size of the 
> > > >>supplied struct.
> > > >>
> > > >>
> > > >>The only conclusion a can draw from this is that the three structs: 
> > > >>sockaddr, sockaddr_in and sockaddr_inarp must all be of equal size. I 
> > > >>have checked the FreeBSD source repository that this is the case for the 
> > > >>original code.
> > > >>
> > > >>
> > > >>While browsing the FreeBSD source repository I discovered that the 
> > > >>sa_data character array member of the sockaddr struct was increased in 
> > > >>size in the eCos FreeBSD stack. Does anyone know why this increase was 
> > > >>introduced in eCos?
> > 
> > I beleave Nick did this for IPv6. This bigger size allows an IPv6
> > address to be placed into a sockaddr which you cannot normally do.
> 
> I thought that initially and had a look. The change I made was to
> struct sockaddr_in to make it match the increased sockaddr size so
> that some punned data structures would match. I think the change to
> struct sockaddr was done by Gary during the initial port to make IPv6
> addresses fit correctly. I'm sure it all has something to do with the
> lack of user/kernel space and the fact that we don't copy things to
> and fro as much as the BSD kernel does.

This is as I remember it as well.  Note the comment in the structure
(which I did not add or change) that bodes of this inconsistency.

Of course, if someone has a better way to handle it, feel free to
post a patch :-)

-- 
------------------------------------------------------------
Gary Thomas                 |  Consulting for the
MLB Associates              |    Embedded world
------------------------------------------------------------


-- 
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss

[ECOS] RE : [ECOS] Possible fix for duplicated ARP entries in the FreeBSDstack

[Arnaud Chataignier]

Thank you all for the analysis. I was encountering this bug from a long
time now, and fixing it is great news for me.

One person did post a temporary patch to this problem a long time ago,
but it didn't seem to really correct the cause of the problem, it was
just avoiding it :
http://sourceware.org/ml/ecos-discuss/2004-11/msg00150.html

May anyone post a patch corresponding to this real correction ? As far
as I understand, it is only a matter of changing the size of the sa_data
member back to 14 ?

Thank you,
Arnaud.

-----Message d'origine-----
De : ecos-discuss-owner@ecos.sourceware.org
[mailto:ecos-discuss-owner@ecos.sourceware.org] De la part de Gary
Thomas
Envoyé : vendredi 17 juin 2005 14:57
À : Nick Garnett
Cc : Andrew Lunn; Sturle Mastberg; eCos Discussion
Objet : Re: [ECOS] Possible fix for duplicated ARP entries in the
FreeBSDstack


On Fri, 2005-06-17 at 13:53 +0100, Nick Garnett wrote:
> Andrew Lunn <andrew@lunn.ch> writes:
> 
> > On Fri, Jun 17, 2005 at 12:57:49PM +0200, Sturle Mastberg wrote:
> > > Gary Thomas wrote:
> > > >On Fri, 2005-06-17 at 10:52 +0200, Sturle Mastberg wrote:
> > > >
> > > >>Hello,
> > > >>
> > > >>For some time I've had problem with duplicated ARP entries that 
> > > >>have
> > > >>caused all sorts of problems. I searched the archive and
discovered that 
> > > >>the problem had been reported before:
> > > >>
> > > >>
> > > >>http://sourceware.org/ml/ecos-discuss/2004-11/msg00097.html
> > > >>
> > > >>
> > > >>My proposal to a fix is to make the sockaddr_inarp struct
> > > >>(include/netinet/if_ether.h) equal in size to the sockaddr
struct by 
> > > >>padding it at the end. This is exactly what is done to the
sockaddr_in 
> > > >>struct (include/netinet/in.h) for different reasons.
> > > >>
> > > >>
> > > >>I reached this conclusion after I discovered that two virutally
> > > >>identical calls to rtalloc1 (net/route.c) returned different
results. 
> > > >>The first instance appears in arplookup (netinet/if_ether.c)
where the 
> > > >>first parameter to rtalloc1 is a struct sockaddr_inarp cast to a
struct 
> > > >>sockaddr. The second instance appears in ip_output
(netinet/ip_output) 
> > > >>via rtalloc_ign (net/route.c) where the first parameter to
rtalloc1 is 
> > > >>an actual struct sockaddr. The rtalloc1 function does a radix
tree 
> > > >>search with a call to the rn_match function (net/radix.c). A
closer look 
> > > >>at this code reveals that it does indeed depend on the size of
the 
> > > >>supplied struct.
> > > >>
> > > >>
> > > >>The only conclusion a can draw from this is that the three 
> > > >>structs:
> > > >>sockaddr, sockaddr_in and sockaddr_inarp must all be of equal
size. I 
> > > >>have checked the FreeBSD source repository that this is the case
for the 
> > > >>original code.
> > > >>
> > > >>
> > > >>While browsing the FreeBSD source repository I discovered that 
> > > >>the
> > > >>sa_data character array member of the sockaddr struct was
increased in 
> > > >>size in the eCos FreeBSD stack. Does anyone know why this
increase was 
> > > >>introduced in eCos?
> > 
> > I beleave Nick did this for IPv6. This bigger size allows an IPv6 
> > address to be placed into a sockaddr which you cannot normally do.
> 
> I thought that initially and had a look. The change I made was to 
> struct sockaddr_in to make it match the increased sockaddr size so 
> that some punned data structures would match. I think the change to 
> struct sockaddr was done by Gary during the initial port to make IPv6 
> addresses fit correctly. I'm sure it all has something to do with the 
> lack of user/kernel space and the fact that we don't copy things to 
> and fro as much as the BSD kernel does.

This is as I remember it as well.  Note the comment in the structure
(which I did not add or change) that bodes of this inconsistency.

Of course, if someone has a better way to handle it, feel free to post a
patch :-)

-- 
------------------------------------------------------------
Gary Thomas                 |  Consulting for the
MLB Associates              |    Embedded world
------------------------------------------------------------


-- 
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss


--
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss

[ECOS] Re: RE : [ECOS] Possible fix for duplicated ARP entries in the FreeBSDstack

[Sturle Mastberg]

Arnaud Chataignier wrote:
> Thank you all for the analysis. I was encountering this bug from a long
> time now, and fixing it is great news for me.
> 
> One person did post a temporary patch to this problem a long time ago,
> but it didn't seem to really correct the cause of the problem, it was
> just avoiding it :
> http://sourceware.org/ml/ecos-discuss/2004-11/msg00150.html
> 
> May anyone post a patch corresponding to this real correction ? As far
> as I understand, it is only a matter of changing the size of the sa_data
> member back to 14 ?

No, the increase in size of the sa_data member was apparently needed for 
IPv6. The fix is as I stated first: the sockaddr_inarp struct must be 
padded to match the size of the sockaddr struct. This can be done by 
adding a sin_zero member at the end of this struct, defined in 
include/netinet/if_ether.h. Like this:

struct sockaddr_inarp {
   u_char  sin_len;
   u_char  sin_family;
   u_short sin_port;
   struct  in_addr sin_addr;
   struct  in_addr sin_srcaddr;
   u_short sin_tos;
   u_short sin_other;
   char    sin_zero[16];
#define SIN_PROXY 1
};

Regards,
SM

-- 
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss

Re: [ECOS] RE : [ECOS] Possible fix for duplicated ARP entries in the FreeBSDstack

[Nick Garnett]

"Arnaud Chataignier" <achataignier@neotion.com> writes:

> Thank you all for the analysis. I was encountering this bug from a long
> time now, and fixing it is great news for me.
> 
> One person did post a temporary patch to this problem a long time ago,
> but it didn't seem to really correct the cause of the problem, it was
> just avoiding it :
> http://sourceware.org/ml/ecos-discuss/2004-11/msg00150.html
> 
> May anyone post a patch corresponding to this real correction ? As far
> as I understand, it is only a matter of changing the size of the sa_data
> member back to 14 ?

I think the analysis indicates that the sa_data field should not be
changed back to 14, that would break IPv6 support. I suspect the
correct solution is to increase sockaddr_inarp to match sockaddr and
sockaddr_in. However, this code is Gary's baby, so he should have the
last say.

The root cause of this is some rather questionable assumptions in the
BSD stack about equivalence of sizes of various structures. These
probably don't have any effect in BSD since the copying between user
and kernel space masks problem.

-- 
Nick Garnett                                     eCos Kernel Architect
http://www.ecoscentric.com                The eCos and RedBoot experts


-- 
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss