From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.1 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3F8C3C43381 for ; Fri, 15 Feb 2019 10:02:19 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id E953921B1A for ; Fri, 15 Feb 2019 10:02:18 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=Mellanox.com header.i=@Mellanox.com header.b="BMIHQavV" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2405450AbfBOKCR (ORCPT ); Fri, 15 Feb 2019 05:02:17 -0500 Received: from mail-eopbgr80079.outbound.protection.outlook.com ([40.107.8.79]:61696 "EHLO EUR04-VI1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1726122AbfBOKCR (ORCPT ); Fri, 15 Feb 2019 05:02:17 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=Mellanox.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=Av6Lh04EC4lc5wxLf3qQZoXLy7hNqkHgBG7SwlYy10k=; b=BMIHQavVi1g2lSLoE8n09eUI2hKPTIDSsQHiKZA4a1V3H9pzdLTZl3AQc9aFDtcLoXuidm0Qg0vTKRnG2wK5wSQtcyr/87hKvWbPSNHWkObUwwxClDKQjyBiPmwDJyxFp5BNlbzVPXhFkoRJOrWKFZbFnRdQPOeIXJFdi7WnxSs= Received: from HE1PR0502MB3641.eurprd05.prod.outlook.com (10.167.127.11) by HE1PR0502MB2940.eurprd05.prod.outlook.com (10.175.35.23) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.1622.16; Fri, 15 Feb 2019 10:02:11 +0000 Received: from HE1PR0502MB3641.eurprd05.prod.outlook.com ([fe80::4041:bb68:2af3:eab8]) by HE1PR0502MB3641.eurprd05.prod.outlook.com ([fe80::4041:bb68:2af3:eab8%5]) with mapi id 15.20.1622.018; Fri, 15 Feb 2019 10:02:11 +0000 From: Vlad Buslov To: Ido Schimmel CC: "netdev@vger.kernel.org" , "jhs@mojatatu.com" , "xiyou.wangcong@gmail.com" , "jiri@resnulli.us" , "davem@davemloft.net" , "ast@kernel.org" , "daniel@iogearbox.net" Subject: Re: [PATCH net-next v4 07/17] net: sched: protect filter_chain list with filter_chain_lock mutex Thread-Topic: [PATCH net-next v4 07/17] net: sched: protect filter_chain list with filter_chain_lock mutex Thread-Index: AQHUwefD/Cm+p7M7pEKHPk7WVBdPDaXfoUsAgAEF54A= Date: Fri, 15 Feb 2019 10:02:11 +0000 Message-ID: References: <20190211085548.7190-1-vladbu@mellanox.com> <20190211085548.7190-8-vladbu@mellanox.com> <20190214182442.GA19269@splinter> In-Reply-To: <20190214182442.GA19269@splinter> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-clientproxiedby: LO2P265CA0189.GBRP265.PROD.OUTLOOK.COM (2603:10a6:600:a::33) To HE1PR0502MB3641.eurprd05.prod.outlook.com (2603:10a6:7:85::11) authentication-results: spf=none (sender IP is ) smtp.mailfrom=vladbu@mellanox.com; x-ms-exchange-messagesentrepresentingtype: 1 x-originating-ip: [37.142.13.130] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: d5e86ffd-5335-4e1d-364e-08d6932ca71e x-ms-office365-filtering-ht: Tenant x-microsoft-antispam: BCL:0;PCL:0;RULEID:(2390118)(7020095)(4652040)(8989299)(4534185)(4627221)(201703031133081)(201702281549075)(8990200)(5600110)(711020)(4605077)(4618075)(2017052603328)(7153060)(7193020);SRVR:HE1PR0502MB2940; x-ms-traffictypediagnostic: HE1PR0502MB2940: x-microsoft-exchange-diagnostics: =?iso-8859-1?Q?1;HE1PR0502MB2940;23:U8m6Aan1623PiorLZV3nmmZbghr0shXWu3VNW?= =?iso-8859-1?Q?ph3XPCU0btFR2trkkxrrN9KA8Znmy2tJTJT1MCK3WCfzWuIOrIfVAV+l6+?= =?iso-8859-1?Q?VHB9CeFZaJIJo5YqY6XhREtXCOHETq4wHkr7QtERTJGuMYJxqGe0ZXddEn?= =?iso-8859-1?Q?/pjl2MvH2C80TlVkDxI3h61D8THUzVfY2v9DZuRtqq2q6wy9+tYDFW+xfQ?= =?iso-8859-1?Q?Deh/wjS1X/SYCKDtH/QJSb0m8cSNciDoi0d0Ak62JhQnsR0U60GcAhj7X9?= =?iso-8859-1?Q?Uyfn7+uBJz5tFk8K1fjqOW8AM+vzo62Xeif5xUMcZwNutaUK9w3AvErjBU?= =?iso-8859-1?Q?leJXywrr2yJJIDLU628e8qzdfGas4euL2aPuKeXA9ehKob1h/jIMF7jSZR?= =?iso-8859-1?Q?uFZt0lUucfbIQaLEXd60GmhpovhC77dqH6qjpPYp3bP8YtjWCYqX+tmjpW?= =?iso-8859-1?Q?aOofhpAYTfOoSgPTKN15c111T+22YOh4qkeHLPdalkDe9Ddm8ryOvYPFwM?= =?iso-8859-1?Q?TlAGswbrHiIrb9wRF1izVyncHHBZAjTk+VIMMZRSRac8dVQXDBPCmFRx1/?= =?iso-8859-1?Q?tPygF2AA6/JDjbiGTCD3CPwogcL1AH6xgRUEOBFpvhe6XHHU91wy2FWnNC?= =?iso-8859-1?Q?jaTZZwXhkD2EM+sRjBjQJylITQHqDII1R+3RFEcJ91wtAb4A1z4+XBJE7u?= =?iso-8859-1?Q?avuuxMLGGjzIWnBeeHTSERcy+H6hWJpn9HJ7529jgkJYO7kKFvIPo8Nk7B?= =?iso-8859-1?Q?gdtQSBUDqUcusd42vrVDN2S/6lWkImzvIGo/Xtk09PaZAqpq+JkMZBNH5c?= =?iso-8859-1?Q?RI8oewhuk/AUuvADe9Vb87OLt9qX7MaPx40iE8uTAh4RYTukHjXRWvXw2g?= =?iso-8859-1?Q?wBQUj9cA0PzCCIwbK4ZBgtI27ZiJnepCPMwgEmwFBCZmb8IP8BkMK3lql0?= =?iso-8859-1?Q?Leklw6yD72tyYkkOzpCWbVG5EYcFyTa0EVdNFI9ysARRccSfguajJzm1iO?= =?iso-8859-1?Q?Loj9MxggEBpzr2I9JcfwolWnliRzeqTJz/GbjVE8HOKrPqu2WXptEx0wnF?= =?iso-8859-1?Q?DqUIbE6SJXzVkA9ptzB5aJPgClKT1ULW2vpBVABVftqdewxemXeXmMpfpK?= =?iso-8859-1?Q?mB1iCsWZz48u2JBH8RaSVSKYVm1stQjv4qczdasqYq/FAs8Ff+Z02e+GV5?= =?iso-8859-1?Q?CFu6bWSkiZA+qH0i3RwxoaiJOr2xGcHaRvgMFfTNFa8OiEHsi94wBOXKPE?= =?iso-8859-1?Q?2xMwr+nH0hrMdKqehFR?= x-microsoft-antispam-prvs: x-forefront-prvs: 09497C15EB x-forefront-antispam-report: SFV:NSPM;SFS:(10009020)(346002)(396003)(39850400004)(136003)(376002)(366004)(199004)(189003)(486006)(66066001)(316002)(71200400001)(54906003)(6512007)(53936002)(478600001)(71190400001)(14454004)(76176011)(6246003)(2906002)(6916009)(68736007)(7736002)(52116002)(305945005)(36756003)(106356001)(99286004)(105586002)(446003)(97736004)(11346002)(476003)(2616005)(3846002)(6116002)(6436002)(229853002)(8936002)(6506007)(26005)(386003)(102836004)(14444005)(86362001)(81166006)(256004)(6486002)(81156014)(186003)(25786009)(4326008)(8676002);DIR:OUT;SFP:1101;SCL:1;SRVR:HE1PR0502MB2940;H:HE1PR0502MB3641.eurprd05.prod.outlook.com;FPR:;SPF:None;LANG:en;PTR:InfoNoRecords;A:1;MX:1; received-spf: None (protection.outlook.com: mellanox.com does not designate permitted sender hosts) x-ms-exchange-senderadcheck: 1 x-microsoft-antispam-message-info: sGXy2pq+M5xvBd4soYtwdTnGbK5PHPvQ35zx0bW0gRzj8fIwW776QXYtJDold6GCcLVvFtwPQcfC7kSCQOV3XgMGE7XlwY7EWcKdOATxVAGQeARLTNuaUcjtyHQwmWsJzwPujQk1FQ7vBIzb6XyfNCnHAzpUKU1tenGoH1yX+kEUSQyMBQKpP9aX2j0JyI7ma6+SNDIQdT8V7UFLep0YKKBmoZSSqkKHeIUJLGXRKTV0LDDklSHyf6Z6ZlEL8KxuO4jsvmbsm6Qj9TYXSdvHdRS2nusmhDibGK8QkvouSkbPqo6P7YfQ2SbjY5X1aYSXbtJ68T86vQZ8UCag5OGoPOXZ3c3wpvgOv4VpRlpLt86CXbRi7WkFd8qyPeEA1XdLmkDG/09rJwBUeVW96cWFMLL3HJaQ5Ah2rpOkFYqeDJ0= Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: Mellanox.com X-MS-Exchange-CrossTenant-Network-Message-Id: d5e86ffd-5335-4e1d-364e-08d6932ca71e X-MS-Exchange-CrossTenant-originalarrivaltime: 15 Feb 2019 10:02:10.0535 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-id: a652971c-7d2e-4d9b-a6a4-d149256f461b X-MS-Exchange-Transport-CrossTenantHeadersStamped: HE1PR0502MB2940 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Thu 14 Feb 2019 at 18:24, Ido Schimmel wrote: > On Mon, Feb 11, 2019 at 10:55:38AM +0200, Vlad Buslov wrote: >> Extend tcf_chain with new filter_chain_lock mutex. Always lock the chain >> when accessing filter_chain list, instead of relying on rtnl lock. >> Dereference filter_chain with tcf_chain_dereference() lockdep macro to >> verify that all users of chain_list have the lock taken. >> >> Rearrange tp insert/remove code in tc_new_tfilter/tc_del_tfilter to exec= ute >> all necessary code while holding chain lock in order to prevent >> invalidation of chain_info structure by potential concurrent change. Thi= s >> also serializes calls to tcf_chain0_head_change(), which allows head cha= nge >> callbacks to rely on filter_chain_lock for synchronization instead of rt= nl >> mutex. >> >> Signed-off-by: Vlad Buslov >> Acked-by: Jiri Pirko > > With this sequence [1] I get the following trace [2]. Bisected it to > this patch. Note that second filter is rejected by the device driver > (that's the intention). I guess it is not properly removed from the > filter chain in the error path? > > Thanks > > [1] > # tc qdisc add dev swp3 clsact > # tc filter add dev swp3 ingress pref 1 matchall skip_sw \ > action mirred egress mirror dev swp7 > # tc filter add dev swp3 ingress pref 2 matchall skip_sw \ > action mirred egress mirror dev swp7 > RTNETLINK answers: File exists > We have an error talking to the kernel, -1 > # tc qdisc del dev swp3 clsact > > [2] > [ 70.545131] kasan: GPF could be caused by NULL-ptr deref or user memor= y access > [ 70.553394] general protection fault: 0000 [#1] PREEMPT SMP KASAN PTI > [ 70.560618] CPU: 2 PID: 2268 Comm: tc Not tainted 5.0.0-rc5-custom-021= 03-g415d39427317 #1304 > [ 70.570065] Hardware name: Mellanox Technologies Ltd. MSN2100-CB2FO/SA= 001017, BIOS 5.6.5 06/07/2016 > [ 70.580204] RIP: 0010:mall_reoffload+0x14a/0x760 > [ 70.585382] Code: c1 0f 85 ba 05 00 00 31 c0 4d 8d 6c 24 34 b9 06 00 0= 0 00 4c 89 ff f3 48 ab 4c 89 ea 48 b8 00 00 00 00 00 fc ff df 48 c1 ea 03 <= 0f> b6 14 02 4c 89 e8 83 > e0 07 83 c0 03 38 d0 7c 08 84 d2 0f 85 bd > [ 70.606382] RSP: 0018:ffff888231faefc0 EFLAGS: 00010207 > [ 70.612235] RAX: dffffc0000000000 RBX: 1ffff110463f5dfe RCX: 000000000= 0000000 > [ 70.620220] RDX: 0000000000000006 RSI: 1ffff110463f5e01 RDI: ffff88823= 1faf040 > [ 70.628206] RBP: ffff8881ef151a00 R08: 0000000000000000 R09: ffffed104= 63f5dfa > [ 70.636192] R10: ffffed10463f5dfa R11: 0000000000000003 R12: 000000000= 0000000 > [ 70.644177] R13: 0000000000000034 R14: 0000000000000000 R15: ffff88823= 1faf010 > [ 70.652163] FS: 00007f46b5bf0800(0000) GS:ffff888236c00000(0000) knlG= S:0000000000000000 > [ 70.661218] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > [ 70.667649] CR2: 0000000001d590a8 CR3: 0000000231c3c000 CR4: 000000000= 01006e0 > [ 70.675633] Call Trace: > [ 70.693046] tcf_block_playback_offloads+0x94/0x230 > [ 70.710617] __tcf_block_cb_unregister+0xf7/0x2d0 > [ 70.734143] mlxsw_sp_setup_tc+0x20f/0x660 > [ 70.738739] tcf_block_offload_unbind+0x22b/0x350 > [ 70.748898] __tcf_block_put+0x203/0x630 > [ 70.769700] tcf_block_put_ext+0x2f/0x40 > [ 70.774098] clsact_destroy+0x7a/0xb0 > [ 70.782604] qdisc_destroy+0x11a/0x5c0 > [ 70.786810] qdisc_put+0x5a/0x70 > [ 70.790435] notify_and_destroy+0x8e/0xa0 > [ 70.794933] qdisc_graft+0xbb7/0xef0 > [ 70.809009] tc_get_qdisc+0x518/0xa30 > [ 70.821530] rtnetlink_rcv_msg+0x397/0xa20 > [ 70.835510] netlink_rcv_skb+0x132/0x380 > [ 70.848419] netlink_unicast+0x4c0/0x690 > [ 70.866019] netlink_sendmsg+0x929/0xe10 > [ 70.882134] sock_sendmsg+0xc8/0x110 > [ 70.886144] ___sys_sendmsg+0x77a/0x8f0 > [ 70.924064] __sys_sendmsg+0xf7/0x250 > [ 70.955727] do_syscall_64+0x14d/0x610 Hi Ido, Thanks for reporting this. I looked at the code and problem seems to be matchall classifier specific. My implementation of unlocked cls API assumes that concurrent insertions are possible and checks for it when deleting "empty" tp. Since classifiers don't expose number of elements, the only way to test this is to do tp->walk() on them and assume that walk callback is called once per filter on every classifier. In your example new tp is created for second filter, filter insertion fails, number of elements on newly created tp is checked with tp->walk() before deleting it. However, matchall classifier always calls the tp->walk() callback once, even when it doesn't have a valid filter (in this case with NULL filter pointer). Possible ways to fix this: 1) Check for NULL filter pointer in tp->walk() callback and ignore it when counting filters on tp - will work but I don't like it because I don't think it is ever correct to call tp->walk() callback with NULL filter pointer. 2) Fix matchall to not call tp->walk() callback with NULL filter pointers - my preferred simple solution. 3) Extend tp API to have direct way to count filters by implementing tp->count - requires change to every classifier. Or maybe add it as optional API that only unlocked classifiers are required to implement and just delete rtnl lock dependent tp's without checking for concurrent insertions. What do you think? Regards, Vlad