diff mbox

[PATCHv7,4/9] slirp: Factorizing tcpiphdr structure with an union

Message ID a02c9bf062ce8900092e43c735bd33bc129ffcb8.1454927009.git.samuel.thibault@ens-lyon.org (mailing list archive)
State New, archived
Headers show

Commit Message

Samuel Thibault Feb. 8, 2016, 10:28 a.m. UTC 
From: Guillaume Subiron <maethor@subiron.org>

This patch factorizes the tcpiphdr structure to put the IPv4 fields in
an union, for addition of version 6 in further patch.
Using some macros, retrocompatibility of the existing code is assured.

This patch also fixes the SLIRP_MSIZE and margin computation in various
functions, and makes them compatible with the new tcpiphdr structure,
whose size will be bigger than sizeof(struct tcphdr) + sizeof(struct ip)

Signed-off-by: Guillaume Subiron <maethor@subiron.org>
Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
---
 slirp/if.h         |  4 ++--
 slirp/mbuf.c       |  3 ++-
 slirp/slirp.c      | 15 ++++++++-------
 slirp/socket.c     | 13 ++++++++++++-
 slirp/tcp_input.c  | 31 ++++++++++++++++++++-----------
 slirp/tcp_output.c | 18 +++++++++++++-----
 slirp/tcp_subr.c   | 31 ++++++++++++++++++++++---------
 slirp/tcpip.h      | 31 +++++++++++++++++++++++--------
 8 files changed, 102 insertions(+), 44 deletions(-)

Comments

Thomas Huth Feb. 10, 2016, 8:05 a.m. UTC | #1 
On 08.02.2016 11:28, Samuel Thibault wrote:
> From: Guillaume Subiron <maethor@subiron.org>
> 
> This patch factorizes the tcpiphdr structure to put the IPv4 fields in
> an union, for addition of version 6 in further patch.
> Using some macros, retrocompatibility of the existing code is assured.
> 
> This patch also fixes the SLIRP_MSIZE and margin computation in various
> functions, and makes them compatible with the new tcpiphdr structure,
> whose size will be bigger than sizeof(struct tcphdr) + sizeof(struct ip)
> 
> Signed-off-by: Guillaume Subiron <maethor@subiron.org>
> Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> ---
>  slirp/if.h         |  4 ++--
>  slirp/mbuf.c       |  3 ++-
>  slirp/slirp.c      | 15 ++++++++-------
>  slirp/socket.c     | 13 ++++++++++++-
>  slirp/tcp_input.c  | 31 ++++++++++++++++++++-----------
>  slirp/tcp_output.c | 18 +++++++++++++-----
>  slirp/tcp_subr.c   | 31 ++++++++++++++++++++++---------
>  slirp/tcpip.h      | 31 +++++++++++++++++++++++--------
>  8 files changed, 102 insertions(+), 44 deletions(-)
> 
> diff --git a/slirp/if.h b/slirp/if.h
> index 3327023..c7a5c57 100644
> --- a/slirp/if.h
> +++ b/slirp/if.h
> @@ -17,7 +17,7 @@
>  #define IF_MRU 1500
>  #define	IF_COMP IF_AUTOCOMP	/* Flags for compression */
>  
> -/* 2 for alignment, 14 for ethernet, 40 for TCP/IP */
> -#define IF_MAXLINKHDR (2 + 14 + 40)
> +/* 2 for alignment, 14 for ethernet */
> +#define IF_MAXLINKHDR (2 + ETH_HLEN)
>  
>  #endif
> diff --git a/slirp/mbuf.c b/slirp/mbuf.c
> index c959758..f081c69 100644
> --- a/slirp/mbuf.c
> +++ b/slirp/mbuf.c
> @@ -24,7 +24,8 @@
>   * Find a nice value for msize
>   * XXX if_maxlinkhdr already in mtu
>   */
> -#define SLIRP_MSIZE (IF_MTU + IF_MAXLINKHDR + offsetof(struct mbuf, m_dat) + 6)
> +#define SLIRP_MSIZE\
> +    (offsetof(struct mbuf, m_dat) + IF_MAXLINKHDR + TCPIPHDR_DELTA + IF_MTU)

I'm somehow having a hard time to understand why TCPIPHDR_DELTA is used
here. As far as I understand, TCPIPHDR_DELTA is the difference between
the size of struct tcpiphdr and the size of the IPv4 + TCP header. But
if it's just the difference, where does the base size of the headers
come from in this define, since the headers are stored in the mbuf, too,
aren't they? ... I've got the feeling that I miss something here, could
you enlighten me?

 Thomas
