Re: [PATCH net-next v2 5/5] selftests/net: test ipip packets in gro.sh

[Richard Gobert <richardbgobert@gmail.com>]

Willem de Bruijn wrote:
> Richard Gobert wrote:
>> Add IPIP test-cases to the GRO selftest.
>>
>> This selftest already contains IP ID test-cases. They are now
>> also tested for encapsulated packets.
>>
>> Signed-off-by: Richard Gobert <richardbgobert@gmail.com>
>> ---
>>  tools/testing/selftests/net/gro.c  | 49 ++++++++++++++++++++++++------
>>  tools/testing/selftests/net/gro.sh |  5 +--
>>  2 files changed, 42 insertions(+), 12 deletions(-)
>>
>> diff --git a/tools/testing/selftests/net/gro.c b/tools/testing/selftests/net/gro.c
>> index 3d4a82a2607c..aeb4973418a4 100644
>> --- a/tools/testing/selftests/net/gro.c
>> +++ b/tools/testing/selftests/net/gro.c
>> @@ -93,6 +93,7 @@ static bool tx_socket = true;
>>  static int tcp_offset = -1;
>>  static int total_hdr_len = -1;
>>  static int ethhdr_proto = -1;
>> +static bool ipip;
>>  static const int num_flush_id_cases = 6;
>>  
>>  static void vlog(const char *fmt, ...)
>> @@ -114,7 +115,9 @@ static void setup_sock_filter(int fd)
>>  	int ipproto_off, opt_ipproto_off;
>>  	int next_off;
>>  
>> -	if (proto == PF_INET)
>> +	if (ipip)
>> +		next_off = sizeof(struct iphdr) + offsetof(struct iphdr, protocol);
>> +	else if (proto == PF_INET)
>>  		next_off = offsetof(struct iphdr, protocol);
>>  	else
>>  		next_off = offsetof(struct ipv6hdr, nexthdr);
>> @@ -244,7 +247,7 @@ static void fill_datalinklayer(void *buf)
>>  	eth->h_proto = ethhdr_proto;
>>  }
>>  
>> -static void fill_networklayer(void *buf, int payload_len)
>> +static void fill_networklayer(void *buf, int payload_len, int protocol)
>>  {
>>  	struct ipv6hdr *ip6h = buf;
>>  	struct iphdr *iph = buf;
>> @@ -254,7 +257,7 @@ static void fill_networklayer(void *buf, int payload_len)
>>  
>>  		ip6h->version = 6;
>>  		ip6h->payload_len = htons(sizeof(struct tcphdr) + payload_len);
>> -		ip6h->nexthdr = IPPROTO_TCP;
>> +		ip6h->nexthdr = protocol;
>>  		ip6h->hop_limit = 8;
>>  		if (inet_pton(AF_INET6, addr6_src, &ip6h->saddr) != 1)
>>  			error(1, errno, "inet_pton source ip6");
>> @@ -266,7 +269,7 @@ static void fill_networklayer(void *buf, int payload_len)
>>  		iph->version = 4;
>>  		iph->ihl = 5;
>>  		iph->ttl = 8;
>> -		iph->protocol	= IPPROTO_TCP;
>> +		iph->protocol	= protocol;
>>  		iph->tot_len = htons(sizeof(struct tcphdr) +
>>  				payload_len + sizeof(struct iphdr));
>>  		iph->frag_off = htons(0x4000); /* DF = 1, MF = 0 */
>> @@ -313,9 +316,19 @@ static void create_packet(void *buf, int seq_offset, int ack_offset,
>>  {
>>  	memset(buf, 0, total_hdr_len);
>>  	memset(buf + total_hdr_len, 'a', payload_len);
>> +
>>  	fill_transportlayer(buf + tcp_offset, seq_offset, ack_offset,
>>  			    payload_len, fin);
>> -	fill_networklayer(buf + ETH_HLEN, payload_len);
>> +
>> +	if (ipip) {
>> +		fill_networklayer(buf + ETH_HLEN + sizeof(struct iphdr),
>> +				  payload_len, IPPROTO_TCP);
>> +		fill_networklayer(buf + ETH_HLEN, payload_len + sizeof(struct iphdr),
>> +				  IPPROTO_IPIP);
>> +	} else {
>> +		fill_networklayer(buf + ETH_HLEN, payload_len, IPPROTO_TCP);
>> +	}
>> +
>>  	fill_datalinklayer(buf);
>>  }
>>  
>> @@ -416,6 +429,13 @@ static void recompute_packet(char *buf, char *no_ext, int extlen)
>>  		iph->tot_len = htons(ntohs(iph->tot_len) + extlen);
>>  		iph->check = 0;
>>  		iph->check = checksum_fold(iph, sizeof(struct iphdr), 0);
>> +
>> +		if (ipip) {
>> +			iph += 1;
>> +			iph->tot_len = htons(ntohs(iph->tot_len) + extlen);
>> +			iph->check = 0;
>> +			iph->check = checksum_fold(iph, sizeof(struct iphdr), 0);
>> +		}
>>  	} else {
>>  		ip6h->payload_len = htons(ntohs(ip6h->payload_len) + extlen);
>>  	}
>> @@ -777,7 +797,7 @@ static void send_fragment4(int fd, struct sockaddr_ll *daddr)
>>  	 */
>>  	memset(buf + total_hdr_len, 'a', PAYLOAD_LEN * 2);
>>  	fill_transportlayer(buf + tcp_offset, PAYLOAD_LEN, 0, PAYLOAD_LEN * 2, 0);
>> -	fill_networklayer(buf + ETH_HLEN, PAYLOAD_LEN);
>> +	fill_networklayer(buf + ETH_HLEN, PAYLOAD_LEN, IPPROTO_TCP);
>>  	fill_datalinklayer(buf);
>>  
>>  	iph->frag_off = htons(0x6000); // DF = 1, MF = 1
>> @@ -1071,7 +1091,7 @@ static void gro_sender(void)
>>  		 * and min ipv6hdr size. Like MAX_HDR_SIZE,
>>  		 * MAX_PAYLOAD is defined with the larger header of the two.
>>  		 */
>> -		int offset = proto == PF_INET ? 20 : 0;
>> +		int offset = (proto == PF_INET && !ipip) ? 20 : 0;
>>  		int remainder = (MAX_PAYLOAD + offset) % MSS;
>>  
>>  		send_large(txfd, &daddr, remainder);
>> @@ -1221,7 +1241,7 @@ static void gro_receiver(void)
>>  			check_recv_pkts(rxfd, correct_payload, 2);
>>  		}
>>  	} else if (strcmp(testname, "large") == 0) {
>> -		int offset = proto == PF_INET ? 20 : 0;
>> +		int offset = (proto == PF_INET && !ipip) ? 20 : 0;
>>  		int remainder = (MAX_PAYLOAD + offset) % MSS;
>>  
>>  		correct_payload[0] = (MAX_PAYLOAD + offset);
>> @@ -1250,6 +1270,7 @@ static void parse_args(int argc, char **argv)
>>  		{ "iface", required_argument, NULL, 'i' },
>>  		{ "ipv4", no_argument, NULL, '4' },
>>  		{ "ipv6", no_argument, NULL, '6' },
>> +		{ "ipip", no_argument, NULL, 'e' },
>>  		{ "rx", no_argument, NULL, 'r' },
>>  		{ "saddr", required_argument, NULL, 's' },
>>  		{ "smac", required_argument, NULL, 'S' },
>> @@ -1259,7 +1280,7 @@ static void parse_args(int argc, char **argv)
>>  	};
>>  	int c;
>>  
>> -	while ((c = getopt_long(argc, argv, "46d:D:i:rs:S:t:v", opts, NULL)) != -1) {
>> +	while ((c = getopt_long(argc, argv, "46ed:D:i:rs:S:t:v", opts, NULL)) != -1) {
> 
> Please keep alphabetical order.
> 
>>  		switch (c) {
>>  		case '4':
>>  			proto = PF_INET;
>> @@ -1269,6 +1290,11 @@ static void parse_args(int argc, char **argv)
>>  			proto = PF_INET6;
>>  			ethhdr_proto = htons(ETH_P_IPV6);
>>  			break;
>> +		case 'e':
>> +			ipip = true;
>> +			proto = PF_INET;
>> +			ethhdr_proto = htons(ETH_P_IP);
>> +			break;
>>  		case 'd':
>>  			addr4_dst = addr6_dst = optarg;
>>  			break;
>> @@ -1304,7 +1330,10 @@ int main(int argc, char **argv)
>>  {
>>  	parse_args(argc, argv);
>>  
>> -	if (proto == PF_INET) {
>> +	if (ipip) {
>> +		tcp_offset = ETH_HLEN + sizeof(struct iphdr) * 2;
>> +		total_hdr_len = tcp_offset + sizeof(struct tcphdr);
>> +	} else if (proto == PF_INET) {
>>  		tcp_offset = ETH_HLEN + sizeof(struct iphdr);
>>  		total_hdr_len = tcp_offset + sizeof(struct tcphdr);
>>  	} else if (proto == PF_INET6) {
>> diff --git a/tools/testing/selftests/net/gro.sh b/tools/testing/selftests/net/gro.sh
>> index 9e3f186bc2a1..d16ec365b3cf 100755
>> --- a/tools/testing/selftests/net/gro.sh
>> +++ b/tools/testing/selftests/net/gro.sh
>> @@ -4,7 +4,7 @@
>>  readonly SERVER_MAC="aa:00:00:00:00:02"
>>  readonly CLIENT_MAC="aa:00:00:00:00:01"
>>  readonly TESTS=("data" "ack" "flags" "tcp" "ip" "large")
>> -readonly PROTOS=("ipv4" "ipv6")
>> +readonly PROTOS=("ipv4" "ipv6" "ipip")
>>  dev=""
>>  test="all"
>>  proto="ipv4"
>> @@ -31,7 +31,8 @@ run_test() {
>>        1>>log.txt
>>      wait "${server_pid}"
>>      exit_code=$?
>> -    if [[ ${test} == "large" && -n "${KSFT_MACHINE_SLOW}" && \
>> +    if [[ ( ${test} == "large" || ${protocol} == "ipip" ) && \
>> +          -n "${KSFT_MACHINE_SLOW}" && \
> 
> Why does the ipip test need this debug xfail path?

When I tested on a VM, ipip packets sometimes didn't get merged because they
weren't received quickly enough (optimizing the selftest helped somewhat but
not entirely), but I can't seem to reproduce this anymore.

> 
>>            ${exit_code} -ne 0 ]]; then
>>          echo "Ignoring errors due to slow environment" 1>&2
>>          exit_code=0
>> -- 
>> 2.36.1
>>
> 
>