Ethersubr fix for netiso

[Ignatios Souvatzis]

Hi,

when Perry asked for NETISO testing code, I wrote some, and found bugs.
Basically, when ether_input() calls (indirectly) clnp_input(), it does
horrible things.

Namely, it does an overlapping struct assign (on a memory area
formerly m_adj'ed away!) which might have worked with ancient
compilers, but creates address corruption now. (== netbsd-3-1).

All to get rid of the intermediate 3-byte 802.x llc field.

As the clnp_input is aware of being called by ethernet anyway (and
sort of needs to), I decided to just give it the full ethernet
header with llc, and let it throw it away itself (it does an m_adj
itself, so this is for free). Saving in the ISO networking path:
two memcpy if done right, one memmove if the current code was
trivially repaired.

I suspect that any modern compiler would integrate one additional
compare-to-constant and and conditional branch (which are executed
additionally for the DIX protocols) into the big DIX switch, so no
cost created there, or else, say, 2 instruction cycles (== about one
nanosecond for modern machines).

if_fddisubr and if_tokensubr will need similar patches; at least for
fddi this won't create a penalty because it hasn't any DIX type
field, so there's a convenient switch case already to move the
m_adj() to.

Any comments?

(Code is tested for AF_INET, AF_INET6 and AF_ISO).

Regards,
-is

[Christos Zoulas]

-=-=-=-=-=-
Hi,
when Perry asked for NETISO testing code, I wrote some, and found bugs.
Basically, when ether_input() calls (indirectly) clnp_input(), it does
horrible things.
Namely, it does an overlapping struct assign (on a memory area
formerly m_adj'ed away!) which might have worked with ancient
compilers, but creates address corruption now. (== netbsd-3-1).
All to get rid of the intermediate 3-byte 802.x llc field.
As the clnp_input is aware of being called by ethernet anyway (and
sort of needs to), I decided to just give it the full ethernet
header with llc, and let it throw it away itself (it does an m_adj
two memcpy if done right, one memmove if the current code was
trivially repaired.
I suspect that any modern compiler would integrate one additional
compare-to-constant and and conditional branch (which are executed
additionally for the DIX protocols) into the big DIX switch, so no
cost created there, or else, say, 2 instruction cycles (== about one
nanosecond for modern machines).
if_fddisubr and if_tokensubr will need similar patches; at least for
fddi this won't create a penalty because it hasn't any DIX type
field, so there's a convenient switch case already to move the
m_adj() to.
Any comments?
(Code is tested for AF_INET, AF_INET6 and AF_ISO).

Looks fine to me.

christos


--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Ignatios Souvatzis]

Here's part two of the patches, for FDDI (again, netiso/clnp_input.c and
netinet/fddi_subr.c).

I did test that it compiles, but not run-time - no FDDI here. Testers
can use the package at

ftp://ftp.netbsd.org/pub/NetBSD/misc/is/isotest/

It comes with a README file that explains how to setup two machines and
send an ISO CLTP packet from one to the other.

Regards,
-is

[Ignatios Souvatzis]

Here's part two of the patches, for FDDI (again, netiso/clnp_input.c and
netinet/fddi_subr.c). Sorry, wrong subject on the first try, and I fear
the designated alpha testers wouldn't see it.

I did test that it compiles, but not run-time - no FDDI here. Testers
can use the package at

ftp://ftp.netbsd.org/pub/NetBSD/misc/is/isotest/

It comes with a README file that explains how to setup two machines and
send an ISO CLTP packet from one to the other.

Regards,
-is

Index: sys/netiso/clnp_input.c
===================================================================
RCS file: /cvsroot/src/sys/netiso/clnp_input.c,v
retrieving revision 1.31
diff -u -r1.31 clnp_input.c
--- sys/netiso/clnp_input.c 1 Dec 2006 18:43:40 -0000 1.31
+++ sys/netiso/clnp_input.c 4 Dec 2006 21:20:11 -0000
@@ -204,9 +204,8 @@
case IFT_FDDI:
bcopy((caddr_t) (mtod(m, struct fddi_header *)->fddi_dhost),
(caddr_t) sh.snh_dhost, 2 * sizeof(sh.snh_dhost));
- m->m_data += sizeof(struct fddi_header);
- m->m_len -= sizeof(struct fddi_header);
- m->m_pkthdr.len -= sizeof(struct fddi_header);
+
+ m_adj(m, sizeof(struct fddi_header) + 3);
break;
case IFT_PTPSERIAL:
case IFT_GIF:
Index: sys/net/if_fddisubr.c
===================================================================
RCS file: /cvsroot/src/sys/net/if_fddisubr.c,v
retrieving revision 1.63
diff -u -r1.63 if_fddisubr.c
--- sys/net/if_fddisubr.c 7 Sep 2006 02:40:33 -0000 1.63
+++ sys/net/if_fddisubr.c 4 Dec 2006 21:20:12 -0000
@@ -580,10 +580,7 @@
m->m_flags |= M_LINK0;
#endif

