tc/act: remove unneeded RCU lock in action callback

Each lockless action currently does its own RCU locking in ->act().
This allows using plain RCU accessor, even if the context
is really RCU BH.

This change drops the per action RCU lock, replace the accessors
with the _bh variant, cleans up a bit the surrounding code and
documents the RCU status in the relevant header.
No functional nor performance change is intended.

The goal of this patch is clarifying that the RCU critical section
used by the tc actions extends up to the classifier's caller.

v1 -> v2:
 - preserve rcu lock in act_bpf: it's needed by eBPF helpers,
   as pointed out by Daniel

v3 -> v4:
 - fixed some typos in the commit message (JiriP)

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/include/net/act_api.h b/include/net/act_api.h
index 683ce41053d9359487a4523812b5442631affe73..8c9bc02d05e1c9997d17f6b0ecaaabc716e4abe9 100644 (file)
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -85,7 +85,7 @@ struct tc_action_ops {
        size_t  size;
        struct module           *owner;
        int     (*act)(struct sk_buff *, const struct tc_action *,
-                      struct tcf_result *);
+                      struct tcf_result *); /* called under RCU BH lock*/
        int     (*dump)(struct sk_buff *, struct tc_action *, int, int);
        void    (*cleanup)(struct tc_action *);
        int     (*lookup)(struct net *net, struct tc_action **a, u32 index,

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index c5432362dc26a16cf091c46049303c265af03dfa..bcae181c1857d4b456adbda97ee13d2052dcc726 100644 (file)
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -285,6 +285,8 @@ struct tcf_proto {
        /* Fast access part */
        struct tcf_proto __rcu  *next;
        void __rcu              *root;
+
+       /* called under RCU BH lock*/
        int                     (*classify)(struct sk_buff *,
                                            const struct tcf_proto *,
                                            struct tcf_result *);

diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
index 4e8c383f379e21a016ea821f82c79555ca1e8307..648a3a35b720112eec3b46bf9aada9a8e772e796 100644 (file)
--- a/net/sched/act_csum.c
+++ b/net/sched/act_csum.c
@@ -561,15 +561,14 @@ static int tcf_csum(struct sk_buff *skb, const struct tc_action *a,
        u32 update_flags;
        int action;
 
-       rcu_read_lock();
-       params = rcu_dereference(p->params);
+       params = rcu_dereference_bh(p->params);
 
        tcf_lastuse_update(&p->tcf_tm);
        bstats_cpu_update(this_cpu_ptr(p->common.cpu_bstats), skb);
 
        action = READ_ONCE(p->tcf_action);
        if (unlikely(action == TC_ACT_SHOT))
-               goto drop_stats;
+               goto drop;
 
        update_flags = params->update_flags;
        switch (tc_skb_protocol(skb)) {
@@ -583,16 +582,11 @@ static int tcf_csum(struct sk_buff *skb, const struct tc_action *a,
                break;
        }
 
-unlock:
-       rcu_read_unlock();
        return action;
 
 drop:
-       action = TC_ACT_SHOT;
-
-drop_stats:
        qstats_drop_inc(this_cpu_ptr(p->common.cpu_qstats));
-       goto unlock;
+       return TC_ACT_SHOT;
 }
 
 static int tcf_csum_dump(struct sk_buff *skb, struct tc_action *a, int bind,

diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
index 3d6e265758c06b6ee07c20f9ac4c1786eb022e48..df4060e32d43e85708678ebb283319a5344db481 100644 (file)
--- a/net/sched/act_ife.c
+++ b/net/sched/act_ife.c
@@ -820,14 +820,11 @@ static int tcf_ife_act(struct sk_buff *skb, const struct tc_action *a,
        struct tcf_ife_params *p;
        int ret;
 
-       rcu_read_lock();
-       p = rcu_dereference(ife->params);
+       p = rcu_dereference_bh(ife->params);
        if (p->flags & IFE_ENCODE) {
                ret = tcf_ife_encode(skb, a, res, p);
-               rcu_read_unlock();
                return ret;
        }
-       rcu_read_unlock();
 
        return tcf_ife_decode(skb, a, res);
 }

diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index 6afd89a36c69032668bf2f287da9493076c4e5ea..eeb335f031025a274a319a381f0f7c9b2a3f4097 100644 (file)
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -181,11 +181,10 @@ static int tcf_mirred(struct sk_buff *skb, const struct tc_action *a,
        tcf_lastuse_update(&m->tcf_tm);
        bstats_cpu_update(this_cpu_ptr(m->common.cpu_bstats), skb);
 
-       rcu_read_lock();
        m_mac_header_xmit = READ_ONCE(m->tcfm_mac_header_xmit);
        m_eaction = READ_ONCE(m->tcfm_eaction);
        retval = READ_ONCE(m->tcf_action);
-       dev = rcu_dereference(m->tcfm_dev);
+       dev = rcu_dereference_bh(m->tcfm_dev);
        if (unlikely(!dev)) {
                pr_notice_once("tc mirred: target device is gone\n");
                goto out;
@@ -236,7 +235,6 @@ out:
                if (tcf_mirred_is_act_redirect(m_eaction))
                        retval = TC_ACT_SHOT;
        }
-       rcu_read_unlock();
 
        return retval;
 }

diff --git a/net/sched/act_sample.c b/net/sched/act_sample.c
index 3079e7be5bdef54de97692e27fa65299e69406be..2608ccc83e5e7796d565438d6d0b739654695297 100644 (file)
--- a/net/sched/act_sample.c
+++ b/net/sched/act_sample.c
@@ -140,8 +140,7 @@ static int tcf_sample_act(struct sk_buff *skb, const struct tc_action *a,
        bstats_cpu_update(this_cpu_ptr(s->common.cpu_bstats), skb);
        retval = READ_ONCE(s->tcf_action);
 
-       rcu_read_lock();
-       psample_group = rcu_dereference(s->psample_group);
+       psample_group = rcu_dereference_bh(s->psample_group);
 
        /* randomly sample packets according to rate */
        if (psample_group && (prandom_u32() % s->rate == 0)) {
@@ -165,7 +164,6 @@ static int tcf_sample_act(struct sk_buff *skb, const struct tc_action *a,
                        skb_pull(skb, skb->mac_len);
        }
 
-       rcu_read_unlock();
        return retval;
 }
 

diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
index da56e6938c9e094e8e7e7c39edac5b7d473d2c11..a6db47ebec112ad79413acf102f6593b5863f50c 100644 (file)
--- a/net/sched/act_skbedit.c
+++ b/net/sched/act_skbedit.c
@@ -43,8 +43,7 @@ static int tcf_skbedit(struct sk_buff *skb, const struct tc_action *a,
        tcf_lastuse_update(&d->tcf_tm);
        bstats_cpu_update(this_cpu_ptr(d->common.cpu_bstats), skb);
 
-       rcu_read_lock();
-       params = rcu_dereference(d->params);
+       params = rcu_dereference_bh(d->params);
        action = READ_ONCE(d->tcf_action);
 
        if (params->flags & SKBEDIT_F_PRIORITY)
@@ -77,14 +76,11 @@ static int tcf_skbedit(struct sk_buff *skb, const struct tc_action *a,
        }
        if (params->flags & SKBEDIT_F_PTYPE)
                skb->pkt_type = params->ptype;
-
-unlock:
-       rcu_read_unlock();
        return action;
+
 err:
        qstats_drop_inc(this_cpu_ptr(d->common.cpu_qstats));
-       action = TC_ACT_SHOT;
-       goto unlock;
+       return TC_ACT_SHOT;
 }
 
 static const struct nla_policy skbedit_policy[TCA_SKBEDIT_MAX + 1] = {

diff --git a/net/sched/act_skbmod.c b/net/sched/act_skbmod.c
index cdc6bacfb19078a8be10d846ea5b356c47f849ed..c437c6d51a71763bf77e8350e41ccf6f5be000e3 100644 (file)
--- a/net/sched/act_skbmod.c
+++ b/net/sched/act_skbmod.c
@@ -41,20 +41,14 @@ static int tcf_skbmod_run(struct sk_buff *skb, const struct tc_action *a,
         * then MAX_EDIT_LEN needs to change appropriately
        */
        err = skb_ensure_writable(skb, MAX_EDIT_LEN);
-       if (unlikely(err)) { /* best policy is to drop on the floor */
-               qstats_overlimit_inc(this_cpu_ptr(d->common.cpu_qstats));
-               return TC_ACT_SHOT;
-       }
+       if (unlikely(err)) /* best policy is to drop on the floor */
+               goto drop;
 
-       rcu_read_lock();
        action = READ_ONCE(d->tcf_action);
-       if (unlikely(action == TC_ACT_SHOT)) {
-               qstats_overlimit_inc(this_cpu_ptr(d->common.cpu_qstats));
-               rcu_read_unlock();
-               return action;
-       }
+       if (unlikely(action == TC_ACT_SHOT))
+               goto drop;
 
-       p = rcu_dereference(d->skbmod_p);
+       p = rcu_dereference_bh(d->skbmod_p);
        flags = p->flags;
        if (flags & SKBMOD_F_DMAC)
                ether_addr_copy(eth_hdr(skb)->h_dest, p->eth_dst);
@@ -62,7 +56,6 @@ static int tcf_skbmod_run(struct sk_buff *skb, const struct tc_action *a,
                ether_addr_copy(eth_hdr(skb)->h_source, p->eth_src);
        if (flags & SKBMOD_F_ETYPE)
                eth_hdr(skb)->h_proto = p->eth_type;
-       rcu_read_unlock();
 
        if (flags & SKBMOD_F_SWAPMAC) {
                u16 tmpaddr[ETH_ALEN / 2]; /* ether_addr_copy() requirement */
@@ -73,6 +66,10 @@ static int tcf_skbmod_run(struct sk_buff *skb, const struct tc_action *a,
        }
 
        return action;
+
+drop:
+       qstats_overlimit_inc(this_cpu_ptr(d->common.cpu_qstats));
+       return TC_ACT_SHOT;
 }
 
 static const struct nla_policy skbmod_policy[TCA_SKBMOD_MAX + 1] = {

diff --git a/net/sched/act_tunnel_key.c b/net/sched/act_tunnel_key.c
index f811850fd1d051361fab5971cd1f1f308e075f9b..d42d9e112789f3ca4d3037d395a8e30a1a31b989 100644 (file)
--- a/net/sched/act_tunnel_key.c
+++ b/net/sched/act_tunnel_key.c
@@ -31,9 +31,7 @@ static int tunnel_key_act(struct sk_buff *skb, const struct tc_action *a,
        struct tcf_tunnel_key_params *params;
        int action;
 
-       rcu_read_lock();
-
-       params = rcu_dereference(t->params);
+       params = rcu_dereference_bh(t->params);
 
        tcf_lastuse_update(&t->tcf_tm);
        bstats_cpu_update(this_cpu_ptr(t->common.cpu_bstats), skb);
@@ -53,8 +51,6 @@ static int tunnel_key_act(struct sk_buff *skb, const struct tc_action *a,
                break;
        }
 
-       rcu_read_unlock();
-
        return action;
 }
 

diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
index ad37f308175ad0ab574356552a5c9d9328d5b2a2..15a0ee214c9cfc171300dbe2266569f7a4387d3c 100644 (file)
--- a/net/sched/act_vlan.c
+++ b/net/sched/act_vlan.c
@@ -40,11 +40,9 @@ static int tcf_vlan(struct sk_buff *skb, const struct tc_action *a,
        if (skb_at_tc_ingress(skb))
                skb_push_rcsum(skb, skb->mac_len);
 
-       rcu_read_lock();
-
        action = READ_ONCE(v->tcf_action);
 
-       p = rcu_dereference(v->vlan_p);
+       p = rcu_dereference_bh(v->vlan_p);
 
        switch (p->tcfv_action) {
        case TCA_VLAN_ACT_POP:
@@ -61,7 +59,7 @@ static int tcf_vlan(struct sk_buff *skb, const struct tc_action *a,
        case TCA_VLAN_ACT_MODIFY:
                /* No-op if no vlan tag (either hw-accel or in-payload) */
                if (!skb_vlan_tagged(skb))
-                       goto unlock;
+                       goto out;
                /* extract existing tag (and guarantee no hw-accel tag) */
                if (skb_vlan_tag_present(skb)) {
                        tci = skb_vlan_tag_get(skb);
@@ -86,18 +84,15 @@ static int tcf_vlan(struct sk_buff *skb, const struct tc_action *a,
                BUG();
        }
 
-       goto unlock;
-
-drop:
-       action = TC_ACT_SHOT;
-       qstats_drop_inc(this_cpu_ptr(v->common.cpu_qstats));
-
-unlock:
-       rcu_read_unlock();
+out:
        if (skb_at_tc_ingress(skb))
                skb_pull_rcsum(skb, skb->mac_len);
 
        return action;
+
+drop:
+       qstats_drop_inc(this_cpu_ptr(v->common.cpu_qstats));
+       return TC_ACT_SHOT;
 }
 
 static const struct nla_policy vlan_policy[TCA_VLAN_MAX + 1] = {