Add support of tcp option wildcard "wscale *"

[liu-song-6]

Summary:
In current script, we can either specify every tcp option of a packet,
or use <...> for "any tcp options". This is not flexible enough, as
some script may have requirements for some tcp options; while be flexible
with other options.

This patch tries to address this by adding wildcard for option wscale.
Specifically, "wscale *" in the tcp option list means any wscale value.

To mark a tcp option in the script as wildcard, we leverage the highest
bit of tcp_option->kind, as TCPOPT_WILDCARD (0x80). tcp_option->kind of
TCPOPT_WILDCARD | TCPOPT_WINDOW (0x83) means wscale option of any value.

verify_outbound_live_tcp_options() is updated to be able to understand
TCPOPT_WILDCARD.

Reviewed-by: Lawrence Brakmo [email protected]
Signed-off-by: Song Liu [email protected]

[googlebot]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

---

What to do if you already signed the CLA

Individual signers

• It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

• Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
• The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
• The email used to register you as an authorized contributor must also be attached to your GitHub account.

[liu-song-6]

I signed it!

[liu-song-6]

I signed it!

[googlebot]

CLAs look good, thanks!

[liu-song-6]

@nealcardwell Could you please help review this PR? Or maybe there is any restrictions on external contributions?

Thanks in advance!

[nealcardwell]

I think we only want to skip the verify_outbound_tcp_option() call if the script packet has the TCPOPT_WILDCARD bit set. If the actual packet has bit 0x80 set for some reason then AFAICT this indicates a kernel bug, and not something we should ignore. How about:

if (s_opt->kind & TCPOPT_WILDCARD)

[nealcardwell]

There are options with the 0x80 bit set:
https://www.iana.org/assignments/tcp-parameters/tcp-parameters.xhtml

So I would suggest tightening this up:

            /* Sometimes skip btye-to-byte comparison of wildcard option */
	if (s_opt->kind == (TCPOPT_WILDCARD | TCPOPT_WINDOW))

[nealcardwell]

This seems like it would ignore cases where the actual packet has bit 0x80 set for some reason; AFAICT this would indicate a kernel bug, and is not something we should ignore.

There is also the case for an actual TCP option number that sets the 0x80 bit:
#define TCPOPT_EXP 254 /* Experimental */

If the script says the option type is supposed to be TCPOPT_EXP then we should check that all bits really match TCPOPT_EXP.

[nealcardwell]

I would suggest tightening this up with something like:

if (a_opt == NULL || s_opt == NULL ||
(a_opt->kind != s_opt->kind &&
!(s_opt->kind == (TCPOPT_WILDCARD | TCPOPT_WINDOW) &&
a_opt->kind == TCPOPT_WINDOW)) {

[nealcardwell]

Thanks for posting this patch! Sorry for the very delayed review.

nit: for consistency with the other patches in the packetdrill code base, can you please use a first summary line like:

net-test: packetdrill: add support of tcp option wildcard "wscale *"

Thanks!

[chantra]

Hi @nealcardwell !

So... a few years have passed by since this PR was last updated.

I like to get this going, but before putting work into it, want to check that this is something that you would still accept after addressing the comments.

Thanks

[chantra]

@liu-song-6 and I discusses this offline and I will likely spinning up a new PR to take over his initial work.

[nealcardwell]

@liu-song-6 and I discusses this offline and I will likely spinning up a new PR to take over his initial work.

Great. I added some new comments today. Please address those as well:
#10

Thanks!

[chantra]

ok, I have pushed this change in #75 .

I need to sort out the CLA and will probably force-push with my corporate email later. In the meantime, feedback is welcome.

Thanks