- /* Strip off the FDDI header. */
- m_adj(m, sizeof(struct fddi_header));
-
- l = mtod(m, struct llc *);
+ l = (struct llc *)(fh+1);
switch (l->llc_dsap) {
#if defined(INET) || defined(INET6) || defined(NS) || defined(DECNET) || defined(IPX) || defined(NETATALK)
case LLC_SNAP_LSAP:
@@ -591,6 +588,10 @@
u_int16_t etype;
if (l->llc_control != LLC_UI || l->llc_ssap != LLC_SNAP_LSAP)
goto dropanyway;
+
+ /* Strip off the FDDI header. */
+ m_adj(m, sizeof(struct fddi_header));
+
#ifdef NETATALK
if (Bcmp(&(l->llc_snap_org_code)[0], at_org_code,
sizeof(at_org_code)) == 0 &&
@@ -684,14 +685,7 @@
/* LLC_UI_P forbidden in class 1 service */
if ((l->llc_dsap == LLC_ISO_LSAP) &&
(l->llc_ssap == LLC_ISO_LSAP)) {
- /* LSAP for ISO */
- m->m_data += 3; /* XXX */
- m->m_len -= 3; /* XXX */
- m->m_pkthdr.len -= 3; /* XXX */
- M_PREPEND(m, sizeof *fh, M_DONTWAIT);
- if (m == 0)
- return;
- *mtod(m, struct fddi_header *) = *fh;
+
schednetisr(NETISR_ISO);
inq = &clnlintrq;
break;
@@ -700,7 +694,7 @@

case LLC_XID:
case LLC_XID_P:
- if(m->m_len < 6)
+ if(m->m_len < 6 + sizeof(struct fddi_header))
goto dropanyway;
l->llc_window = 0;
l->llc_fid = 9;
@@ -727,6 +721,7 @@
eh->ether_dhost[i] = fh->fddi_shost[i];
}
eh->ether_type = 0;
+ m_adj(m, sizeof(struct fddi_header));
ifp->if_output(ifp, m, &sa, NULL);
return;
}

[David Young]

Post by Ignatios Souvatzis
Here's part two of the patches, for FDDI (again, netiso/clnp_input.c and
netinet/fddi_subr.c). Sorry, wrong subject on the first try, and I fear
the designated alpha testers wouldn't see it.
I did test that it compiles, but not run-time - no FDDI here. Testers
can use the package at
ftp://ftp.netbsd.org/pub/NetBSD/misc/is/isotest/
It comes with a README file that explains how to setup two machines and
send an ISO CLTP packet from one to the other.

I count at least three anonymous numbers (see below). I am mystified. :-)

Post by Ignatios Souvatzis
Index: sys/netiso/clnp_input.c
===================================================================
RCS file: /cvsroot/src/sys/netiso/clnp_input.c,v
retrieving revision 1.31
diff -u -r1.31 clnp_input.c
--- sys/netiso/clnp_input.c 1 Dec 2006 18:43:40 -0000 1.31
+++ sys/netiso/clnp_input.c 4 Dec 2006 21:20:11 -0000
@@ -204,9 +204,8 @@
bcopy((caddr_t) (mtod(m, struct fddi_header *)->fddi_dhost),
(caddr_t) sh.snh_dhost, 2 * sizeof(sh.snh_dhost));
- m->m_data += sizeof(struct fddi_header);
- m->m_len -= sizeof(struct fddi_header);
- m->m_pkthdr.len -= sizeof(struct fddi_header);
+
+ m_adj(m, sizeof(struct fddi_header) + 3);

3?

Post by Ignatios Souvatzis
- if(m->m_len < 6)
+ if(m->m_len < 6 + sizeof(struct fddi_header))
goto dropanyway;

6?

Post by Ignatios Souvatzis
l->llc_window = 0;
l->llc_fid = 9;

9?

Dave

[Ignatios Souvatzis]

Post by David Young

