On Wed, Dec 21, 2016 at 10:51 AM, Cong Wang <xiyou.wangc...@gmail.com> wrote:
> On Wed, Dec 21, 2016 at 9:04 AM, Daniel Borkmann <dan...@iogearbox.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)
