Re: [PATCH net] net, sched: fix soft lockup in tc_classify

On Wed, Dec 21, 2016 at 9:04 AM, Daniel Borkmann [off-list ref] 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;
 }