On 13/12/17 - 08:30:19, Mat Martineau wrote:
> 
> On Tue, 12 Dec 2017, Christoph Paasch wrote:
> 
> > On 12/12/17 - 15:35:09, Mat Martineau wrote:
> > > 
> > > On Mon, 11 Dec 2017, Christoph Paasch wrote:
> > > 
> > > > 3rd version of the set.
> > > > 
> > > > Now, fully tested and bugs fixed.
> > > > 
> > > > There are a two rather big changes (most of it is commented on in the notes):
> > > > 
> > > > * Made the tcp_option_list and RCU-protected hlist. We need RCU-protection
> > > >  because listener sockets are lockless nowadays.
> > > 
> > > I'll pay attention to this as I go through the patches. Do we need to
> > > support modification of the option list while the socket is handling
> > > packets? If we require that the extra option list is not changed after
> > > connect()/listen(), that might simplify the implementation (and ensure that
> > > the option list is unchanged between the prepare and write calls for
> > > outgoing packets).
> > > 
> > > The current TCP_MD5 implementation has a comment for tcp_md5_do_add() saying
> > > it _can_ be called on a newly created socket, but I don't see anything
> > > _requiring_ the TCP_MD5SIG socket option to be used before
> > > connect()/listen() - so maybe we're stuck with allowing modifications to
> > > tcp_option_list so the TCP_MD5SIG option behaves just like before.
> > 
> > Yes, as far as I can see, one can change the MD5-keys in the middle of a
> > connection.
> 
> If the extra option list was read-only after connect()/listen(), that would
> still allow for keys to change mid-connection. Once the option list contains
> the MD5 hooks, those hooks can utilize whatever keys are currently
> configured.
> 
> The problem would be if the connection was started without TCP_MD5, then
> TCP_MD5 was enabled after packets had been exchanged. A read-only extra
> option list at that phase of the connection would be a problem. I'm not sure
> "start a connection without MD5 and then switch it on" is a use case for
> TCP_MD5, but it seems to be allowed by the current implementation so we'd
> have to keep that behavior.

I will add a test for this scenario as well, where I start without MD5 and
then add it. As well as changing keys halfway through.


Thanks,
Christoph

> 
> 
> Mat
> 
> 
> > > > * Previously, in tcp_v4_send_reset, we would lookup the MD5-signature and
> > > >  if there was one, go look for the listening-socket (if sk == NULL) to see
> > > >  if there is an MD5-key.
> > > >  Nowadays, we can't do this anymore because extra-options are only parsed when
> > > >  we actually have a socket. However, this shouldn't have an impact as far as
> > > >  I can see. Because, if there is a listening-socket, __inet_lookup_skb will
> > > >  match on it and when we then send a RST when coming from tcp_rcv_state_process
> > > >  we have the pointer to the listening socket.
> > > >  The only scenario wherer tcp_v4_send_reset() gets called with a NULL sk is
> > > >  when coming straight from tcp_v4_rcv.
> > > 
> > > This looks ok - that last scenario only happens when there is no socket of
> > > any kind to match up with the packet, so there's nothing for TCP_MD5 to do.
> > 
> > Cool!
> > 
> > Btw., I talked with Alexei about the framework as well, because at FB they
> > are planning to do BPF-based extensions for TCP-options. He was interested to
> > see if our framework suits their use-case, so that they can hook their BPF
> > into it.
> > 
> > 
> > Thanks,
> > Christoph
> > 
> > > 
> > > Mat
> > > 
> > > 
> > > > 
> > > > 
> > > > If there are no major comments, I would like to go to netdev as a next step.
> > > > 
> > > > Christoph Paasch (14):
> > > >  tcp md5sig: Use skb's saddr when replying to an incoming segment
> > > >  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: 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          |   39 ++
> > > > include/net/inet_sock.h          |    3 +-
> > > > include/net/tcp.h                |  209 +++---
> > > > net/ipv4/Makefile                |    1 +
> > > > net/ipv4/syncookies.c            |    6 +-
> > > > net/ipv4/tcp.c                   |  399 +++++++++---
> > > > net/ipv4/tcp_diag.c              |   81 +--
> > > > net/ipv4/tcp_input.c             |  140 ++--
> > > > net/ipv4/tcp_ipv4.c              |  545 ++--------------
> > > > net/ipv4/tcp_md5.c               | 1340 ++++++++++++++++++++++++++++++++++++++
> > > > net/ipv4/tcp_minisocks.c         |   75 +--
> > > > net/ipv4/tcp_output.c            |  179 +----
> > > > net/ipv6/syncookies.c            |    6 +-
> > > > net/ipv6/tcp_ipv6.c              |  382 +----------
> > > > net/smc/af_smc.c                 |  175 ++++-
> > > > 18 files changed, 2181 insertions(+), 1445 deletions(-)
> > > > create mode 100644 include/linux/tcp_md5.h
> > > > create mode 100644 net/ipv4/tcp_md5.c
> > > > 
> > > > --
> > > > 2.15.0
> > > > 
> > > > 
> > > 
> > > --
> > > Mat Martineau
> > > Intel OTC
> > 
> 
> --
> Mat Martineau
> Intel OTC