Page MenuHomeFreeBSD

netinet6: Add ip6_hdr_pseudo{} to ip6.h; diff reduce and apply.
Needs ReviewPublic

Authored by bms on Wed, Mar 4, 11:11 PM.
Tags
None
Referenced Files
F147281431: D55663.diff
Mon, Mar 9, 4:15 PM
F147243063: D55663.diff
Mon, Mar 9, 9:39 AM
Unknown Object (File)
Sat, Mar 7, 2:54 PM
Unknown Object (File)
Fri, Mar 6, 4:26 PM
Unknown Object (File)
Thu, Mar 5, 9:04 AM
Unknown Object (File)
Thu, Mar 5, 8:52 AM
Unknown Object (File)
Thu, Mar 5, 8:01 AM
Unknown Object (File)
Thu, Mar 5, 7:05 AM
Subscribers

Details

Reviewers
glebius
imp
pouria
Group Reviewers
network
Summary

Stop rolling our own ip6_hdr_pseudo{}. This actually lives in <netinet/ip6.h>.
Use it in netipsec/xform_tcp.c, with a C99 block-scope designated-initializer.
There are probably other refactoring candidates.
The CTASSERT()s were obtained from NetBSD and added to ip6_input.c.

Prerequisite for merging Andre Oppermann's (unfinished) TCP-AO implementation.

Obtained from: NetBSD, OpenBSD

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 71189
Build 68072: arc lint + arc unit

Event Timeline

bms requested review of this revision.Wed, Mar 4, 11:11 PM

Alternatively the entire function can be rewritten in declarative manner:

	const struct ip6_hdr *ip6 = mtod(m, struct ip6_hdr *);
	struct ip6_hdr_pseudo ip6ph = {
		.ip6ph_src = ip6->ip6_src,
		.ip6ph_dst = ip6->ip6_dst,
		.ip6ph_len = htonl(m->m_pkthdr.len - (ip6->ip6_nxt == IPPROTO_UDP ? sizeof(struct ip6) + sizeof(struct udphdr) : sizeof(struct ip6)),
		.ip6ph_nxt = htonl(IPPROTO_TCP),
	};

	MD5Update(ctx, &ip6ph, sizeof(ip6ph));

	return (ntohl(ip6ph.ip6ph_len));
}

I am not strongly suggesting that, up to your taste. The feature of such implementation, is that no extra zeroing before assignment is performed.
sys/netinet/ip6.h
113–116

If it is nonstandard and kernel only, let's just put into netinet6/ip6_var.h and no need for XXX :)

120–122

Please use C99 uintXX_t types instead of 8-ish BSD u_intXX_t in any new code.

sys/netinet6/ip6_input.c
129–133

We no longer use CTASSERT() in new code. It was used back before we were supporting old compilers. Now we use the standard C11 keyword.

Question: do we want to assert it equal to the magic number 40 or do we want to assert two structures are of equal size?

sys/netipsec/xform_tcp.c
159–161

This declaration zeroes the entire struct. If this was the intent, then you need just struct ip6_hdr_pseudo ip6ph = {}.

174

You lost htonl() around IPPROTO_TCP. Is this intentional?

175

The char * cast is not needed at all.

sys/netinet/ip6.h
113–116

See below.

120–122

The intent was diff reduction with other BSDs (and to facilitate porting Andre's existing code, which requires a whole bunch of sys/crypto additions downstreamed from OpenBSD).

If you are stipulating that C99 types are now required, that is likely to affect future such merges. I've been away for nearly a decade, so I don't know what the consensus is here.

sys/netinet6/ip6_input.c
129–133

I assume you mean it needs to be _Static_assert() instead then. Again, new to many C99 constructs. Yes, we are checking that the struct was not packed or aligned incorrectly.

sys/netipsec/xform_tcp.c
159–161

I'm new to some of the C99 constructs so I will have to take your word for that.

174

Look again -- IPv6 next header fields are 8 bits. htonl() was an artefact of the use of a 32-bit wide field in the previous hand-rolled ip6 pseudo header.

175

That was already there, diff is non-semantic so it's picking up on my change of local struct name.

sys/netinet/ip6.h
120–122

IMHO, after 30 years of independent development, there is little sense left to keep diffs to other BSDs minimal. The code bases diverged a lot! The automatic patch application from OpenBSD was already not possible 15 years ago, when I was working on pf. Even if you are lucky and a patch applied, very high chance it will not work as intended. Only thoughtful merging is possible.

Of course, we should not create diffs just to create diffs. But if on one side of scale is reducing standard header pollution and on the other side is reducing diff to OpenBSD, the first outweighs the latter. Same for using C99 types instead of non-standard types.

This is my IMHO, but to my knowledge it is aligned with most active developers today. Feel free to bring more reviewers to discuss this point in general.

sys/netipsec/xform_tcp.c
174

Look again -- IPv6 next header fields are 8 bits. htonl() was an artefact of the use of a 32-bit wide field in the previous hand-rolled ip6 pseudo header.

Indeed, thanks!

175

That was already there, diff is non-semantic so it's picking up on my change of local struct name.

Since you are changing this line, please improve it and remove the unneeded cast.

Thank you, I'll review this revision after you address Gleb comments.