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 Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3F0C9C54791 for ; Wed, 13 Mar 2024 21:05:30 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id A2F7C8005D; Wed, 13 Mar 2024 17:05:29 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 9B819940010; Wed, 13 Mar 2024 17:05:29 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 832748005D; Wed, 13 Mar 2024 17:05:29 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id 6BBA4940010 for ; Wed, 13 Mar 2024 17:05:29 -0400 (EDT) Received: from smtpin19.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id 4095E160531 for ; Wed, 13 Mar 2024 21:05:29 +0000 (UTC) X-FDA: 81893246778.19.0A7D0B5 Received: from mail-wr1-f46.google.com (mail-wr1-f46.google.com [209.85.221.46]) by imf26.hostedemail.com (Postfix) with ESMTP id 5DD7F140011 for ; Wed, 13 Mar 2024 21:05:27 +0000 (UTC) Authentication-Results: imf26.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=O+0RCFde; spf=pass (imf26.hostedemail.com: domain of alexei.starovoitov@gmail.com designates 209.85.221.46 as permitted sender) smtp.mailfrom=alexei.starovoitov@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1710363927; a=rsa-sha256; cv=none; b=A1sPf4Up7R6L7/KwPqWJSAbCNIpIM/bts6zHOrQnJrVvIMU0UN1wEHQlLocaN3MtgvY3Ei tSU1lMW298GAbVrHeVS+4TekMFw7dXV1V9TYpW1mZHh4WILPr8LQUiZS+ZH94a+qmR3PM2 0wTMR3ik6hx0cXLsoo7YlQhJpTG363E= ARC-Authentication-Results: i=1; imf26.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=O+0RCFde; spf=pass (imf26.hostedemail.com: domain of alexei.starovoitov@gmail.com designates 209.85.221.46 as permitted sender) smtp.mailfrom=alexei.starovoitov@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1710363927; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=yZzVp1B0o1fPUeYOWrqe1Q57q16dmLKhwAmLef5VA14=; b=XGD/UWh3PoZNH6D6MVNH0rp3C9OV5DqxmB0py0khbIicxMPDOYB1lewthSrcne7L+CJAfr Ck76xcAD1h1A7VCwr5Vg4jHRr0bSNnOE9cNeFDIAvxr0bxf2CBdO6EWAgkcHvu59g4AKAF VKhzZ1b0XL3zfCq9SDgL7bMp7xjqRGg= Received: by mail-wr1-f46.google.com with SMTP id ffacd0b85a97d-33e94c12f33so171727f8f.3 for ; Wed, 13 Mar 2024 14:05:27 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1710363926; x=1710968726; darn=kvack.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=yZzVp1B0o1fPUeYOWrqe1Q57q16dmLKhwAmLef5VA14=; b=O+0RCFdew63fg7jLTZ6pkr4vk7tuLW8jqWunc05fPA7jkkuFicuq3zhiANFTPEL8ry alznYvf6ASmDUZ1+nez2M1ovwD8yly7u7cobWm2gqFD75AH3woGW1m1LuPo8i9s0qKqs wcyQABcFCEiJvjopwnSryL10sDHmhd5U4P4V2c13cMuE8tPY3dgrncPFJIYDB0A0qHPo SBZ1X4jE5YTvRD1EVgQ3mfOLFBuASFLTyDA19Zd09G5BKysj9tSXLc27Ovf0PbvRf0BC E1NuqLKiSUHlRhhJq6PSUetMVHxvlSDu8zeuFPKVtvRtlwSgOx9nLihjxBNCrXaXGVyF chqw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1710363926; x=1710968726; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=yZzVp1B0o1fPUeYOWrqe1Q57q16dmLKhwAmLef5VA14=; b=gNBV4QSmMB9NC5vjFZlxAk51QiOV3ig9qpfvWN97ZzjxjDV+RJ1jzTD7FmfNohar3O 2pdIFnYeta5UMyKFIlwzTuGNpKFckw9fyPN7o9FW9xzCuPbpZ+OOEut/DsgE+4EBY59e 8VNR99EskZTe7pwWp+FDvzB1SbWo1V4eQFQiPtBwVOgCdLiK7JW1e9pwFbShheE5BIlw nk0vRKy6xSKynbtj+pXP8belZgsq8E94GvXurnJkGf1mG+gRp5OdfipzwCEvtyWDxve1 VNSFi7tsPq26VEJrowbE7lFYdMHD6ewealO1OFsFTv5rdJKU5L3xZ7ieXwaT0AoU6jCR 5WuA== X-Forwarded-Encrypted: i=1; AJvYcCUmPtMWMVjpe+sFoO2dfucx1Jm4gUgIb9+ZxDm/SJ8TiYnXp96UsNklgswvd2fSzAK+Z7FZK/YIk0z60lSM5niMcg8= X-Gm-Message-State: AOJu0YzIAnZduBaKNvcStcZ9GlmrBhi7votHumFESGM32GHrphvVGaG/ 4F0k9AmvChDjuAYgd1itJ9lLhfmRf+FM5tLoAydyIZ8dqAbNaVd8bmJYg7m5ryrIHJB33Z+ik7A K4Uu9bw81Zo/GYMDkSYIpvL6Cw4U= X-Google-Smtp-Source: AGHT+IFDFscIvQkKhWL6uTHrhCEwdvqJ5pjqZbod6Uv4pkjnS2CrY1wfRH/QplSvK4c2Ab2/TnZ4YDaEDqej2LD7zf8= X-Received: by 2002:adf:ffcc:0:b0:33d:2775:1e63 with SMTP id x12-20020adfffcc000000b0033d27751e63mr2732491wrs.41.1710363925428; Wed, 13 Mar 2024 14:05:25 -0700 (PDT) MIME-Version: 1.0 References: <20240306-flach-tragbar-b2b3c531bf0d@brauner> <20240306-sandgrube-flora-a61409c2f10c@brauner> <20240307-phosphor-entnahmen-8ef28b782abf@brauner> <20240308-kleben-eindecken-73c993fb3ebd@brauner> <20240311-geglaubt-kursverfall-500a27578cca@brauner> In-Reply-To: <20240311-geglaubt-kursverfall-500a27578cca@brauner> From: Alexei Starovoitov Date: Wed, 13 Mar 2024 14:05:13 -0700 Message-ID: Subject: Re: [PATCH v2 bpf-next 0/9] add new acquire/release BPF kfuncs To: Christian Brauner Cc: Matt Bobrowski , bpf , Alexei Starovoitov , Andrii Nakryiko , KP Singh , Jann Horn , Jiri Olsa , Daniel Borkmann , Linus Torvalds , Linux-Fsdevel , Andrew Morton , linux-mm , LSM List Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Server: rspam08 X-Rspamd-Queue-Id: 5DD7F140011 X-Stat-Signature: 49n4jrc6swg7qjfa7m9jbe7c1ggfyb45 X-Rspam-User: X-HE-Tag: 1710363927-308223 X-HE-Meta: U2FsdGVkX18H/CyPiEb8o18O7Hh9s3JPMWupK9nsiF+ZCc8yyNn9N9C3Q/iK9o42KxlHHrFML2FlvQEOHmWv2ia2MSOiBF9MinrNTKVgQLtK3oNFWxypY8cb4YRxrUHhVATq6s6tWbdy7yMZgzlysjTyXwOiX2Wg8rQBEWqBoNwwSKAR3VcBkxjqFLPdjtwZ13g7JbikvFiCpXTvFF/0/OgCkLuusNaM0i7tQpOj62TZAbZcYYGe0dQ8el55enxv176Vb5WQn4kF+op6C9LwFQ6DRFmGFtQqKZmmGflUxchsL5gXCae1PXnEhdo4kYaNz/J3fTLNpKrUKtdACvnLK7ClpMjdbamNuV8lR8lcnhtbH+SQQjibQNTchEGxBmP5n/hhu+MbjoUq3TLttTdyBIzfQbhsFS7p3hAIB/mzlW6Lafd5alHEzF/CAava4lvB2oPHg0cS937PcldBasI5J2HMuJpMNVvzg7FH5BhQvzll7RFV+iWn3HzRzWLjJhbp6ICcrytsnVYylCpfgVqShazgmmh7RmJViaNYd9W1YE8kFKtvH7fGU++Y1t1PY4tqcIBGTXkP5NjQ5CyzN+iOpukw6ubo0GzsJwyQoSb0q/Sbsl1Gf7JcAEh5hOtiQYKAKkKn8eBrW1CK8KZg4PNiTtqgmzg9QfEntFRXl3JDwLSPwg/4fRPILxAaoiLF5CiFN8hbgS/+Nxfd2geWmb48O94ZaFgjDAlnbPCS3hsUo0yAJSY7dUnjqov+7YjfDVDu/425e3Vv9VwflNDJU0xIkkkQQQMPXwyiThi7+sPrNK2UyY0FIxPpEQ687IU/FfL3mNvxGtDYw4vphRUkSfcYkQIxyGHJJ6xNIjO1cYmn2bM2mESHXU2H28B1BtVVbUQUJkpmAFsJlSkA6Zx5E5hhSVYCEuXEZB7mN+Co33UBgRSYMBiILQe5jtpypUV6ZWKndquBENOlt91Pj0RLDGs e/nS+xCI Cn295gxtQmE2UqU+LszTr1bRdDhXHg7yXa0QshS2srlaA0HP4qYR6UQ9pfvTsdqsfZVA5BZMyun+Ot9rfhKvJ62quGpnjfxjrpSHiEkh+ldQAJTgglXxnyMr1IgSlIyifGA2+ljmUWgDApIOPtN8j0aE0IMmeSiAtUYJR08xlRcnGQUyviqnqnSVHJ3SlnKMUh1d/+kwvB19+mI0oRtCnxhbmQgn1QLN6+ZgMQ+2DT1MUu3O9UP0QBc/mSND75MkXxAfmtLTtFHMVJPx7Uh5hne7hcTSTeyzfufmG667Px4cRJSo+6Hu3QPY1QvgVtx+b36p41AM2hukRGprNrIAw+/MUPjC7YSitG7GE7S0XOo+sTe9jRuzjYF2NLKgBQcDIHs+mPDeJXkBgy2uxHljlur/lXQ== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On Mon, Mar 11, 2024 at 5:01=E2=80=AFAM Christian Brauner wrote: > > > > > One can argue that get_mm_exe_file() is not exported, > > > > but it's nothing but rcu_lock-wrap plus get_file_rcu() > > > > which is EXPORT_SYMBOL. > > > > > > Oh, good spot. That's an accident. get_file_rcu() definitely shouldn'= t > > > be exported. So that'll be removed asap. > > > > So, just to make a point that > > "Included in that set are functions that aren't currently even > > exported to modules" > > you want to un-export get_file_rcu() ? > > No. The reason it was exported was because of the drm subsystem and we > already quite disliked that. But it turned out that's not needed so in > commit 61d4fb0b349e ("file, i915: fix file reference for > mmap_singleton()") they were moved away from this helper. Arguably that commit 61d4fb0b349e should have had Fixes: 0ede61d8589c ("file: convert to SLAB_TYPESAFE_BY_RCU") i915 was buggy before you touched it and safe_by_rcu exposed the bug. I can see why you guys looked at it, saw issues, and decided to look away. Though your guess in commit 61d4fb0b349e " Now, there might be delays until file->f_op->release::singleton_release() is called and i915->gem.mmap_singleton is set to NULL. " feels unlikely. I suspect release() delay cannot be that long to cause rcu stall. In the log prior to the splat there are just two mmap related calls from selftests in i915_gem_mman_live_selftests(): i915: Running i915_gem_mman_live_selftests/igt_mmap_offset_exhaustion i915: Running i915_gem_mman_live_selftests/igt_mmap 1st mmap test passed, but 2nd failed. So it looks like it's not a race, but an issue with cleanup in that driver. And instead of getting to the bottom of the issue you've decided to paper over with get_file_active(). I agree with that trade-off. But the bug in i915 is still there and it's probably an UAF. get_file_active() is probably operating on a broken 'struct file' that got to zero, but somehow it still around or it's just a garbage memory and file->f_count just happened to be zero. My point is that it's not ok to have such double standards. On one side you're arguing that we shouldn't introduce kfunc: +__bpf_kfunc struct file *bpf_get_task_exe_file(struct task_struct *task) +{ + return get_task_exe_file(task); +} that cleanly takes ref cnt on task->mm->exe_file and _not_ using lower level get_file/get_file_rcu/get_file_active api-s directly which are certainly problematic to expose anywhere, since safe_by_rcu protocol is delicate. But on the other side there is buggy i915 that does questionable dance with get_file_active(). It's EXPORT_SYMBOL_GPL as well and out of tree driver can ruin safe_by_rcu file properties with hard to debug consequences. > There is absolutely no way that any userspace will > get access to such low-level helpers. They have zero business to be > involved in the lifetimes of objects on this level just as no module has. correct, and kfuncs do not give bpf prog to do direct get_file*() access because we saw how tricky safe_by_rcu is. Hence kfuncs acquire file via get_task_exe_file or get_mm_exe_file and release via fput. That's the same pattern that security/tomoyo/util.c is doing: exe_file =3D get_mm_exe_file(mm); if (!exe_file) return NULL; cp =3D tomoyo_realpath_from_path(&exe_file->f_path); fput(exe_file); in bpf_lsm case it will be: exe_file =3D bpf_get_mm_exe_file(mm); if (!exe_file) // the verifier will enforce that bpf prog has this NULL check here // because we annotate kfunc as: BTF_ID_FLAGS(func, bpf_get_mm_exe_file, KF_ACQUIRE | KF_TRUSTED_ARGS | KF_RET_NULL) bpf_path_d_path(&exe_file->f_path, ...); bpf_put_file(exe_file); // and the verifier will enforce that bpf_put_file() is called too. // and there is no path out of this bpf program that can take file refcnt // without releasing. So really these kfuncs are a nop from vfs pov. If there is a bug in the verifier we will debug it and we will fix it. You keep saying that bpf_d_path() is a mess. Right. It is a mess now and we're fixing it. When it was introduced 4 years ago it was safe at that time. The unrelated verifier "smartness" made it possible to use it in UAF. We found the issue now and we're fixing it. Over these years we didn't ask vfs folks to help fix such bugs, and not asking for help now. You're being cc-ed on the patches to be aware on how we plan to fix this bpf_d_path() mess. If you have a viable alternative please suggest. As it stands the new kfuncs are clean and safe way to solve this mess.