Re: CVS commit: src/sys

[Ryo Shimizu <ryo%nerv.org@localhost>]

>> > > This clearly is a layering/abstraction violation and would have been
>> > > good to discuss upfront.
>> > >
>> > > Where do you make use of that information? What about other packet injection
>> > > paths?
>> >
>> > The next commit uses it in if_arp to ensure that the DaD probe sending interface
>> > hardware address matches the sending hardware address in the ARP packet as
>> > specified in RFC 5227 section 1.1
>> >
>> > I couldn't think of a better way of achieving this.
>>
>> RFC 5227 says senders must follow the spec but doesn't say receivers'
>> check is must IIUC.
>>
>> I don't think it is a good idea to increase the mbuf size just for
>> broken clients.
>> I think it is better to make the strict check optional (by sysctl or
>> something) and use mtag,
>> so the change doesn't impact on most of us.
>
>Well... another possible option is to unionize l2_* with pattr_*.
>This is possible (IIUC)
>because l2_* are used only on receiving packets while pattr_* are used only on
>sending packets.


Since l2_sha continues to point outside of m_data manipulated by m_adj(),
it can be corrupted by subsequent m_pullup() or other mbuf m_*() operations.
I still believe that using MTAG is appropriate.

How about something like this? (not tested)

git diff -u -u .
diff --git a/sys/net/if_ethersubr.c b/sys/net/if_ethersubr.c
index 34be2d1cf91..18c8c1dcef9 100644
--- a/sys/net/if_ethersubr.c
+++ b/sys/net/if_ethersubr.c
@@ -886,9 +886,17 @@ ether_input(struct ifnet *ifp, struct mbuf *m)
 #endif
 	}
 
-	/* Store the senders hardware address */
-	m->m_pkthdr.l2_sha = &eh->ether_shost;
-	m->m_pkthdr.l2_shalen = ETHER_ADDR_LEN;
+	//if (arp_do_strict_check_sha)	/* XXX: sysctl? */
+	if (etype == ETHERTYPE_ARP) {
+		/* Store the senders hardware address */
+		struct m_tag *mtag;
+		mtag = m_tag_get(PACKET_TAG_ETHERNET_SRC, ETHER_ADDR_LEN,
+		    M_NOWAIT);
+		if (mtag != NULL) {
+			memcpy(mtag + 1, &eh->ether_shost, ETHER_ADDR_LEN);
+			m_tag_prepend(m, mtag);
+		}
+	}
 
 	/* Strip off the Ethernet header. */
 	m_adj(m, ehlen);
diff --git a/sys/netinet/if_arp.c b/sys/netinet/if_arp.c
index 90b0be9ff59..3334452c496 100644
--- a/sys/netinet/if_arp.c
+++ b/sys/netinet/if_arp.c
@@ -945,18 +945,23 @@ again:
 	    (in_hosteq(isaddr, myaddr) ||
 	    (in_nullhost(isaddr) && in_hosteq(itaddr, myaddr) &&
 	     m->m_flags & M_BCAST &&
-	     ia->ia4_flags & (IN_IFF_TENTATIVE | IN_IFF_DUPLICATED))) &&
-	    m->m_pkthdr.l2_shalen == ah->ar_hln && (
-	    ah->ar_hln == 0 ||
-	    memcmp(m->m_pkthdr.l2_sha, ar_sha(ah), ah->ar_hln) == 0))
+	     ia->ia4_flags & (IN_IFF_TENTATIVE | IN_IFF_DUPLICATED))))
 	{
 		struct sockaddr_dl sdl, *sdlp;
+		struct m_tag *mtag = NULL;
 
-		sdlp = sockaddr_dl_init(&sdl, sizeof(sdl),
-		    ifp->if_index, ifp->if_type,
-		    NULL, 0, ar_sha(ah), ah->ar_hln);
-		arp_dad_duplicated((struct ifaddr *)ia, sdlp);
-		goto out;
+		//if (arp_do_strict_check_sha)	/* XXX: sysctl? */
+		mtag = m_tag_find(m, PACKET_TAG_ETHERNET_SRC);
+
+		if (mtag == NULL || (ah->ar_hln == ETHER_ADDR_LEN &&
+		    memcmp(mtag + 1, ar_sha(ah), ETHER_ADDR_LEN) == 0)) {
+
+			sdlp = sockaddr_dl_init(&sdl, sizeof(sdl),
+			    ifp->if_index, ifp->if_type,
+			    NULL, 0, ar_sha(ah), ah->ar_hln);
+			arp_dad_duplicated((struct ifaddr *)ia, sdlp);
+			goto out;
+		}
 	}
 
 	/*
diff --git a/sys/sys/mbuf.h b/sys/sys/mbuf.h
index 6b441b56bd8..bf69372f083 100644
--- a/sys/sys/mbuf.h
+++ b/sys/sys/mbuf.h
@@ -178,8 +178,8 @@ struct m_hdr {
  * checksum) -- this is so we can accumulate the checksum for fragmented
  * packets during reassembly.
  *
- * Size ILP32: 48
- *       LP64: 72
+ * Size ILP32: 40
+ *       LP64: 56
  */
 struct pkthdr {
 	union {
@@ -203,9 +203,6 @@ struct pkthdr {
 	int	pattr_af;		/* ALTQ: address family */
 	void	*pattr_class;		/* ALTQ: sched class set by classifier */
 	void	*pattr_hdr;		/* ALTQ: saved header position in mbuf */
-
-	void		*l2_sha;		/* l2 sender host address */
-	size_t		l2_shalen;		/* length of the sender address */
 };
 
 /* Checksumming flags (csum_flags). */
@@ -803,6 +800,7 @@ int	m_tag_copy_chain(struct mbuf *, struct mbuf *);
 					    */
 #define PACKET_TAG_MPLS			29 /* Indicate it's for MPLS */
 #define PACKET_TAG_SRCROUTE		30 /* IPv4 source routing */
+#define PACKET_TAG_ETHERNET_SRC		31
 
 /*
  * Return the number of bytes in the mbuf chain, m.



Since l2_shalen is fixed to ETHER_ADDR_LEN for now, the tag name should be
PACKET_TAG_ETHERNET_SRC instead of PACKET_TAG_L2SHA for now.
If all L2 addresses are to be treated extensively, the structure of mtag
should include the size, but that will not be necessary just yet.

-- 
ryo shimizu