crossbow discuss - Jan 2009 - Crossbow requires MAC addresses to be > 6 bytes

I''m in the process of doing the Clearview iptun and Crossbow merge, and
one problem I''m having is that mac_datapath_setup() fails in
mac_flow_add() -> flow_l2_accept_fe() which contains the following:

        if (i == fd->fd_mac_len || fd->fd_mac_len < ETHERADDRL)
                return (EINVAL);

IPv4 tunnel "MAC" addresses are 4 bytes, and so this doesn''t
work.  What
is the intent of this code?  How can this be made to work for MAC
drivers with addresses < 6 bytes?  As you''re probably aware, there
are
MACs that have no MAC addresses (e.g., PPP links), and the framework
needs to eventually work for those as well.

-Seb

On Thu, Jan 08, 2009 at 12:07:11AM -0500, Sebastien Roy
wrote:
> I''m in the process of doing the Clearview iptun and Crossbow
merge, and
> one problem I''m having is that mac_datapath_setup() fails in
> mac_flow_add() -> flow_l2_accept_fe() which contains the following:
> 
>         if (i == fd->fd_mac_len || fd->fd_mac_len < ETHERADDRL)
>                 return (EINVAL);
> 
> IPv4 tunnel "MAC" addresses are 4 bytes, and so this
doesn''t work.  What
> is the intent of this code?  How can this be made to work for MAC
> drivers with addresses < 6 bytes?
the intention was to check if the mac len is a sane value and we
overlooked the < 6 bytes case.
I think you can safely change this check to "fd->fd_mac_len != 0"
as
there''s no code that assumes the mac len must be >= 6.

> As you''re probably aware, there are
> MACs that have no MAC addresses (e.g., PPP links), and the framework
> needs to eventually work for those as well.
>
agreed. we will change the framework to support these mac types as
necessary and there will be more things to consider than just address
lengths.

eric

On Wed, Jan 07, 2009 at 10:15:49PM -0800, Eric Cheng
wrote:
> On Thu, Jan 08, 2009 at 12:07:11AM -0500, Sebastien Roy wrote:
> > I''m in the process of doing the Clearview iptun and Crossbow
merge, and
> > one problem I''m having is that mac_datapath_setup() fails in
> > mac_flow_add() -> flow_l2_accept_fe() which contains the following:
> > 
> >         if (i == fd->fd_mac_len || fd->fd_mac_len <
ETHERADDRL)
> >                 return (EINVAL);
> > 
> > IPv4 tunnel "MAC" addresses are 4 bytes, and so this
doesn''t work.  What
> > is the intent of this code?  How can this be made to work for MAC
> > drivers with addresses < 6 bytes?
> 
> the intention was to check if the mac len is a sane value and we
> overlooked the < 6 bytes case.
> I think you can safely change this check to "fd->fd_mac_len !=
0" as
> there''s no code that assumes the mac len must be >= 6.
>
sorry, I meant "fd->fd_mac_len == 0".

eric

Eric Cheng writes:
> On Wed, Jan 07, 2009 at 10:15:49PM -0800, Eric Cheng wrote:
> > On Thu, Jan 08, 2009 at 12:07:11AM -0500, Sebastien Roy wrote:
> > > I''m in the process of doing the Clearview iptun and
Crossbow merge, and
> > > one problem I''m having is that mac_datapath_setup()
fails in
> > > mac_flow_add() -> flow_l2_accept_fe() which contains the
following:
> > > 
> > >         if (i == fd->fd_mac_len || fd->fd_mac_len <
ETHERADDRL)
> > >                 return (EINVAL);
> > > 
> > > IPv4 tunnel "MAC" addresses are 4 bytes, and so this
doesn''t work.  What
> > > is the intent of this code?  How can this be made to work for MAC
> > > drivers with addresses < 6 bytes?
> > 
> > the intention was to check if the mac len is a sane value and we
> > overlooked the < 6 bytes case.
> > I think you can safely change this check to "fd->fd_mac_len !=
0" as
> > there''s no code that assumes the mac len must be >= 6.
> >
> 
> sorry, I meant "fd->fd_mac_len == 0".
Technically, PPP links have an L2 address length of exactly zero.

But I can''t say I was expecting the MAC layer to support anything
other than Ethernet for quite some time.

