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=-1.1 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,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 A1288C43381 for ; Thu, 7 Mar 2019 14:51:15 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 4D7D920835 for ; Thu, 7 Mar 2019 14:51:15 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=Mellanox.com header.i=@Mellanox.com header.b="T7joljrh" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726286AbfCGOvO (ORCPT ); Thu, 7 Mar 2019 09:51:14 -0500 Received: from mail-eopbgr80071.outbound.protection.outlook.com ([40.107.8.71]:15782 "EHLO EUR04-VI1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1726127AbfCGOvN (ORCPT ); Thu, 7 Mar 2019 09:51:13 -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=Gd6g32zvVwN0p/CF2DN1JETrcFLoorDb1AiJ7hct9mo=; b=T7joljrhf+ro1eIncsJkuLpWWkMls0PbfkkBnh2GGSaer7sCNkeVxBPiKTzlu4HSHoKKNMwlj6pCu8oQihntnKYaHXx9PUK+qi/xXq2ltustoTMLTJIvufdY9I9Wv8QlT+ESL9c5dzMvC0ARBvrgNAjBpGfbmLU79VuUovkcjfM= Received: from HE1PR0502MB3641.eurprd05.prod.outlook.com (10.167.127.11) by HE1PR0502MB3097.eurprd05.prod.outlook.com (10.175.29.8) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.1665.18; Thu, 7 Mar 2019 14:51:07 +0000 Received: from HE1PR0502MB3641.eurprd05.prod.outlook.com ([fe80::b03d:8cd4:d259:f749]) by HE1PR0502MB3641.eurprd05.prod.outlook.com ([fe80::b03d:8cd4:d259:f749%6]) with mapi id 15.20.1686.018; Thu, 7 Mar 2019 14:51:07 +0000 From: Vlad Buslov To: Davide Caratti CC: Cong Wang , "David S. Miller" , Jamal Hadi Salim , Jiri Pirko , Paolo Abeni , Linux Kernel Network Developers Subject: Re: [PATCH net 03/16] net/sched: act_csum: validate the control action inside init() Thread-Topic: [PATCH net 03/16] net/sched: act_csum: validate the control action inside init() Thread-Index: AQHUzsOgqSP14JDKCkOHwDmyY+fspaX0cm4AgAKh3YCAAGUvAIAIxDSAgAAPJgA= Date: Thu, 7 Mar 2019 14:51:07 +0000 Message-ID: References: <81d683a7b6ea12e69cb9954b9bad84a9d2a2520f.camel@redhat.com> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-clientproxiedby: LO2P265CA0438.GBRP265.PROD.OUTLOOK.COM (2603:10a6:600:e::18) 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: 2ccf2867-e69b-440e-248b-08d6a30c5468 x-ms-office365-filtering-ht: Tenant x-microsoft-antispam: BCL:0;PCL:0;RULEID:(2390118)(7020095)(4652040)(8989299)(4534185)(4627221)(201703031133081)(201702281549075)(8990200)(5600127)(711020)(4605104)(4618075)(2017052603328)(7153060)(7193020);SRVR:HE1PR0502MB3097; x-ms-traffictypediagnostic: HE1PR0502MB3097: x-microsoft-exchange-diagnostics: =?iso-8859-1?Q?1;HE1PR0502MB3097;23:hJkZI1JOtJoAAXxYGpZ76AGwTt7yK7cWiKPxY?= =?iso-8859-1?Q?DNFOPYR6GTJ/geLpCLANRz4tloX6vEl8PXbhDHFG25OivM5riP2Wg6J091?= =?iso-8859-1?Q?Fa43RmZ4GH3fQMZIytxF8V5+OSbeHVxIE6L7IlvIu5m0lsjsUk1B45EGpF?= =?iso-8859-1?Q?VcmRxW56uHITEahkp242C1BkEBZkauqCgPWH1YZiOHtBFQt4P8OWVCOZ2C?= =?iso-8859-1?Q?90ACOF9seJOWQrCpNY/61BSLsDI96RKxsfpLQWDQjo65QCq9uC3JQVQn+e?= =?iso-8859-1?Q?LK8PQlTxU+sYbR7RcHu023ZbU5d/KBkF2scWR+UXyAh5I42qtmKy8enwYg?= =?iso-8859-1?Q?53Iz5QyOegigjmCXT2Y/NcjBuvM50RsncnoTBUejw1R9iThNAcxYzZYjLl?= =?iso-8859-1?Q?u3tQFwU9+yqPApPZX+aFZ7EPo82EoHRS9sXUASg0WSV3/WGXy4+gS9Q1Io?= =?iso-8859-1?Q?HtJQOQIc+REi6VXPP/agQscCrH7t6uOblLCbtk3A7x91H7GwYK+wHIPkip?= =?iso-8859-1?Q?3U+69JT5SDB7dnr2DtUHLXQJUEgS2IeiLunOK6+udiuvyg7rRZOWQ0s3zI?= =?iso-8859-1?Q?cx1HXy16Jp6IChF2rIytAkEswh0Xgu7pLqNXlC9ccj7Ila25L3uO4JHdSj?= =?iso-8859-1?Q?ZYB9Hd3pXG7uohzJXpoYUyZVz/J0euZ0UXs1WzCyk1ZCAIgXoZhzI6tPTy?= =?iso-8859-1?Q?a8Yw+EeVJ5WfecqtErolG7WioGUV3DSra65gCa/fTQ9Hvu5OvE1u0VfvVG?= =?iso-8859-1?Q?07M/5kIdVYoGSDGntLilizgWnMcSvphI9L9kMkv2F9JOlD4cQPpgS/aXCe?= =?iso-8859-1?Q?aRylMVgdNGXJcJLEI5Ly/KcQHIFyrNdLPRKZq6BvyNjic/YKFLm3xJideJ?= =?iso-8859-1?Q?hxSx3PDkjNpmaGV85+VbCUHBFEq0TUpwRD7QQyZE70YukWwEHNXU+dzD0/?= =?iso-8859-1?Q?spg1y6jhL7ozOkM2zC/OtgrUxjOoPinqCUN9gmk6FHmiQ8ErsegwiTFQN8?= =?iso-8859-1?Q?TSRcjIy1VMis3swnLODFQ39w9YSKZ/Kv+Vo7KdV+4JZws3ALb8va90qVv5?= =?iso-8859-1?Q?k+gj0u+4BRzVA1LRlT0w9d2dUiK7yhN29/OeWIron+z9qqRPlK6pQcjWLr?= =?iso-8859-1?Q?/hs1u8TEe4sCnHrF/FlkdMoygtGrzUBnmXD/xq+FTI4gnzUHUlkelCfox8?= =?iso-8859-1?Q?8uQ6n/7bg8LEuugvDVxwdPr9b/vmjUJzvVm2f0N/x+dvdS4xB1mH8/gXYp?= =?iso-8859-1?Q?nWTdNX7fwuwI+UbdaR23WoYx/0xbt7/uuyPeKvZHCFV2uxOVXx3EAz/I14?= =?iso-8859-1?Q?6ZchpKWZb8VC16VRQwUu5o6h7Q4gWBnExK6uxK/lngORn014Ly8nrn5QL3?= =?iso-8859-1?Q?asF3BZx/SrBSY07PT5eWCY5PUvBycIN?= x-microsoft-antispam-prvs: x-forefront-prvs: 096943F07A x-forefront-antispam-report: SFV:NSPM;SFS:(10009020)(376002)(366004)(136003)(396003)(39860400002)(346002)(199004)(189003)(51444003)(504964003)(76176011)(6116002)(316002)(476003)(6436002)(52116002)(6486002)(229853002)(99286004)(71200400001)(14454004)(71190400001)(25786009)(6512007)(6246003)(14444005)(256004)(93886005)(53936002)(2906002)(36756003)(68736007)(6916009)(8936002)(478600001)(26005)(15650500001)(66066001)(105586002)(86362001)(4326008)(2616005)(81166006)(305945005)(81156014)(54906003)(446003)(8676002)(486006)(11346002)(386003)(97736004)(7736002)(6506007)(53546011)(186003)(102836004)(5660300002)(3846002)(106356001);DIR:OUT;SFP:1101;SCL:1;SRVR:HE1PR0502MB3097;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: fF2tbWcj77yKUuoSnEOPXaieemxG4YLx9+kgPB0dnmXcsvQtmQ/WQyXx5DXB9E3Q65uxk6I8rK6XTY8TWB9ZYlAu0Mgu0YAgi9ti4g0Gx6QUbaDSXczUXct4odwLRZu48pbvEgrNWFXKp2zOcxP3x+jvO2cio6PnEQliMEZaTokjVVPEvim7ed9LNMtbRiZZURuWSmsgyd49cKcbAwd5Y+vy659OkzT4aIxZxNEUH/cteABUNUlX9SR8j7d1d5innHOLVMNG6+F4Y15EeVPIkfLvHc9oYcBQeSBL1X14WdOKR/SiD75NLjVK3PQ9qm1bc6DrAB1wrIcf3O9sHUqG52CzOSOS1Z+VsLb4k4p8Hwa4FdDNqbdU6phl38ukPHNHY6E9RE+EDDh61CFbEZ7IKPbGclSHbHUbCZdTbn8PIg8= 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: 2ccf2867-e69b-440e-248b-08d6a30c5468 X-MS-Exchange-CrossTenant-originalarrivaltime: 07 Mar 2019 14:51:07.4372 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: a652971c-7d2e-4d9b-a6a4-d149256f461b X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-Transport-CrossTenantHeadersStamped: HE1PR0502MB3097 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Thu 07 Mar 2019 at 15:56, Davide Caratti wrote: > On Fri, 2019-03-01 at 16:04 -0800, Cong Wang wrote: >> > On Fri, Mar 1, 2019 at 10:02 AM Davide Caratti w= rote: >> > >> > if I well understand the question, you are worried about >> > tcf_action_goto_chain_exec(), that can dereference 'oldchain' while we= are >> > overwriting the action. A call to tcf_chain_put_by_act(oldchain) would >> > decrease refcounts and eventually call kfree(oldchain). >> > >> > But this would result in a use-after-free only in case the chain has o= nly >> > refcount held by 1 action (the one we are overwriting), and 0 filters:= is >> > this a condition where packets can go through this action's data plane= ? >> >> Hmm? Isn't goto chain can be arbitrary? Packets can be routed >> from this action to any filter chain, so even if the target chain has 0 >> filter this action still has traffic as long as itself is not on the sam= e >> chain? > > hi, > > sorry for the delay: it took some time to verify experimentally if we > really need this or not. So, we want to ensure the control path doesn't d= o > > tcf_csum_init(..., ovr=3D1, ...) > tcf_chain_put_by_act(oldchain) > tcf_chain_destroy(oldchain, ...) > kfree(oldchain); > > while the traffic path of the action is doing > > res->goto_tp =3D rcu_dereference_bh(oldchain->filter_chain); > > I iterated this script many times on a KASAN kernel, > > # tc chain add dev crash0 chain 42 ingress protocol ip flower ip_proto ud= p action pass > # tc filter add dev crash0 ingress protocol ip flower ip_proto udp action= csum udp goto chain 42 index 66 > # tc chain del dev crash0 chain 42 ingress > (start netperf traffic) > # tc action replace action csum udp pass index 66 > > and reproduced twice the following splat: > > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > BUG: KASAN: null-ptr-deref in tcf_action_exec+0x181/0x200 > Read of size 8 at addr 0000000000000000 by task netperf/5777 > > CPU: 0 PID: 5777 Comm: netperf Not tainted 5.0.0-rc7.goto_chain_fix+ #55= 1 > Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011 > Call Trace: > > dump_stack+0xc7/0x15b > ? show_regs_print_info+0x5/0x5 > ? _raw_read_lock_irq+0x40/0x40 > ? tcf_action_exec+0x181/0x200 > ? tcf_action_exec+0x181/0x200 > kasan_report+0x176/0x192 > ? tcf_action_exec+0x181/0x200 > tcf_action_exec+0x181/0x200 > ? find_dump_kind+0x170/0x170 > ? rcu_irq_exit+0x153/0x210 > fl_classify+0x81a/0x830 > > so, I think that the answer to your question: > > On Wed, 2019-02-27 at 17:50 -0800, Cong Wang wrote: >> > > > + if (oldchain) >> > > > + tcf_chain_put_by_act(oldchain); >> > > >> > > Do we need to respect RCU grace period here? > > is a "yes, we do". > Now I'm trying something similar to what's done in tcf_bpf_init(), to > release the bpf program on 'replace' operations: > > 365 if (res =3D=3D ACT_P_CREATED) { > 366 tcf_idr_insert(tn, *act); > 367 } else { > 368 /* make sure the program being replaced is no longer = executing */ > 369 synchronize_rcu(); > 370 tcf_bpf_cfg_cleanup(&old); > 371 } > > do you think it's worth going in this direction? > thank you in advance! Hi Davide, Using synchronize_rcu() will impact rule update rate performance and I don't think we really need it. I don't see any reason why we can't just update chain to be rcu-friendly. Data path is already rcu_read protected, in fact it only needs chain to read rcu-pointer to tp list when jumping to chain. So it should be enough to do the following: 1) Update tcf_chain_destroy() to free chain after rcu grace period. 2) Convert tc_action->goto_chain to be a proper rcu pointer. (mark it with "__rcu", assign with rcu_assign_pointer(), read it with rcu_dereference{_bh}(), etc.) Am I missing anything? Regards, Vlad