[B.A.T.M.A.N.] [RFCv2] batctl tcpdump: add IPv6 support to tcpdump parser

[B.A.T.M.A.N.] [RFCv2] batctl tcpdump: add IPv6 support to tcpdump parser

[Marco Dalla Torre]

Extends 'tcpdump' functionality in 'batctl' to parse and report IPv6
communications. 'batctl' now recognizes TCP, UDP packets and the most
common ICMPv6 packet types:
	-ECHO request and ECHO reply
	-unreachable destination and subcases
	-time exceeded
	-neighbor discovery protocol 'solicit' and 'advert'

TCP and UDP parsing code has been moved from the IPv4 specific
codepath to separate functions in order to allow reuse regardless
the underlying protocol.

Signed-off-by: Marco Dalla Torre <marco.dallato@gmail.com>
---
CHANGES FROM v1
-Fixed code formatting style
 tcpdump.c | 308 ++++++++++++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 250 insertions(+), 58 deletions(-)

diff --git a/tcpdump.c b/tcpdump.c
index 7e0987b..3904667 100644
--- a/tcpdump.c
+++ b/tcpdump.c
@@ -32,9 +32,11 @@
 #include <net/if_arp.h>
 #include <netinet/in.h>
 #include <netinet/ip.h>
+#include <netinet/ip6.h>
 #include <netinet/tcp.h>
 #include <netinet/udp.h>
 #include <netinet/ip_icmp.h>
+#include <netinet/icmp6.h>
 #include <netinet/if_ether.h>
 
 #include "main.h"
@@ -47,6 +49,8 @@
 #define ETH_P_BATMAN	0x4305
 #endif /* ETH_P_BATMAN */
 