-- 
James Carlson, Solaris Networking              <james.d.carlson at
sun.com>
Sun Microsystems / 35 Network Drive        71.232W   Vox +1 781 442 2084
MS UBUR02-212 / Burlington MA 01803-2757   42.496N   Fax +1 781 442 1677

On Wed, 2009-01-07 at 23:47 -0800, Eric Cheng wrote:
> On Wed, Jan 07, 2009 at 10:15:49PM -0800, Eric Cheng wrote:
> > On Thu, Jan 08, 2009 at 12:07:11AM -0500, Sebastien Roy wrote:
> > > I''m in the process of doing the Clearview iptun and
Crossbow merge, and
> > > one problem I''m having is that mac_datapath_setup()
fails in
> > > mac_flow_add() -> flow_l2_accept_fe() which contains the
following:
> > > 
> > >         if (i == fd->fd_mac_len || fd->fd_mac_len <
ETHERADDRL)
> > >                 return (EINVAL);
> > > 
> > > IPv4 tunnel "MAC" addresses are 4 bytes, and so this
doesn''t work.  What
> > > is the intent of this code?  How can this be made to work for MAC
> > > drivers with addresses < 6 bytes?
> > 
> > the intention was to check if the mac len is a sane value and we
> > overlooked the < 6 bytes case.
> > I think you can safely change this check to "fd->fd_mac_len !=
0" as
> > there''s no code that assumes the mac len must be >= 6.
> >
> 
> sorry, I meant "fd->fd_mac_len == 0".
You mean remove the check entirely?  Checking that the MAC length == 0
would make this code fail for all MAC types except those that have a MAC
length of 0.

-Seb

On Thu, 2009-01-08 at 07:23 -0500, James Carlson wrote:
> Technically, PPP links have an L2 address length of exactly zero.
> 
> But I can''t say I was expecting the MAC layer to support anything
> other than Ethernet for quite some time.
I was, since the MAC layer supported MACs with arbitrary MAC lengths
(including 0) prior to build 105.  I did a significant amount of work a
few years back to make sure that all (or at least most, since VLANs were
still a wart in dls) references to Ethernet and MAC address lengths and
other MAC-type-specific concepts were removed from the framework and
placed into the plugin modules.

-Seb

Sebastien Roy writes:
> 
> On Thu, 2009-01-08 at 07:23 -0500, James Carlson wrote:
> > Technically, PPP links have an L2 address length of exactly zero.
> > 
> > But I can''t say I was expecting the MAC layer to support
anything
> > other than Ethernet for quite some time.
> 
> I was, since the MAC layer supported MACs with arbitrary MAC lengths
> (including 0) prior to build 105.  I did a significant amount of work a
> few years back to make sure that all (or at least most, since VLANs were
> still a wart in dls) references to Ethernet and MAC address lengths and
> other MAC-type-specific concepts were removed from the framework and
> placed into the plugin modules.
OK ... it was the VLAN and aggr references (some of which seem to run
pretty deep) that had me convinced that MAC wouldn''t be ready for PPP
for a while.

-- 
James Carlson, Solaris Networking              <james.d.carlson at
sun.com>
Sun Microsystems / 35 Network Drive        71.232W   Vox +1 781 442 2084
MS UBUR02-212 / Burlington MA 01803-2757   42.496N   Fax +1 781 442 1677

On Thu, 2009-01-08 at 08:03 -0500, James Carlson wrote:
> Sebastien Roy writes:
> > 
> > On Thu, 2009-01-08 at 07:23 -0500, James Carlson wrote:
> > > Technically, PPP links have an L2 address length of exactly zero.
> > > 
> > > But I can''t say I was expecting the MAC layer to support
anything
> > > other than Ethernet for quite some time.
> > 
> > I was, since the MAC layer supported MACs with arbitrary MAC lengths
> > (including 0) prior to build 105.  I did a significant amount of work
a
> > few years back to make sure that all (or at least most, since VLANs
were
> > still a wart in dls) references to Ethernet and MAC address lengths
and
> > other MAC-type-specific concepts were removed from the framework and
> > placed into the plugin modules.
> 
> OK ... it was the VLAN and aggr references (some of which seem to run
> pretty deep) that had me convinced that MAC wouldn''t be ready for
PPP
> for a while.
Supporting PPP in GLDv3 (before build 105) would have simply required
writing a PPP MAC-type plugin.  The hard work or porting PPP to GLDv3 is
in the sppp module and pppd daemon.  The GLDv3 framework was basically
ready for it.

