[RFC,v2,00/14] Generic TCP-option framework and adoption for TCP-SMC and TCP-MD5

Resubmit as v2 RFC to get some feedback before net-next opens up again.
Only minor changes (see below).


This patchset introduces a generic framework for handling TCP-options.

TCP-options like TCP_MD5 and SMC are rather rare use-cases, but their
implementation is rather intrusive to the TCP-stack. Other, more recent
TCP extensions like TCP-crypt, MPTCP or TCP-AO would make this situation
even worse.

This new framework allows to add these TCP-options in a modular way. Writing,
reading and acting upon these options is done through callbacks that get
registered to a TCP-socket. A TCP-socket has a list of "extra" TCP-options
that it will use.

We make TCP-SMC and TCP-MD5SIG adopt this new framework. As can be seen, there
is now no more TCP-SMC code in the TCP-files and the TCP-MD5 code has been
reduced to a bare minimum.

This patchset is admittedly rather big, but we wanted to show where the
framework will lead to and what it enables. Suggestions as to how to better
structure the patchset is appreciated.

There is still work to be done to more efficiently check for extra TCP options
in performance-sensitive code paths. A rate-limited static key would nearly
eliminate overhead if no extra TCP options are in use system-wide, or a flag
in a likely-hot cache line could work well.

For now we opted for a simple if (unlikely(!hlist_empty(...)) check.

Feedback is very welcome!

Thanks,
Mat & Christoph


Changelog:
===
v1 -> v2:
	* Some minor fixes thanks to the buildbot when certain configs
	  are disabled (Patch 5 and 12)
	* Add spdx-header in the new files (Patch 11)
	* Added Ivan Delande to the CC-list as he did some TCP-MD5
	  changes in the past.


Christoph Paasch (13):
  tcp: Write options after the header has been fully done
  tcp: Pass sock and skb to tcp_options_write
  tcp: Allow tcp_fast_parse_options to drop segments
  tcp_smc: Make smc_parse_options return 1 on success
  tcp_smc: Make SMC use TCP extra-option framework
  tcp_md5: Don't pass along md5-key
  tcp_md5: Detect key inside tcp_v4_send_ack instead of passing it as an
    argument
  tcp_md5: Detect key inside tcp_v6_send_response instead of passing it
    as an argument
  tcp_md5: Check for TCP_MD5 after TCP Timestamps in
    tcp_established_options
  tcp_md5: Move TCP-MD5 code out of TCP itself
  tcp_md5: Use tcp_extra_options in output path
  tcp_md5: Cleanup TCP-code
  tcp_md5: Use TCP extra-options on the input path

Mat Martineau (1):
  tcp: Register handlers for extra TCP options

 drivers/infiniband/hw/cxgb4/cm.c |    2 +-
 include/linux/inet_diag.h        |    1 +
 include/linux/tcp.h              |   43 +-
 include/linux/tcp_md5.h          |   40 ++
 include/net/inet_sock.h          |    3 +-
 include/net/tcp.h                |  213 +++---
 net/ipv4/Makefile                |    1 +
 net/ipv4/syncookies.c            |    6 +-
 net/ipv4/tcp.c                   |  391 ++++++++---
 net/ipv4/tcp_diag.c              |   81 +--
 net/ipv4/tcp_input.c             |  137 ++--
 net/ipv4/tcp_ipv4.c              |  556 ++--------------
 net/ipv4/tcp_md5.c               | 1359 ++++++++++++++++++++++++++++++++++++++
 net/ipv4/tcp_minisocks.c         |   75 +--
 net/ipv4/tcp_output.c            |  182 +----
 net/ipv6/syncookies.c            |    6 +-
 net/ipv6/tcp_ipv6.c              |  390 ++---------
 net/smc/af_smc.c                 |  190 +++++-
 18 files changed, 2228 insertions(+), 1448 deletions(-)
 create mode 100644 include/linux/tcp_md5.h
 create mode 100644 net/ipv4/tcp_md5.c

From: Christoph Paasch <cpaasch@apple.com>
Date: Wed, 31 Jan 2018 16:07:02 -0800

> TCP-options like TCP_MD5 and SMC are rather rare use-cases, but their
> implementation is rather intrusive to the TCP-stack. Other, more recent
> TCP extensions like TCP-crypt, MPTCP or TCP-AO would make this situation
> even worse.

Yet, this current implementation is what allows us to optimize things
properly.

And now we're going to do indirect calls to callbacks instead of
inline tests as well?

Also, requiring such direct surgery for new TCP options forces the
developer to consider the consequences of what the new TCP option
does and how it effect both the slow and the fast path.

With abstraction layers, people tend to turn their brains off when
it comes to these issues.

Sorry, I'm really not thrilled about this.

I would rather see the new TCP option features be proposed using
the existing code and then see how it all can be abstracted away
after they are all added.

I can already see in your patches that new overhead is added, new
tests in the packet building fast paths that are completely
unnecessary with the existing code, etc.

Hello,

On 01/02/18 - 10:15:46, David Miller wrote:
> From: Christoph Paasch <cpaasch@apple.com>
> Date: Wed, 31 Jan 2018 16:07:02 -0800
> 
> > TCP-options like TCP_MD5 and SMC are rather rare use-cases, but their
> > implementation is rather intrusive to the TCP-stack. Other, more recent
> > TCP extensions like TCP-crypt, MPTCP or TCP-AO would make this situation
> > even worse.
> 
> Yet, this current implementation is what allows us to optimize things
> properly.
> 
> And now we're going to do indirect calls to callbacks instead of
> inline tests as well?

Wrt to the potential performance impact, we have considered using static
keys that would only be enabled if one socket is using an option that is
part of the framework. That way the framework shouldn't cause any
performance issues as long as nobody uses these less common options.

Would that be an option to alleviate such concerns?


Or is your concern here that we are impacting the performance of TCP-MD5?

If it's the latter, we indeed are intentionally trading performance of
TCP-MD5 for a general "cleaner" TCP-stack (one that has no if-statements
for seldom used TCP-options like SMC and MD5). We thought the gain was worth
the cost, which can be arguable of course.

> Also, requiring such direct surgery for new TCP options forces the
> developer to consider the consequences of what the new TCP option
> does and how it effect both the slow and the fast path.
> 
> With abstraction layers, people tend to turn their brains off when
> it comes to these issues.
> 
> Sorry, I'm really not thrilled about this.

I can see your point and the risk that the abstraction layer brings, but let
me try to clarify what we think the framework would enable:

Our goal with the framework is to optimize/design for the common-case while still
allowing extensibility and less common features.

That way we can keep the TCP-stack clean with the most-used TCP-options (SACK,
Timestamps,...), while still allowing the more exotic features like MD5. And
only sockets that enable TCP-MD5 will pay the potential performance price for
it.

And for new options, the framework forces people to think about how to make the
feature work without spreading if-statements across the stack.
If the new option becomes the common-case after some time, it can always be fully
integrated into the stack at that moment.

> I would rather see the new TCP option features be proposed using
> the existing code and then see how it all can be abstracted away
> after they are all added.

Would a possible path forward be that we rather post the patches that
cleanup the TCP-MD5 code.
(meaning roughly patches 7 to 14 but without the option-framework adoption)

That way, we cleanup the TCP-stack at least by factoring out TCP-MD5.

> I can already see in your patches that new overhead is added, new
> tests in the packet building fast paths that are completely
> unnecessary with the existing code, etc.

Point taken, we will double-check our impact on the fast-path and make sure
to change that.


Thanks for your feedback!


Christoph