| 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
| ||
|
Message-ID: <vbfbm39qbsw.fsf@mellanox.com>
Date: Mon, 18 Feb 2019 16:52:20 +0000
From: Vlad Buslov <vladbu@...lanox.com>
To: Davide Caratti <dcaratti@...hat.com>
CC: Jamal Hadi Salim <jhs@...atatu.com>,
Cong Wang <xiyou.wangcong@...il.com>,
Jiri Pirko <jiri@...nulli.us>,
"David S. Miller" <davem@...emloft.net>,
Paolo Abeni <pabeni@...hat.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH RFC 2/5] net/sched: prepare TC actions to properly
validate the control action
Hi Davide,
I really like the idea of putting all action validation code in single
place, instead of spreading it between act API and specific actions init
code.
See my comment regarding minor problem with using
tcf_action_set_ctrlact() on current net-next below.
On Fri 15 Feb 2019 at 23:06, Davide Caratti <dcaratti@...hat.com> wrote:
> - add tcf_action_check_ctrlact(), and pass a pointer to struct tcf_proto
> in each actions's init() function, to allow validation of 'goto chain'
> control action.
> - add tcf_action_set_ctrlact(), to set the control action, release the
> previous 'goto_chain' handle and replace it with the new one.
>
> Signed-off-by: Davide Caratti <dcaratti@...hat.com>
> ---
> include/net/act_api.h | 9 +++++-
> net/sched/act_api.c | 58 ++++++++++++++++++++++++++++++++++++--
> net/sched/act_bpf.c | 2 +-
> net/sched/act_connmark.c | 1 +
> net/sched/act_csum.c | 2 +-
> net/sched/act_gact.c | 2 +-
> net/sched/act_ife.c | 2 +-
> net/sched/act_ipt.c | 11 ++++----
> net/sched/act_mirred.c | 1 +
> net/sched/act_nat.c | 3 +-
> net/sched/act_pedit.c | 2 +-
> net/sched/act_police.c | 1 +
> net/sched/act_sample.c | 2 +-
> net/sched/act_simple.c | 2 +-
> net/sched/act_skbedit.c | 1 +
> net/sched/act_skbmod.c | 1 +
> net/sched/act_tunnel_key.c | 1 +
> net/sched/act_vlan.c | 2 +-
> 18 files changed, 86 insertions(+), 17 deletions(-)
>
> diff --git a/include/net/act_api.h b/include/net/act_api.h
> index dbc795ec659e..55a8d44ac0c7 100644
> --- a/include/net/act_api.h
> +++ b/include/net/act_api.h
> @@ -90,7 +90,7 @@ struct tc_action_ops {
> int (*lookup)(struct net *net, struct tc_action **a, u32 index);
> int (*init)(struct net *net, struct nlattr *nla,
> struct nlattr *est, struct tc_action **act, int ovr,
> - int bind, bool rtnl_held,
> + int bind, bool rtnl_held, struct tcf_proto *tp,
> struct netlink_ext_ack *extack);
> int (*walk)(struct net *, struct sk_buff *,
> struct netlink_callback *, int,
> @@ -181,6 +181,13 @@ int tcf_action_dump_old(struct sk_buff *skb, struct tc_action *a, int, int);
> int tcf_action_dump_1(struct sk_buff *skb, struct tc_action *a, int, int);
> int tcf_action_copy_stats(struct sk_buff *, struct tc_action *, int);
>
> +int tcf_action_check_ctrlact(int action, struct tcf_proto *tp,
> + struct tcf_chain **handle,
> + struct netlink_ext_ack *extack);
> +
> +void tcf_action_set_ctrlact(struct tc_action *p, int action,
> + struct tcf_chain *goto_chain);
> +
> #endif /* CONFIG_NET_CLS_ACT */
>
> static inline void tcf_action_stats_update(struct tc_action *a, u64 bytes,
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index 91d79fac8cb2..088b0d846bde 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -71,6 +71,60 @@ static void tcf_set_action_cookie(struct tc_cookie __rcu **old_cookie,
> call_rcu(&old->rcu, tcf_free_cookie_rcu);
> }
>
> +int tcf_action_check_ctrlact(int action, struct tcf_proto *tp,
> + struct tcf_chain **handle,
> + struct netlink_ext_ack *extack)
> +{
> + int opcode = TC_ACT_EXT_OPCODE(action), ret = -EINVAL;
> + u32 chain_index;
> +
> + if (!opcode)
> + ret = action > TC_ACT_VALUE_MAX ? -EINVAL : 0;
> + else if (opcode <= TC_ACT_EXT_OPCODE_MAX || action == TC_ACT_UNSPEC)
> + ret = 0;
> + if (ret) {
> + NL_SET_ERR_MSG(extack, "invalid control action");
> + goto end;
> + }
> +
> + if (TC_ACT_EXT_CMP(action, TC_ACT_GOTO_CHAIN)) {
> + chain_index = action & TC_ACT_EXT_VAL_MASK;
> + if (!tp) {
> + ret = -EINVAL;
> + NL_SET_ERR_MSG(extack,
> + "can't use goto_chain with NULL proto");
> + goto end;
> + }
> + if (!handle) {
> + ret = -EINVAL;
> + NL_SET_ERR_MSG(extack,
> + "can't put goto_chain on NULL handle");
> + goto end;
> + }
> + *handle = tcf_chain_get_by_act(tp->chain->block, chain_index);
> + if (!*handle) {
> + ret = -ENOMEM;
> + NL_SET_ERR_MSG(extack,
> + "can't allocate goto_chain handle");
> + }
> + }
> +end:
> + return ret;
> +}
> +EXPORT_SYMBOL(tcf_action_check_ctrlact);
> +
> +void tcf_action_set_ctrlact(struct tc_action *p, int action,
> + struct tcf_chain *goto_chain)
> +{
> + struct tcf_chain *old;
> +
> + old = xchg(&p->goto_chain, goto_chain);
> + if (old)
> + tcf_chain_put_by_act(old);
This is blocking in current net-next because tcf_chain_put_by_act()
obtains block->lock mutex, so you can't call it while holding tcf_lock
spinlock like you do in following patches. Its not a big problem but I
guess you will have to just inline this code into all init handlers
instead of tcf_action_set_ctrlact() function.
BTW is atomic xchg strictly necessary if you hold tcf_lock while
re-assigning goto_chain pointer?
> + p->tcfa_action = action;
> +}
> +EXPORT_SYMBOL(tcf_action_set_ctrlact);
> +
> /* XXX: For standalone actions, we don't need a RCU grace period either, because
> * actions are always connected to filters and filters are already destroyed in
> * RCU callbacks, so after a RCU grace period actions are already disconnected
> @@ -890,10 +944,10 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
> /* backward compatibility for policer */
> if (name == NULL)
> err = a_o->init(net, tb[TCA_ACT_OPTIONS], est, &a, ovr, bind,
> - rtnl_held, extack);
> + rtnl_held, tp, extack);
> else
> err = a_o->init(net, nla, est, &a, ovr, bind, rtnl_held,
> - extack);
> + tp, extack);
> if (err < 0)
> goto err_mod;
>
> diff --git a/net/sched/act_bpf.c b/net/sched/act_bpf.c
> index c7633843e223..88a729bdab25 100644
> --- a/net/sched/act_bpf.c
> +++ b/net/sched/act_bpf.c
> @@ -278,7 +278,7 @@ static void tcf_bpf_prog_fill_cfg(const struct tcf_bpf *prog,
> static int tcf_bpf_init(struct net *net, struct nlattr *nla,
> struct nlattr *est, struct tc_action **act,
> int replace, int bind, bool rtnl_held,
> - struct netlink_ext_ack *extack)
> + struct tcf_proto *tp, struct netlink_ext_ack *extack)
> {
> struct tc_action_net *tn = net_generic(net, bpf_net_id);
> struct nlattr *tb[TCA_ACT_BPF_MAX + 1];
> diff --git a/net/sched/act_connmark.c b/net/sched/act_connmark.c
> index 8475913f2070..30c4c109c80c 100644
> --- a/net/sched/act_connmark.c
> +++ b/net/sched/act_connmark.c
> @@ -97,6 +97,7 @@ static const struct nla_policy connmark_policy[TCA_CONNMARK_MAX + 1] = {
> static int tcf_connmark_init(struct net *net, struct nlattr *nla,
> struct nlattr *est, struct tc_action **a,
> int ovr, int bind, bool rtnl_held,
> + struct tcf_proto *tp,
> struct netlink_ext_ack *extack)
> {
> struct tc_action_net *tn = net_generic(net, connmark_net_id);
> diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
> index 3dc25b7806d7..1ae120c9ab02 100644
> --- a/net/sched/act_csum.c
> +++ b/net/sched/act_csum.c
> @@ -46,7 +46,7 @@ static struct tc_action_ops act_csum_ops;
>
> static int tcf_csum_init(struct net *net, struct nlattr *nla,
> struct nlattr *est, struct tc_action **a, int ovr,
> - int bind, bool rtnl_held,
> + int bind, bool rtnl_held, struct tcf_proto *tp,
> struct netlink_ext_ack *extack)
> {
> struct tc_action_net *tn = net_generic(net, csum_net_id);
> diff --git a/net/sched/act_gact.c b/net/sched/act_gact.c
> index b61c20ebb314..727bbca9534b 100644
> --- a/net/sched/act_gact.c
> +++ b/net/sched/act_gact.c
> @@ -57,7 +57,7 @@ static const struct nla_policy gact_policy[TCA_GACT_MAX + 1] = {
> static int tcf_gact_init(struct net *net, struct nlattr *nla,
> struct nlattr *est, struct tc_action **a,
> int ovr, int bind, bool rtnl_held,
> - struct netlink_ext_ack *extack)
> + struct tcf_proto *tp, struct netlink_ext_ack *extack)
> {
> struct tc_action_net *tn = net_generic(net, gact_net_id);
> struct nlattr *tb[TCA_GACT_MAX + 1];
> diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
> index 30b63fa23ee2..9b2eb941e093 100644
> --- a/net/sched/act_ife.c
> +++ b/net/sched/act_ife.c
> @@ -469,7 +469,7 @@ static int populate_metalist(struct tcf_ife_info *ife, struct nlattr **tb,
> static int tcf_ife_init(struct net *net, struct nlattr *nla,
> struct nlattr *est, struct tc_action **a,
> int ovr, int bind, bool rtnl_held,
> - struct netlink_ext_ack *extack)
> + struct tcf_proto *tp, struct netlink_ext_ack *extack)
> {
> struct tc_action_net *tn = net_generic(net, ife_net_id);
> struct nlattr *tb[TCA_IFE_MAX + 1];
> diff --git a/net/sched/act_ipt.c b/net/sched/act_ipt.c
> index 8af6c11d2482..4b3c844ca988 100644
> --- a/net/sched/act_ipt.c
> +++ b/net/sched/act_ipt.c
> @@ -97,7 +97,8 @@ static const struct nla_policy ipt_policy[TCA_IPT_MAX + 1] = {
>
> static int __tcf_ipt_init(struct net *net, unsigned int id, struct nlattr *nla,
> struct nlattr *est, struct tc_action **a,
> - const struct tc_action_ops *ops, int ovr, int bind)
> + const struct tc_action_ops *ops, int ovr, int bind,
> + struct tcf_proto *tp)
> {
> struct tc_action_net *tn = net_generic(net, id);
> struct nlattr *tb[TCA_IPT_MAX + 1];
> @@ -206,20 +207,20 @@ static int __tcf_ipt_init(struct net *net, unsigned int id, struct nlattr *nla,
>
> static int tcf_ipt_init(struct net *net, struct nlattr *nla,
> struct nlattr *est, struct tc_action **a, int ovr,
> - int bind, bool rtnl_held,
> + int bind, bool rtnl_held, struct tcf_proto *tp,
> struct netlink_ext_ack *extack)
> {
> return __tcf_ipt_init(net, ipt_net_id, nla, est, a, &act_ipt_ops, ovr,
> - bind);
> + bind, tp);
> }
>
> static int tcf_xt_init(struct net *net, struct nlattr *nla,
> struct nlattr *est, struct tc_action **a, int ovr,
> - int bind, bool unlocked,
> + int bind, bool unlocked, struct tcf_proto *tp,
> struct netlink_ext_ack *extack)
> {
> return __tcf_ipt_init(net, xt_net_id, nla, est, a, &act_xt_ops, ovr,
> - bind);
> + bind, tp);
> }
>
> static int tcf_ipt_act(struct sk_buff *skb, const struct tc_action *a,
> diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
> index c8cf4d10c435..69dda57f1097 100644
> --- a/net/sched/act_mirred.c
> +++ b/net/sched/act_mirred.c
> @@ -94,6 +94,7 @@ static struct tc_action_ops act_mirred_ops;
> static int tcf_mirred_init(struct net *net, struct nlattr *nla,
> struct nlattr *est, struct tc_action **a,
> int ovr, int bind, bool rtnl_held,
> + struct tcf_proto *tp,
> struct netlink_ext_ack *extack)
> {
> struct tc_action_net *tn = net_generic(net, mirred_net_id);
> diff --git a/net/sched/act_nat.c b/net/sched/act_nat.c
> index c5c1e23add77..526c4c99bcce 100644
> --- a/net/sched/act_nat.c
> +++ b/net/sched/act_nat.c
> @@ -38,7 +38,8 @@ static const struct nla_policy nat_policy[TCA_NAT_MAX + 1] = {
>
> static int tcf_nat_init(struct net *net, struct nlattr *nla, struct nlattr *est,
> struct tc_action **a, int ovr, int bind,
> - bool rtnl_held, struct netlink_ext_ack *extack)
> + bool rtnl_held, struct tcf_proto *tp,
> + struct netlink_ext_ack *extack)
> {
> struct tc_action_net *tn = net_generic(net, nat_net_id);
> struct nlattr *tb[TCA_NAT_MAX + 1];
> diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
> index 2b372a06b432..1c7a0db7b466 100644
> --- a/net/sched/act_pedit.c
> +++ b/net/sched/act_pedit.c
> @@ -138,7 +138,7 @@ static int tcf_pedit_key_ex_dump(struct sk_buff *skb,
> static int tcf_pedit_init(struct net *net, struct nlattr *nla,
> struct nlattr *est, struct tc_action **a,
> int ovr, int bind, bool rtnl_held,
> - struct netlink_ext_ack *extack)
> + struct tcf_proto *tp, struct netlink_ext_ack *extack)
> {
> struct tc_action_net *tn = net_generic(net, pedit_net_id);
> struct nlattr *tb[TCA_PEDIT_MAX + 1];
> diff --git a/net/sched/act_police.c b/net/sched/act_police.c
> index ec8ec55e0fe8..a444dd78a244 100644
> --- a/net/sched/act_police.c
> +++ b/net/sched/act_police.c
> @@ -83,6 +83,7 @@ static const struct nla_policy police_policy[TCA_POLICE_MAX + 1] = {
> static int tcf_police_init(struct net *net, struct nlattr *nla,
> struct nlattr *est, struct tc_action **a,
> int ovr, int bind, bool rtnl_held,
> + struct tcf_proto *tp,
> struct netlink_ext_ack *extack)
> {
> int ret = 0, tcfp_result = TC_ACT_OK, err, size;
> diff --git a/net/sched/act_sample.c b/net/sched/act_sample.c
> index 1a0c682fd734..b2154edcb535 100644
> --- a/net/sched/act_sample.c
> +++ b/net/sched/act_sample.c
> @@ -37,7 +37,7 @@ static const struct nla_policy sample_policy[TCA_SAMPLE_MAX + 1] = {
>
> static int tcf_sample_init(struct net *net, struct nlattr *nla,
> struct nlattr *est, struct tc_action **a, int ovr,
> - int bind, bool rtnl_held,
> + int bind, bool rtnl_held, struct tcf_proto *tp,
> struct netlink_ext_ack *extack)
> {
> struct tc_action_net *tn = net_generic(net, sample_net_id);
> diff --git a/net/sched/act_simple.c b/net/sched/act_simple.c
> index 902957beceb3..640ee5b785dc 100644
> --- a/net/sched/act_simple.c
> +++ b/net/sched/act_simple.c
> @@ -80,7 +80,7 @@ static const struct nla_policy simple_policy[TCA_DEF_MAX + 1] = {
> static int tcf_simp_init(struct net *net, struct nlattr *nla,
> struct nlattr *est, struct tc_action **a,
> int ovr, int bind, bool rtnl_held,
> - struct netlink_ext_ack *extack)
> + struct tcf_proto *tp, struct netlink_ext_ack *extack)
> {
> struct tc_action_net *tn = net_generic(net, simp_net_id);
> struct nlattr *tb[TCA_DEF_MAX + 1];
> diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
> index 64dba3708fce..9fc8cfdd35b1 100644
> --- a/net/sched/act_skbedit.c
> +++ b/net/sched/act_skbedit.c
> @@ -96,6 +96,7 @@ static const struct nla_policy skbedit_policy[TCA_SKBEDIT_MAX + 1] = {
> static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
> struct nlattr *est, struct tc_action **a,
> int ovr, int bind, bool rtnl_held,
> + struct tcf_proto *tp,
> struct netlink_ext_ack *extack)
> {
> struct tc_action_net *tn = net_generic(net, skbedit_net_id);
> diff --git a/net/sched/act_skbmod.c b/net/sched/act_skbmod.c
> index 59710a183bd3..35572d0e4576 100644
> --- a/net/sched/act_skbmod.c
> +++ b/net/sched/act_skbmod.c
> @@ -82,6 +82,7 @@ static const struct nla_policy skbmod_policy[TCA_SKBMOD_MAX + 1] = {
> static int tcf_skbmod_init(struct net *net, struct nlattr *nla,
> struct nlattr *est, struct tc_action **a,
> int ovr, int bind, bool rtnl_held,
> + struct tcf_proto *tp,
> struct netlink_ext_ack *extack)
> {
> struct tc_action_net *tn = net_generic(net, skbmod_net_id);
> diff --git a/net/sched/act_tunnel_key.c b/net/sched/act_tunnel_key.c
> index 8b43fe0130f7..c4adc53e0fb4 100644
> --- a/net/sched/act_tunnel_key.c
> +++ b/net/sched/act_tunnel_key.c
> @@ -209,6 +209,7 @@ static void tunnel_key_release_params(struct tcf_tunnel_key_params *p)
> static int tunnel_key_init(struct net *net, struct nlattr *nla,
> struct nlattr *est, struct tc_action **a,
> int ovr, int bind, bool rtnl_held,
> + struct tcf_proto *tp,
> struct netlink_ext_ack *extack)
> {
> struct tc_action_net *tn = net_generic(net, tunnel_key_net_id);
> diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
> index 93fdaf707313..80fd0e238a10 100644
> --- a/net/sched/act_vlan.c
> +++ b/net/sched/act_vlan.c
> @@ -105,7 +105,7 @@ static const struct nla_policy vlan_policy[TCA_VLAN_MAX + 1] = {
> static int tcf_vlan_init(struct net *net, struct nlattr *nla,
> struct nlattr *est, struct tc_action **a,
> int ovr, int bind, bool rtnl_held,
> - struct netlink_ext_ack *extack)
> + struct tcf_proto *tp, struct netlink_ext_ack *extack)
> {
> struct tc_action_net *tn = net_generic(net, vlan_net_id);
> struct nlattr *tb[TCA_VLAN_MAX + 1];
Powered by blists - more mailing lists