On Mon, May 28, 2018 at 12:17:22AM +0300, Vlad Buslov wrote:
> Without rtnl lock protection it is no longer safe to use pointer to tc
> action without holding reference to it. (it can be destroyed concurrently)
>
> Remove unsafe action idr lookup function. Instead of it, implement safe tcf
> idr check function that atomically looks up action in idr and increments
> its reference and bind counters. Implement both action search and check
> using new safe function
>
> Reference taken by idr check is temporal and should not be accounted by
> userspace clients (both logically and to preserver current API behavior).
> Subtract temporal reference when dumping action to userspace using existing
> tca_get_fill function arguments.
>
> Signed-off-by: Vlad Buslov <vla...@mellanox.com>
Reviewed-by: Marcelo Ricardo Leitner <marcelo.leit...@gmail.com>
> ---
> Changes from V1 to V2:
> - Make __tcf_idr_check function static
> - Merge changes that take reference to action when performing lookup and
> changes that account for this additional reference when dumping action
> to user space into single patch.
>
> net/sched/act_api.c | 46 ++++++++++++++++++++--------------------------
> 1 file changed, 20 insertions(+), 26 deletions(-)
>
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index 256b0c93916c..aa304d36fee0 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -284,44 +284,38 @@ int tcf_generic_walker(struct tc_action_net *tn, struct
> sk_buff *skb,
> }
> EXPORT_SYMBOL(tcf_generic_walker);
>
> -static struct tc_action *tcf_idr_lookup(u32 index, struct tcf_idrinfo
> *idrinfo)
> +static bool __tcf_idr_check(struct tc_action_net *tn, u32 index,
> + struct tc_action **a, int bind)
> {
> - struct tc_action *p = NULL;
> + struct tcf_idrinfo *idrinfo = tn->idrinfo;
> + struct tc_action *p;
>
> spin_lock(&idrinfo->lock);
> p = idr_find(&idrinfo->action_idr, index);
> + if (p) {
> + refcount_inc(&p->tcfa_refcnt);
> + if (bind)
> + atomic_inc(&p->tcfa_bindcnt);
> + }
> spin_unlock(&idrinfo->lock);
>
> - return p;
> + if (p) {
> + *a = p;
> + return true;
> + }
> + return false;
> }
>
> int tcf_idr_search(struct tc_action_net *tn, struct tc_action **a, u32 index)
> {
> - struct tcf_idrinfo *idrinfo = tn->idrinfo;
> - struct tc_action *p = tcf_idr_lookup(index, idrinfo);
> -
> - if (p) {
> - *a = p;
> - return 1;
> - }
> - return 0;
> + return __tcf_idr_check(tn, index, a, 0);
> }
> EXPORT_SYMBOL(tcf_idr_search);
>
> bool tcf_idr_check(struct tc_action_net *tn, u32 index, struct tc_action **a,
> int bind)
> {
> - struct tcf_idrinfo *idrinfo = tn->idrinfo;
> - struct tc_action *p = tcf_idr_lookup(index, idrinfo);
> -
> - if (index && p) {
> - if (bind)
> - atomic_inc(&p->tcfa_bindcnt);
> - refcount_inc(&p->tcfa_refcnt);
> - *a = p;
> - return true;
> - }
> - return false;
> + return __tcf_idr_check(tn, index, a, bind);
> }
> EXPORT_SYMBOL(tcf_idr_check);
>
> @@ -932,7 +926,7 @@ tcf_get_notify(struct net *net, u32 portid, struct
> nlmsghdr *n,
> if (!skb)
> return -ENOBUFS;
> if (tca_get_fill(skb, actions, portid, n->nlmsg_seq, 0, event,
> - 0, 0) <= 0) {
> + 0, 1) <= 0) {
> NL_SET_ERR_MSG(extack, "Failed to fill netlink attributes while
> adding TC action");
> kfree_skb(skb);
> return -EINVAL;
> @@ -1072,7 +1066,7 @@ tcf_del_notify(struct net *net, struct nlmsghdr *n,
> struct list_head *actions,
> return -ENOBUFS;
>
> if (tca_get_fill(skb, actions, portid, n->nlmsg_seq, 0, RTM_DELACTION,
> - 0, 1) <= 0) {
> + 0, 2) <= 0) {
> NL_SET_ERR_MSG(extack, "Failed to fill netlink TC action
> attributes");
> kfree_skb(skb);
> return -EINVAL;
> @@ -1131,14 +1125,14 @@ tca_action_gd(struct net *net, struct nlattr *nla,
> struct nlmsghdr *n,
> if (event == RTM_GETACTION)
> ret = tcf_get_notify(net, portid, n, &actions, event, extack);
> else { /* delete */
> + cleanup_a(&actions, 1); /* lookup took reference */
> ret = tcf_del_notify(net, n, &actions, portid, attr_size,
> extack);
> if (ret)
> goto err;
> return ret;
> }
> err:
> - if (event != RTM_GETACTION)
> - tcf_action_destroy(&actions, 0);
> + tcf_action_destroy(&actions, 0);
> return ret;
> }
>
> --
> 2.7.5
>