+#define IPV6_MIN_MTU	1280
+
 #define LEN_CHECK(buff_len, check_len, desc) \
 if ((size_t)(buff_len) < (check_len)) { \
 	fprintf(stderr, "Warning - dropping received %s packet as it is smaller than expected (%zu): %zu\n", \
@@ -188,11 +192,210 @@ static void dump_arp(unsigned char *packet_buff, ssize_t buff_len,
 	}
 }
 
+static void parse_tcp(
+	unsigned char *packet_buff,
+	ssize_t buff_len,
+	size_t header_len,
+	char *src_addr,
+	char *dst_addr)
+{
+	LEN_CHECK((size_t)buff_len - header_len,
+		sizeof(struct tcphdr), "TCP");
+	struct tcphdr *tcphdr;
+
+	tcphdr = (struct tcphdr *)(packet_buff + header_len);
+	printf("IP %s.%i > ", src_addr, ntohs(tcphdr->source));
+	printf("%s.%i: TCP, flags [%c%c%c%c%c%c], length %zu\n",
+		dst_addr, ntohs(tcphdr->dest),
+		(tcphdr->fin ? 'F' : '.'), (tcphdr->syn ? 'S' : '.'),
+		(tcphdr->rst ? 'R' : '.'), (tcphdr->psh ? 'P' : '.'),
+		(tcphdr->ack ? 'A' : '.'), (tcphdr->urg ? 'U' : '.'),
+		(size_t)buff_len - header_len - sizeof(struct tcphdr));
+}
+
+static void parse_udp(
+	unsigned char *packet_buff,
+	ssize_t buff_len,
+	size_t header_len,
+	char *src_addr,
+	char *dst_addr)
+{
+	LEN_CHECK((size_t)buff_len - header_len, sizeof(struct udphdr), "UDP");
+	struct udphdr *udphdr;
+
+	udphdr = (struct udphdr *)(packet_buff + header_len);
+	printf("IP %s.%i > ",
+		src_addr,
+		ntohs(udphdr->source));
+
+	switch (ntohs(udphdr->dest)) {
+	case 67:
+		LEN_CHECK(
+			(size_t)buff_len - header_len - sizeof(struct udphdr),
+			(size_t) 44,
+			"DHCP");
+		printf("%s.67: BOOTP/DHCP, Request from %s, length %zu\n",
+			dst_addr,
+			ether_ntoa_long(
+				(struct ether_addr *)(((char *)udphdr)
+					+ sizeof(struct udphdr) + 28)),
+			(size_t)buff_len - header_len - sizeof(struct udphdr));
+		break;
+	case 68:
+		printf("%s.68: BOOTP/DHCP, Reply, length %zu\n",
+			dst_addr,
+			(size_t)buff_len - header_len - sizeof(struct udphdr));
+		break;
+	default:
+		printf("%s.%i: UDP, length %zu\n",
+			dst_addr,
+			ntohs(udphdr->dest),
+			(size_t)buff_len - header_len - sizeof(struct udphdr));
+		break;
+	}
+}
+
+static void dump_ipv6(
+	unsigned char *packet_buff,
+	ssize_t buff_len,
+	int time_printed)
+{
+	struct ip6_hdr *iphdr, *tmp_iphdr;
+	struct udphdr *tmp_udphdr;
+	struct icmp6_hdr *icmphdr;
+
+	char ip_saddr[40];
+	char ip_daddr[40];
+	char nd_na_target[40];
+	char nd_ipv6_addr[40];
+	struct nd_neighbor_solicit *nd_neigh_sol;
+	struct nd_neighbor_advert *nd_advert;
+
+	iphdr = (struct ip6_hdr *)packet_buff;
+	LEN_CHECK((size_t)buff_len, (size_t)(sizeof(struct ip6_hdr)), "IPv6");
+	
+	if (!time_printed)
+		print_time();
+
+	switch (iphdr->ip6_nxt) {
+	case IPPROTO_ICMPV6:
+		LEN_CHECK(
+			(size_t)buff_len - (size_t)(sizeof(struct ip6_hdr)),
+			sizeof(struct icmp6_hdr),
+			"ICMPv6");
+		icmphdr = (struct icmp6_hdr *)(packet_buff +
+					       sizeof(struct ip6_hdr));
+		inet_ntop(AF_INET6, &(iphdr->ip6_src), nd_ipv6_addr, 40);
+		printf("IPv6 %s > ", nd_ipv6_addr);
+		switch (icmphdr->icmp6_type) {
+		case ICMP6_DST_UNREACH:
+			if ((size_t)(buff_len) < IPV6_MIN_MTU) {
+				fprintf(stderr,
+					"Warning - dropping received 'ICMPv6 destination unreached' packet as it is bigger than maximum allowed size (%u): %zu\n",
+					IPV6_MIN_MTU, (size_t)(buff_len));
+				return;
+			}
+
+			switch (icmphdr->icmp6_code) {
+			case ICMP6_DST_UNREACH_NOPORT:
+				tmp_iphdr = (struct ip6_hdr *)(((char *)icmphdr)
+					    + sizeof(struct icmp6_hdr));
+
+				tmp_udphdr = (struct udphdr *)
+					     ((char *)tmp_iphdr)
+					     + sizeof(struct ip6_hdr);
+
+				inet_ntop(AF_INET6, &(iphdr->ip6_dst),
+					  nd_ipv6_addr, 40);
+				printf("%s: ICMP ", nd_ipv6_addr);
+				inet_ntop(AF_INET6, &(tmp_iphdr->ip6_dst),
+					  nd_ipv6_addr, 40);
+				printf("%s udp port %hu unreachable, length %zu\n",
+				       nd_ipv6_addr, ntohs(tmp_udphdr->dest),
+				       (size_t)buff_len -
+				       sizeof(struct ip6_hdr));
+				break;
+			default:
+				inet_ntop(AF_INET6, &(iphdr->ip6_dst),
+					  nd_ipv6_addr, 40);
+				printf("%s: ICMP unreachable %hhu, length %zu\n",
+					nd_ipv6_addr, icmphdr->icmp6_code,
+					(size_t)buff_len -
+					sizeof(struct icmp6_hdr));
+			}
+			break;
+		case ICMP6_ECHO_REQUEST:
+			inet_ntop(AF_INET6, &(iphdr->ip6_dst), nd_ipv6_addr,
+				  40);
+			printf("%s: ICMPv6 echo request, id: %d, seq: %d, length: %hu\n",
+			       nd_ipv6_addr, icmphdr->icmp6_id,
+			       icmphdr->icmp6_seq, iphdr->ip6_plen);
+			break;
+		case ICMP6_ECHO_REPLY:
+			inet_ntop(AF_INET6, &(iphdr->ip6_dst),
+				  nd_ipv6_addr, 40);
+			printf("%s: ICMPv6 echo reply, id: %d, seq: %d, length: %hu\n",
+			       nd_ipv6_addr, icmphdr->icmp6_id,
+			       icmphdr->icmp6_seq, iphdr->ip6_plen);
+			break;
+		case ICMP6_TIME_EXCEEDED:
+			inet_ntop(AF_INET6, &(iphdr->ip6_dst), nd_ipv6_addr,
+				  40);
+			printf("%s: ICMPv6 time exceeded in-transit, length %zu\n",
+			       nd_ipv6_addr,
+			       (size_t)buff_len - sizeof(struct icmp6_hdr));
+			break;
+		case ND_NEIGHBOR_SOLICIT:
+			nd_neigh_sol = (struct nd_neighbor_solicit *)icmphdr;
+
+			inet_ntop(AF_INET6, &(nd_neigh_sol->nd_ns_target),
+				  nd_ipv6_addr, 40);
+			printf("NEIGHBOR SOLICITATION, Request who-has %s",
+			       nd_ipv6_addr);
+
+			inet_ntop(AF_INET6, &(iphdr->ip6_src), nd_ipv6_addr,
+				  40);
+			printf(" tell %s, length %zd\n", nd_ipv6_addr,
+			       buff_len);
+			break;
+		case ND_NEIGHBOR_ADVERT:
+			nd_advert = (struct nd_neighbor_advert *)icmphdr;
+			inet_ntop(AF_INET6, &(nd_advert->nd_na_target),
+				  nd_na_target, 40);
+			inet_ntop(AF_INET6, &(iphdr->ip6_dst), nd_ipv6_addr,
+				  40);
+			printf("NEIGHBOR ADVERTISEMENT, Reply to %s for target %s, length %zd\n",
+			       nd_na_target, nd_ipv6_addr, buff_len);
+			break;
+		default:
+			inet_ntop(AF_INET6, &(iphdr->ip6_dst), nd_ipv6_addr,
+				  40);
+			printf("%s: ICMPv6 type %hhu, length %zu\n",
+			       nd_ipv6_addr, icmphdr->icmp6_type,
+			       (size_t)buff_len - sizeof(struct icmp6_hdr));
+		}
+		break;
+	case IPPROTO_TCP:
+		inet_ntop(AF_INET6, &(iphdr->ip6_src), ip_saddr, 40);
+		inet_ntop(AF_INET6, &(iphdr->ip6_dst), ip_daddr, 40);
+		parse_tcp(packet_buff, buff_len, sizeof(struct ip6_hdr),
+			  ip_saddr, ip_daddr);
+		break;
+	case IPPROTO_UDP:
+		inet_ntop(AF_INET6, &(iphdr->ip6_src), ip_saddr, 40);
+		inet_ntop(AF_INET6, &(iphdr->ip6_dst), ip_daddr, 40);
+		parse_udp(packet_buff, buff_len, sizeof(struct ip6_hdr),
+			  ip_saddr, ip_daddr);
+		break;
+	default:
+		printf("IPv6 unknown protocol: %i\n", iphdr->ip6_nxt);
+	}
+}
+
 static void dump_ip(unsigned char *packet_buff, ssize_t buff_len, int time_printed)
 {
 	struct iphdr *iphdr, *tmp_iphdr;
-	struct tcphdr *tcphdr;
-	struct udphdr *udphdr, *tmp_udphdr;
+	struct udphdr *tmp_udphdr;
 	struct icmphdr *icmphdr;
 
 	iphdr = (struct iphdr *)packet_buff;
@@ -203,7 +406,8 @@ static void dump_ip(unsigned char *packet_buff, ssize_t buff_len, int time_print
 
 	switch (iphdr->protocol) {
 	case IPPROTO_ICMP:
-		LEN_CHECK((size_t)buff_len - (iphdr->ihl * 4), sizeof(struct icmphdr), "ICMP");
+		LEN_CHECK((size_t)buff_len - (iphdr->ihl * 4),
+			  sizeof(struct icmphdr), "ICMP");
 
 		icmphdr = (struct icmphdr *)(packet_buff + (iphdr->ihl * 4));
 		printf("IP %s > ", inet_ntoa(*(struct in_addr *)&iphdr->saddr));
@@ -212,91 +416,75 @@ static void dump_ip(unsigned char *packet_buff, ssize_t buff_len, int time_print
 		case ICMP_ECHOREPLY:
 			printf("%s: ICMP echo reply, id %hu, seq %hu, length %zu\n",
 				inet_ntoa(*(struct in_addr *)&iphdr->daddr),
-				ntohs(icmphdr->un.echo.id), ntohs(icmphdr->un.echo.sequence),
+				ntohs(icmphdr->un.echo.id),
+				ntohs(icmphdr->un.echo.sequence),
 				(size_t)buff_len - (iphdr->ihl * 4));
 			break;
 		case ICMP_DEST_UNREACH:
-			LEN_CHECK((size_t)buff_len - (iphdr->ihl * 4) - sizeof(struct icmphdr),
-				sizeof(struct iphdr) + 8, "ICMP DEST_UNREACH");
+			LEN_CHECK((size_t)buff_len - (iphdr->ihl * 4) -
+				  sizeof(struct icmphdr),
+				  sizeof(struct iphdr) + 8,
+				  "ICMP DEST_UNREACH");
 
 			switch (icmphdr->code) {
 			case ICMP_PORT_UNREACH:
-				tmp_iphdr = (struct iphdr *)(((char *)icmphdr) + sizeof(struct icmphdr));
-				tmp_udphdr = (struct udphdr *)(((char *)tmp_iphdr) + (tmp_iphdr->ihl * 4));
+				tmp_iphdr = (struct iphdr *)(((char *)icmphdr) +
+					     sizeof(struct icmphdr));
+				tmp_udphdr = (struct udphdr *)
+				             (((char *)tmp_iphdr) +
+				             (tmp_iphdr->ihl * 4));
 
-				printf("%s: ICMP ", inet_ntoa(*(struct in_addr *)&iphdr->daddr));
+				printf("%s: ICMP ",
+				       inet_ntoa(*(struct in_addr *)
+				       	         &iphdr->daddr));
 				printf("%s udp port %hu unreachable, length %zu\n",
-					inet_ntoa(*(struct in_addr *)&tmp_iphdr->daddr),
-					ntohs(tmp_udphdr->dest), (size_t)buff_len - (iphdr->ihl * 4));
+				       inet_ntoa(*(struct in_addr *)
+				       	         &tmp_iphdr->daddr),
+				       ntohs(tmp_udphdr->dest),
+				       (size_t)buff_len - (iphdr->ihl * 4));
 				break;
 			default:
 				printf("%s: ICMP unreachable %hhu, length %zu\n",
-					inet_ntoa(*(struct in_addr *)&iphdr->daddr),
-					icmphdr->code, (size_t)buff_len - (iphdr->ihl * 4));
+				       inet_ntoa(*(struct in_addr *)
+				       	         &iphdr->daddr),
+				       icmphdr->code,
+				       (size_t)buff_len - (iphdr->ihl * 4));
 				break;
 			}
 
 			break;
 		case ICMP_ECHO:
 			printf("%s: ICMP echo request, id %hu, seq %hu, length %zu\n",
-				inet_ntoa(*(struct in_addr *)&iphdr->daddr),
-				ntohs(icmphdr->un.echo.id), ntohs(icmphdr->un.echo.sequence),
-				(size_t)buff_len - (iphdr->ihl * 4));
+			       inet_ntoa(*(struct in_addr *)&iphdr->daddr),
+			       ntohs(icmphdr->un.echo.id),
+			       ntohs(icmphdr->un.echo.sequence),
+			       (size_t)buff_len - (iphdr->ihl * 4));
 			break;
 		case ICMP_TIME_EXCEEDED:
 			printf("%s: ICMP time exceeded in-transit, length %zu\n",
-				inet_ntoa(*(struct in_addr *)&iphdr->daddr),
-				(size_t)buff_len - (iphdr->ihl * 4));
+			       inet_ntoa(*(struct in_addr *)&iphdr->daddr),
+			       (size_t)buff_len - (iphdr->ihl * 4));
 			break;
 		default:
 			printf("%s: ICMP type %hhu, length %zu\n",
-				inet_ntoa(*(struct in_addr *)&iphdr->daddr), icmphdr->type,
-				(size_t)buff_len - (iphdr->ihl * 4));
+			       inet_ntoa(*(struct in_addr *)&iphdr->daddr),
+			       icmphdr->type,
+			       (size_t)buff_len - (iphdr->ihl * 4));
 			break;
 		}
 
 		break;
 	case IPPROTO_TCP:
-		LEN_CHECK((size_t)buff_len - (iphdr->ihl * 4), sizeof(struct tcphdr), "TCP");
-
-		tcphdr = (struct tcphdr *)(packet_buff + (iphdr->ihl * 4));
-		printf("IP %s.%i > ", inet_ntoa(*(struct in_addr *)&iphdr->saddr), ntohs(tcphdr->source));
-		printf("%s.%i: TCP, flags [%c%c%c%c%c%c], length %zu\n",
-			inet_ntoa(*(struct in_addr *)&iphdr->daddr), ntohs(tcphdr->dest),
-			(tcphdr->fin ? 'F' : '.'), (tcphdr->syn ? 'S' : '.'),
-			(tcphdr->rst ? 'R' : '.'), (tcphdr->psh ? 'P' : '.'),
-			(tcphdr->ack ? 'A' : '.'), (tcphdr->urg ? 'U' : '.'),
-			(size_t)buff_len - (iphdr->ihl * 4) - sizeof(struct tcphdr));
+		parse_tcp(
+			packet_buff, buff_len, iphdr->ihl * 4,
+			inet_ntoa(*(struct in_addr *)&iphdr->saddr),
+			inet_ntoa(*(struct in_addr *)&iphdr->daddr));
 		break;
 	case IPPROTO_UDP:
-		LEN_CHECK((size_t)buff_len - (iphdr->ihl * 4), sizeof(struct udphdr), "UDP");
-
-		udphdr = (struct udphdr *)(packet_buff + (iphdr->ihl * 4));
-		printf("IP %s.%i > ", inet_ntoa(*(struct in_addr *)&iphdr->saddr), ntohs(udphdr->source));
-
-		switch (ntohs(udphdr->dest)) {
-		case 67:
-                        LEN_CHECK((size_t)buff_len - (iphdr->ihl * 4) - sizeof(struct udphdr), (size_t) 44, "DHCP");
-			printf("%s.67: BOOTP/DHCP, Request from %s, length %zu\n",
-				inet_ntoa(*(struct in_addr *)&iphdr->daddr),
-				ether_ntoa_long((struct ether_addr *)(((char *)udphdr) + sizeof(struct udphdr) + 28)),
-				(size_t)buff_len - (iphdr->ihl * 4) - sizeof(struct udphdr));
-			break;
-		case 68:
-			printf("%s.68: BOOTP/DHCP, Reply, length %zu\n",
-				inet_ntoa(*(struct in_addr *)&iphdr->daddr),
-				(size_t)buff_len - (iphdr->ihl * 4) - sizeof(struct udphdr));
-			break;
-		default:
-			printf("%s.%i: UDP, length %zu\n",
-				inet_ntoa(*(struct in_addr *)&iphdr->daddr), ntohs(udphdr->dest),
-				(size_t)buff_len - (iphdr->ihl * 4) - sizeof(struct udphdr));
-			break;
-		}
-
-		break;
-	case IPPROTO_IPV6:
-		printf("IP6: not implemented yet\n");
+		parse_udp(
+			packet_buff, buff_len, iphdr->ihl * 4,
+			inet_ntoa(*(struct in_addr *)&iphdr->saddr),
+			inet_ntoa(*(struct in_addr *)&iphdr->daddr));
 		break;
 	default:
 		printf("IP unknown protocol: %i\n", iphdr->protocol);
