From patchwork Wed Dec 21 19:10:59 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Cong Wang X-Patchwork-Id: 707940 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 3tkPQR04Wbz9sCX for ; Thu, 22 Dec 2016 06:11:27 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.b="NDeBLHVN"; dkim-atps=neutral Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757662AbcLUTLW (ORCPT ); Wed, 21 Dec 2016 14:11:22 -0500 Received: from mail-qt0-f196.google.com ([209.85.216.196]:32771 "EHLO mail-qt0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754086AbcLUTLV (ORCPT ); Wed, 21 Dec 2016 14:11:21 -0500 Received: by mail-qt0-f196.google.com with SMTP id n6so27949683qtd.0 for ; Wed, 21 Dec 2016 11:11:20 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=nYyPfkk+5gYGK/7MSZyWgjxoBDXNxq8ngcki144bUnI=; b=NDeBLHVNqpV8s9O5B5e0JP3S3txFNbtCsdRkLhXHV7RUhroegHOIDL+pbvGFl+/tTh ady/DYzDRIpycVHMEzqYtpukB51iE//KlqlAv4IGQs7kIoK/kTGAqFy3QsZA6XHPKC5b oi9B1f4/BtzdxQcS3iOSbafYe/eRG3NjScu4t40e/KFIAbcFZTFSL4eODlPtXtjcj9za +MtWvL/X4L9wxXziydEYK5gWWdxhG/48Db27g/WqysTIJ32FY6gw2TBJKIntAiPl+DkY +4GhGSJtNcQWJxKEszAu/ls4jyVJBueobZCeWC4Y+CtamEureX9q9LQRpawchPflQrc+ mTKA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=nYyPfkk+5gYGK/7MSZyWgjxoBDXNxq8ngcki144bUnI=; b=O1O+72VrTUttuSDcj1/oMt1w8xHJ9/GBARTaeuRDSkXkNUZeEzfnyEF34FK01hufI/ eFXrxd1Mgd6SCuZKUn/9WiO/QQEGF9vG7Wkri9xy5YZeYpcb1mIHE+hHVqbmDf4iZZyd 1KoPXwS7Lhv0OR/QGzkHARmQPnO5fLVfkvjMb6745TJ2xxOxq3UiFg9YIY+LfxnxJ2rz drPXzX4te9yIXDm2N6/rpIgfljOMh/3bXwJT2FTOggPVvKKn+crR9RSdecTLeURmoos4 t+4YSxyDE0XgFH5iw+USAccOytmccHNljOGqhxWnIu+3B6DyBifFBK8pHwRwdPRRWk09 glzg== X-Gm-Message-State: AIkVDXKBiITw1CSkmUIwYO80wm9HaySPMhwkWwGUAV66vH/ITMl8OOikrx3govBHnx8DWTAzb6DQVqku6G1esA== X-Received: by 10.200.45.6 with SMTP id n6mr6286293qta.220.1482347480216; Wed, 21 Dec 2016 11:11:20 -0800 (PST) MIME-Version: 1.0 Received: by 10.12.181.10 with HTTP; Wed, 21 Dec 2016 11:10:59 -0800 (PST) In-Reply-To: References: <1954223e8b7aa90ff835357e8a3ef7537be33e43.1482337600.git.daniel@iogearbox.net> From: Cong Wang Date: Wed, 21 Dec 2016 11:10:59 -0800 Message-ID: Subject: Re: [PATCH net] net, sched: fix soft lockup in tc_classify To: Daniel Borkmann Cc: David Miller , Shahar Klein , Or Gerlitz , Roi Dayan , Jiri Pirko , John Fastabend , Linux Kernel Network Developers Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Wed, Dec 21, 2016 at 10:51 AM, Cong Wang wrote: > On Wed, Dec 21, 2016 at 9:04 AM, Daniel Borkmann 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: 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) 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)