1. 07 Nov, 2019 2 commits
    • Jakub Kicinski's avatar
      net/tls: add a TX lock · 79ffe608
      Jakub Kicinski authored
      TLS TX needs to release and re-acquire the socket lock if send buffer
      fills up.
      TLS SW TX path currently depends on only allowing one thread to enter
      the function by the abuse of sk_write_pending. If another writer is
      already waiting for memory no new ones are allowed in.
      This has two problems:
       - writers don't wake other threads up when they leave the kernel;
         meaning that this scheme works for single extra thread (second
         application thread or delayed work) because memory becoming
         available will send a wake up request, but as Mallesham and
         Pooja report with larger number of threads it leads to threads
         being put to sleep indefinitely;
       - the delayed work does not get _scheduled_ but it may _run_ when
         other writers are present leading to crashes as writers don't
         expect state to change under their feet (same records get pushed
         and freed multiple times); it's hard to reliably bail from the
         work, however, because the mere presence of a writer does not
         guarantee that the writer will push pending records before exiting.
      Ensuring wakeups always happen will make the code basically open
      code a mutex. Just use a mutex.
      The TLS HW TX path does not have any locking (not even the
      sk_write_pending hack), yet it uses a per-socket sg_tx_data
      array to push records.
      Fixes: a42055e8 ("net/tls: Add support for async encryption of records for performance")
      Reported-by: default avatarMallesham  Jatharakonda <mallesh537@gmail.com>
      Reported-by: default avatarPooja Trivedi <poojatrivedi@gmail.com>
      Signed-off-by: default avatarJakub Kicinski <jakub.kicinski@netronome.com>
      Reviewed-by: default avatarSimon Horman <simon.horman@netronome.com>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
    • Jakub Kicinski's avatar
      net/tls: don't pay attention to sk_write_pending when pushing partial records · 02b1fa07
      Jakub Kicinski authored
      sk_write_pending being not zero does not guarantee that partial
      record will be pushed. If the thread waiting for memory times out
      the pending record may get stuck.
      In case of tls_device there is no path where parial record is
      set and writer present in the first place. Partial record is
      set only in tls_push_sg() and tls_push_sg() will return an
      error immediately. All tls_device callers of tls_push_sg()
      will return (and not wait for memory) if it failed.
      Fixes: a42055e8 ("net/tls: Add support for async encryption of records for performance")
      Signed-off-by: default avatarJakub Kicinski <jakub.kicinski@netronome.com>
      Reviewed-by: default avatarSimon Horman <simon.horman@netronome.com>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
  2. 07 Sep, 2019 4 commits
  3. 05 Sep, 2019 5 commits
  4. 01 Sep, 2019 2 commits
  5. 15 Aug, 2019 1 commit
  6. 10 Aug, 2019 1 commit
  7. 09 Aug, 2019 1 commit
    • Jakub Kicinski's avatar
      net/tls: prevent skb_orphan() from leaking TLS plain text with offload · 41477662
      Jakub Kicinski authored
      sk_validate_xmit_skb() and drivers depend on the sk member of
      struct sk_buff to identify segments requiring encryption.
      Any operation which removes or does not preserve the original TLS
      socket such as skb_orphan() or skb_clone() will cause clear text
      Make the TCP socket underlying an offloaded TLS connection
      mark all skbs as decrypted, if TLS TX is in offload mode.
      Then in sk_validate_xmit_skb() catch skbs which have no socket
      (or a socket with no validation) and decrypted flag set.
      sk->sk_validate_xmit_skb are slightly interchangeable right now,
      they all imply TLS offload. The new checks are guarded by
      CONFIG_TLS_DEVICE because that's the option guarding the
      sk_buff->decrypted member.
      Second, smaller issue with orphaning is that it breaks
      the guarantee that packets will be delivered to device
      queues in-order. All TLS offload drivers depend on that
      scheduling property. This means skb_orphan_partial()'s
      trick of preserving partial socket references will cause
      issues in the drivers. We need a full orphan, and as a
      result netem delay/throttling will cause all TLS offload
      skbs to be dropped.
      Reusing the sk_buff->decrypted flag also protects from
      leaking clear text when incoming, decrypted skb is redirected
      (e.g. by TC).
      See commit 0608c69c ("bpf: sk_msg, sock{map|hash} redirect
      through ULP") for justification why the internal flag is safe.
      The only location which could leak the flag in is tcp_bpf_sendmsg(),
      which is taken care of by clearing the previously unused bit.
       - remove superfluous decrypted mark copy (Willem);
       - remove the stale doc entry (Boris);
       - rely entirely on EOR marking to prevent coalescing (Boris);
       - use an internal sendpages flag instead of marking the socket
      v3 (Willem):
       - reorganize the can_skb_orphan_partial() condition;
       - fix the flag leak-in through tcp_bpf_sendmsg.
      Signed-off-by: default avatarJakub Kicinski <jakub.kicinski@netronome.com>
      Acked-by: default avatarWillem de Bruijn <willemb@google.com>
      Reviewed-by: default avatarBoris Pismenny <borisp@mellanox.com>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
  8. 05 Aug, 2019 1 commit
  9. 30 Jul, 2019 1 commit
  10. 23 Jul, 2019 1 commit
  11. 22 Jul, 2019 6 commits
  12. 09 Jul, 2019 3 commits
  13. 07 Jul, 2019 1 commit
  14. 02 Jul, 2019 2 commits
  15. 24 Jun, 2019 1 commit
    • Dirk van der Merwe's avatar
      net/tls: fix page double free on TX cleanup · 9354544c
      Dirk van der Merwe authored
      With commit 94850257 ("tls: Fix tls_device handling of partial records")
      a new path was introduced to cleanup partial records during sk_proto_close.
      This path does not handle the SW KTLS tx_list cleanup.
      This is unnecessary though since the free_resources calls for both
      SW and offload paths will cleanup a partial record.
      The visible effect is the following warning, but this bug also causes
      a page double free.
          WARNING: CPU: 7 PID: 4000 at net/core/stream.c:206 sk_stream_kill_queues+0x103/0x110
          RIP: 0010:sk_stream_kill_queues+0x103/0x110
          RSP: 0018:ffffb6df87e07bd0 EFLAGS: 00010206
          RAX: 0000000000000000 RBX: ffff8c21db4971c0 RCX: 0000000000000007
          RDX: ffffffffffffffa0 RSI: 000000000000001d RDI: ffff8c21db497270
          RBP: ffff8c21db497270 R08: ffff8c29f4748600 R09: 000000010020001a
          R10: ffffb6df87e07aa0 R11: ffffffff9a445600 R12: 0000000000000007
          R13: 0000000000000000 R14: ffff8c21f03f2900 R15: ffff8c21f03b8df0
          Call Trace:
           ? tcp_check_oom+0x120/0x120
           ? release_sock+0x43/0x90
           ? SYSC_recvfrom+0x11d/0x130
          RIP: 0033:0x7fe9b9abc10d
          RSP: 002b:00007fe9b19a1d48 EFLAGS: 00000246 ORIG_RAX: 00000000000000ca
          RAX: fffffffffffffe00 RBX: 0000000000000006 RCX: 00007fe9b9abc10d
          RDX: 0000000000000002 RSI: 0000000000000080 RDI: 00007fe948003430
          RBP: 00007fe948003410 R08: 00007fe948003430 R09: 0000000000000000
          R10: 0000000000000000 R11: 0000000000000246 R12: 00005603739d9080
          R13: 00007fe9b9ab9f90 R14: 00007fe948003430 R15: 0000000000000000
      Fixes: 94850257 ("tls: Fix tls_device handling of partial records")
      Signed-off-by: default avatarDirk van der Merwe <dirk.vandermerwe@netronome.com>
      Signed-off-by: default avatarJakub Kicinski <jakub.kicinski@netronome.com>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
  16. 12 Jun, 2019 1 commit
    • John Fastabend's avatar
      net: tls, correctly account for copied bytes with multiple sk_msgs · 648ee6ce
      John Fastabend authored
      tls_sw_do_sendpage needs to return the total number of bytes sent
      regardless of how many sk_msgs are allocated. Unfortunately, copied
      (the value we return up the stack) is zero'd before each new sk_msg
      is allocated so we only return the copied size of the last sk_msg used.
      The caller (splice, etc.) of sendpage will then believe only part
      of its data was sent and send the missing chunks again. However,
      because the data actually was sent the receiver will get multiple
      copies of the same data.
      To reproduce this do multiple sendfile calls with a length close to
      the max record size. This will in turn call splice/sendpage, sendpage
      may use multiple sk_msg in this case and then returns the incorrect
      number of bytes. This will cause splice to resend creating duplicate
      data on the receiver. Andre created a C program that can easily
      generate this case so we will push a similar selftest for this to
      bpf-next shortly.
      The fix is to _not_ zero the copied field so that the total sent
      bytes is returned.
      Reported-by: default avatarSteinar H. Gunderson <steinar+kernel@gunderson.no>
      Reported-by: default avatarAndre Tomt <andre@tomt.net>
      Tested-by: default avatarAndre Tomt <andre@tomt.net>
      Fixes: d829e9c4 ("tls: convert to generic sk_msg interface")
      Signed-off-by: default avatarJohn Fastabend <john.fastabend@gmail.com>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
  17. 11 Jun, 2019 6 commits
  18. 06 Jun, 2019 1 commit