1. 12 Oct, 2020 7 commits
    • Alexei Starovoitov's avatar
      Merge branch 'sockmap/sk_skb program memory acct fixes' · 20a6d915
      Alexei Starovoitov authored
      John Fastabend says:
      
      ====================
      
      Users of sockmap and skmsg trying to build proxys and other tools
      have pointed out to me the error handling can be problematic. If
      the proxy is under-provisioned and/or the BPF admin does not have
      the ability to update/modify memory provisions on the sockets
      its possible data may be dropped. For some things we have retries
      so everything works out OK, but for most things this is likely
      not great. And things go bad.
      
      The original design dropped memory accounting on the receive
      socket as early as possible. We did this early in sk_skb
      handling and then charged it to the redirect socket immediately
      after running the BPF program.
      
      But, this design caused a fundamental problem. Namely, what should we do
      if we redirect to a socket that has already reached its socket memory
      limits. For proxy use cases the network admin can tune memory limits.
      But, in general we punted on this problem and told folks to simply make
      your memory limits high enough to handle your workload. This is not a
      really good answer. When deploying into environments where we expect this
      to be transparent its no longer the case because we need to tune params.
      In fact its really only viable in cases where we have fine grained
      control over the application. For example a proxy redirecting from an
      ingress socket to an egress socket. The result is I get bug
      reports because its surprising for one, but more importantly also breaks
      some use cases. So lets fix it.
      
      This series cleans up the different cases so that in many common
      modes, such as passing packet up to receive socket, we can simply
      use the underlying assumption that the TCP stack already has done
      memory accounting.
      
      Next instead of trying to do memory accounting against the socket
      we plan to redirect into we keep memory accounting on the receive
      socket until the skb can be put on the redirect socket. This means
      if we do an egress redirect to a socket and sock_writable() returns
      EAGAIN we can requeue the skb on the workqueue and try again. The
      same scenario plays out for ingress. If the skb can not be put on
      the receive queue of the redirect socket than we simply requeue and
      retry. In both cases memory is still accounted for against the
      receiving socket.
      
      This also handles head of line blocking. With the above scheme the
      skb is on a queue associated with the socket it will be sent/recv'd
      on, but the memory accounting is against the received socket. This
      means the receive socket can advance to the next skb and avoid head
      of line blocking. At least until its receive memory on the socket
      runs out. This will put some maximum size on the amount of data any
      socket can enqueue giving us bounds on the skb lists so they can't grow
      indefinitely.
      
      Overall I think this is a win. Tested with test_sockmap.
      
      These are fixes, but I tagged it for bpf-next considering we are
      at -rc8.
      
      v1->v2: Fix uninitialized/unused variables (kernel test robot)
      v2->v3: fix typo in patch2 err=0 needs to be <0 so use err=-EIO
      ---
      ====================
      Signed-off-by: default avatarAlexei Starovoitov <ast@kernel.org>
      20a6d915
    • John Fastabend's avatar
      bpf, sockmap: Add memory accounting so skbs on ingress lists are visible · 0b17ad25
      John Fastabend authored
      Move skb->sk assignment out of sk_psock_bpf_run() and into individual
      callers. Then we can use proper skb_set_owner_r() call to assign a
      sk to a skb. This improves things by also charging the truesize against
      the sockets sk_rmem_alloc counter. With this done we get some accounting
      in place to ensure the memory associated with skbs on the workqueue are
      still being accounted for somewhere. Finally, by using skb_set_owner_r
      the destructor is setup so we can just let the normal skb_kfree logic
      recover the memory. Combined with previous patch dropping skb_orphan()
      we now can recover from memory pressure and maintain accounting.
      
      Note, we will charge the skbs against their originating socket even
      if being redirected into another socket. Once the skb completes the
      redirect op the kfree_skb will give the memory back. This is important
      because if we charged the socket we are redirecting to (like it was
      done before this series) the sock_writeable() test could fail because
      of the skb trying to be sent is already charged against the socket.
      
      Also TLS case is special. Here we wait until we have decided not to
      simply PASS the packet up the stack. In the case where we PASS the
      packet up the stack we already have an skb which is accounted for on
      the TLS socket context.
      
      For the parser case we continue to just set/clear skb->sk this is
      because the skb being used here may be combined with other skbs or
      turned into multiple skbs depending on the parser logic. For example
      the parser could request a payload length greater than skb->len so
      that the strparser needs to collect multiple skbs. At any rate
      the final result will be handled in the strparser recv callback.
      
      Fixes: 604326b4 ("bpf, sockmap: convert to generic sk_msg interface")
      Signed-off-by: default avatarJohn Fastabend <john.fastabend@gmail.com>
      Signed-off-by: default avatarAlexei Starovoitov <ast@kernel.org>
      Link: https://lore.kernel.org/bpf/160226867513.5692.10579573214635925960.stgit@john-Precision-5820-Tower
      0b17ad25
    • John Fastabend's avatar
      bpf, sockmap: Remove skb_orphan and let normal skb_kfree do cleanup · 10d58d00
      John Fastabend authored
      Calling skb_orphan() is unnecessary in the strp rcv handler because the skb
      is from a skb_clone() in __strp_recv. So it never has a destructor or a
      sk assigned. Plus its confusing to read because it might hint to the reader
      that the skb could have an sk assigned which is not true. Even if we did
      have an sk assigned it would be cleaner to simply wait for the upcoming
      kfree_skb().
      
      Additionally, move the comment about strparser clone up so its closer to
      the logic it is describing and add to it so that it is more complete.
      
      Fixes: 604326b4 ("bpf, sockmap: convert to generic sk_msg interface")
      Signed-off-by: default avatarJohn Fastabend <john.fastabend@gmail.com>
      Signed-off-by: default avatarAlexei Starovoitov <ast@kernel.org>
      Link: https://lore.kernel.org/bpf/160226865548.5692.9098315689984599579.stgit@john-Precision-5820-Tower
      10d58d00
    • John Fastabend's avatar
      bpf, sockmap: Remove dropped data on errors in redirect case · 9047f19e
      John Fastabend authored
      In the sk_skb redirect case we didn't handle the case where we overrun
      the sk_rmem_alloc entry on ingress redirect or sk_wmem_alloc on egress.
      Because we didn't have anything implemented we simply dropped the skb.
      This meant data could be dropped if socket memory accounting was in
      place.
      
      This fixes the above dropped data case by moving the memory checks
      later in the code where we actually do the send or recv. This pushes
      those checks into the workqueue and allows us to return an EAGAIN error
      which in turn allows us to try again later from the workqueue.
      
      Fixes: 51199405 ("bpf: skb_verdict, support SK_PASS on RX BPF path")
      Signed-off-by: default avatarJohn Fastabend <john.fastabend@gmail.com>
      Signed-off-by: default avatarAlexei Starovoitov <ast@kernel.org>
      Link: https://lore.kernel.org/bpf/160226863689.5692.13861422742592309285.stgit@john-Precision-5820-Tower
      9047f19e
    • John Fastabend's avatar
      bpf, sockmap: Remove skb_set_owner_w wmem will be taken later from sendpage · 29545f49
      John Fastabend authored
      The skb_set_owner_w is unnecessary here. The sendpage call will create a
      fresh skb and set the owner correctly from workqueue. Its also not entirely
      harmless because it consumes cycles, but also impacts resource accounting
      by increasing sk_wmem_alloc. This is charging the socket we are going to
      send to for the skb, but we will put it on the workqueue for some time
      before this happens so we are artifically inflating sk_wmem_alloc for
      this period. Further, we don't know how many skbs will be used to send the
      packet or how it will be broken up when sent over the new socket so
      charging it with one big sum is also not correct when the workqueue may
      break it up if facing memory pressure. Seeing we don't know how/when
      this is going to be sent drop the early accounting.
      
      A later patch will do proper accounting charged on receive socket for
      the case where skbs get enqueued on the workqueue.
      
      Fixes: 604326b4 ("bpf, sockmap: convert to generic sk_msg interface")
      Signed-off-by: default avatarJohn Fastabend <john.fastabend@gmail.com>
      Signed-off-by: default avatarAlexei Starovoitov <ast@kernel.org>
      Link: https://lore.kernel.org/bpf/160226861708.5692.17964237936462425136.stgit@john-Precision-5820-Tower
      29545f49
    • John Fastabend's avatar
      bpf, sockmap: On receive programs try to fast track SK_PASS ingress · 9ecbfb06
      John Fastabend authored
      When we receive an skb and the ingress skb verdict program returns
      SK_PASS we currently set the ingress flag and put it on the workqueue
      so it can be turned into a sk_msg and put on the sk_msg ingress queue.
      Then finally telling userspace with data_ready hook.
      
      Here we observe that if the workqueue is empty then we can try to
      convert into a sk_msg type and call data_ready directly without
      bouncing through a workqueue. Its a common pattern to have a recv
      verdict program for visibility that always returns SK_PASS. In this
      case unless there is an ENOMEM error or we overrun the socket we
      can avoid the workqueue completely only using it when we fall back
      to error cases caused by memory pressure.
      
      By doing this we eliminate another case where data may be dropped
      if errors occur on memory limits in workqueue.
      
      Fixes: 51199405 ("bpf: skb_verdict, support SK_PASS on RX BPF path")
      Signed-off-by: default avatarJohn Fastabend <john.fastabend@gmail.com>
      Signed-off-by: default avatarAlexei Starovoitov <ast@kernel.org>
      Link: https://lore.kernel.org/bpf/160226859704.5692.12929678876744977669.stgit@john-Precision-5820-Tower
      9ecbfb06
    • John Fastabend's avatar
      bpf, sockmap: Skb verdict SK_PASS to self already checked rmem limits · cfea28f8
      John Fastabend authored
      For sk_skb case where skb_verdict program returns SK_PASS to continue to
      pass packet up the stack, the memory limits were already checked before
      enqueuing in skb_queue_tail from TCP side. So, lets remove the extra checks
      here. The theory is if the TCP stack believes we have memory to receive
      the packet then lets trust the stack and not double check the limits.
      
      In fact the accounting here can cause a drop if sk_rmem_alloc has increased
      after the stack accepted this packet, but before the duplicate check here.
      And worse if this happens because TCP stack already believes the data has
      been received there is no retransmit.
      
      Fixes: 51199405 ("bpf: skb_verdict, support SK_PASS on RX BPF path")
      Signed-off-by: default avatarJohn Fastabend <john.fastabend@gmail.com>
      Signed-off-by: default avatarAlexei Starovoitov <ast@kernel.org>
      Link: https://lore.kernel.org/bpf/160226857664.5692.668205469388498375.stgit@john-Precision-5820-Tower
      cfea28f8
  2. 11 Oct, 2020 13 commits
  3. 09 Oct, 2020 7 commits
  4. 08 Oct, 2020 5 commits
    • Alexei Starovoitov's avatar
      Merge branch 'libbpf: auto-resize relocatable LOAD/STORE instructions' · 1e9259ec
      Alexei Starovoitov authored
      Andrii Nakryiko says:
      
      ====================
      Patch set implements logic in libbpf to auto-adjust memory size (1-, 2-, 4-,
      8-bytes) of load/store (LD/ST/STX) instructions which have BPF CO-RE field
      offset relocation associated with it. In practice this means transparent
      handling of 32-bit kernels, both pointer and unsigned integers. Signed
      integers are not relocatable with zero-extending loads/stores, so libbpf
      poisons them and generates a warning. If/when BPF gets support for
      sign-extending loads/stores, it would be possible to automatically relocate
      them as well.
      
      All the details are contained in patch #2 comments and commit message.
      Patch #3 is a simple change in libbpf to make advanced testing with custom BTF
      easier. Patch #4 validates correct uses of auto-resizable loads, as well as
      check that libbpf fails invalid uses. Patch #1 skips CO-RE relocation for
      programs that had bpf_program__set_autoload(prog, false) set on them, reducing
      warnings and noise.
      
      v2->v3:
        - fix copyright (Alexei);
      v1->v2:
        - more consistent names for instruction mem size convertion routines (Alexei);
        - extended selftests to use relocatable STX instructions (Alexei);
        - added a fix for skipping CO-RE relocation for non-loadable programs.
      
      Cc: Luka Perkov <luka.perkov@sartura.hr>
      Cc: Tony Ambardar <tony.ambardar@gmail.com>
      ====================
      Signed-off-by: default avatarAlexei Starovoitov <ast@kernel.org>
      1e9259ec
    • Andrii Nakryiko's avatar
      selftests/bpf: Validate libbpf's auto-sizing of LD/ST/STX instructions · 888d83b9
      Andrii Nakryiko authored
      Add selftests validating libbpf's auto-resizing of load/store instructions
      when used with CO-RE relocations. An explicit and manual approach with using
      bpf_core_read() is also demonstrated and tested. Separate BPF program is
      supposed to fail due to using signed integers of sizes that differ from
      kernel's sizes.
      
      To reliably simulate 32-bit BTF (i.e., the one with sizeof(long) ==
      sizeof(void *) == 4), selftest generates its own custom BTF and passes it as
      a replacement for real kernel BTF. This allows to test 32/64-bitness mix on
      all architectures.
      Signed-off-by: default avatarAndrii Nakryiko <andrii@kernel.org>
      Signed-off-by: default avatarAlexei Starovoitov <ast@kernel.org>
      Link: https://lore.kernel.org/bpf/20201008001025.292064-5-andrii@kernel.org
      888d83b9
    • Andrii Nakryiko's avatar
      libbpf: Allow specifying both ELF and raw BTF for CO-RE BTF override · 2b7d88c2
      Andrii Nakryiko authored
      Use generalized BTF parsing logic, making it possible to parse BTF both from
      ELF file, as well as a raw BTF dump. This makes it easier to write custom
      tests with manually generated BTFs.
      Signed-off-by: default avatarAndrii Nakryiko <andrii@kernel.org>
      Signed-off-by: default avatarAlexei Starovoitov <ast@kernel.org>
      Link: https://lore.kernel.org/bpf/20201008001025.292064-4-andrii@kernel.org
      2b7d88c2
    • Andrii Nakryiko's avatar
      libbpf: Support safe subset of load/store instruction resizing with CO-RE · a66345bc
      Andrii Nakryiko authored
      Add support for patching instructions of the following form:
        - rX = *(T *)(rY + <off>);
        - *(T *)(rX + <off>) = rY;
        - *(T *)(rX + <off>) = <imm>, where T is one of {u8, u16, u32, u64}.
      
      For such instructions, if the actual kernel field recorded in CO-RE relocation
      has a different size than the one recorded locally (e.g., from vmlinux.h),
      then libbpf will adjust T to an appropriate 1-, 2-, 4-, or 8-byte loads.
      
      In general, such transformation is not always correct and could lead to
      invalid final value being loaded or stored. But two classes of cases are
      always safe:
        - if both local and target (kernel) types are unsigned integers, but of
        different sizes, then it's OK to adjust load/store instruction according to
        the necessary memory size. Zero-extending nature of such instructions and
        unsignedness make sure that the final value is always correct;
        - pointer size mismatch between BPF target architecture (which is always
        64-bit) and 32-bit host kernel architecture can be similarly resolved
        automatically, because pointer is essentially an unsigned integer. Loading
        32-bit pointer into 64-bit BPF register with zero extension will leave
        correct pointer in the register.
      
      Both cases are necessary to support CO-RE on 32-bit kernels, as `unsigned
      long` in vmlinux.h generated from 32-bit kernel is 32-bit, but when compiled
      with BPF program for BPF target it will be treated by compiler as 64-bit
      integer. Similarly, pointers in vmlinux.h are 32-bit for kernel, but treated
      as 64-bit values by compiler for BPF target. Both problems are now resolved by
      libbpf for direct memory reads.
      
      But similar transformations are useful in general when kernel fields are
      "resized" from, e.g., unsigned int to unsigned long (or vice versa).
      
      Now, similar transformations for signed integers are not safe to perform as
      they will result in incorrect sign extension of the value. If such situation
      is detected, libbpf will emit helpful message and will poison the instruction.
      Not failing immediately means that it's possible to guard the instruction
      based on kernel version (or other conditions) and make sure it's not
      reachable.
      
      If there is a need to read signed integers that change sizes between different
      kernels, it's possible to use BPF_CORE_READ_BITFIELD() macro, which works both
      with bitfields and non-bitfield integers of any signedness and handles
      sign-extension properly. Also, bpf_core_read() with proper size and/or use of
      bpf_core_field_size() relocation could allow to deal with such complicated
      situations explicitly, if not so conventiently as direct memory reads.
      
      Selftests added in a separate patch in progs/test_core_autosize.c demonstrate
      both direct memory and probed use cases.
      
      BPF_CORE_READ() is not changed and it won't deal with such situations as
      automatically as direct memory reads due to the signedness integer
      limitations, which are much harder to detect and control with compiler macro
      magic. So it's encouraged to utilize direct memory reads as much as possible.
      Signed-off-by: default avatarAndrii Nakryiko <andrii@kernel.org>
      Signed-off-by: default avatarAlexei Starovoitov <ast@kernel.org>
      Link: https://lore.kernel.org/bpf/20201008001025.292064-3-andrii@kernel.org
      a66345bc
    • Andrii Nakryiko's avatar
      libbpf: Skip CO-RE relocations for not loaded BPF programs · 47f7cf63
      Andrii Nakryiko authored
      Bypass CO-RE relocations step for BPF programs that are not going to be
      loaded. This allows to have BPF programs compiled in and disabled dynamically
      if kernel is not supposed to provide enough relocation information. In such
      case, there won't be unnecessary warnings about failed relocations.
      
      Fixes: d9297581 ("libbpf: Support disabling auto-loading BPF programs")
      Signed-off-by: default avatarAndrii Nakryiko <andrii@kernel.org>
      Signed-off-by: default avatarAlexei Starovoitov <ast@kernel.org>
      Link: https://lore.kernel.org/bpf/20201008001025.292064-2-andrii@kernel.org
      47f7cf63
  5. 07 Oct, 2020 5 commits
  6. 06 Oct, 2020 3 commits