@@ -497,6 +685,10 @@ static void parse_eth_hdr(unsigned char *packet_buff, ssize_t buff_len, int read
 		if ((dump_level & DUMP_TYPE_NONBAT) || (time_printed))
 			dump_ip(packet_buff + ETH_HLEN, buff_len - ETH_HLEN, time_printed);
 		break;
+	case ETH_P_IPV6:
+		if ((dump_level & DUMP_TYPE_NONBAT) || (time_printed))
+			dump_ipv6(packet_buff + ETH_HLEN, buff_len - ETH_HLEN, time_printed);
+		break;
 	case ETH_P_8021Q:
 		if ((dump_level & DUMP_TYPE_NONBAT) || (time_printed))
 			dump_vlan(packet_buff, buff_len, read_opt, time_printed);
-- 
1.8.3.2

Re: [B.A.T.M.A.N.] [RFCv2] batctl tcpdump: add IPv6 support to tcpdump parser

[Antonio Quartulli]

[-- Attachment #1: Type: text/plain, Size: 16079 bytes --]

On Tue, Sep 17, 2013 at 02:08:14PM +0200, Marco Dalla Torre wrote:
> @@ -47,6 +49,8 @@
>  #define ETH_P_BATMAN	0x4305
>  #endif /* ETH_P_BATMAN */
>  
> +#define IPV6_MIN_MTU	1280

I have it in /usr/include/linux/ipv6.h don't you?
If it has been introduced in a recent version of the headers, please add an
#ifndef to avoid defining the symbol twice.

> +
>  #define LEN_CHECK(buff_len, check_len, desc) \
>  if ((size_t)(buff_len) < (check_len)) { \
>  	fprintf(stderr, "Warning - dropping received %s packet as it is smaller than expected (%zu): %zu\n", \
> @@ -188,11 +192,210 @@ static void dump_arp(unsigned char *packet_buff, ssize_t buff_len,
>  	}
>  }
>  
> +static void parse_tcp(

Why don't you call this function dump_tcp like all the other dump_* ?

> +	unsigned char *packet_buff,
> +	ssize_t buff_len,
> +	size_t header_len,
> +	char *src_addr,
> +	char *dst_addr)

Did we discuss this "freestyle" alignment in the last RFC?

> +{
> +	LEN_CHECK((size_t)buff_len - header_len,
> +		sizeof(struct tcphdr), "TCP");
> +	struct tcphdr *tcphdr;

put all the declarations first and then perform the LEN_CHECK, like int he other
functions.

> +
> +	tcphdr = (struct tcphdr *)(packet_buff + header_len);
> +	printf("IP %s.%i > ", src_addr, ntohs(tcphdr->source));
> +	printf("%s.%i: TCP, flags [%c%c%c%c%c%c], length %zu\n",
> +		dst_addr, ntohs(tcphdr->dest),
> +		(tcphdr->fin ? 'F' : '.'), (tcphdr->syn ? 'S' : '.'),
> +		(tcphdr->rst ? 'R' : '.'), (tcphdr->psh ? 'P' : '.'),
> +		(tcphdr->ack ? 'A' : '.'), (tcphdr->urg ? 'U' : '.'),
> +		(size_t)buff_len - header_len - sizeof(struct tcphdr));
> +}
> +
> +static void parse_udp(
> +	unsigned char *packet_buff,
> +	ssize_t buff_len,
> +	size_t header_len,
> +	char *src_addr,
> +	char *dst_addr)

as before

> +{
> +	LEN_CHECK((size_t)buff_len - header_len, sizeof(struct udphdr), "UDP");
> +	struct udphdr *udphdr;

as before

> +
> +	udphdr = (struct udphdr *)(packet_buff + header_len);
> +	printf("IP %s.%i > ",
> +		src_addr,
> +		ntohs(udphdr->source));

does not fit on one line?

> +
> +	switch (ntohs(udphdr->dest)) {
> +	case 67:
> +		LEN_CHECK(
> +			(size_t)buff_len - header_len - sizeof(struct udphdr),
> +			(size_t) 44,
> +			"DHCP");

same alignment problem as before. If one argument is too long, feel free to use
a temporary variable.

> +		printf("%s.67: BOOTP/DHCP, Request from %s, length %zu\n",
> +			dst_addr,
> +			ether_ntoa_long(
> +				(struct ether_addr *)(((char *)udphdr)
> +					+ sizeof(struct udphdr) + 28)),

alignment.

> +			(size_t)buff_len - header_len - sizeof(struct udphdr));
> +		break;
> +	case 68:
> +		printf("%s.68: BOOTP/DHCP, Reply, length %zu\n",
> +			dst_addr,

you can put arguments after the format string if there is enough space.

> +			(size_t)buff_len - header_len - sizeof(struct udphdr));
> +		break;
> +	default:
> +		printf("%s.%i: UDP, length %zu\n",
> +			dst_addr,
> +			ntohs(udphdr->dest),
> +			(size_t)buff_len - header_len - sizeof(struct udphdr));
> +		break;
> +	}
> +}
> +
> +static void dump_ipv6(
> +	unsigned char *packet_buff,
> +	ssize_t buff_len,
> +	int time_printed)

alignment.

> +{
> +	struct ip6_hdr *iphdr, *tmp_iphdr;
> +	struct udphdr *tmp_udphdr;
> +	struct icmp6_hdr *icmphdr;
> +
> +	char ip_saddr[40];
> +	char ip_daddr[40];
> +	char nd_na_target[40];
> +	char nd_ipv6_addr[40];

you can put these ones on one line too..

> +	struct nd_neighbor_solicit *nd_neigh_sol;
> +	struct nd_neighbor_advert *nd_advert;
> +
> +	iphdr = (struct ip6_hdr *)packet_buff;
> +	LEN_CHECK((size_t)buff_len, (size_t)(sizeof(struct ip6_hdr)), "IPv6");
> +	

there is a useless tab in the line above.

> +	if (!time_printed)
> +		print_time();
> +
> +	switch (iphdr->ip6_nxt) {
> +	case IPPROTO_ICMPV6:
> +		LEN_CHECK(
> +			(size_t)buff_len - (size_t)(sizeof(struct ip6_hdr)),
> +			sizeof(struct icmp6_hdr),
> +			"ICMPv6");

alignment.

> +		icmphdr = (struct icmp6_hdr *)(packet_buff +
> +					       sizeof(struct ip6_hdr));
> +		inet_ntop(AF_INET6, &(iphdr->ip6_src), nd_ipv6_addr, 40);
> +		printf("IPv6 %s > ", nd_ipv6_addr);
> +		switch (icmphdr->icmp6_type) {
> +		case ICMP6_DST_UNREACH:
> +			if ((size_t)(buff_len) < IPV6_MIN_MTU) {
> +				fprintf(stderr,
> +					"Warning - dropping received 'ICMPv6 destination unreached' packet as it is bigger than maximum allowed size (%u): %zu\n",
> +					IPV6_MIN_MTU, (size_t)(buff_len));
> +				return;
> +			}
> +
> +			switch (icmphdr->icmp6_code) {
> +			case ICMP6_DST_UNREACH_NOPORT:
> +				tmp_iphdr = (struct ip6_hdr *)(((char *)icmphdr)
> +					    + sizeof(struct icmp6_hdr));
> +
> +				tmp_udphdr = (struct udphdr *)
> +					     ((char *)tmp_iphdr)

there is no need for the parenthesis around the cast. and I'd suggest to cast to
uint8_t or unsigned char. That is the 'recommended' type for this kind of pointer
operations.

> +					     + sizeof(struct ip6_hdr);
> +
> +				inet_ntop(AF_INET6, &(iphdr->ip6_dst),
> +					  nd_ipv6_addr, 40);
> +				printf("%s: ICMP ", nd_ipv6_addr);
> +				inet_ntop(AF_INET6, &(tmp_iphdr->ip6_dst),
> +					  nd_ipv6_addr, 40);
> +				printf("%s udp port %hu unreachable, length %zu\n",
> +				       nd_ipv6_addr, ntohs(tmp_udphdr->dest),
> +				       (size_t)buff_len -
> +				       sizeof(struct ip6_hdr));
> +				break;
> +			default:
> +				inet_ntop(AF_INET6, &(iphdr->ip6_dst),
> +					  nd_ipv6_addr, 40);
> +				printf("%s: ICMP unreachable %hhu, length %zu\n",
> +					nd_ipv6_addr, icmphdr->icmp6_code,
> +					(size_t)buff_len -
> +					sizeof(struct icmp6_hdr));
> +			}
> +			break;
> +		case ICMP6_ECHO_REQUEST:
> +			inet_ntop(AF_INET6, &(iphdr->ip6_dst), nd_ipv6_addr,
> +				  40);
> +			printf("%s: ICMPv6 echo request, id: %d, seq: %d, length: %hu\n",
> +			       nd_ipv6_addr, icmphdr->icmp6_id,
> +			       icmphdr->icmp6_seq, iphdr->ip6_plen);
> +			break;
> +		case ICMP6_ECHO_REPLY:
> +			inet_ntop(AF_INET6, &(iphdr->ip6_dst),
> +				  nd_ipv6_addr, 40);
> +			printf("%s: ICMPv6 echo reply, id: %d, seq: %d, length: %hu\n",
> +			       nd_ipv6_addr, icmphdr->icmp6_id,
> +			       icmphdr->icmp6_seq, iphdr->ip6_plen);
> +			break;
> +		case ICMP6_TIME_EXCEEDED:
> +			inet_ntop(AF_INET6, &(iphdr->ip6_dst), nd_ipv6_addr,
> +				  40);
> +			printf("%s: ICMPv6 time exceeded in-transit, length %zu\n",
> +			       nd_ipv6_addr,
> +			       (size_t)buff_len - sizeof(struct icmp6_hdr));
> +			break;
> +		case ND_NEIGHBOR_SOLICIT:
> +			nd_neigh_sol = (struct nd_neighbor_solicit *)icmphdr;
> +
> +			inet_ntop(AF_INET6, &(nd_neigh_sol->nd_ns_target),
> +				  nd_ipv6_addr, 40);
> +			printf("NEIGHBOR SOLICITATION, Request who-has %s",
> +			       nd_ipv6_addr);

Does (the real) tcpdump print "who-has" also in this case? I have not checked,
but we try to reproduce the same output to avoid confusion and make it easier
to read.

> +
> +			inet_ntop(AF_INET6, &(iphdr->ip6_src), nd_ipv6_addr,
> +				  40);
> +			printf(" tell %s, length %zd\n", nd_ipv6_addr,
> +			       buff_len);
> +			break;
> +		case ND_NEIGHBOR_ADVERT:
> +			nd_advert = (struct nd_neighbor_advert *)icmphdr;
> +			inet_ntop(AF_INET6, &(nd_advert->nd_na_target),
> +				  nd_na_target, 40);
> +			inet_ntop(AF_INET6, &(iphdr->ip6_dst), nd_ipv6_addr,
> +				  40);
> +			printf("NEIGHBOR ADVERTISEMENT, Reply to %s for target %s, length %zd\n",

As above, check what tcpdump prints. I don't think it says "Reply" since there
is no ARP Request/Reply in this case.

> +			       nd_na_target, nd_ipv6_addr, buff_len);
> +			break;
> +		default:
> +			inet_ntop(AF_INET6, &(iphdr->ip6_dst), nd_ipv6_addr,
> +				  40);
> +			printf("%s: ICMPv6 type %hhu, length %zu\n",
> +			       nd_ipv6_addr, icmphdr->icmp6_type,
> +			       (size_t)buff_len - sizeof(struct icmp6_hdr));
> +		}
> +		break;
> +	case IPPROTO_TCP:
> +		inet_ntop(AF_INET6, &(iphdr->ip6_src), ip_saddr, 40);
> +		inet_ntop(AF_INET6, &(iphdr->ip6_dst), ip_daddr, 40);
> +		parse_tcp(packet_buff, buff_len, sizeof(struct ip6_hdr),
> +			  ip_saddr, ip_daddr);
> +		break;
> +	case IPPROTO_UDP:
> +		inet_ntop(AF_INET6, &(iphdr->ip6_src), ip_saddr, 40);
> +		inet_ntop(AF_INET6, &(iphdr->ip6_dst), ip_daddr, 40);
> +		parse_udp(packet_buff, buff_len, sizeof(struct ip6_hdr),
> +			  ip_saddr, ip_daddr);
> +		break;
> +	default:
> +		printf("IPv6 unknown protocol: %i\n", iphdr->ip6_nxt);
> +	}
> +}
> +
>  static void dump_ip(unsigned char *packet_buff, ssize_t buff_len, int time_printed)
>  {
>  	struct iphdr *iphdr, *tmp_iphdr;
> -	struct tcphdr *tcphdr;
> -	struct udphdr *udphdr, *tmp_udphdr;
> +	struct udphdr *tmp_udphdr;
>  	struct icmphdr *icmphdr;
>  
>  	iphdr = (struct iphdr *)packet_buff;
> @@ -203,7 +406,8 @@ static void dump_ip(unsigned char *packet_buff, ssize_t buff_len, int time_print
>  
>  	switch (iphdr->protocol) {
>  	case IPPROTO_ICMP:
> -		LEN_CHECK((size_t)buff_len - (iphdr->ihl * 4), sizeof(struct icmphdr), "ICMP");
> +		LEN_CHECK((size_t)buff_len - (iphdr->ihl * 4),
> +			  sizeof(struct icmphdr), "ICMP");

This change is unrelated to this patch.

>  
>  		icmphdr = (struct icmphdr *)(packet_buff + (iphdr->ihl * 4));
>  		printf("IP %s > ", inet_ntoa(*(struct in_addr *)&iphdr->saddr));
> @@ -212,91 +416,75 @@ static void dump_ip(unsigned char *packet_buff, ssize_t buff_len, int time_print
>  		case ICMP_ECHOREPLY:
>  			printf("%s: ICMP echo reply, id %hu, seq %hu, length %zu\n",
>  				inet_ntoa(*(struct in_addr *)&iphdr->daddr),
> -				ntohs(icmphdr->un.echo.id), ntohs(icmphdr->un.echo.sequence),
> +				ntohs(icmphdr->un.echo.id),
> +				ntohs(icmphdr->un.echo.sequence),

This change is unrelated to this patch.

>  				(size_t)buff_len - (iphdr->ihl * 4));
>  			break;
>  		case ICMP_DEST_UNREACH:
> -			LEN_CHECK((size_t)buff_len - (iphdr->ihl * 4) - sizeof(struct icmphdr),
> -				sizeof(struct iphdr) + 8, "ICMP DEST_UNREACH");
> +			LEN_CHECK((size_t)buff_len - (iphdr->ihl * 4) -
> +				  sizeof(struct icmphdr),
> +				  sizeof(struct iphdr) + 8,
> +				  "ICMP DEST_UNREACH");
>  
>  			switch (icmphdr->code) {
>  			case ICMP_PORT_UNREACH:
> -				tmp_iphdr = (struct iphdr *)(((char *)icmphdr) + sizeof(struct icmphdr));
> -				tmp_udphdr = (struct udphdr *)(((char *)tmp_iphdr) + (tmp_iphdr->ihl * 4));
> +				tmp_iphdr = (struct iphdr *)(((char *)icmphdr) +
> +					     sizeof(struct icmphdr));
> +				tmp_udphdr = (struct udphdr *)
> +				             (((char *)tmp_iphdr) +
> +				             (tmp_iphdr->ihl * 4));

unrelated

>  
> -				printf("%s: ICMP ", inet_ntoa(*(struct in_addr *)&iphdr->daddr));
> +				printf("%s: ICMP ",
> +				       inet_ntoa(*(struct in_addr *)
> +				       	         &iphdr->daddr));

very unrelated

>  				printf("%s udp port %hu unreachable, length %zu\n",
> -					inet_ntoa(*(struct in_addr *)&tmp_iphdr->daddr),
> -					ntohs(tmp_udphdr->dest), (size_t)buff_len - (iphdr->ihl * 4));
> +				       inet_ntoa(*(struct in_addr *)
> +				       	         &tmp_iphdr->daddr),
> +				       ntohs(tmp_udphdr->dest),
> +				       (size_t)buff_len - (iphdr->ihl * 4));

unrelated

>  				break;
>  			default:
>  				printf("%s: ICMP unreachable %hhu, length %zu\n",
> -					inet_ntoa(*(struct in_addr *)&iphdr->daddr),
> -					icmphdr->code, (size_t)buff_len - (iphdr->ihl * 4));
> +				       inet_ntoa(*(struct in_addr *)
> +				       	         &iphdr->daddr),
> +				       icmphdr->code,
> +				       (size_t)buff_len - (iphdr->ihl * 4));

maybe you don't know that with git you can select the changes to include in a
commit? (man git-add)

>  				break;
>  			}
>  
>  			break;
>  		case ICMP_ECHO:
>  			printf("%s: ICMP echo request, id %hu, seq %hu, length %zu\n",
> -				inet_ntoa(*(struct in_addr *)&iphdr->daddr),
> -				ntohs(icmphdr->un.echo.id), ntohs(icmphdr->un.echo.sequence),
> -				(size_t)buff_len - (iphdr->ihl * 4));
> +			       inet_ntoa(*(struct in_addr *)&iphdr->daddr),
> +			       ntohs(icmphdr->un.echo.id),
> +			       ntohs(icmphdr->un.echo.sequence),
> +			       (size_t)buff_len - (iphdr->ihl * 4));

unrelated

>  			break;
>  		case ICMP_TIME_EXCEEDED:
>  			printf("%s: ICMP time exceeded in-transit, length %zu\n",
> -				inet_ntoa(*(struct in_addr *)&iphdr->daddr),
> -				(size_t)buff_len - (iphdr->ihl * 4));
> +			       inet_ntoa(*(struct in_addr *)&iphdr->daddr),
> +			       (size_t)buff_len - (iphdr->ihl * 4));

unrelated

>  			break;
>  		default:
>  			printf("%s: ICMP type %hhu, length %zu\n",
> -				inet_ntoa(*(struct in_addr *)&iphdr->daddr), icmphdr->type,
> -				(size_t)buff_len - (iphdr->ihl * 4));
> +			       inet_ntoa(*(struct in_addr *)&iphdr->daddr),
> +			       icmphdr->type,
> +			       (size_t)buff_len - (iphdr->ihl * 4));

unrelated

>  			break;
>  		}
>  
>  		break;
>  	case IPPROTO_TCP:
> -		LEN_CHECK((size_t)buff_len - (iphdr->ihl * 4), sizeof(struct tcphdr), "TCP");
> -
> -		tcphdr = (struct tcphdr *)(packet_buff + (iphdr->ihl * 4));
> -		printf("IP %s.%i > ", inet_ntoa(*(struct in_addr *)&iphdr->saddr), ntohs(tcphdr->source));
> -		printf("%s.%i: TCP, flags [%c%c%c%c%c%c], length %zu\n",
> -			inet_ntoa(*(struct in_addr *)&iphdr->daddr), ntohs(tcphdr->dest),
> -			(tcphdr->fin ? 'F' : '.'), (tcphdr->syn ? 'S' : '.'),
> -			(tcphdr->rst ? 'R' : '.'), (tcphdr->psh ? 'P' : '.'),
> -			(tcphdr->ack ? 'A' : '.'), (tcphdr->urg ? 'U' : '.'),
> -			(size_t)buff_len - (iphdr->ihl * 4) - sizeof(struct tcphdr));
> +		parse_tcp(
> +			packet_buff, buff_len, iphdr->ihl * 4,
> +			inet_ntoa(*(struct in_addr *)&iphdr->saddr),
> +			inet_ntoa(*(struct in_addr *)&iphdr->daddr));

alignment

>  		break;
>  	case IPPROTO_UDP:
> -		LEN_CHECK((size_t)buff_len - (iphdr->ihl * 4), sizeof(struct udphdr), "UDP");
> -
> -		udphdr = (struct udphdr *)(packet_buff + (iphdr->ihl * 4));
> -		printf("IP %s.%i > ", inet_ntoa(*(struct in_addr *)&iphdr->saddr), ntohs(udphdr->source));
> -
> -		switch (ntohs(udphdr->dest)) {
> -		case 67:
> -                        LEN_CHECK((size_t)buff_len - (iphdr->ihl * 4) - sizeof(struct udphdr), (size_t) 44, "DHCP");
> -			printf("%s.67: BOOTP/DHCP, Request from %s, length %zu\n",
> -				inet_ntoa(*(struct in_addr *)&iphdr->daddr),
> -				ether_ntoa_long((struct ether_addr *)(((char *)udphdr) + sizeof(struct udphdr) + 28)),
> -				(size_t)buff_len - (iphdr->ihl * 4) - sizeof(struct udphdr));
> -			break;
> -		case 68:
> -			printf("%s.68: BOOTP/DHCP, Reply, length %zu\n",
> -				inet_ntoa(*(struct in_addr *)&iphdr->daddr),
> -				(size_t)buff_len - (iphdr->ihl * 4) - sizeof(struct udphdr));
> -			break;
> -		default:
> -			printf("%s.%i: UDP, length %zu\n",
> -				inet_ntoa(*(struct in_addr *)&iphdr->daddr), ntohs(udphdr->dest),
> -				(size_t)buff_len - (iphdr->ihl * 4) - sizeof(struct udphdr));
> -			break;
> -		}
> -
> -		break;
> -	case IPPROTO_IPV6:
> -		printf("IP6: not implemented yet\n");
> +		parse_udp(
> +			packet_buff, buff_len, iphdr->ihl * 4,
> +			inet_ntoa(*(struct in_addr *)&iphdr->saddr),
> +			inet_ntoa(*(struct in_addr *)&iphdr->daddr));
>  		break;
>  	default:
>  		printf("IP unknown protocol: %i\n", iphdr->protocol);
> @@ -497,6 +685,10 @@ static void parse_eth_hdr(unsigned char *packet_buff, ssize_t buff_len, int read
>  		if ((dump_level & DUMP_TYPE_NONBAT) || (time_printed))
>  			dump_ip(packet_buff + ETH_HLEN, buff_len - ETH_HLEN, time_printed);
>  		break;
> +	case ETH_P_IPV6:
> +		if ((dump_level & DUMP_TYPE_NONBAT) || (time_printed))
> +			dump_ipv6(packet_buff + ETH_HLEN, buff_len - ETH_HLEN, time_printed);
> +		break;
>  	case ETH_P_8021Q:
>  		if ((dump_level & DUMP_TYPE_NONBAT) || (time_printed))
>  			dump_vlan(packet_buff, buff_len, read_opt, time_printed);
> -- 
> 1.8.3.2
> 


Cheers,

-- 
Antonio Quartulli

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [B.A.T.M.A.N.] [RFCv2] batctl tcpdump: add IPv6 support to tcpdump parser

[Marco Dalla Torre]

Hi,

On 09/17/13 14:31, Antonio Quartulli wrote:
> On Tue, Sep 17, 2013 at 02:08:14PM +0200, Marco Dalla Torre wrote:
>> >@@ -47,6 +49,8 @@
>> >  #define ETH_P_BATMAN	0x4305
>> >  #endif /* ETH_P_BATMAN */
>> >
>> >+#define IPV6_MIN_MTU	1280
> I have it in /usr/include/linux/ipv6.h don't you?
> If it has been introduced in a recent version of the headers, please add an
> #ifndef to avoid defining the symbol twice.
>

I'm aware of ipv6.h, in fact this is exactly where the definition I used 
comes from. Unfortunately including it in tcpdump.c generates all sort 
of namespace clashes with the other include statements, and I felt that 
doing namespace housecleaning was beyond the scope of the patch...
I'm not sure of what you mean with the #ifndef, since that statement is 
already present in the file.
Can you suggest a solution (or explain better what you meant if that one 
is the solution)?

>> >+
>> >  #define LEN_CHECK(buff_len, check_len, desc) \
>> >  if ((size_t)(buff_len) < (check_len)) { \
>> >  	fprintf(stderr, "Warning - dropping received %s packet as it is smaller than expected (%zu): %zu\n", \
>> >@@ -188,11 +192,210 @@ static void dump_arp(unsigned char *packet_buff, ssize_t buff_len,
>> >  	}
>> >  }
>> >
>> >+static void parse_tcp(
> Why don't you call this function dump_tcp like all the other dump_* ?

Because, I wasn't aware of this rule. Now I know! :P

>
>> >+	unsigned char *packet_buff,
>> >+	ssize_t buff_len,
>> >+	size_t header_len,
>> >+	char *src_addr,
>> >+	char *dst_addr)
> Did we discuss this "freestyle" alignment in the last RFC?
>

For this and all the other styling problems like that: yes sorry when I 
wrote this patch some time ago I wasn't aware of these formatting rules. 
And I clearly failed correcting all while reviewing it...

>> >+{
>> >+	LEN_CHECK((size_t)buff_len - header_len,
>> >+		sizeof(struct tcphdr), "TCP");
>> >+	struct tcphdr *tcphdr;
> put all the declarations first and then perform the LEN_CHECK, like int he other
> functions.
>

Ok, it just didn't feel right to me to declare something even if you 
might even get to use it, but I can understand that from a style 
conformity point of view.

[...]

>> >+
>> >+	udphdr = (struct udphdr *)(packet_buff + header_len);
>> >+	printf("IP %s.%i > ",
>> >+		src_addr,
>> >+		ntohs(udphdr->source));
> does not fit on one line?

Yeah, it does now with the temporary variable, but when I did format it 
before introducing *udphdr was still much longer for one line... oh 
nevermind I'll fix it of course :)

>> >+
>> >+	switch (ntohs(udphdr->dest)) {
>> >+	case 67:
>> >+		LEN_CHECK(
>> >+			(size_t)buff_len - header_len - sizeof(struct udphdr),
>> >+			(size_t) 44,
>> >+			"DHCP");
> same alignment problem as before. If one argument is too long, feel free to use
> a temporary variable.

Ack.

[...]

>> >+			switch (icmphdr->icmp6_code) {
>> >+			case ICMP6_DST_UNREACH_NOPORT:
>> >+				tmp_iphdr = (struct ip6_hdr *)(((char *)icmphdr)
>> >+					    + sizeof(struct icmp6_hdr));
>> >+
>> >+				tmp_udphdr = (struct udphdr *)
>> >+					     ((char *)tmp_iphdr)
> there is no need for the parenthesis around the cast. and I'd suggest to cast to
> uint8_t or unsigned char. That is the 'recommended' type for this kind of pointer
> operations.

I'll keep it in mind, but I hope you don't mind if I sometimes happen to 
use them to clear things up for my poor C-untrained eye :-D

>> >+		case ICMP6_ECHO_REQUEST:
>> >+			inet_ntop(AF_INET6, &(iphdr->ip6_dst), nd_ipv6_addr,
>> >+				  40);
>> >+			printf("%s: ICMPv6 echo request, id: %d, seq: %d, length: %hu\n",
>> >+			       nd_ipv6_addr, icmphdr->icmp6_id,
>> >+			       icmphdr->icmp6_seq, iphdr->ip6_plen);
>> >+			break;
>> >+		case ICMP6_ECHO_REPLY:
>> >+			inet_ntop(AF_INET6, &(iphdr->ip6_dst),
>> >+				  nd_ipv6_addr, 40);
>> >+			printf("%s: ICMPv6 echo reply, id: %d, seq: %d, length: %hu\n",
>> >+			       nd_ipv6_addr, icmphdr->icmp6_id,
>> >+			       icmphdr->icmp6_seq, iphdr->ip6_plen);
>> >+			break;
>> >+		case ICMP6_TIME_EXCEEDED:
>> >+			inet_ntop(AF_INET6, &(iphdr->ip6_dst), nd_ipv6_addr,
>> >+				  40);
>> >+			printf("%s: ICMPv6 time exceeded in-transit, length %zu\n",
>> >+			       nd_ipv6_addr,
>> >+			       (size_t)buff_len - sizeof(struct icmp6_hdr));
>> >+			break;
>> >+		case ND_NEIGHBOR_SOLICIT:
>> >+			nd_neigh_sol = (struct nd_neighbor_solicit *)icmphdr;
>> >+
>> >+			inet_ntop(AF_INET6, &(nd_neigh_sol->nd_ns_target),
>> >+				  nd_ipv6_addr, 40);
>> >+			printf("NEIGHBOR SOLICITATION, Request who-has %s",
>> >+			       nd_ipv6_addr);
> Does (the real) tcpdump print "who-has" also in this case? I have not checked,
> but we try to reproduce the same output to avoid confusion and make it easier
> to read.
>

I wasn't aware of this "output equality" goal, I'll look into it.

>> >+
>> >+			inet_ntop(AF_INET6, &(iphdr->ip6_src), nd_ipv6_addr,
>> >+				  40);
>> >+			printf(" tell %s, length %zd\n", nd_ipv6_addr,
>> >+			       buff_len);
>> >+			break;
>> >+		case ND_NEIGHBOR_ADVERT:
>> >+			nd_advert = (struct nd_neighbor_advert *)icmphdr;
>> >+			inet_ntop(AF_INET6, &(nd_advert->nd_na_target),
>> >+				  nd_na_target, 40);
>> >+			inet_ntop(AF_INET6, &(iphdr->ip6_dst), nd_ipv6_addr,
>> >+				  40);
>> >+			printf("NEIGHBOR ADVERTISEMENT, Reply to %s for target %s, length %zd\n",
> As above, check what tcpdump prints. I don't think it says "Reply" since there
> is no ARP Request/Reply in this case.

Yes, there is no ARP but the semantics in this case are pretty much the 
same. But as before, I'll look into it.

>> >  	switch (iphdr->protocol) {
>> >  	case IPPROTO_ICMP:
>> >-		LEN_CHECK((size_t)buff_len - (iphdr->ihl * 4), sizeof(struct icmphdr), "ICMP");
>> >+		LEN_CHECK((size_t)buff_len - (iphdr->ihl * 4),
>> >+			  sizeof(struct icmphdr), "ICMP");
> This change is unrelated to this patch.

For this and the others unrelated CHANGES: ok, didn't feel wrong to fix 
some offending formatting, but I'll revert those changes.
By the way: now I understand you! Fixing bad code formatting is like 
drugs! :D

>
> Cheers,
>
> -- Antonio Quartulli
>

Thanks,
Marco

Re: [B.A.T.M.A.N.] [RFCv2] batctl tcpdump: add IPv6 support to tcpdump parser

[Antonio Quartulli]

[-- Attachment #1: Type: text/plain, Size: 2329 bytes --]

On Tue, Sep 17, 2013 at 04:01:02PM +0200, Marco Dalla Torre wrote:
> Hi,
> 
> On 09/17/13 14:31, Antonio Quartulli wrote:
> > On Tue, Sep 17, 2013 at 02:08:14PM +0200, Marco Dalla Torre wrote:
> >> >@@ -47,6 +49,8 @@
> >> >  #define ETH_P_BATMAN	0x4305
> >> >  #endif /* ETH_P_BATMAN */
> >> >
> >> >+#define IPV6_MIN_MTU	1280
> > I have it in /usr/include/linux/ipv6.h don't you?
> > If it has been introduced in a recent version of the headers, please add an
> > #ifndef to avoid defining the symbol twice.
> >
> 
> I'm aware of ipv6.h, in fact this is exactly where the definition I used 
> comes from. Unfortunately including it in tcpdump.c generates all sort 
> of namespace clashes with the other include statements, and I felt that 
> doing namespace housecleaning was beyond the scope of the patch...
> I'm not sure of what you mean with the #ifndef, since that statement is 
> already present in the file.
> Can you suggest a solution (or explain better what you meant if that one 
> is the solution)?

oh ok.

my ideas was to do something like:

#ifndef IPV6_MIN_MTU
#define IPV6_MIN_MTU     1280


but given your problem I think there is something else we should care about...I
will check into this namespace issue.

> 
> >> >+
> >> >  #define LEN_CHECK(buff_len, check_len, desc) \
> >> >  if ((size_t)(buff_len) < (check_len)) { \
> >> >  	fprintf(stderr, "Warning - dropping received %s packet as it is smaller than expected (%zu): %zu\n", \
> >> >@@ -188,11 +192,210 @@ static void dump_arp(unsigned char *packet_buff, ssize_t buff_len,
> >> >  	}
> >> >  }
> >> >
> >> >+static void parse_tcp(
> > Why don't you call this function dump_tcp like all the other dump_* ?
> 
> Because, I wasn't aware of this rule. Now I know! :P

They are all over in the same file :-P

> 
> >
> >> >+	unsigned char *packet_buff,
> >> >+	ssize_t buff_len,
> >> >+	size_t header_len,
> >> >+	char *src_addr,
> >> >+	char *dst_addr)
> > Did we discuss this "freestyle" alignment in the last RFC?
> >
> 
> For this and all the other styling problems like that: yes sorry when I 
> wrote this patch some time ago I wasn't aware of these formatting rules. 
> And I clearly failed correcting all while reviewing it...

I guessed so :)

Cheers,

-- 
Antonio Quartulli

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [B.A.T.M.A.N.] [RFCv2] batctl tcpdump: add IPv6 support to tcpdump parser

[Antonio Quartulli]

[-- Attachment #1: Type: text/plain, Size: 972 bytes --]

On Tue, Sep 17, 2013 at 02:08:14PM +0200, Marco Dalla Torre wrote:
> +	switch (iphdr->ip6_nxt) {
> +	case IPPROTO_ICMPV6:
> +		LEN_CHECK(
> +			(size_t)buff_len - (size_t)(sizeof(struct ip6_hdr)),
> +			sizeof(struct icmp6_hdr),
> +			"ICMPv6");
> +		icmphdr = (struct icmp6_hdr *)(packet_buff +
> +					       sizeof(struct ip6_hdr));
> +		inet_ntop(AF_INET6, &(iphdr->ip6_src), nd_ipv6_addr, 40);
> +		printf("IPv6 %s > ", nd_ipv6_addr);
> +		switch (icmphdr->icmp6_type) {
> +		case ICMP6_DST_UNREACH:
> +			if ((size_t)(buff_len) < IPV6_MIN_MTU) {
> +				fprintf(stderr,
> +					"Warning - dropping received 'ICMPv6 destination unreached' packet as it is bigger than maximum allowed size (%u): %zu\n",
> +					IPV6_MIN_MTU, (size_t)(buff_len));

Is the message wrong? should it be the other way around? "packet is smaller than
minimum size" ? And why should _only_ this ICMP packet be bigger of IPV6_MIN_MTU
?

Cheers,

-- 
Antonio Quartulli

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [B.A.T.M.A.N.] [RFCv2] batctl tcpdump: add IPv6 support to tcpdump parser

[Marco Dalla Torre]

Antonio,

On 09/17/13 17:51, Antonio Quartulli wrote:
> On Tue, Sep 17, 2013 at 02:08:14PM +0200, Marco Dalla Torre wrote:
>> +	switch (iphdr->ip6_nxt) {
>> +	case IPPROTO_ICMPV6:
>> +		LEN_CHECK(
>> +			(size_t)buff_len - (size_t)(sizeof(struct ip6_hdr)),
>> +			sizeof(struct icmp6_hdr),
>> +			"ICMPv6");
>> +		icmphdr = (struct icmp6_hdr *)(packet_buff +
>> +					       sizeof(struct ip6_hdr));
>> +		inet_ntop(AF_INET6, &(iphdr->ip6_src), nd_ipv6_addr, 40);
>> +		printf("IPv6 %s > ", nd_ipv6_addr);
>> +		switch (icmphdr->icmp6_type) {
>> +		case ICMP6_DST_UNREACH:
>> +			if ((size_t)(buff_len) < IPV6_MIN_MTU) {
>> +				fprintf(stderr,
>> +					"Warning - dropping received 'ICMPv6 destination unreached' packet as it is bigger than maximum allowed size (%u): %zu\n",
>> +					IPV6_MIN_MTU, (size_t)(buff_len));
>
> Is the message wrong? should it be the other way around? "packet is smaller than
> minimum size" ? And why should _only_ this ICMP packet be bigger of IPV6_MIN_MTU
> ?
>
> Cheers,
>

Ehm, sort of... I believe the error is that the check is the other way 
around. If I remember correctly I included the guard after reading this 
from http://tools.ietf.org/html/rfc4443#section-2.4 :

"Every ICMPv6 error message (type < 128) MUST include as much of
the IPv6 offending (invoking) packet (the packet that caused the
error) as possible without making the error message packet exceed
the minimum IPv6 MTU"

With that in mind, the check should really be:

"((size_t)(buff_len) > IPV6_MIN_MTU"

The message should be ok.
And it should be done for all the error messages.

>>
>>>>> +
>>>>>  #define LEN_CHECK(buff_len, check_len, desc) \
>>>>>  if ((size_t)(buff_len) < (check_len)) { \
>>>>>  	fprintf(stderr, "Warning - dropping received %s packet as it is smaller than expected (%zu): %zu\n", \
>>>>> @@ -188,11 +192,210 @@ static void dump_arp(unsigned char *packet_buff, ssize_t buff_len,
>>>>>  	}
>>>>>  }
>>>>>
>>>>> +static void parse_tcp(
>>> Why don't you call this function dump_tcp like all the other dump_* ?
>>
>> Because, I wasn't aware of this rule. Now I know! :P
>
> They are all over in the same file
>

Yes, but there also was that very one "parse_eth_hdr" that must have 
completely captivated my attention :D
Jokes aside, I'll try to pay more attention to the semantics behind 
prefixes in names. Maybe the problem is that some OO-formed guys like me 
are just not used to have that inside function names... :)


Thanks,
Marco