Samuel Thibault Feb. 10, 2016, 9:28 a.m. UTC | #2 
That one is tricky, yes :)

Thomas Huth, on Wed 10 Feb 2016 09:05:32 +0100, wrote:
> > -#define SLIRP_MSIZE (IF_MTU + IF_MAXLINKHDR + offsetof(struct mbuf, m_dat) + 6)
> > +#define SLIRP_MSIZE\
> > +    (offsetof(struct mbuf, m_dat) + IF_MAXLINKHDR + TCPIPHDR_DELTA + IF_MTU)
> 
> I'm somehow having a hard time to understand why TCPIPHDR_DELTA is used
> here. As far as I understand, TCPIPHDR_DELTA is the difference between
> the size of struct tcpiphdr and the size of the IPv4 + TCP header. But
> if it's just the difference, where does the base size of the headers
> come from in this define, since the headers are stored in the mbuf, too,
> aren't they? ... I've got the feeling that I miss something here, could
> you enlighten me?

TCP/IP headers are within IF_MTU.

Samuel
Thomas Huth Feb. 10, 2016, 10:08 a.m. UTC | #3 
On 10.02.2016 10:28, Samuel Thibault wrote:
> That one is tricky, yes :)
> 
> Thomas Huth, on Wed 10 Feb 2016 09:05:32 +0100, wrote:
>>> -#define SLIRP_MSIZE (IF_MTU + IF_MAXLINKHDR + offsetof(struct mbuf, m_dat) + 6)
>>> +#define SLIRP_MSIZE\
>>> +    (offsetof(struct mbuf, m_dat) + IF_MAXLINKHDR + TCPIPHDR_DELTA + IF_MTU)
>>
>> I'm somehow having a hard time to understand why TCPIPHDR_DELTA is used
>> here. As far as I understand, TCPIPHDR_DELTA is the difference between
>> the size of struct tcpiphdr and the size of the IPv4 + TCP header. But
>> if it's just the difference, where does the base size of the headers
>> come from in this define, since the headers are stored in the mbuf, too,
>> aren't they? ... I've got the feeling that I miss something here, could
>> you enlighten me?
> 
> TCP/IP headers are within IF_MTU.

Ah, of course, that makes sense, thanks! ... so in the old definition of
SLIRP_MSIZE, the TCP/IP headers were counted twice, I guess? Anyway, the
definition looks fine to me now.

 Thomas
Samuel Thibault Feb. 10, 2016, 12:20 p.m. UTC | #4 
Thomas Huth, on Wed 10 Feb 2016 11:08:55 +0100, wrote:
> On 10.02.2016 10:28, Samuel Thibault wrote:
> > Thomas Huth, on Wed 10 Feb 2016 09:05:32 +0100, wrote:
> >>> -#define SLIRP_MSIZE (IF_MTU + IF_MAXLINKHDR + offsetof(struct mbuf, m_dat) + 6)
> >>> +#define SLIRP_MSIZE\
> >>> +    (offsetof(struct mbuf, m_dat) + IF_MAXLINKHDR + TCPIPHDR_DELTA + IF_MTU)
> >>
> >> I'm somehow having a hard time to understand why TCPIPHDR_DELTA is used
> >> here. As far as I understand, TCPIPHDR_DELTA is the difference between
> >> the size of struct tcpiphdr and the size of the IPv4 + TCP header. But
> >> if it's just the difference, where does the base size of the headers
> >> come from in this define, since the headers are stored in the mbuf, too,
> >> aren't they? ... I've got the feeling that I miss something here, could
> >> you enlighten me?
> > 
> > TCP/IP headers are within IF_MTU.
> 
> Ah, of course, that makes sense, thanks! ... so in the old definition of
> SLIRP_MSIZE, the TCP/IP headers were counted twice, I guess?

Something like this, yes. The formula was looking very magic (what is
that 6 from?), so we rewrote it to make it clear for sure.

Samuel
diff mbox

Patch

diff --git a/slirp/if.h b/slirp/if.h
index 3327023..c7a5c57 100644
--- a/slirp/if.h
+++ b/slirp/if.h
@@ -17,7 +17,7 @@ 
 #define IF_MRU 1500
 #define	IF_COMP IF_AUTOCOMP	/* Flags for compression */
 
-/* 2 for alignment, 14 for ethernet, 40 for TCP/IP */
-#define IF_MAXLINKHDR (2 + 14 + 40)
+/* 2 for alignment, 14 for ethernet */
+#define IF_MAXLINKHDR (2 + ETH_HLEN)
 
 #endif
