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=DKIMWL_WL_HIGH,DKIMWL_WL_MED, 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 9D246C43381 for ; Sat, 2 Mar 2019 20:22:04 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 5639D20836 for ; Sat, 2 Mar 2019 20:22:04 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=fb.com header.i=@fb.com header.b="UozX7UZs"; dkim=pass (1024-bit key) header.d=fb.onmicrosoft.com header.i=@fb.onmicrosoft.com header.b="II7Sr6/F" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726577AbfCBUWD (ORCPT ); Sat, 2 Mar 2019 15:22:03 -0500 Received: from mx0a-00082601.pphosted.com ([67.231.145.42]:52788 "EHLO mx0a-00082601.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726295AbfCBUWD (ORCPT ); Sat, 2 Mar 2019 15:22:03 -0500 Received: from pps.filterd (m0044008.ppops.net [127.0.0.1]) by mx0a-00082601.pphosted.com (8.16.0.27/8.16.0.27) with SMTP id x22KJvB9007714; Sat, 2 Mar 2019 12:21:39 -0800 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=fb.com; h=from : to : cc : subject : date : message-id : references : in-reply-to : content-type : content-id : content-transfer-encoding : mime-version; s=facebook; bh=PLzQIWBw65JHCfWLvbtxmml7FUdck459wbHLuEqypQ0=; b=UozX7UZsLOB7E3MN2noeN7fiMz8NLMBv1w4lEjfDc8dvkQubi1c2UMR+eyxeRhEV3tuf LoZl1sAuf0zmRxV0gK7ni1huokoGSLJ9KZPxi7/hHXliSuWFHoLHV/R2u1b9ZeTLe9hv wXebbfXfUy6neECc2n46yfonVWjAMeGPslk= Received: from mail.thefacebook.com ([199.201.64.23]) by mx0a-00082601.pphosted.com with ESMTP id 2qyw9kreyc-2 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-SHA384 bits=256 verify=NOT); Sat, 02 Mar 2019 12:21:39 -0800 Received: from prn-hub06.TheFacebook.com (2620:10d:c081:35::130) by prn-hub01.TheFacebook.com (2620:10d:c081:35::125) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384) id 15.1.1713.5; Sat, 2 Mar 2019 12:21:38 -0800 Received: from NAM05-DM3-obe.outbound.protection.outlook.com (192.168.54.28) by o365-in.thefacebook.com (192.168.16.30) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384) id 15.1.1713.5 via Frontend Transport; Sat, 2 Mar 2019 12:21:38 -0800 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=fb.onmicrosoft.com; s=selector1-fb-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=PLzQIWBw65JHCfWLvbtxmml7FUdck459wbHLuEqypQ0=; b=II7Sr6/Fx+yzASVIvhMkhzLoikGEjxCtxc8n8RfXstvUZ785OEy81OS1PvLnIoFAE5yYlCIS0uaefhRqwimHfBQoGeyj6HmZWzQyIQ6bi9U5McHTdMJwW1SrZa62NOwbOyDXYzch9LdiZLAm9CNIx5MAILaYSF9Ebyx8Z/ss3ZU= Received: from MWHPR15MB1790.namprd15.prod.outlook.com (10.174.255.19) by MWHPR15MB1744.namprd15.prod.outlook.com (10.174.255.9) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.1665.15; Sat, 2 Mar 2019 20:21:18 +0000 Received: from MWHPR15MB1790.namprd15.prod.outlook.com ([fe80::28ff:27f2:89b9:efbe]) by MWHPR15MB1790.namprd15.prod.outlook.com ([fe80::28ff:27f2:89b9:efbe%4]) with mapi id 15.20.1665.019; Sat, 2 Mar 2019 20:21:18 +0000 From: Martin Lau To: Alexei Starovoitov CC: "netdev@vger.kernel.org" , Alexei Starovoitov , Daniel Borkmann , Kernel Team , Lorenz Bauer Subject: Re: [PATCH v3 bpf-next 1/2] bpf: Fix bpf_tcp_sock and bpf_sk_fullsock issue related to bpf_sk_release Thread-Topic: [PATCH v3 bpf-next 1/2] bpf: Fix bpf_tcp_sock and bpf_sk_fullsock issue related to bpf_sk_release Thread-Index: AQHU0RJtENJEArRHuEGXz2lOt2GPQqX4ojaAgAAmm4A= Date: Sat, 2 Mar 2019 20:21:18 +0000 Message-ID: <20190302202113.qo42ftjoydp2efgy@kafai-mbp.dhcp.thefacebook.com> References: <20190302161009.2478578-1-kafai@fb.com> <20190302161010.2478707-1-kafai@fb.com> <20190302180301.liwjbsz7qlsg33ov@ast-mbp.dhcp.thefacebook.com> In-Reply-To: <20190302180301.liwjbsz7qlsg33ov@ast-mbp.dhcp.thefacebook.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-clientproxiedby: BN6PR1101CA0002.namprd11.prod.outlook.com (2603:10b6:405:4a::12) To MWHPR15MB1790.namprd15.prod.outlook.com (2603:10b6:301:4e::19) x-ms-exchange-messagesentrepresentingtype: 1 x-originating-ip: [2620:10d:c091:180::d5bc] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: d924dcb4-8300-4ee3-a69d-08d69f4ca0a8 x-microsoft-antispam: BCL:0;PCL:0;RULEID:(2390118)(7020095)(4652040)(8989299)(4534185)(4627221)(201703031133081)(201702281549075)(8990200)(5600127)(711020)(4605104)(2017052603328)(7153060)(7193020);SRVR:MWHPR15MB1744; x-ms-traffictypediagnostic: MWHPR15MB1744: x-microsoft-exchange-diagnostics: 1;MWHPR15MB1744;20:7rWQHXL0w58bpS4sASbvDKhzgaJDIOCC5oy/ybGcD5j9zdWVptv3L7t4cDkM0rI3GBFr9BOSiJJy75Wyrq9D6IslW5ufIywS0v0+dExGBRM/drnKtZ25LNlaAxZxESat51YlIg3E97QCidBX8+j7i7lLJS3Y3xgElhX6pSXtYhE= x-microsoft-antispam-prvs: x-forefront-prvs: 09645BAC66 x-forefront-antispam-report: SFV:NSPM;SFS:(10019020)(136003)(346002)(376002)(366004)(396003)(39860400002)(189003)(199004)(105586002)(8936002)(316002)(14454004)(81156014)(6916009)(1076003)(68736007)(6506007)(386003)(76176011)(106356001)(81166006)(446003)(476003)(486006)(8676002)(46003)(6246003)(52116002)(2906002)(102836004)(11346002)(256004)(229853002)(305945005)(6486002)(25786009)(97736004)(478600001)(9686003)(4326008)(53936002)(54906003)(99286004)(5660300002)(6116002)(186003)(7736002)(71190400001)(71200400001)(6512007)(86362001)(6436002);DIR:OUT;SFP:1102;SCL:1;SRVR:MWHPR15MB1744;H:MWHPR15MB1790.namprd15.prod.outlook.com;FPR:;SPF:None;LANG:en;PTR:InfoNoRecords;MX:1;A:1; received-spf: None (protection.outlook.com: fb.com does not designate permitted sender hosts) x-ms-exchange-senderadcheck: 1 x-microsoft-antispam-message-info: 5342Bix4w+z68cSPGfn276KIjgjoRMZGyEBaiUmobFTex9iT3YVNPyYG+HElzy0mVjrZxs12hqAaVVmmhNtiALiNNaJzIWNWIBAfgPo2MATGzTFjr4Frjm+EKa5/tQuEb/LD3O9FZAyGkMWXbrPanXqwezJDqi0zSAb60UiNZWAHC5kRoKV2ICUf6NWIZxssT8TFSvdIGnymCBgDtw1mFeeeNYKgJTjgzwl9bULAMEpSUGDyJLuKijQLotfZxfK51lDHtzlYFrQNsmm6VCSxS+ObCZrjEJV3bEk1AVORAwR89OZpdRbW9nTdmhkkanSp4G1PvRbVAoJ6sRgtoyeXU6xXZNvMphWfBTO2EtQfVoFWujjUW8x/e/A2Fqr9LnjnisqHt7uWsk2ozTIKbpOq3yVi4Y+TmhBCokZwAP1sTFY= Content-Type: text/plain; charset="us-ascii" Content-ID: <981BA12BFADB8A47A20756A50658F027@namprd15.prod.outlook.com> Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-MS-Exchange-CrossTenant-Network-Message-Id: d924dcb4-8300-4ee3-a69d-08d69f4ca0a8 X-MS-Exchange-CrossTenant-originalarrivaltime: 02 Mar 2019 20:21:18.7066 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2 X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR15MB1744 X-OriginatorOrg: fb.com X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2019-03-02_12:,, signatures=0 X-Proofpoint-Spam-Reason: safe X-FB-Internal: Safe Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Sat, Mar 02, 2019 at 10:03:03AM -0800, Alexei Starovoitov wrote: > On Sat, Mar 02, 2019 at 08:10:10AM -0800, Martin KaFai Lau wrote: > > Lorenz Bauer [thanks!] reported that a ptr returned by bpf_tcp_sock(sk) > > can still be accessed after bpf_sk_release(sk). > > Both bpf_tcp_sock() and bpf_sk_fullsock() have the same issue. > > This patch addresses them together. > >=20 > > A simple reproducer looks like this: > >=20 > > sk =3D bpf_sk_lookup_tcp(); > > /* if (!sk) ... */ > > tp =3D bpf_tcp_sock(sk); > > /* if (!tp) ... */ > > bpf_sk_release(sk); > > snd_cwnd =3D tp->snd_cwnd; /* oops! The verifier does not complain. */ > >=20 > > The problem is the verifier did not scrub the register's states of > > the tcp_sock ptr (tp) after bpf_sk_release(sk). > >=20 > > [ Note that when calling bpf_tcp_sock(sk), the sk is not always > > refcount-acquired. e.g. bpf_tcp_sock(skb->sk). The verifier works > > fine for this case. ] > >=20 > > Currently, the verifier does not track if a helper's return ptr (in REG= _0) > > is "carry"-ing one of its argument's refcount status. To carry this inf= o, > > the reg1->id needs to be stored in reg0. The reg0->id has already > > been used for NULL checking purpose. Hence, a new "refcount_id" > > is needed in "struct bpf_reg_state". > >=20 > > With refcount_id, when bpf_sk_release(sk) is called, the verifier can s= crub > > all reg states which has a refcount_id match. It is done with the chan= ges > > in release_reg_references(). > >=20 > > When acquiring and releasing a refcount, the reg->id is still used. > > Hence, we cannot do "bpf_sk_release(tp)" in the above reproducer > > example. >=20 > I think the choice of returning listener full sock from req sock > in sk_to_full_sk() was a wrong one. > It seems better to make semantics of bpf_tcp_sock() and bpf_sk_fullsock()= as=20 > always type cast or null. > And have a separate helper for req socket that returns inet_reqsk(sk)->rs= k_listener. >=20 > Then it will be ok to call bpf_sk_release(tp) when tp came from bpf_sk_lo= okup_tcp. > The verifier will know that it's the case because its ID will be in acqui= red_refs. >=20 > The additional refcount_id won't be necessary. > bpf_sk_fullsock() and bpf_tcp_sock() will not call sk_to_full_sk > and the verifier will be copying reg1->id into reg0->id. >=20 > In release_reference() the verifier will do > if (regs[i].id =3D=3D id) > mark_reg_unknown(env, regs, i); > for all socket types. >=20 > release_reference_state() will stay as-is. >=20 > imo such logic will be easier to follow. >=20 > This implicit sk_to_full_sk() makes the whole thing much harder for the v= erifier > and for the bpf program writers. >=20 > The new bpf_get_listener_sock(sk) doesn't have to copy ID from reg1 to re= g0 > since req socket will not be returned from bpf_sk_lookup_tcp and its ID > will not be stored in acuired_refs. >=20 > Does it make sense ? I like this idea. Many thanks for thinking it through! Allowing bpf_sk_release(tp), no need to call bpf_sk_release() on ptr returned from bpf_get_listener_sock(sk) and keep one reg->id. I think it should work. I will rework the patches.