09 Nov, 2017

1 commit

  • Hold netns refcnt before call_rcu() and release it after
    the tcf_exts_destroy() is done.

    Note, on ->destroy() path we have to respect the return value
    of tcf_exts_get_net(), on other paths it should always return
    true, so we don't need to care.

    Cc: Lucas Bates
    Cc: Jamal Hadi Salim
    Cc: Jiri Pirko
    Signed-off-by: Cong Wang
    Signed-off-by: David S. Miller

    Cong Wang
     

29 Oct, 2017

1 commit

  • Defer the tcf_exts_destroy() in RCU callback to
    tc filter workqueue and get RTNL lock.

    Reported-by: Chris Mi
    Cc: Daniel Borkmann
    Cc: Jiri Pirko
    Cc: John Fastabend
    Cc: Jamal Hadi Salim
    Cc: "Paul E. McKenney"
    Signed-off-by: Cong Wang
    Signed-off-by: David S. Miller

    Cong Wang
     

08 Aug, 2017

1 commit

  • Now we use 'unsigned long fh' as a pointer in every place,
    it is safe to convert it to a void pointer now. This gets
    rid of many casts to pointer.

    Cc: Jamal Hadi Salim
    Cc: Jiri Pirko
    Signed-off-by: Cong Wang
    Acked-by: Jamal Hadi Salim
    Signed-off-by: David S. Miller

    WANG Cong
     

05 Aug, 2017

2 commits


22 Apr, 2017

1 commit

  • We could have a race condition where in ->classify() path we
    dereference tp->root and meanwhile a parallel ->destroy() makes it
    a NULL. Daniel cured this bug in commit d936377414fa
    ("net, sched: respect rcu grace period on cls destruction").

    This happens when ->destroy() is called for deleting a filter to
    check if we are the last one in tp, this tp is still linked and
    visible at that time. The root cause of this problem is the semantic
    of ->destroy(), it does two things (for non-force case):

    1) check if tp is empty
    2) if tp is empty we could really destroy it

    and its caller, if cares, needs to check its return value to see if it
    is really destroyed. Therefore we can't unlink tp unless we know it is
    empty.

    As suggested by Daniel, we could actually move the test logic to ->delete()
    so that we can safely unlink tp after ->delete() tells us the last one is
    just deleted and before ->destroy().

    Fixes: 1e052be69d04 ("net_sched: destroy proto tp when all filters are gone")
    Cc: Roi Dayan
    Cc: Daniel Borkmann
    Cc: John Fastabend
    Cc: Jamal Hadi Salim
    Signed-off-by: Cong Wang
    Acked-by: Daniel Borkmann
    Signed-off-by: David S. Miller

    WANG Cong
     

14 Apr, 2017

1 commit


25 Mar, 2017

1 commit


02 Feb, 2017

1 commit


28 Nov, 2016

