netdev - Re: [PATCH net] net, sched: fix soft lockup in tc

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAM_iQpX3Sk_YGDmvY-yj0CxPbG5qfT7P7ioNgY5Lb_DqsTWh6Q@mail.gmail.com>
Date:   Wed, 21 Dec 2016 11:10:59 -0800
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     David Miller <davem@...emloft.net>,
        Shahar Klein <shahark@...lanox.com>,
        Or Gerlitz <gerlitz.or@...il.com>,
        Roi Dayan <roid@...lanox.com>, Jiri Pirko <jiri@...lanox.com>,
        John Fastabend <john.fastabend@...il.com>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: [PATCH net] net, sched: fix soft lockup in tc_classify

On Wed, Dec 21, 2016 at 10:51 AM, Cong Wang <xiyou.wangcong@...il.com> wrote:
> On Wed, Dec 21, 2016 at 9:04 AM, Daniel Borkmann <daniel@...earbox.net> wrote:
>> What happens is that in tc_ctl_tfilter(), thread A allocates a new
>> tp, initializes it, sets tp_created to 1, and calls into tp->ops->change()
>> with it. In that classifier callback we had to unlock/lock the rtnl
>> mutex and returned with -EAGAIN. One reason why we need to drop there
>> is, for example, that we need to request an action module to be loaded.
>
> Excellent catch!
>
> But why do we have to replay the request here? Shouldn't we just return
> EAGAIN to user-space and let user-space decide to retry or not?
> Replaying is the root of the evil here.

Answer myself: probably due to historical reasons, but still replaying
inside such a big function is just error-prone, we should promote it
out:

diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 3fbba79..7d5b42b 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -129,7 +129,7 @@ static inline u32 tcf_auto_prio(struct tcf_proto *tp)

 /* Add/change/delete/get a filter node */

-static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n)
+static int __tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n)
 {
        struct net *net = sock_net(skb->sk);
        struct nlattr *tca[TCA_MAX + 1];
@@ -154,7 +154,6 @@ static int tc_ctl_tfilter(struct sk_buff *skb,
struct nlmsghdr *n)
            !netlink_ns_capable(skb, net->user_ns, CAP_NET_ADMIN))
                return -EPERM;

-replay:
        err = nlmsg_parse(n, sizeof(*t), tca, TCA_MAX, NULL);
        if (err < 0)
                return err;
@@ -378,12 +377,19 @@ static int tc_ctl_tfilter(struct sk_buff *skb,
struct nlmsghdr *n)
 errout:
        if (cl)
                cops->put(q, cl);
-       if (err == -EAGAIN)
-               /* Replay the request. */
-               goto replay;
        return err;
 }

+static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n)
+{
+       int ret;
+replay:
+       ret = __tc_ctl_tfilter(skb, n);
+       if (ret == -EAGAIN)
+               goto replay;
+       return ret;
+}
+
 static int tcf_fill_node(struct net *net, struct sk_buff *skb,
                         struct tcf_proto *tp, unsigned long fh, u32 portid,
                         u32 seq, u16 flags, int event)