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]

Date:   Wed, 21 Dec 2016 10:51:37 -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 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.

I mean:

diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 3fbba79..a21f7d66 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -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;
@@ -278,8 +277,7 @@ static int tc_ctl_tfilter(struct sk_buff *skb,
struct nlmsghdr *n)
                                tp_ops = tcf_proto_lookup_ops(kind);
                                /* We dropped the RTNL semaphore in order to
                                 * perform the module load.  So, even if we
-                                * succeeded in loading the module we have to
-                                * replay the request.  We indicate this using
+                                * succeeded in loading the module we return
                                 * -EAGAIN.
                                 */
                                if (tp_ops != NULL) {
@@ -378,9 +376,6 @@ 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;
 }