diff --git a/slirp/mbuf.c b/slirp/mbuf.c
index c959758..f081c69 100644
--- a/slirp/mbuf.c
+++ b/slirp/mbuf.c
@@ -24,7 +24,8 @@ 
  * Find a nice value for msize
  * XXX if_maxlinkhdr already in mtu
  */
-#define SLIRP_MSIZE (IF_MTU + IF_MAXLINKHDR + offsetof(struct mbuf, m_dat) + 6)
+#define SLIRP_MSIZE\
+    (offsetof(struct mbuf, m_dat) + IF_MAXLINKHDR + TCPIPHDR_DELTA + IF_MTU)
 
 void
 m_init(Slirp *slirp)
diff --git a/slirp/slirp.c b/slirp/slirp.c
index 0e652bd..551f100 100644
--- a/slirp/slirp.c
+++ b/slirp/slirp.c
@@ -753,15 +753,16 @@  void slirp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len)
         m = m_get(slirp);
         if (!m)
             return;
-        /* Note: we add to align the IP header */
-        if (M_FREEROOM(m) < pkt_len + 2) {
-            m_inc(m, pkt_len + 2);
+        /* Note: we add 2 to align the IP header on 4 bytes,
+         * and add the margin for the tcpiphdr overhead  */
+        if (M_FREEROOM(m) < pkt_len + TCPIPHDR_DELTA + 2) {
+            m_inc(m, pkt_len + TCPIPHDR_DELTA + 2);
         }
-        m->m_len = pkt_len + 2;
-        memcpy(m->m_data + 2, pkt, pkt_len);
+        m->m_len = pkt_len + TCPIPHDR_DELTA + 2;
+        memcpy(m->m_data + TCPIPHDR_DELTA + 2, pkt, pkt_len);
 
-        m->m_data += 2 + ETH_HLEN;
-        m->m_len -= 2 + ETH_HLEN;
+        m->m_data += TCPIPHDR_DELTA + 2 + ETH_HLEN;
+        m->m_len -= TCPIPHDR_DELTA + 2 + ETH_HLEN;
 
         if (proto == ETH_P_IP) {
             ip_input(m);
diff --git a/slirp/socket.c b/slirp/socket.c
index b79ddec..d4b02c8 100644
--- a/slirp/socket.c
+++ b/slirp/socket.c
@@ -483,7 +483,18 @@  sorecvfrom(struct socket *so)
 	  if (!m) {
 	      return;
 	  }
-	  m->m_data += IF_MAXLINKHDR;
+	  switch (so->so_ffamily) {
+	  case AF_INET:
+	      m->m_data += IF_MAXLINKHDR + sizeof(struct udpiphdr);
+	      break;
+	  case AF_INET6:
+	      m->m_data += IF_MAXLINKHDR + sizeof(struct ip6)
+	                                 + sizeof(struct udphdr);
+	      break;
+	  default:
+	      g_assert_not_reached();
+	      break;
+	  }
 
 	  /*
 	   * XXX Shouldn't FIONREAD packets destined for port 53,
diff --git a/slirp/tcp_input.c b/slirp/tcp_input.c
index 5f845da..26b0c8b 100644
--- a/slirp/tcp_input.c
+++ b/slirp/tcp_input.c
@@ -256,11 +256,6 @@  tcp_input(struct mbuf *m, int iphlen, struct socket *inso)
 	}
 	slirp = m->slirp;
 
-	/*
-	 * Get IP and TCP header together in first mbuf.
-	 * Note: IP leaves IP header in first mbuf.
-	 */
-	ti = mtod(m, struct tcpiphdr *);
 	if (iphlen > sizeof(struct ip )) {
 	  ip_stripoptions(m, (struct mbuf *)0);
 	  iphlen=sizeof(struct ip );
@@ -277,14 +272,28 @@  tcp_input(struct mbuf *m, int iphlen, struct socket *inso)
 	save_ip.ip_len+= iphlen;
 
 	/*
+	 * Get IP and TCP header together in first mbuf.
+	 * Note: IP leaves IP header in first mbuf.
+	 */
+	m->m_data -= sizeof(struct tcpiphdr) - (sizeof(struct ip)
+	                                     + sizeof(struct tcphdr));
+	m->m_len += sizeof(struct tcpiphdr) - (sizeof(struct ip)
+	                                    + sizeof(struct tcphdr));
+	ti = mtod(m, struct tcpiphdr *);
+
+	/*
 	 * Checksum extended TCP header and data.
 	 */
-	tlen = ((struct ip *)ti)->ip_len;
-        tcpiphdr2qlink(ti)->next = tcpiphdr2qlink(ti)->prev = NULL;
-        memset(&ti->ti_i.ih_mbuf, 0 , sizeof(struct mbuf_ptr));
-	ti->ti_x1 = 0;
+	tlen = ip->ip_len;
+	tcpiphdr2qlink(ti)->next = tcpiphdr2qlink(ti)->prev = NULL;
+	memset(&ti->ih_mbuf, 0 , sizeof(struct mbuf_ptr));
+	memset(&ti->ti, 0, sizeof(ti->ti));
+	ti->ti_x0 = 0;
+	ti->ti_src = save_ip.ip_src;
+	ti->ti_dst = save_ip.ip_dst;
+	ti->ti_pr = save_ip.ip_p;
 	ti->ti_len = htons((uint16_t)tlen);
-	len = sizeof(struct ip ) + tlen;
+	len = ((sizeof(struct tcpiphdr) - sizeof(struct tcphdr)) + tlen);
 	if(cksum(m, len)) {
 	  goto drop;
 	}
@@ -1475,7 +1484,7 @@  tcp_mss(struct tcpcb *tp, u_int offer)
 	DEBUG_ARG("tp = %p", tp);
 	DEBUG_ARG("offer = %d", offer);
 
-	mss = min(IF_MTU, IF_MRU) - sizeof(struct tcpiphdr);
+	mss = min(IF_MTU, IF_MRU) - sizeof(struct tcphdr) + sizeof(struct ip);
 	if (offer)
 		mss = min(mss, offer);
 	mss = max(mss, 32);
diff --git a/slirp/tcp_output.c b/slirp/tcp_output.c
index 34e4d2e..7fc6a87 100644
--- a/slirp/tcp_output.c
+++ b/slirp/tcp_output.c
@@ -448,15 +448,23 @@  send:
 	 */
 	m->m_len = hdrlen + len; /* XXX Needed? m_len should be correct */
 
-    {
+	struct tcpiphdr tcpiph_save = *(mtod(m, struct tcpiphdr *));
+	m->m_data += sizeof(struct tcpiphdr) - sizeof(struct tcphdr)
+	                                     - sizeof(struct ip);
+	m->m_len  -= sizeof(struct tcpiphdr) - sizeof(struct tcphdr)
+	                                     - sizeof(struct ip);
+	struct ip *ip = mtod(m, struct ip *);
 
-	((struct ip *)ti)->ip_len = m->m_len;
+	ip->ip_len = m->m_len;
+	ip->ip_dst = tcpiph_save.ti_dst;
+	ip->ip_src = tcpiph_save.ti_src;
+	ip->ip_p = tcpiph_save.ti_pr;
 
-	((struct ip *)ti)->ip_ttl = IPDEFTTL;
-	((struct ip *)ti)->ip_tos = so->so_iptos;
+	ip->ip_ttl = IPDEFTTL;
+	ip->ip_tos = so->so_iptos;
 
 	error = ip_output(so, m);
-    }
+
 	if (error) {
 out:
 		return (error);
diff --git a/slirp/tcp_subr.c b/slirp/tcp_subr.c
index b1aa1f2..cd021df 100644
--- a/slirp/tcp_subr.c
+++ b/slirp/tcp_subr.c
@@ -76,9 +76,10 @@  tcp_template(struct tcpcb *tp)
 	register struct tcpiphdr *n = &tp->t_template;
 
 	n->ti_mbuf = NULL;
-	n->ti_x1 = 0;
+	memset(&n->ti, 0, sizeof(n->ti));
+	n->ti_x0 = 0;
 	n->ti_pr = IPPROTO_TCP;
-	n->ti_len = htons(sizeof (struct tcpiphdr) - sizeof (struct ip));
+	n->ti_len = htons(sizeof(struct tcphdr));
 	n->ti_src = so->so_faddr;
 	n->ti_dst = so->so_laddr;
 	n->ti_sport = so->so_fport;
@@ -131,6 +132,7 @@  tcp_respond(struct tcpcb *tp, struct tcpiphdr *ti, struct mbuf *m,
 		m->m_data += IF_MAXLINKHDR;
 		*mtod(m, struct tcpiphdr *) = *ti;
 		ti = mtod(m, struct tcpiphdr *);
+		memset(&ti->ti, 0, sizeof(ti->ti));
 		flags = TH_ACK;
 	} else {
 		/*
@@ -150,8 +152,8 @@  tcp_respond(struct tcpcb *tp, struct tcpiphdr *ti, struct mbuf *m,
 	tlen += sizeof (struct tcpiphdr);
 	m->m_len = tlen;
 
-        ti->ti_mbuf = NULL;
-	ti->ti_x1 = 0;
+	ti->ti_mbuf = NULL;
+	ti->ti_x0 = 0;
 	ti->ti_seq = htonl(seq);
 	ti->ti_ack = htonl(ack);
 	ti->ti_x2 = 0;
@@ -164,12 +166,23 @@  tcp_respond(struct tcpcb *tp, struct tcpiphdr *ti, struct mbuf *m,
 	ti->ti_urp = 0;
 	ti->ti_sum = 0;
 	ti->ti_sum = cksum(m, tlen);
-	((struct ip *)ti)->ip_len = tlen;
 
-	if(flags & TH_RST)
-	  ((struct ip *)ti)->ip_ttl = MAXTTL;
-	else
-	  ((struct ip *)ti)->ip_ttl = IPDEFTTL;
+	struct tcpiphdr tcpiph_save = *(mtod(m, struct tcpiphdr *));
+	m->m_data += sizeof(struct tcpiphdr) - sizeof(struct tcphdr)
+	                                     - sizeof(struct ip);
+	m->m_len  -= sizeof(struct tcpiphdr) - sizeof(struct tcphdr)
+	                                     - sizeof(struct ip);
+	struct ip *ip = mtod(m, struct ip *);
+	ip->ip_len = tlen;
+	ip->ip_dst = tcpiph_save.ti_dst;
+	ip->ip_src = tcpiph_save.ti_src;
+	ip->ip_p = tcpiph_save.ti_pr;
+
+	if (flags & TH_RST) {
+		ip->ip_ttl = MAXTTL;
+	} else {
+		ip->ip_ttl = IPDEFTTL;
+	}
 
 	(void) ip_output((struct socket *)0, m);
 }
diff --git a/slirp/tcpip.h b/slirp/tcpip.h
index 7974ce3..d9b5d70 100644
--- a/slirp/tcpip.h
+++ b/slirp/tcpip.h
@@ -37,15 +37,23 @@ 
  * Tcp+ip header, after ip options removed.
  */
 struct tcpiphdr {
-	struct 	ipovly ti_i;		/* overlaid ip structure */
-	struct	tcphdr ti_t;		/* tcp header */
+    struct mbuf_ptr ih_mbuf;	/* backpointer to mbuf */
+    union {
+        struct {
+            struct  in_addr ih_src; /* source internet address */
+            struct  in_addr ih_dst; /* destination internet address */
+            uint8_t ih_x1;          /* (unused) */
+            uint8_t ih_pr;          /* protocol */
+        } ti_i4;
+    } ti;
+    uint16_t    ti_x0;
+    uint16_t    ti_len;             /* protocol length */
+    struct      tcphdr ti_t;        /* tcp header */
 };
-#define	ti_mbuf		ti_i.ih_mbuf.mptr
-#define	ti_x1		ti_i.ih_x1
-#define	ti_pr		ti_i.ih_pr
-#define	ti_len		ti_i.ih_len
-#define	ti_src		ti_i.ih_src
-#define	ti_dst		ti_i.ih_dst
+#define	ti_mbuf		ih_mbuf.mptr
+#define	ti_pr		ti.ti_i4.ih_pr
+#define	ti_src		ti.ti_i4.ih_src
+#define	ti_dst		ti.ti_i4.ih_dst
 #define	ti_sport	ti_t.th_sport
 #define	ti_dport	ti_t.th_dport
 #define	ti_seq		ti_t.th_seq
@@ -65,6 +73,13 @@  struct tcpiphdr {
 #define tcpfrag_list_end(F, T) (tcpiphdr2qlink(F) == (struct qlink*)(T))
 #define tcpfrag_list_empty(T) ((T)->seg_next == (struct tcpiphdr*)(T))
 
+/* This is the difference between the size of a tcpiphdr structure, and the
+ * size of actual ip+tcp headers, rounded up since we need to align data.  */
+#define TCPIPHDR_DELTA\
+    (max(0,\
+         (sizeof(struct tcpiphdr)\
+          - sizeof(struct ip) - sizeof(struct tcphdr) + 3)&~3))
+
 /*
  * Just a clean way to get to the first byte
  * of the packet