| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 22 Nov 2021 10:13:00 +0000
From: Baowen Zheng <baowen.zheng@...igine.com>
To: Vlad Buslov <vladbu@...dia.com>,
Simon Horman <simon.horman@...igine.com>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
Cong Wang <xiyou.wangcong@...il.com>,
Ido Schimmel <idosch@...dia.com>,
Jamal Hadi Salim <jhs@...atatu.com>,
Jiri Pirko <jiri@...nulli.us>, Oz Shlomo <ozsh@...dia.com>,
Roi Dayan <roid@...dia.com>,
Louis Peens <louis.peens@...igine.com>,
oss-drivers <oss-drivers@...igine.com>
Subject: RE: [PATCH v4 08/10] flow_offload: add reoffload process to update
hw_count
On November 20, 2021 4:09 AM, Vlad Buslov wrote:
>On Thu 18 Nov 2021 at 15:08, Simon Horman <simon.horman@...igine.com>
>wrote:
>> From: Baowen Zheng <baowen.zheng@...igine.com>
>>
>> Add reoffload process to update hw_count when driver is inserted or
>> removed.
>>
>> When reoffloading actions, we still offload the actions that are added
>> independent of filters.
>>
>> Signed-off-by: Baowen Zheng <baowen.zheng@...igine.com>
>> Signed-off-by: Louis Peens <louis.peens@...igine.com>
>> Signed-off-by: Simon Horman <simon.horman@...igine.com>
>> ---
>> include/net/act_api.h | 24 +++++
>> net/core/flow_offload.c | 4 +
>> net/sched/act_api.c | 213
>++++++++++++++++++++++++++++++++++++----
>> 3 files changed, 222 insertions(+), 19 deletions(-)
>>
>> diff --git a/include/net/act_api.h b/include/net/act_api.h index
>> 7900598d2dd3..e5e6e58df618 100644
>> --- a/include/net/act_api.h
>> +++ b/include/net/act_api.h
>> @@ -7,6 +7,7 @@
>> */
>>
>> #include <linux/refcount.h>
>> +#include <net/flow_offload.h>
>> #include <net/sch_generic.h>
>> #include <net/pkt_sched.h>
>> #include <net/net_namespace.h>
>> @@ -243,11 +244,26 @@ static inline void flow_action_hw_count_set(struct
>tc_action *act,
>> act->in_hw_count = hw_count;
>> }
>>
>> +static inline void flow_action_hw_count_inc(struct tc_action *act,
>> + u32 hw_count)
>> +{
>> + act->in_hw_count += hw_count;
>> +}
>> +
>> +static inline void flow_action_hw_count_dec(struct tc_action *act,
>> + u32 hw_count)
>> +{
>> + act->in_hw_count = act->in_hw_count > hw_count ?
>> + act->in_hw_count - hw_count : 0; }
>> +
>> void tcf_action_update_stats(struct tc_action *a, u64 bytes, u64 packets,
>> u64 drops, bool hw);
>> int tcf_action_copy_stats(struct sk_buff *, struct tc_action *, int);
>>
>> int tcf_action_update_hw_stats(struct tc_action *action);
>> +int tcf_action_reoffload_cb(flow_indr_block_bind_cb_t *cb,
>> + void *cb_priv, bool add);
>> int tcf_action_check_ctrlact(int action, struct tcf_proto *tp,
>> struct tcf_chain **handle,
>> struct netlink_ext_ack *newchain); @@ -259,6
>+275,14 @@
>> DECLARE_STATIC_KEY_FALSE(tcf_frag_xmit_count);
>> #endif
>>
>> int tcf_dev_queue_xmit(struct sk_buff *skb, int (*xmit)(struct
>> sk_buff *skb));
>> +
>> +#else /* !CONFIG_NET_CLS_ACT */
>> +
>> +static inline int tcf_action_reoffload_cb(flow_indr_block_bind_cb_t *cb,
>> + void *cb_priv, bool add) {
>> + return 0;
>> +}
>> +
>> #endif /* CONFIG_NET_CLS_ACT */
>>
>> static inline void tcf_action_stats_update(struct tc_action *a, u64
>> bytes, diff --git a/net/core/flow_offload.c b/net/core/flow_offload.c
>> index 6676431733ef..92000164ac37 100644
>> --- a/net/core/flow_offload.c
>> +++ b/net/core/flow_offload.c
>> @@ -1,6 +1,7 @@
>> /* SPDX-License-Identifier: GPL-2.0 */ #include <linux/kernel.h>
>> #include <linux/slab.h>
>> +#include <net/act_api.h>
>> #include <net/flow_offload.h>
>> #include <linux/rtnetlink.h>
>> #include <linux/mutex.h>
>> @@ -418,6 +419,8 @@ int
>flow_indr_dev_register(flow_indr_block_bind_cb_t *cb, void *cb_priv)
>> existing_qdiscs_register(cb, cb_priv);
>> mutex_unlock(&flow_indr_block_lock);
>>
>> + tcf_action_reoffload_cb(cb, cb_priv, true);
>> +
>> return 0;
>> }
>> EXPORT_SYMBOL(flow_indr_dev_register);
>> @@ -470,6 +473,7 @@ void
>flow_indr_dev_unregister(flow_indr_block_bind_cb_t *cb, void *cb_priv,
>> __flow_block_indr_cleanup(release, cb_priv, &cleanup_list);
>> mutex_unlock(&flow_indr_block_lock);
>>
>> + tcf_action_reoffload_cb(cb, cb_priv, false);
>> flow_block_indr_notify(&cleanup_list);
>> kfree(indr_dev);
>> }
>> diff --git a/net/sched/act_api.c b/net/sched/act_api.c index
>> f5834d47a392..ada51b2df851 100644
>> --- a/net/sched/act_api.c
>> +++ b/net/sched/act_api.c
>> @@ -225,15 +225,11 @@ static int flow_action_init(struct
>flow_offload_action *fl_action,
>> return 0;
>> }
>>
>> -static int tcf_action_offload_cmd(struct flow_offload_action *fl_act,
>> - u32 *hw_count,
>> - struct netlink_ext_ack *extack)
>> +static int tcf_action_offload_cmd_ex(struct flow_offload_action *fl_act,
>> + u32 *hw_count)
>> {
>> int err;
>>
>> - if (IS_ERR(fl_act))
>> - return PTR_ERR(fl_act);
>> -
>> err = flow_indr_dev_setup_offload(NULL, NULL, TC_SETUP_ACT,
>> fl_act, NULL, NULL);
>> if (err < 0)
>> @@ -245,9 +241,41 @@ static int tcf_action_offload_cmd(struct
>flow_offload_action *fl_act,
>> return 0;
>> }
>>
>> +static int tcf_action_offload_cmd_cb_ex(struct flow_offload_action
>*fl_act,
>> + u32 *hw_count,
>> + flow_indr_block_bind_cb_t *cb,
>> + void *cb_priv)
>> +{
>> + int err;
>> +
>> + err = cb(NULL, NULL, cb_priv, TC_SETUP_ACT, NULL, fl_act, NULL);
>> + if (err < 0)
>> + return err;
>> +
>> + if (hw_count)
>> + *hw_count = 1;
>> +
>> + return 0;
>> +}
>> +
>> +static int tcf_action_offload_cmd(struct flow_offload_action *fl_act,
>> + u32 *hw_count,
>> + flow_indr_block_bind_cb_t *cb,
>> + void *cb_priv)
>> +{
>> + if (IS_ERR(fl_act))
>> + return PTR_ERR(fl_act);
>> +
>> + return cb ? tcf_action_offload_cmd_cb_ex(fl_act, hw_count,
>> + cb, cb_priv) :
>> + tcf_action_offload_cmd_ex(fl_act, hw_count); }
>> +
>> /* offload the tc command after inserted */ -static int
>> tcf_action_offload_add(struct tc_action *action,
>> - struct netlink_ext_ack *extack)
>> +static int tcf_action_offload_add_ex(struct tc_action *action,
>> + struct netlink_ext_ack *extack,
>> + flow_indr_block_bind_cb_t *cb,
>> + void *cb_priv)
>> {
>> bool skip_sw = tc_act_skip_sw(action->tcfa_flags);
>> struct tc_action *actions[TCA_ACT_MAX_PRIO] = { @@ -275,9 +303,10
>@@
>> static int tcf_action_offload_add(struct tc_action *action,
>> goto fl_err;
>> }
>>
>> - err = tcf_action_offload_cmd(fl_action, &in_hw_count, extack);
>> + err = tcf_action_offload_cmd(fl_action, &in_hw_count, cb, cb_priv);
>> if (!err)
>> - flow_action_hw_count_set(action, in_hw_count);
>> + cb ? flow_action_hw_count_inc(action, in_hw_count) :
>> + flow_action_hw_count_set(action, in_hw_count);
>>
>> if (skip_sw && !tc_act_in_hw(action))
>> err = -EINVAL;
>> @@ -290,6 +319,12 @@ static int tcf_action_offload_add(struct tc_action
>*action,
>> return err;
>> }
>>
>> +static int tcf_action_offload_add(struct tc_action *action,
>> + struct netlink_ext_ack *extack) {
>> + return tcf_action_offload_add_ex(action, extack, NULL, NULL); }
>> +
>> int tcf_action_update_hw_stats(struct tc_action *action) {
>> struct flow_offload_action fl_act = {}; @@ -302,7 +337,7 @@ int
>> tcf_action_update_hw_stats(struct tc_action *action)
>> if (err)
>> return err;
>>
>> - err = tcf_action_offload_cmd(&fl_act, NULL, NULL);
>> + err = tcf_action_offload_cmd(&fl_act, NULL, NULL, NULL);
>> if (!err) {
>> preempt_disable();
>> tcf_action_stats_update(action, fl_act.stats.bytes, @@ -321,7
>> +356,9 @@ int tcf_action_update_hw_stats(struct tc_action *action) }
>> EXPORT_SYMBOL(tcf_action_update_hw_stats);
>>
>> -static int tcf_action_offload_del(struct tc_action *action)
>> +static int tcf_action_offload_del_ex(struct tc_action *action,
>> + flow_indr_block_bind_cb_t *cb,
>> + void *cb_priv)
>> {
>> struct flow_offload_action fl_act;
>> u32 in_hw_count = 0;
>> @@ -337,16 +374,25 @@ static int tcf_action_offload_del(struct tc_action
>*action)
>> if (err)
>> return err;
>>
>> - err = tcf_action_offload_cmd(&fl_act, &in_hw_count, NULL);
>> - if (err)
>> + err = tcf_action_offload_cmd(&fl_act, &in_hw_count, cb, cb_priv);
>> + if (err < 0)
>> return err;
>>
>> - if (action->in_hw_count != in_hw_count)
>> + if (!cb && action->in_hw_count != in_hw_count)
>> return -EINVAL;
>>
>> + /* do not need to update hw state when deleting action */
>> + if (cb && in_hw_count)
>> + flow_action_hw_count_dec(action, in_hw_count);
>> +
>> return 0;
>> }
>>
>> +static int tcf_action_offload_del(struct tc_action *action) {
>> + return tcf_action_offload_del_ex(action, NULL, NULL); }
>> +
>> static void tcf_action_cleanup(struct tc_action *p) {
>> tcf_action_offload_del(p);
>> @@ -841,6 +887,59 @@ EXPORT_SYMBOL(tcf_idrinfo_destroy);
>>
>> static LIST_HEAD(act_base);
>> static DEFINE_RWLOCK(act_mod_lock);
>> +/* since act ops id is stored in pernet subsystem list,
>> + * then there is no way to walk through only all the action
>> + * subsystem, so we keep tc action pernet ops id for
>> + * reoffload to walk through.
>> + */
>> +static LIST_HEAD(act_pernet_id_list); static
>> +DEFINE_MUTEX(act_id_mutex); struct tc_act_pernet_id {
>> + struct list_head list;
>> + unsigned int id;
>> +};
>> +
>> +static int tcf_pernet_add_id_list(unsigned int id) {
>> + struct tc_act_pernet_id *id_ptr;
>> + int ret = 0;
>> +
>> + mutex_lock(&act_id_mutex);
>> + list_for_each_entry(id_ptr, &act_pernet_id_list, list) {
>> + if (id_ptr->id == id) {
>> + ret = -EEXIST;
>> + goto err_out;
>> + }
>> + }
>> +
>> + id_ptr = kzalloc(sizeof(*id_ptr), GFP_KERNEL);
>> + if (!id_ptr) {
>> + ret = -ENOMEM;
>> + goto err_out;
>> + }
>> + id_ptr->id = id;
>> +
>> + list_add_tail(&id_ptr->list, &act_pernet_id_list);
>> +
>> +err_out:
>> + mutex_unlock(&act_id_mutex);
>> + return ret;
>> +}
>> +
>> +static void tcf_pernet_del_id_list(unsigned int id) {
>> + struct tc_act_pernet_id *id_ptr;
>> +
>> + mutex_lock(&act_id_mutex);
>> + list_for_each_entry(id_ptr, &act_pernet_id_list, list) {
>> + if (id_ptr->id == id) {
>> + list_del(&id_ptr->list);
>> + kfree(id_ptr);
>> + break;
>> + }
>> + }
>> + mutex_unlock(&act_id_mutex);
>> +}
>>
>> int tcf_register_action(struct tc_action_ops *act,
>> struct pernet_operations *ops)
>> @@ -859,18 +958,30 @@ int tcf_register_action(struct tc_action_ops *act,
>> if (ret)
>> return ret;
>>
>> + if (ops->id) {
>> + ret = tcf_pernet_add_id_list(*ops->id);
>> + if (ret)
>> + goto id_err;
>> + }
>> +
>> write_lock(&act_mod_lock);
>> list_for_each_entry(a, &act_base, head) {
>> if (act->id == a->id || (strcmp(act->kind, a->kind) == 0)) {
>> - write_unlock(&act_mod_lock);
>> - unregister_pernet_subsys(ops);
>> - return -EEXIST;
>> + ret = -EEXIST;
>> + goto err_out;
>> }
>> }
>> list_add_tail(&act->head, &act_base);
>> write_unlock(&act_mod_lock);
>>
>> return 0;
>> +
>> +err_out:
>> + write_unlock(&act_mod_lock);
>> + tcf_pernet_del_id_list(*ops->id);
>> +id_err:
>
>Rename to 'err_id' to harmonize label naming.
Ok, will make the change.
>
>> + unregister_pernet_subsys(ops);
>> + return ret;
>> }
>> EXPORT_SYMBOL(tcf_register_action);
>>
>> @@ -889,12 +1000,76 @@ int tcf_unregister_action(struct tc_action_ops
>*act,
>> }
>> }
>> write_unlock(&act_mod_lock);
>> - if (!err)
>> + if (!err) {
>> unregister_pernet_subsys(ops);
>> + if (ops->id)
>> + tcf_pernet_del_id_list(*ops->id);
>> + }
>> return err;
>> }
>> EXPORT_SYMBOL(tcf_unregister_action);
>>
>> +int tcf_action_reoffload_cb(flow_indr_block_bind_cb_t *cb,
>> + void *cb_priv, bool add)
>> +{
>> + struct tc_act_pernet_id *id_ptr;
>> + struct tcf_idrinfo *idrinfo;
>> + struct tc_action_net *tn;
>> + struct tc_action *p;
>> + unsigned int act_id;
>> + unsigned long tmp;
>> + unsigned long id;
>> + struct idr *idr;
>> + struct net *net;
>> + int ret;
>> +
>> + if (!cb)
>> + return -EINVAL;
>> +
>> + down_read(&net_rwsem);
>> + mutex_lock(&act_id_mutex);
>> +
>> + for_each_net(net) {
>> + list_for_each_entry(id_ptr, &act_pernet_id_list, list) {
>> + act_id = id_ptr->id;
>> + tn = net_generic(net, act_id);
>> + if (!tn)
>> + continue;
>> + idrinfo = tn->idrinfo;
>> + if (!idrinfo)
>> + continue;
>> +
>> + mutex_lock(&idrinfo->lock);
>> + idr = &idrinfo->action_idr;
>> + idr_for_each_entry_ul(idr, p, tmp, id) {
>> + if (IS_ERR(p) || tc_act_bind(p->tcfa_flags))
>> + continue;
>> + if (add) {
>> + tcf_action_offload_add_ex(p, NULL,
>cb,
>> + cb_priv);
>> + continue;
>> + }
>> +
>> + /* cb unregister to update hw count */
>> + ret = tcf_action_offload_del_ex(p, cb,
>cb_priv);
>> + if (ret < 0)
>> + continue;
>> + if (tc_act_skip_sw(p->tcfa_flags) &&
>> + !tc_act_in_hw(p)) {
>> + ret = tcf_idr_release_unsafe(p);
>
>Deleting skip_sw action that is no longer offloaded to any hardware device is
>reasonable, but note that in this case no netlink notification is ever generated.
>Don't know if it is a problem, just highlighting the fact since the whole
>behavior of implicitly deleting such actions is completely omitted from the
>change log.
Thanks for bring this to us. We will think about to add the notification implement and also we will
change commit message to mention about this.
>
>> + if (ret == ACT_P_DELETED)
>> + module_put(p->ops->owner);
>> + }
>> + }
>> + mutex_unlock(&idrinfo->lock);
>> + }
>> + }
>> + mutex_unlock(&act_id_mutex);
>> + up_read(&net_rwsem);
>> +
>> + return 0;
>> +}
>> +
>> /* lookup by name */
>> static struct tc_action_ops *tc_lookup_action_n(char *kind) {
Powered by blists - more mailing lists