Post by Ignatios Souvatzis
Here's part two of the patches, for FDDI (again, netiso/clnp_input.c and
netinet/fddi_subr.c). Sorry, wrong subject on the first try, and I fear
the designated alpha testers wouldn't see it.
I did test that it compiles, but not run-time - no FDDI here. Testers
can use the package at
ftp://ftp.netbsd.org/pub/NetBSD/misc/is/isotest/
It comes with a README file that explains how to setup two machines and
send an ISO CLTP packet from one to the other.

I count at least three anonymous numbers (see below). I am mystified. :-)

Post by Ignatios Souvatzis
Index: sys/netiso/clnp_input.c
===================================================================
RCS file: /cvsroot/src/sys/netiso/clnp_input.c,v
retrieving revision 1.31
diff -u -r1.31 clnp_input.c
--- sys/netiso/clnp_input.c 1 Dec 2006 18:43:40 -0000 1.31
+++ sys/netiso/clnp_input.c 4 Dec 2006 21:20:11 -0000
@@ -204,9 +204,8 @@
bcopy((caddr_t) (mtod(m, struct fddi_header *)->fddi_dhost),
(caddr_t) sh.snh_dhost, 2 * sizeof(sh.snh_dhost));
- m->m_data += sizeof(struct fddi_header);
- m->m_len -= sizeof(struct fddi_header);
- m->m_pkthdr.len -= sizeof(struct fddi_header);
+
+ m_adj(m, sizeof(struct fddi_header) + 3);

3?

That's LLC_UFRAMELEN, and anonymous in the old version of the code, too.
I planned to replace that seperately, but could do that in the same step, if
you insist.

Post by David Young

Post by Ignatios Souvatzis
- if(m->m_len < 6)
+ if(m->m_len < 6 + sizeof(struct fddi_header))
goto dropanyway;

6?

I didn't change the 6. No idea. Just changed the length offset as above.

Post by David Young

Post by Ignatios Souvatzis
l->llc_window = 0;
l->llc_fid = 9;

9?

I didn't change the 9. No idea. Just changed the length offset as above.

I notice you didn't complain when I posted and committed the ether_subr
code, which is the same...

I guess you can look them up in 802.2 .

(And IMO, that code should not depend on the llc SAP codes, but be generic
for all that has an LLC the the LLC commands involved. But I won't change
that with this change.

Regards,
-is

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[David Young]

Post by Ignatios Souvatzis

Post by David Young

Post by Ignatios Souvatzis
Here's part two of the patches, for FDDI (again, netiso/clnp_input.c and
netinet/fddi_subr.c). Sorry, wrong subject on the first try, and I fear
the designated alpha testers wouldn't see it.
I did test that it compiles, but not run-time - no FDDI here. Testers
can use the package at
ftp://ftp.netbsd.org/pub/NetBSD/misc/is/isotest/
It comes with a README file that explains how to setup two machines and
send an ISO CLTP packet from one to the other.

I count at least three anonymous numbers (see below). I am mystified. :-)

Post by Ignatios Souvatzis
Index: sys/netiso/clnp_input.c
===================================================================
RCS file: /cvsroot/src/sys/netiso/clnp_input.c,v
retrieving revision 1.31
diff -u -r1.31 clnp_input.c
--- sys/netiso/clnp_input.c 1 Dec 2006 18:43:40 -0000 1.31
+++ sys/netiso/clnp_input.c 4 Dec 2006 21:20:11 -0000
@@ -204,9 +204,8 @@
bcopy((caddr_t) (mtod(m, struct fddi_header *)->fddi_dhost),
(caddr_t) sh.snh_dhost, 2 * sizeof(sh.snh_dhost));
- m->m_data += sizeof(struct fddi_header);
- m->m_len -= sizeof(struct fddi_header);
- m->m_pkthdr.len -= sizeof(struct fddi_header);
+
+ m_adj(m, sizeof(struct fddi_header) + 3);

3?

That's LLC_UFRAMELEN, and anonymous in the old version of the code, too.
I planned to replace that seperately, but could do that in the same step, if
you insist.

I was just mentioning, in case you were planning a more general cleanup.
I insist on nothing.

Dave

[Ignatios Souvatzis]

Post by Ignatios Souvatzis

Post by David Young
I was just mentioning, in case you were planning a more general cleanup.
I insist on nothing.

I looked up the XID constants, and actually one of them was wrong
(I guess binary 10000001 vs. 1001 error).

Committed; the XID format code correction was committed seperately,
just in case.

-is

[Ignatios Souvatzis]

Post by David Young
I was just mentioning, in case you were planning a more general cleanup.
I insist on nothing.

I looked up the XID constants, and actually one of them was wrong
(I guess binary 10000001 vs. 1001 error).

-is