1 commit

  • Roi reported a crash in flower where tp->root was NULL in ->classify()
    callbacks. Reason is that in ->destroy() tp->root is set to NULL via
    RCU_INIT_POINTER(). It's problematic for some of the classifiers, because
    this doesn't respect RCU grace period for them, and as a result, still
    outstanding readers from tc_classify() will try to blindly dereference
    a NULL tp->root.

    The tp->root object is strictly private to the classifier implementation
    and holds internal data the core such as tc_ctl_tfilter() doesn't know
    about. Within some classifiers, such as cls_bpf, cls_basic, etc, tp->root
    is only checked for NULL in ->get() callback, but nowhere else. This is
    misleading and seemed to be copied from old classifier code that was not
    cleaned up properly. For example, d3fa76ee6b4a ("[NET_SCHED]: cls_basic:
    fix NULL pointer dereference") moved tp->root initialization into ->init()
    routine, where before it was part of ->change(), so ->get() had to deal
    with tp->root being NULL back then, so that was indeed a valid case, after
    d3fa76ee6b4a, not really anymore. We used to set tp->root to NULL long
    ago in ->destroy(), see 47a1a1d4be29 ("pkt_sched: remove unnecessary xchg()
    in packet classifiers"); but the NULLifying was reintroduced with the
    RCUification, but it's not correct for every classifier implementation.

    In the cases that are fixed here with one exception of cls_cgroup, tp->root
    object is allocated and initialized inside ->init() callback, which is always
    performed at a point in time after we allocate a new tp, which means tp and
    thus tp->root was not globally visible in the tp chain yet (see tc_ctl_tfilter()).
    Also, on destruction tp->root is strictly kfree_rcu()'ed in ->destroy()
    handler, same for the tp which is kfree_rcu()'ed right when we return
    from ->destroy() in tcf_destroy(). This means, the head object's lifetime
    for such classifiers is always tied to the tp lifetime. The RCU callback
    invocation for the two kfree_rcu() could be out of order, but that's fine
    since both are independent.

    Dropping the RCU_INIT_POINTER(tp->root, NULL) for these classifiers here
    means that 1) we don't need a useless NULL check in fast-path and, 2) that
    outstanding readers of that tp in tc_classify() can still execute under
    respect with RCU grace period as it is actually expected.

    Things that haven't been touched here: cls_fw and cls_route. They each
    handle tp->root being NULL in ->classify() path for historic reasons, so
    their ->destroy() implementation can stay as is. If someone actually
    cares, they could get cleaned up at some point to avoid the test in fast
    path. cls_u32 doesn't set tp->root to NULL. For cls_rsvp, I just added a
    !head should anyone actually be using/testing it, so it at least aligns with
    cls_fw and cls_route. For cls_flower we additionally need to defer rhashtable
    destruction (to a sleepable context) after RCU grace period as concurrent
    readers might still access it. (Note that in this case we need to hold module
    reference to keep work callback address intact, since we only wait on module
    unload for all call_rcu()s to finish.)

    This fixes one race to bring RCU grace period guarantees back. Next step
    as worked on by Cong however is to fix 1e052be69d04 ("net_sched: destroy
    proto tp when all filters are gone") to get the order of unlinking the tp
    in tc_ctl_tfilter() for the RTM_DELTFILTER case right by moving
    RCU_INIT_POINTER() before tcf_destroy() and let the notification for
    removal be done through the prior ->delete() callback. Both are independant
    issues. Once we have that right, we can then clean tp->root up for a number
    of classifiers by not making them RCU pointers, which requires a new callback
    (->uninit) that is triggered from tp's RCU callback, where we just kfree()
    tp->root from there.

    Fixes: 1f947bf151e9 ("net: sched: rcu'ify cls_bpf")
    Fixes: 9888faefe132 ("net: sched: cls_basic use RCU")
    Fixes: 70da9f0bf999 ("net: sched: cls_flow use RCU")
    Fixes: 77b9900ef53a ("tc: introduce Flower classifier")
    Fixes: bf3994d2ed31 ("net/sched: introduce Match-all classifier")
    Fixes: 952313bd6258 ("net: sched: cls_cgroup use RCU")
    Reported-by: Roi Dayan
    Signed-off-by: Daniel Borkmann
    Cc: Cong Wang
    Cc: John Fastabend
    Cc: Roi Dayan
    Cc: Jiri Pirko
    Acked-by: John Fastabend
    Acked-by: Cong Wang
    Signed-off-by: David S. Miller

    Daniel Borkmann
     

20 Sep, 2016

1 commit


11 Sep, 2016

1 commit


23 Aug, 2016

1 commit

  • After commit 22dc13c837c3 ("net_sched: convert tcf_exts from list to pointer array")
    we do dynamic allocation in tcf_exts_init(), therefore we need
    to handle the ENOMEM case properly.

    Cc: Jamal Hadi Salim
    Signed-off-by: Cong Wang
    Acked-by: Jamal Hadi Salim
    Acked-by: Jamal Hadi Salim
    Signed-off-by: David S. Miller

    WANG Cong
     

09 Nov, 2015

1 commit


02 Sep, 2015

1 commit


21 Jul, 2015

1 commit

  • The following test case causes a NULL pointer dereference in cls_flow:

    tc filter add dev foo parent 1: handle 0x1 flow hash keys dst action ok
    tc filter replace dev foo parent 1: pref 49152 handle 0x1 \
    flow hash keys mark action drop

    To be more precise, actually two different panics are fixed, the first
    occurs because tcf_exts_init() is not called on the newly allocated
    filter when we do a replace. And the second panic uncovered after that
    happens since the arguments of list_replace_rcu() are swapped, the old
    element needs to be the first argument and the new element the second.

    Fixes: 70da9f0bf999 ("net: sched: cls_flow use RCU")
    Signed-off-by: Daniel Borkmann
    Acked-by: John Fastabend
    Signed-off-by: David S. Miller

    Daniel Borkmann
     

05 Jun, 2015

1 commit

  • This patch adds full IPv6 addresses into flow_keys and uses them as
    input to the flow hash function. The implementation supports either
    IPv4 or IPv6 addresses in a union, and selector is used to determine
    how may words to input to jhash2.

    We also add flow_get_u32_dst and flow_get_u32_src functions which are
    used to get a u32 representation of the source and destination
    addresses. For IPv6, ipv6_addr_hash is called. These functions retain
    getting the legacy values of src and dst in flow_keys.

    With this patch, Ethertype and IP protocol are now included in the
    flow hash input.

    Signed-off-by: Tom Herbert
    Signed-off-by: David S. Miller

    Tom Herbert
     

14 May, 2015

3 commits


10 Mar, 2015

1 commit

  • Kernel automatically creates a tp for each
    (kind, protocol, priority) tuple, which has handle 0,
    when we add a new filter, but it still is left there
    after we remove our own, unless we don't specify the
    handle (literally means all the filters under
    the tuple). For example this one is left:

    # tc filter show dev eth0
    filter parent 8001: protocol arp pref 49152 basic

    The user-space is hard to clean up these for kernel
    because filters like u32 are organized in a complex way.
    So kernel is responsible to remove it after all filters
    are gone. Each type of filter has its own way to
    store the filters, so each type has to provide its
    way to check if all filters are gone.

    Cc: Jamal Hadi Salim
    Signed-off-by: Cong Wang
    Signed-off-by: Cong Wang
    Acked-by: Jamal Hadi Salim
    Signed-off-by: David S. Miller

    Cong Wang
     

14 Jan, 2015

1 commit

  • tc code implicitly considers skb->protocol even in case of accelerated
    vlan paths and expects vlan protocol type here. However, on rx path,
    if the vlan header was already stripped, skb->protocol contains value
    of next header. Similar situation is on tx path.

    So for skbs that use skb->vlan_tci for tagging, use skb->vlan_proto instead.

    Reported-by: Jamal Hadi Salim
    Signed-off-by: Jiri Pirko
    Acked-by: Jamal Hadi Salim
    Signed-off-by: David S. Miller

    Jiri Pirko
     

10 Dec, 2014

2 commits


09 Dec, 2014

2 commits


08 Oct, 2014

1 commit

  • Testing xmit_more support with netperf and connected UDP sockets,
    I found strange dst refcount false sharing.

    Current handling of IFF_XMIT_DST_RELEASE is not optimal.

    Dropping dst in validate_xmit_skb() is certainly too late in case
    packet was queued by cpu X but dequeued by cpu Y

    The logical point to take care of drop/force is in __dev_queue_xmit()
    before even taking qdisc lock.

    As Julian Anastasov pointed out, need for skb_dst() might come from some
    packet schedulers or classifiers.

    This patch adds new helper to cleanly express needs of various drivers
    or qdiscs/classifiers.

    Drivers that need skb_dst() in their ndo_start_xmit() should call
    following helper in their setup instead of the prior :

    dev->priv_flags &= ~IFF_XMIT_DST_RELEASE;
    ->
    netif_keep_dst(dev);

    Instead of using a single bit, we use two bits, one being
    eventually rebuilt in bonding/team drivers.

    The other one, is permanent and blocks IFF_XMIT_DST_RELEASE being
    rebuilt in bonding/team. Eventually, we could add something
    smarter later.

    Signed-off-by: Eric Dumazet
    Cc: Julian Anastasov
    Signed-off-by: David S. Miller

    Eric Dumazet
     

07 Oct, 2014

1 commit

  • This removes the tcf_proto argument from the ematch code paths that
    only need it to reference the net namespace. This allows simplifying
    qdisc code paths especially when we need to tear down the ematch
    from an RCU callback. In this case we can not guarentee that the
    tcf_proto structure is still valid.

    Signed-off-by: John Fastabend
    Acked-by: Cong Wang
    Signed-off-by: David S. Miller

    John Fastabend
     

29 Sep, 2014

1 commit


14 Sep, 2014

1 commit


28 Apr, 2014

1 commit

  • When actions are attached to a filter, they are a part of the filter
    itself, so when changing a filter we should allow to overwrite the actions
    inside as well.

    In my specific case, when I tried to _append_ a new action to an existing
    filter which already has an action, I got EEXIST since kernel refused
    to overwrite the existing one in kernel.

    This patch checks if we are changing the filter checking NLM_F_CREATE flag
    (Sigh, filters don't use NLM_F_REPLACE...) and then passes the boolean down
    to actions. This fixes the problem above.

    Cc: Jamal Hadi Salim
    Cc: David S. Miller
    Signed-off-by: Cong Wang
    Signed-off-by: Cong Wang
    Signed-off-by: Jamal Hadi Salim
    Signed-off-by: David S. Miller

    Cong Wang
     

14 Jan, 2014

1 commit


19 Dec, 2013

2 commits

  • These information can be saved in tcf_exts, and this will
    simplify the code.

    Cc: Jamal Hadi Salim
    Cc: David S. Miller
    Signed-off-by: Cong Wang
    Signed-off-by: Jamal Hadi Salim
    Signed-off-by: David S. Miller

    WANG Cong
     
  • Currently actions are chained by a singly linked list,
    therefore it is a bit hard to add and remove a specific
    entry. Convert it to struct list_head so that in the
    latter patch we can remove an action without finding
    its head.

    Cc: Jamal Hadi Salim
    Cc: David S. Miller
    Signed-off-by: Cong Wang
    Signed-off-by: Jamal Hadi Salim
    Signed-off-by: David S. Miller

    WANG Cong
     

18 Dec, 2013

1 commit


20 Apr, 2013

1 commit


15 Jan, 2013

1 commit

  • Eric Dumazet pointed out that act_mirred needs to find the current net_ns,
    and struct net pointer is not provided in the call chain. His original
    patch made use of current->nsproxy->net_ns to find the network namespace,
    but this fails to work correctly for userspace code that makes use of
    netlink sockets in different network namespaces. Instead, pass the
    "struct net *" down along the call chain to where it is needed.

    This version removes the ifb changes as Eric has submitted that patch
    separately, but is otherwise identical to the previous version.

    Signed-off-by: Benjamin LaHaise
    Tested-by: Eric Dumazet
    Acked-by: Jamal Hadi Salim
    Signed-off-by: David S. Miller

    Benjamin LaHaise
     

15 Aug, 2012

2 commits

  • The flow classifier can use uids and gids of the sockets that
    are transmitting packets and do insert those uids and gids
    into the packet classification calcuation. I don't fully
    understand the details but it appears that we can depend
    on specific uids and gids when making traffic classification
    decisions.

    To work with user namespaces enabled map from kuids and kgids
    into uids and gids in the initial user namespace giving raw
    integer values the code can play with and depend on.

    To avoid issues of userspace depending on uids and gids in
    packet classifiers installed from other user namespaces
    and getting confused deny all packet classifiers that
    use uids or gids that are not comming from a netlink socket
    in the initial user namespace.

    Cc: Patrick McHardy
    Cc: Eric Dumazet
    Cc: Jamal Hadi Salim
    Cc: Changli Gao
    Acked-by: David S. Miller
    Acked-by: Serge Hallyn
    Signed-off-by: Eric W. Biederman

    Eric W. Biederman
     
  • cls_flow.c plays with uids and gids. Unless I misread that
    code it is possible for classifiers to depend on the specific uid and
    gid values. Therefore I need to know the user namespace of the
    netlink socket that is installing the packet classifiers. Pass
    in the rtnetlink skb so I can access the NETLINK_CB of the passed
    packet. In particular I want access to sk_user_ns(NETLINK_CB(in_skb).ssk).

    Pass in not the user namespace but the incomming rtnetlink skb into
    the the classifier change routines as that is generally the more useful
    parameter.

    Cc: Jamal Hadi Salim
    Acked-by: David S. Miller
    Acked-by: Serge Hallyn
    Signed-off-by: Eric W. Biederman

    Eric W. Biederman
     

02 Apr, 2012

1 commit