Aside from this seemingly unnecessary address length constraint, there
appears to be new constraints on what SAP values look like in
mac_flow.c.  I''ll start another discussion on this topic when I run
into
this once I''m further along with my Crossbow merge.

-Seb

Sebastien Roy writes:
> On Thu, 2009-01-08 at 08:03 -0500, James Carlson wrote:
> > OK ... it was the VLAN and aggr references (some of which seem to run
> > pretty deep) that had me convinced that MAC wouldn''t be ready
for PPP
> > for a while.
> 
> Supporting PPP in GLDv3 (before build 105) would have simply required
> writing a PPP MAC-type plugin.  The hard work or porting PPP to GLDv3 is
> in the sppp module and pppd daemon.
Agreed; it''s not really designed that way.
>  The GLDv3 framework was basically
> ready for it.
> 
> Aside from this seemingly unnecessary address length constraint, there
> appears to be new constraints on what SAP values look like in
> mac_flow.c.  I''ll start another discussion on this topic when I
run into
> this once I''m further along with my Crossbow merge.
SAP values should be MAC type specific, so that we can use (say) 0800
for Ethernet but 0021 for PPP.  I think getting that part right will
require some un-doing of long-held assumptions in IP and other clients.

-- 
James Carlson, Solaris Networking              <james.d.carlson at
sun.com>
Sun Microsystems / 35 Network Drive        71.232W   Vox +1 781 442 2084
MS UBUR02-212 / Burlington MA 01803-2757   42.496N   Fax +1 781 442 1677

On Thu, 2009-01-08 at 08:25 -0500, James Carlson wrote:
> Sebastien Roy writes:
> > Aside from this seemingly unnecessary address length constraint, there
> > appears to be new constraints on what SAP values look like in
> > mac_flow.c.  I''ll start another discussion on this topic when
I run into
> > this once I''m further along with my Crossbow merge.
> 
> SAP values should be MAC type specific, so that we can use (say) 0800
> for Ethernet but 0021 for PPP.  I think getting that part right will
> require some un-doing of long-held assumptions in IP and other clients.
IP doesn''t have any difficult SAP-space assumptions to overcome.  For
Clearview iptun, I simply added entries to the ip_m_tbl in ip_if.c to
contain the MAC-type specific value for the "IP SAP" and the
"IPv6 SAP",
and there are only a couple of places where those SAP values are used.
I simply changed that code to use the values from the table instead of
hard-coded values.  To make the ip module more dynamic than this and
have it not use ip_m_tbl would indeed require more design.

Removing such assumptions from other clients would indeed be more
difficult since the approach I used for the ip module can''t possibly
apply there.  There would need to be some more architecture to represent
SAP space to all kinds of clients through both a kernel and user-space
API.  The clients would then need to be made to use this API.

