1. 06 Sep, 2019 1 commit
    • Eric Dumazet's avatar
      net: sched: fix reordering issues · b88dd52c
      Eric Dumazet authored
      Whenever MQ is not used on a multiqueue device, we experience
      serious reordering problems. Bisection found the cited
      commit.
      
      The issue can be described this way :
      
      - A single qdisc hierarchy is shared by all transmit queues.
        (eg : tc qdisc replace dev eth0 root fq_codel)
      
      - When/if try_bulk_dequeue_skb_slow() dequeues a packet targetting
        a different transmit queue than the one used to build a packet train,
        we stop building the current list and save the 'bad' skb (P1) in a
        special queue. (bad_txq)
      
      - When dequeue_skb() calls qdisc_dequeue_skb_bad_txq() and finds this
        skb (P1), it checks if the associated transmit queues is still in frozen
        state. If the queue is still blocked (by BQL or NIC tx ring full),
        we leave the skb in bad_txq and return NULL.
      
      - dequeue_skb() calls q->dequeue() to get another packet (P2)
      
        The other packet can target the problematic queue (that we found
        in frozen state for the bad_txq packet), but another cpu just ran
        TX completion and made room in the txq that is now ready to accept
        new packets.
      
      - Packet P2 is sent while P1 is still held in bad_txq, P1 might be sent
        at next round. In practice P2 is the lead of a big packet train
        (P2,P3,P4 ...) filling the BQL budget and delaying P1 by many packets :/
      
      To solve this problem, we have to block the dequeue process as long
      as the first packet in bad_txq can not be sent. Reordering issues
      disappear and no side effects have been seen.
      
      Fixes: a53851e2
      
       ("net: sched: explicit locking in gso_cpu fallback")
      Signed-off-by: default avatarEric Dumazet <edumazet@google.com>
      Cc: John Fastabend <john.fastabend@gmail.com>
      Acked-by: default avatarJohn Fastabend <john.fastabend@gmail.com>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      b88dd52c
  2. 28 Aug, 2019 2 commits
    • Davide Caratti's avatar
      net/sched: pfifo_fast: fix wrong dereference in pfifo_fast_enqueue · 092e22e5
      Davide Caratti authored
      Now that 'TCQ_F_CPUSTATS' bit can be cleared, depending on the value of
      'TCQ_F_NOLOCK' bit in the parent qdisc, we can't assume anymore that
      per-cpu counters are there in the error path of skb_array_produce().
      Otherwise, the following splat can be seen:
      
       Unable to handle kernel paging request at virtual address 0000600dea430008
       Mem abort info:
         ESR = 0x96000005
         Exception class = DABT (current EL), IL = 32 bits
         SET = 0, FnV = 0
         EA = 0, S1PTW = 0
       Data abort info:
         ISV = 0, ISS = 0x00000005
         CM = 0, WnR = 0
       user pgtable: 64k pages, 48-bit VAs, pgdp = 000000007b97530e
       [0000600dea430008] pgd=0000000000000000, pud=0000000000000000
       Internal error: Oops: 96000005 [#1] SMP
      [...]
       pstate: 10000005 (nzcV daif -PAN -UAO)
       pc : pfifo_fast_enqueue+0x524/0x6e8
       lr : pfifo_fast_enqueue+0x46c/0x6e8
       sp : ffff800d39376fe0
       x29: ffff800d39376fe0 x28: 1ffff001a07d1e40
       x27: ffff800d03e8f188 x26: ffff800d03e8f200
       x25: 0000000000000062 x24: ffff800d393772f0
       x23: 0000000000000000 x22: 0000000000000403
       x21: ffff800cca569a00 x20: ffff800d03e8ee00
       x19: ffff800cca569a10 x18: 00000000000000bf
       x17: 0000000000000000 x16: 0000000000000000
       x15: 0000000000000000 x14: ffff1001a726edd0
       x13: 1fffe4000276a9a4 x12: 0000000000000000
       x11: dfff200000000000 x10: ffff800d03e8f1a0
       x9 : 0000000000000003 x8 : 0000000000000000
       x7 : 00000000f1f1f1f1 x6 : ffff1001a726edea
       x5 : ffff800cca56a53c x4 : 1ffff001bf9a8003
       x3 : 1ffff001bf9a8003 x2 : 1ffff001a07d1dcb
       x1 : 0000600dea430000 x0 : 0000600dea430008
       Process ping (pid: 6067, stack limit = 0x00000000dc0aa557)
       Call trace:
        pfifo_fast_enqueue+0x524/0x6e8
        htb_enqueue+0x660/0x10e0 [sch_htb]
        __dev_queue_xmit+0x123c/0x2de0
        dev_queue_xmit+0x24/0x30
        ip_finish_output2+0xc48/0x1720
        ip_finish_output+0x548/0x9d8
        ip_output+0x334/0x788
        ip_local_out+0x90/0x138
        ip_send_skb+0x44/0x1d0
        ip_push_pending_frames+0x5c/0x78
        raw_sendmsg+0xed8/0x28d0
        inet_sendmsg+0xc4/0x5c0
        sock_sendmsg+0xac/0x108
        __sys_sendto+0x1ac/0x2a0
        __arm64_sys_sendto+0xc4/0x138
        el0_svc_handler+0x13c/0x298
        el0_svc+0x8/0xc
       Code: f9402e80 d538d081 91002000 8b010000 (885f7c03)
      
      Fix this by testing the value of 'TCQ_F_CPUSTATS' bit in 'qdisc->flags',
      before dereferencing 'qdisc->cpu_qstats'.
      
      Fixes: 8a53e616
      
       ("net: sched: when clearing NOLOCK, clear TCQ_F_CPUSTATS, too")
      CC: Paolo Abeni <pabeni@redhat.com>
      CC: Stefano Brivio <sbrivio@redhat.com>
      Reported-by: default avatarLi Shuang <shuali@redhat.com>
      Signed-off-by: default avatarDavide Caratti <dcaratti@redhat.com>
      Acked-by: default avatarPaolo Abeni <pabeni@redhat.com>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      092e22e5
    • Davide Caratti's avatar
      net/sched: pfifo_fast: fix wrong dereference when qdisc is reset · 04d37cf4
      Davide Caratti authored
      Now that 'TCQ_F_CPUSTATS' bit can be cleared, depending on the value of
      'TCQ_F_NOLOCK' bit in the parent qdisc, we need to be sure that per-cpu
      counters are present when 'reset()' is called for pfifo_fast qdiscs.
      Otherwise, the following script:
      
       # tc q a dev lo handle 1: root htb default 100
       # tc c a dev lo parent 1: classid 1:100 htb \
       > rate 95Mbit ceil 100Mbit burst 64k
       [...]
       # tc f a dev lo parent 1: protocol arp basic classid 1:100
       [...]
       # tc q a dev lo parent 1:100 handle 100: pfifo_fast
       [...]
       # tc q d dev lo root
      
      can generate the following splat:
      
       Unable to handle kernel paging request at virtual address dfff2c01bd148000
       Mem abort info:
         ESR = 0x96000004
         Exception class = DABT (current EL), IL = 32 bits
         SET = 0, FnV = 0
         EA = 0, S1PTW = 0
       Data abort info:
         ISV = 0, ISS = 0x00000004
         CM = 0, WnR = 0
       [dfff2c01bd148000] address between user and kernel address ranges
       Internal error: Oops: 96000004 [#1] SMP
       [...]
       pstate: 80000005 (Nzcv daif -PAN -UAO)
       pc : pfifo_fast_reset+0x280/0x4d8
       lr : pfifo_fast_reset+0x21c/0x4d8
       sp : ffff800d09676fa0
       x29: ffff800d09676fa0 x28: ffff200012ee22e4
       x27: dfff200000000000 x26: 0000000000000000
       x25: ffff800ca0799958 x24: ffff1001940f332b
       x23: 0000000000000007 x22: ffff200012ee1ab8
       x21: 0000600de8a40000 x20: 0000000000000000
       x19: ffff800ca0799900 x18: 0000000000000000
       x17: 0000000000000002 x16: 0000000000000000
       x15: 0000000000000000 x14: 0000000000000000
       x13: 0000000000000000 x12: ffff1001b922e6e2
       x11: 1ffff001b922e6e1 x10: 0000000000000000
       x9 : 1ffff001b922e6e1 x8 : dfff200000000000
       x7 : 0000000000000000 x6 : 0000000000000000
       x5 : 1fffe400025dc45c x4 : 1fffe400025dc357
       x3 : 00000c01bd148000 x2 : 0000600de8a40000
       x1 : 0000000000000007 x0 : 0000600de8a40004
       Call trace:
        pfifo_fast_reset+0x280/0x4d8
        qdisc_reset+0x6c/0x370
        htb_reset+0x150/0x3b8 [sch_htb]
        qdisc_reset+0x6c/0x370
        dev_deactivate_queue.constprop.5+0xe0/0x1a8
        dev_deactivate_many+0xd8/0x908
        dev_deactivate+0xe4/0x190
        qdisc_graft+0x88c/0xbd0
        tc_get_qdisc+0x418/0x8a8
        rtnetlink_rcv_msg+0x3a8/0xa78
        netlink_rcv_skb+0x18c/0x328
        rtnetlink_rcv+0x28/0x38
        netlink_unicast+0x3c4/0x538
        netlink_sendmsg+0x538/0x9a0
        sock_sendmsg+0xac/0xf8
        ___sys_sendmsg+0x53c/0x658
        __sys_sendmsg+0xc8/0x140
        __arm64_sys_sendmsg+0x74/0xa8
        el0_svc_handler+0x164/0x468
        el0_svc+0x10/0x14
       Code: 910012a0 92400801 d343fc03 11000c21 (38fb6863)
      
      Fix this by testing the value of 'TCQ_F_CPUSTATS' bit in 'qdisc->flags',
      before dereferencing 'qdisc->cpu_qstats'.
      
      Changes since v1:
       - coding style improvements, thanks to Stefano Brivio
      
      Fixes: 8a53e616
      
       ("net: sched: when clearing NOLOCK, clear TCQ_F_CPUSTATS, too")
      CC: Paolo Abeni <pabeni@redhat.com>
      Reported-by: default avatarLi Shuang <shuali@redhat.com>
      Signed-off-by: default avatarDavide Caratti <dcaratti@redhat.com>
      Acked-by: default avatarPaolo Abeni <pabeni@redhat.com>
      Reviewed-by: default avatarStefano Brivio <sbrivio@redhat.com>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      04d37cf4
  3. 30 May, 2019 1 commit
  4. 04 May, 2019 1 commit
  5. 10 Apr, 2019 3 commits
    • Paolo Abeni's avatar
      Revert: "net: sched: put back q.qlen into a single location" · 73eb628d
      Paolo Abeni authored
      This revert commit 46b1c18f ("net: sched: put back q.qlen into
      a single location").
      After the previous patch, when a NOLOCK qdisc is enslaved to a
      locking qdisc it switches to global stats accounting. As a consequence,
      when a classful qdisc accesses directly a child qdisc's qlen, such
      qdisc is not doing per CPU accounting and qlen value is consistent.
      
      In the control path nobody uses directly qlen since commit
      e5f0e8f8
      
       ("net: sched: introduce and use qdisc tree flush/purge
      helpers"), so we can remove the contented atomic ops from the
      datapath.
      
      v1 -> v2:
       - complete the qdisc_qstats_atomic_qlen_dec() ->
         qdisc_qstats_cpu_qlen_dec() replacement, fix build issue
       - more descriptive commit message
      Signed-off-by: default avatarPaolo Abeni <pabeni@redhat.com>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      73eb628d
    • Paolo Abeni's avatar
      net: sched: when clearing NOLOCK, clear TCQ_F_CPUSTATS, too · 8a53e616
      Paolo Abeni authored
      
      
      Since stats updating is always consistent with TCQ_F_CPUSTATS flag,
      we can disable it at qdisc creation time flipping such bit.
      
      In my experiments, if the NOLOCK flag is cleared, per CPU stats
      accounting does not give any measurable performance gain, but it
      waste some memory.
      
      Let's clear TCQ_F_CPUSTATS together with NOLOCK, when enslaving
      a NOLOCK qdisc to 'lock' one.
      
      Use stats update helper inside pfifo_fast, to cope correctly with
      TCQ_F_CPUSTATS flag change.
      
      As a side effect, q.qlen value for any child qdiscs is always
      consistent for all lock classfull qdiscs.
      Signed-off-by: default avatarPaolo Abeni <pabeni@redhat.com>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      8a53e616
    • Paolo Abeni's avatar
      net: sched: always do stats accounting according to TCQ_F_CPUSTATS · 9c01c9f1
      Paolo Abeni authored
      
      
      The core sched implementation checks independently for NOLOCK flag
      to acquire/release the root spin lock and for qdisc_is_percpu_stats()
      to account per CPU values in many places.
      
      This change update the last few places checking the TCQ_F_NOLOCK to
      do per CPU stats accounting according to qdisc_is_percpu_stats()
      value.
      
      The above allows to clean dev_requeue_skb() implementation a bit
      and makes stats update always consistent with a single flag.
      
      v1 -> v2:
       - do not move qdisc_is_empty definition, fix build issue
      Signed-off-by: default avatarPaolo Abeni <pabeni@redhat.com>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      9c01c9f1
  6. 24 Mar, 2019 1 commit
  7. 02 Mar, 2019 1 commit
    • Eric Dumazet's avatar
      net: sched: put back q.qlen into a single location · 46b1c18f
      Eric Dumazet authored
      In the series fc8b81a5 ("Merge branch 'lockless-qdisc-series'")
      John made the assumption that the data path had no need to read
      the qdisc qlen (number of packets in the qdisc).
      
      It is true when pfifo_fast is used as the root qdisc, or as direct MQ/MQPRIO
      children.
      
      But pfifo_fast can be used as leaf in class full qdiscs, and existing
      logic needs to access the child qlen in an efficient way.
      
      HTB breaks badly, since it uses cl->leaf.q->q.qlen in :
        htb_activate() -> WARN_ON()
        htb_dequeue_tree() to decide if a class can be htb_deactivated
        when it has no more packets.
      
      HFSC, DRR, CBQ, QFQ have similar issues, and some calls to
      qdisc_tree_reduce_backlog() also read q.qlen directly.
      
      Using qdisc_qlen_sum() (which iterates over all possible cpus)
      in the data path is a non starter.
      
      It seems we have to put back qlen in a central location,
      at least for stable kernels.
      
      For all qdisc but pfifo_fast, qlen is guarded by the qdisc lock,
      so the existing q.qlen{++|--} are correct.
      
      For 'lockless' qdisc (pfifo_fast so far), we need to use atomic_{inc|dec}()
      because the spinlock might be not held (for example from
      pfifo_fast_enqueue() and pfifo_fast_dequeue())
      
      This patch adds atomic_qlen (in the same location than qlen)
      and renames the following helpers, since we want to express
      they can be used without qdisc lock, and that qlen is no longer percpu.
      
      - qdisc_qstats_cpu_qlen_dec -> qdisc_qstats_atomic_qlen_dec()
      - qdisc_qstats_cpu_qlen_inc -> qdisc_qstats_atomic_qlen_inc()
      
      Later (net-next) we might revert this patch by tracking all these
      qlen uses and replace them by a more efficient method (not having
      to access a precise qlen, but an empty/non_empty status that might
      be less expensive to maintain/track).
      
      Another possibility is to have a legacy pfifo_fast version that would
      be used when used a a child qdisc, since the parent qdisc needs
      a spinlock anyway. But then, future lockless qdiscs would also
      have the same problem.
      
      Fixes: 7e66016f
      
       ("net: sched: helpers to sum qlen and qlen for per cpu logic")
      Signed-off-by: default avatarEric Dumazet <edumazet@google.com>
      Cc: John Fastabend <john.fastabend@gmail.com>
      Cc: Jamal Hadi Salim <jhs@mojatatu.com>
      Cc: Cong Wang <xiyou.wangcong@gmail.com>
      Cc: Jiri Pirko <jiri@resnulli.us>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      46b1c18f
  8. 26 Feb, 2019 1 commit
  9. 12 Feb, 2019 1 commit
    • Vlad Buslov's avatar
      net: sched: protect filter_chain list with filter_chain_lock mutex · ed76f5ed
      Vlad Buslov authored
      
      
      Extend tcf_chain with new filter_chain_lock mutex. Always lock the chain
      when accessing filter_chain list, instead of relying on rtnl lock.
      Dereference filter_chain with tcf_chain_dereference() lockdep macro to
      verify that all users of chain_list have the lock taken.
      
      Rearrange tp insert/remove code in tc_new_tfilter/tc_del_tfilter to execute
      all necessary code while holding chain lock in order to prevent
      invalidation of chain_info structure by potential concurrent change. This
      also serializes calls to tcf_chain0_head_change(), which allows head change
      callbacks to rely on filter_chain_lock for synchronization instead of rtnl
      mutex.
      Signed-off-by: default avatarVlad Buslov <vladbu@mellanox.com>
      Acked-by: default avatarJiri Pirko <jiri@mellanox.com>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      ed76f5ed
  10. 11 Feb, 2019 1 commit
  11. 01 Dec, 2018 1 commit
    • Paul E. McKenney's avatar
      net/sched: Replace call_rcu_bh() and rcu_barrier_bh() · ae0e3349
      Paul E. McKenney authored
      
      
      Now that call_rcu()'s callback is not invoked until after bh-disable
      regions of code have completed (in addition to explicitly marked
      RCU read-side critical sections), call_rcu() can be used in place
      of call_rcu_bh().  Similarly, rcu_barrier() can be used in place o
      frcu_barrier_bh().  This commit therefore makes these changes.
      Signed-off-by: default avatarPaul E. McKenney <paulmck@linux.ibm.com>
      Cc: Jamal Hadi Salim <jhs@mojatatu.com>
      Cc: Cong Wang <xiyou.wangcong@gmail.com>
      Cc: Jiri Pirko <jiri@resnulli.us>
      Cc: "David S. Miller" <davem@davemloft.net>
      Cc: <netdev@vger.kernel.org>
      ae0e3349
  12. 11 Oct, 2018 1 commit
  13. 28 Sep, 2018 1 commit
  14. 26 Sep, 2018 2 commits
    • Vlad Buslov's avatar
      net: sched: extend Qdisc with rcu · 3a7d0d07
      Vlad Buslov authored
      
      
      Currently, Qdisc API functions assume that users have rtnl lock taken. To
      implement rtnl unlocked classifiers update interface, Qdisc API must be
      extended with functions that do not require rtnl lock.
      
      Extend Qdisc structure with rcu. Implement special version of put function
      qdisc_put_unlocked() that is called without rtnl lock taken. This function
      only takes rtnl lock if Qdisc reference counter reached zero and is
      intended to be used as optimization.
      Signed-off-by: default avatarVlad Buslov <vladbu@mellanox.com>
      Acked-by: default avatarJiri Pirko <jiri@mellanox.com>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      3a7d0d07
    • Vlad Buslov's avatar
      net: sched: rename qdisc_destroy() to qdisc_put() · 86bd446b
      Vlad Buslov authored
      
      
      Current implementation of qdisc_destroy() decrements Qdisc reference
      counter and only actually destroy Qdisc if reference counter value reached
      zero. Rename qdisc_destroy() to qdisc_put() in order for it to better
      describe the way in which this function currently implemented and used.
      
      Extract code that deallocates Qdisc into new private qdisc_destroy()
      function. It is intended to be shared between regular qdisc_put() and its
      unlocked version that is introduced in next patch in this series.
      Signed-off-by: default avatarVlad Buslov <vladbu@mellanox.com>
      Acked-by: default avatarJiri Pirko <jiri@mellanox.com>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      86bd446b
  15. 10 Sep, 2018 1 commit
  16. 31 May, 2018 1 commit
    • Song Liu's avatar
      net: remove bypassed check in sch_direct_xmit() · 4341f830
      Song Liu authored
      Checking netif_xmit_frozen_or_stopped() at the end of sch_direct_xmit()
      is being bypassed. This is because "ret" from sch_direct_xmit() will be
      either NETDEV_TX_OK or NETDEV_TX_BUSY, and only ret == NETDEV_TX_OK == 0
      will reach the condition:
      
          if (ret && netif_xmit_frozen_or_stopped(txq))
              return false;
      
      This patch cleans up the code by removing the whole condition.
      
      For more discussion about this, please refer to
         https://marc.info/?t=152727195700008
      
      Signed-off-by: default avatarSong Liu <songliubraving@fb.com>
      Cc: John Fastabend <john.fastabend@gmail.com>
      Cc: Alexei Starovoitov <ast@kernel.org>
      Cc: David S. Miller <davem@davemloft.net>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      4341f830
  17. 17 May, 2018 2 commits
  18. 16 May, 2018 1 commit
    • Paolo Abeni's avatar
      sched: manipulate __QDISC_STATE_RUNNING in qdisc_run_* helpers · 32f7b44d
      Paolo Abeni authored
      
      
      Currently NOLOCK qdiscs pay a measurable overhead to atomically
      manipulate the __QDISC_STATE_RUNNING. Such bit is flipped twice per
      packet in the uncontended scenario with packet rate below the
      line rate: on packed dequeue and on the next, failing dequeue attempt.
      
      This changeset moves the bit manipulation into the qdisc_run_{begin,end}
      helpers, so that the bit is now flipped only once per packet, with
      measurable performance improvement in the uncontended scenario.
      
      This also allows simplifying the qdisc teardown code path - since
      qdisc_is_running() is now effective for each qdisc type - and avoid a
      possible race between qdisc_run() and dev_deactivate_many(), as now
      the some_qdisc_is_busy() can properly detect NOLOCK qdiscs being busy
      dequeuing packets.
      Signed-off-by: default avatarPaolo Abeni <pabeni@redhat.com>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      32f7b44d
  19. 26 Mar, 2018 1 commit
    • John Fastabend's avatar
      net: sched, fix OOO packets with pfifo_fast · eb82a994
      John Fastabend authored
      After the qdisc lock was dropped in pfifo_fast we allow multiple
      enqueue threads and dequeue threads to run in parallel. On the
      enqueue side the skb bit ooo_okay is used to ensure all related
      skbs are enqueued in-order. On the dequeue side though there is
      no similar logic. What we observe is with fewer queues than CPUs
      it is possible to re-order packets when two instances of
      __qdisc_run() are running in parallel. Each thread will dequeue
      a skb and then whichever thread calls the ndo op first will
      be sent on the wire. This doesn't typically happen because
      qdisc_run() is usually triggered by the same core that did the
      enqueue. However, drivers will trigger __netif_schedule()
      when queues are transitioning from stopped to awake using the
      netif_tx_wake_* APIs. When this happens netif_schedule() calls
      qdisc_run() on the same CPU that did the netif_tx_wake_* which
      is usually done in the interrupt completion context. This CPU
      is selected with the irq affinity which is unrelated to the
      enqueue operations.
      
      To resolve this we add a RUNNING bit to the qdisc to ensure
      only a single dequeue per qdisc is running. Enqueue and dequeue
      operations can still run in parallel and also on multi queue
      NICs we can still have a dequeue in-flight per qdisc, which
      is typically per CPU.
      
      Fixes: c5ad119f
      
       ("net: sched: pfifo_fast use skb_array")
      Reported-by: default avatarJakob Unterwurzacher <jakob.unterwurzacher@theobroma-systems.com>
      Signed-off-by: default avatarJohn Fastabend <john.fastabend@gmail.com>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      eb82a994
  20. 17 Mar, 2018 1 commit
    • Eric Dumazet's avatar
      net: sched: fix uses after free · cce6294c
      Eric Dumazet authored
      syzbot reported one use-after-free in pfifo_fast_enqueue() [1]
      
      Issue here is that we can not reuse skb after a successful skb_array_produce()
      since another cpu might have consumed it already.
      
      I believe a similar problem exists in try_bulk_dequeue_skb_slow()
      in case we put an skb into qdisc_enqueue_skb_bad_txq() for lockless qdisc.
      
      [1]
      BUG: KASAN: use-after-free in qdisc_pkt_len include/net/sch_generic.h:610 [inline]
      BUG: KASAN: use-after-free in qdisc_qstats_cpu_backlog_inc include/net/sch_generic.h:712 [inline]
      BUG: KASAN: use-after-free in pfifo_fast_enqueue+0x4bc/0x5e0 net/sched/sch_generic.c:639
      Read of size 4 at addr ffff8801cede37e8 by task syzkaller717588/5543
      
      CPU: 1 PID: 5543 Comm: syzkaller717588 Not tainted 4.16.0-rc4+ #265
      Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
      Call Trace:
       __dump_stack lib/dump_stack.c:17 [inline]
       dump_stack+0x194/0x24d lib/dump_stack.c:53
       print_address_description+0x73/0x250 mm/kasan/report.c:256
       kasan_report_error mm/kasan/report.c:354 [inline]
       kasan_report+0x23c/0x360 mm/kasan/report.c:412
       __asan_report_load4_noabort+0x14/0x20 mm/kasan/report.c:432
       qdisc_pkt_len include/net/sch_generic.h:610 [inline]
       qdisc_qstats_cpu_backlog_inc include/net/sch_generic.h:712 [inline]
       pfifo_fast_enqueue+0x4bc/0x5e0 net/sched/sch_generic.c:639
       __dev_xmit_skb net/core/dev.c:3216 [inline]
      
      Fixes: c5ad119f
      
       ("net: sched: pfifo_fast use skb_array")
      Signed-off-by: default avatarEric Dumazet <edumazet@google.com>
      Reported-by: syzbot+ed43b6903ab968b16f54@syzkaller.appspotmail.com
      Cc: John Fastabend <john.fastabend@gmail.com>
      Cc: Jamal Hadi Salim <jhs@mojatatu.com>
      Cc:	Cong Wang <xiyou.wangcong@gmail.com>
      Cc:	Jiri Pirko <jiri@resnulli.us>
      Acked-by: default avatarJohn Fastabend <john.fastabend@gmail.com>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      cce6294c
  21. 29 Jan, 2018 2 commits
    • Cong Wang's avatar
      net_sched: implement ->change_tx_queue_len() for pfifo_fast · 7007ba63
      Cong Wang authored
      
      
      pfifo_fast used to drop based on qdisc_dev(qdisc)->tx_queue_len,
      so we have to resize skb array when we change tx_queue_len.
      
      Other qdiscs which read tx_queue_len are fine because they
      all save it to sch->limit or somewhere else in qdisc during init.
      They don't have to implement this, it is nicer if they do so
      that users don't have to re-configure qdisc after changing
      tx_queue_len.
      
      Cc: John Fastabend <john.fastabend@gmail.com>
      Signed-off-by: default avatarCong Wang <xiyou.wangcong@gmail.com>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      7007ba63
    • Cong Wang's avatar
      net_sched: plug in qdisc ops change_tx_queue_len · 48bfd55e
      Cong Wang authored
      
      
      Introduce a new qdisc ops ->change_tx_queue_len() so that
      each qdisc could decide how to implement this if it wants.
      Previously we simply read dev->tx_queue_len, after pfifo_fast
      switches to skb array, we need this API to resize the skb array
      when we change dev->tx_queue_len.
      
      To avoid handling race conditions with TX BH, we need to
      deactivate all TX queues before change the value and bring them
      back after we are done, this also makes implementation easier.
      
      Cc: John Fastabend <john.fastabend@gmail.com>
      Signed-off-by: default avatarCong Wang <xiyou.wangcong@gmail.com>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      48bfd55e
  22. 22 Jan, 2018 1 commit
  23. 16 Jan, 2018 1 commit
    • Daniel Borkmann's avatar
      net, sched: fix panic when updating miniq {b,q}stats · 81d947e2
      Daniel Borkmann authored
      While working on fixing another bug, I ran into the following panic
      on arm64 by simply attaching clsact qdisc, adding a filter and running
      traffic on ingress to it:
      
        [...]
        [  178.188591] Unable to handle kernel read from unreadable memory at virtual address 810fb501f000
        [  178.197314] Mem abort info:
        [  178.200121]   ESR = 0x96000004
        [  178.203168]   Exception class = DABT (current EL), IL = 32 bits
        [  178.209095]   SET = 0, FnV = 0
        [  178.212157]   EA = 0, S1PTW = 0
        [  178.215288] Data abort info:
        [  178.218175]   ISV = 0, ISS = 0x00000004
        [  178.222019]   CM = 0, WnR = 0
        [  178.224997] user pgtable: 4k pages, 48-bit VAs, pgd = 0000000023cb3f33
        [  178.231531] [0000810fb501f000] *pgd=0000000000000000
        [  178.236508] Internal error: Oops: 96000004 [#1] SMP
        [...]
        [  178.311855] CPU: 73 PID: 2497 Comm: ping Tainted: G        W        4.15.0-rc7+ #5
        [  178.319413] Hardware name: FOXCONN R2-1221R-A4/C2U4N_MB, BIOS G31FB18A 03/31/2017
        [  178.326887] pstate: 60400005 (nZCv daif +PAN -UAO)
        [  178.331685] pc : __netif_receive_skb_core+0x49c/0xac8
        [  178.336728] lr : __netif_receive_skb+0x28/0x78
        [  178.341161] sp : ffff00002344b750
        [  178.344465] x29: ffff00002344b750 x28: ffff810fbdfd0580
        [  178.349769] x27: 0000000000000000 x26: ffff000009378000
        [...]
        [  178.418715] x1 : 0000000000000054 x0 : 0000000000000000
        [  178.424020] Process ping (pid: 2497, stack limit = 0x000000009f0a3ff4)
        [  178.430537] Call trace:
        [  178.432976]  __netif_receive_skb_core+0x49c/0xac8
        [  178.437670]  __netif_receive_skb+0x28/0x78
        [  178.441757]  process_backlog+0x9c/0x160
        [  178.445584]  net_rx_action+0x2f8/0x3f0
        [...]
      
      Reason is that sch_ingress and sch_clsact are doing mini_qdisc_pair_init()
      which sets up miniq pointers to cpu_{b,q}stats from the underlying qdisc.
      Problem is that this cannot work since they are actually set up right after
      the qdisc ->init() callback in qdisc_create(), so first packet going into
      sch_handle_ingress() tries to call mini_qdisc_bstats_cpu_update() and we
      therefore panic.
      
      In order to fix this, allocation of {b,q}stats needs to happen before we
      call into ->init(). In net-next, there's already such option through commit
      d59f5ffa ("net: sched: a dflt qdisc may be used with per cpu stats").
      However, the bug needs to be fixed in net still for 4.15. Thus, include
      these bits to reduce any merge churn and reuse the static_flags field to
      set TCQ_F_CPUSTATS, and remove the allocation from qdisc_create() since
      there is no other user left. Prashant Bhole ran into the same issue but
      for net-next, thus adding him below as well as co-author. Same issue was
      also reported by Sandipan Das when using bcc.
      
      Fixes: 46209401 ("net: core: introduce mini_Qdisc and eliminate usage of tp->q for clsact fastpath")
      Reference: https://lists.iovisor.org/pipermail/iovisor-dev/2018-January/001190.html
      
      Reported-by: default avatarSandipan Das <sandipan@linux.vnet.ibm.com>
      Co-authored-by: default avatarPrashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
      Co-authored-by: default avatarJohn Fastabend <john.fastabend@gmail.com>
      Signed-off-by: default avatarDaniel Borkmann <daniel@iogearbox.net>
      Cc: Jiri Pirko <jiri@resnulli.us>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      81d947e2
  24. 02 Jan, 2018 1 commit
    • Wei Yongjun's avatar
      net: sched: fix skb leak in dev_requeue_skb() · 9540d977
      Wei Yongjun authored
      When dev_requeue_skb() is called with bulked skb list, only the
      first skb of the list will be requeued to qdisc layer, and leak
      the others without free them.
      
      TCP is broken due to skb leak since no free skb will be considered
      as still in the host queue and never be retransmitted. This happend
      when dev_requeue_skb() called from qdisc_restart().
        qdisc_restart
        |-- dequeue_skb
        |-- sch_direct_xmit()
            |-- dev_requeue_skb() <-- skb may bluked
      
      Fix dev_requeue_skb() to requeue the full bluked list. Also change
      to use __skb_queue_tail() in __dev_requeue_skb() to avoid skb out
      of order.
      
      Fixes: a53851e2
      
       ("net: sched: explicit locking in gso_cpu fallback")
      Signed-off-by: default avatarWei Yongjun <weiyongjun1@huawei.com>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      9540d977
  25. 26 Dec, 2017 1 commit
  26. 21 Dec, 2017 3 commits
  27. 20 Dec, 2017 1 commit
  28. 19 Dec, 2017 1 commit
  29. 08 Dec, 2017 4 commits
    • John Fastabend's avatar
      net: sched: pfifo_fast use skb_array · c5ad119f
      John Fastabend authored
      
      
      This converts the pfifo_fast qdisc to use the skb_array data structure
      and set the lockless qdisc bit. pfifo_fast is the first qdisc to support
      the lockless bit that can be a child of a qdisc requiring locking. So
      we add logic to clear the lock bit on initialization in these cases when
      the qdisc graft operation occurs.
      
      This also removes the logic used to pick the next band to dequeue from
      and instead just checks a per priority array for packets from top priority
      to lowest. This might need to be a bit more clever but seems to work
      for now.
      Signed-off-by: default avatarJohn Fastabend <john.fastabend@gmail.com>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      c5ad119f
    • John Fastabend's avatar
      net: sched: check for frozen queue before skb_bad_txq check · fd8e8d1a
      John Fastabend authored
      
      
      I can not think of any reason to pull the bad txq skb off the qdisc if
      the txq we plan to send this on is still frozen. So check for frozen
      queue first and abort before dequeuing either skb_bad_txq skb or
      normal qdisc dequeue() skb.
      Signed-off-by: default avatarJohn Fastabend <john.fastabend@gmail.com>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      fd8e8d1a
    • John Fastabend's avatar
      net: sched: use skb list for skb_bad_tx · 70e57d5e
      John Fastabend authored
      
      
      Similar to how gso is handled use skb list for skb_bad_tx this is
      required with lockless qdiscs because we may have multiple cores
      attempting to push skbs into skb_bad_tx concurrently
      Signed-off-by: default avatarJohn Fastabend <john.fastabend@gmail.com>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      70e57d5e
    • John Fastabend's avatar
      net: sched: drop qdisc_reset from dev_graft_qdisc · 7bbde83b
      John Fastabend authored
      
      
      In qdisc_graft_qdisc a "new" qdisc is attached and the 'qdisc_destroy'
      operation is called on the old qdisc. The destroy operation will wait
      a rcu grace period and call qdisc_rcu_free(). At which point
      gso_cpu_skb is free'd along with all stats so no need to zero stats
      and gso_cpu_skb from the graft operation itself.
      
      Further after dropping the qdisc locks we can not continue to call
      qdisc_reset before waiting an rcu grace period so that the qdisc is
      detached from all cpus. By removing the qdisc_reset() here we get
      the correct property of waiting an rcu grace period and letting the
      qdisc_destroy operation clean up the qdisc correctly.
      
      Note, a refcnt greater than 1 would cause the destroy operation to
      be aborted however if this ever happened the reference to the qdisc
      would be lost and we would have a memory leak.
      Signed-off-by: default avatarJohn Fastabend <john.fastabend@gmail.com>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      7bbde83b