I''d still like the framework itself to not make assumptions about SAP
space, even if meeting MAC client requirements means that MAC-type
plugins are forced to all use similar SAP spaces.  That way, we''re not
forcing future projects to re-design the framework over and over
again. :-(

-Seb

Sebastien Roy wrote:
> On Wed, 2009-01-07 at 23:47 -0800, Eric Cheng wrote:
>   
>> On Wed, Jan 07, 2009 at 10:15:49PM -0800, Eric Cheng wrote:
>>     
>>> On Thu, Jan 08, 2009 at 12:07:11AM -0500, Sebastien Roy wrote:
>>>       
>>>> I''m in the process of doing the Clearview iptun and
Crossbow merge, and
>>>> one problem I''m having is that mac_datapath_setup()
fails in
>>>> mac_flow_add() -> flow_l2_accept_fe() which contains the
following:
>>>>
>>>>         if (i == fd->fd_mac_len || fd->fd_mac_len <
ETHERADDRL)
>>>>                 return (EINVAL);
>>>>
>>>> IPv4 tunnel "MAC" addresses are 4 bytes, and so this
doesn''t work.  What
>>>> is the intent of this code?  How can this be made to work for
MAC
>>>> drivers with addresses < 6 bytes?
>>>>         
>>> the intention was to check if the mac len is a sane value and we
>>> overlooked the < 6 bytes case.
>>> I think you can safely change this check to "fd->fd_mac_len
!= 0" as
>>> there''s no code that assumes the mac len must be >= 6.
>>>
>>>       
>> sorry, I meant "fd->fd_mac_len == 0".
>>     
>
> You mean remove the check entirely?  Checking that the MAC length == 0
> would make this code fail for all MAC types except those that have a MAC
> length of 0.
>   
Some time ago I ran into one or two related bugs whilst
I was experimenting with a "null" MAC driver... it might
be interesting to pull that out and see if it finds any other
new bugs (like your hash email)?

http://fr.opensolaris.org/jive/thread.jspa?threadID=56010

Darren

On Fri, 2009-01-09 at 05:40 +1100, Darren Reed wrote:
> Some time ago I ran into one or two related bugs whilst
> I was experimenting with a "null" MAC driver... it might
> be interesting to pull that out and see if it finds any other
> new bugs (like your hash email)?
> 
> http://fr.opensolaris.org/jive/thread.jspa?threadID=56010
Yes, I think that would be really useful, and would also be a good test
to include in the GLDv3 test suite.

-Seb

Sebastien Roy writes:
> On Thu, 2009-01-08 at 08:25 -0500, James Carlson wrote:
> > SAP values should be MAC type specific, so that we can use (say) 0800
> > for Ethernet but 0021 for PPP.  I think getting that part right will
> > require some un-doing of long-held assumptions in IP and other
clients.
> 
> IP doesn''t have any difficult SAP-space assumptions to overcome. 
For
> Clearview iptun, I simply added entries to the ip_m_tbl in ip_if.c to
> contain the MAC-type specific value for the "IP SAP" and the
"IPv6 SAP",
Right; it''s exactly that table that I consider to be the flaw.
> and there are only a couple of places where those SAP values are used.
> I simply changed that code to use the values from the table instead of
> hard-coded values.  To make the ip module more dynamic than this and
> have it not use ip_m_tbl would indeed require more design.
Yes, that''s what I''m referring to.  We could port over PPP to
GLDv3
and continue to use the half-Ethernet-like interface that it has
today, but I think that''d be a worse mistake than just leaving it
alone.
> I''d still like the framework itself to not make assumptions about
SAP
> space, even if meeting MAC client requirements means that MAC-type
> plugins are forced to all use similar SAP spaces.  That way, we''re
not
> forcing future projects to re-design the framework over and over
> again. :-(
True ... though I still think the SAP issue is a hole.

-- 
James Carlson, Solaris Networking              <james.d.carlson at
sun.com>
Sun Microsystems / 35 Network Drive        71.232W   Vox +1 781 442 2084
MS UBUR02-212 / Burlington MA 01803-2757   42.496N   Fax +1 781 442 1677

> > I''d still like the framework itself to not make assumptions
about SAP
 > > space, even if meeting MAC client requirements means that MAC-type
 > > plugins are forced to all use similar SAP spaces.  That way,
we''re not
 > > forcing future projects to re-design the framework over and over
 > > again. :-(
 > 
 > True ... though I still think the SAP issue is a hole.

At least from userland, it seems like we could handle this by enhancing
libdlpi -- e.g., introduce a dlpi_bindproto() or somesuch that takes a
protocol number and handles the conversion to the appropriate SAP value
depending on the underlying link type.

-- 
meem

Peter Memishian writes:
> 
>  > > I''d still like the framework itself to not make
assumptions about SAP
>  > > space, even if meeting MAC client requirements means that
MAC-type
>  > > plugins are forced to all use similar SAP spaces.  That way,
we''re not
>  > > forcing future projects to re-design the framework over and over
>  > > again. :-(
>  > 
>  > True ... though I still think the SAP issue is a hole.
> 
> At least from userland, it seems like we could handle this by enhancing
> libdlpi -- e.g., introduce a dlpi_bindproto() or somesuch that takes a
> protocol number and handles the conversion to the appropriate SAP value
> depending on the underlying link type.
libdlpi seems like a decent home for it, but I''d rather have the
interface allow for handle-less conversion of names to numbers and
numbers to names.  SAPs and MAC types can appear in other contexts
besides just DLPI streams, such as snoop capture files.

-- 
James Carlson, Solaris Networking              <james.d.carlson at
sun.com>
Sun Microsystems / 35 Network Drive        71.232W   Vox +1 781 442 2084
MS UBUR02-212 / Burlington MA 01803-2757   42.496N   Fax +1 781 442 1677

Peter Memishian wrote:
>  > > I''d still like the framework itself to not make
assumptions about SAP
>  > > space, even if meeting MAC client requirements means that
MAC-type
>  > > plugins are forced to all use similar SAP spaces.  That way,
we''re not
>  > > forcing future projects to re-design the framework over and over
>  > > again. :-(
>  > 
>  > True ... though I still think the SAP issue is a hole.
> 
> At least from userland, it seems like we could handle this by enhancing
> libdlpi -- e.g., introduce a dlpi_bindproto() or somesuch that takes a
> protocol number and handles the conversion to the appropriate SAP value
> depending on the underlying link type.
> 
The SAP classification etc was not left out by mistake. We just ran out
of time on that one for putback1. The plan is to get rid of DLS (at
least from data paths for both v4/v6) which means pulling SAP
classification also in MAC and deal with MAC plugins more properly
(the work that Seb had done).

Anyway, we should open a thread on the details in few weeks (as soon
as we round up some of the immediately pending work).

Cheers,
Sunay

On Thu, Jan 08, 2009 at 07:37:40AM -0500, Sebastien Roy
wrote:
> 
> On Wed, 2009-01-07 at 23:47 -0800, Eric Cheng wrote:
> > On Wed, Jan 07, 2009 at 10:15:49PM -0800, Eric Cheng wrote:
> > > On Thu, Jan 08, 2009 at 12:07:11AM -0500, Sebastien Roy wrote:
> > > > I''m in the process of doing the Clearview iptun and
Crossbow merge, and
> > > > one problem I''m having is that mac_datapath_setup()
fails in
> > > > mac_flow_add() -> flow_l2_accept_fe() which contains the
following:
> > > > 
> > > >         if (i == fd->fd_mac_len || fd->fd_mac_len <
ETHERADDRL)
> > > >                 return (EINVAL);
> > > > 
> > > > IPv4 tunnel "MAC" addresses are 4 bytes, and so
this doesn''t work.  What
> > > > is the intent of this code?  How can this be made to work
for MAC
> > > > drivers with addresses < 6 bytes?
> > > 
> > > the intention was to check if the mac len is a sane value and we
> > > overlooked the < 6 bytes case.
> > > I think you can safely change this check to
"fd->fd_mac_len != 0" as
> > > there''s no code that assumes the mac len must be >=
6.
> > >
> > 
> > sorry, I meant "fd->fd_mac_len == 0".
> 
> You mean remove the check entirely?  Checking that the MAC length == 0
> would make this code fail for all MAC types except those that have a MAC
> length of 0.
>
sorry for being unclear. I meant to change the existing check you mentioned
above to:

if (i == fd->fd_mac_len || fd->fd_mac_len == 0)
	return (EINVAL);

this rejects mac lengths of 0.

for mac types with mac len of 0, mac_flow_add() shouldn''t be called
at all since you can''t classify a 0 len address. a different mechanism
would be needed we''ll figure out what to do when we implement these mac
types.

eric

On Thu, 2009-01-08 at 13:17 -0800, Eric Cheng wrote:
> sorry for being unclear. I meant to change the existing check you mentioned
> above to:
> 
> if (i == fd->fd_mac_len || fd->fd_mac_len == 0)
> 	return (EINVAL);
> 
> this rejects mac lengths of 0.
> 
> for mac types with mac len of 0, mac_flow_add() shouldn''t be
called
> at all since you can''t classify a 0 len address. a different
mechanism
> would be needed we''ll figure out what to do when we implement
these mac
> types.
Ah, I see.  Note that the "i == fd->fd_mac_len" check is also
bogus,
since it''s verifying that the address isn''t all 0s.  The mac
module
shouldn''t dictate what values of MAC address are valid or invalid for a
given MAC type.

Thanks,
-Seb

> >  > > I''d still like the framework itself to not make
assumptions about SAP
 > >  > > space, even if meeting MAC client requirements means that
MAC-type
 > >  > > plugins are forced to all use similar SAP spaces.  That
way, we''re not
 > >  > > forcing future projects to re-design the framework over
and over
 > >  > > again. :-(
 > >  > 
 > >  > True ... though I still think the SAP issue is a hole.
 > > 
 > > At least from userland, it seems like we could handle this by
enhancing
 > > libdlpi -- e.g., introduce a dlpi_bindproto() or somesuch that takes
a
 > > protocol number and handles the conversion to the appropriate SAP
value
 > > depending on the underlying link type.
 > 
 > libdlpi seems like a decent home for it, but I''d rather have the
 > interface allow for handle-less conversion of names to numbers and
 > numbers to names.  SAPs and MAC types can appear in other contexts
 > besides just DLPI streams, such as snoop capture files.

Understood.

-- 
meem

On Thu, Jan 08, 2009 at 04:27:02PM -0500, Sebastien Roy
wrote:
> 
> On Thu, 2009-01-08 at 13:17 -0800, Eric Cheng wrote:
> > sorry for being unclear. I meant to change the existing check you
mentioned
> > above to:
> > 
> > if (i == fd->fd_mac_len || fd->fd_mac_len == 0)
> > 	return (EINVAL);
> > 
> > this rejects mac lengths of 0.
> > 
> > for mac types with mac len of 0, mac_flow_add() shouldn''t be
called
> > at all since you can''t classify a 0 len address. a different
mechanism
> > would be needed we''ll figure out what to do when we implement
these mac
> > types.
> 
> Ah, I see.  Note that the "i == fd->fd_mac_len" check is also
bogus,
> since it''s verifying that the address isn''t all 0s.  The
mac module
> shouldn''t dictate what values of MAC address are valid or invalid
for a
> given MAC type.
>
I''m fine with removing the all-zeros check.

eric

Sunay Tripathi wrote:
> 
> The SAP classification etc was not left out by mistake. We just ran out
> of time on that one for putback1. The plan is to get rid of DLS (at
> least from data paths for both v4/v6) which means pulling SAP
> classification also in MAC and deal with MAC plugins more properly
> (the work that Seb had done).
> 
This seems like a rather radical departure from the original cleanly 
layered Nemo design philosophy. It seems like what is required is the 
*extension* of DLS to cope with the diverse number of MAC types allowed 
by the plug-in architecture (remembering that prior to the plug-ins the 
MAC layer was strictly ethernet).
If anything should be removed it should be DLD; it''s about time that IP
interfaced directly to DLS and we finally get rid of STREAMS in the 
common case.

   Paul

Paul Durrant wrote:
> Sunay Tripathi wrote:
>>
>> The SAP classification etc was not left out by mistake. We just ran out
>> of time on that one for putback1. The plan is to get rid of DLS (at
>> least from data paths for both v4/v6) which means pulling SAP
>> classification also in MAC and deal with MAC plugins more properly
>> (the work that Seb had done).
>>
> 
> This seems like a rather radical departure from the original cleanly 
> layered Nemo design philosophy. It seems like what is required is the 
> *extension* of DLS to cope with the diverse number of MAC types allowed 
> by the plug-in architecture (remembering that prior to the plug-ins the 
> MAC layer was strictly ethernet).
> If anything should be removed it should be DLD; it''s about time
that IP
> interfaced directly to DLS and we finally get rid of STREAMS in the 
> common case.
Not really. In the original design, DLS used to do lot of work in
SAP classification, checking the mac addresses and accepting the packets
but now with a much richer MAC layer which uses a combination of
H/W and S/W classifier, the data path doesn''t need to go through DLS.

If I can program the H/W classifier for a particular mac address and
IP is the only client, then then other than scheduling the packet
processing (dynamic polling), very little needs to be done anyway
for normal cases. So in short, the DLS layer doesn''t belong in the
data path and the interfaces still remain very clean (and lesser
layers :)

And yes, IP does interface directly with the MAC layer. Now DLS as
a data link layer which exports an API that people can use is probably
fine.

Cheers,
Sunay


-- 
Sunay Tripathi
Distinguished Engineer
Solaris Core Operating System
Sun MicroSystems Inc.

Solaris Networking:     http://www.opensolaris.org/os/community/networking
Project Crossbow:       http://www.opensolaris.org/os/project/crossbow