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 6B78FD637C4 for ; Wed, 13 Nov 2024 21:22:41 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id CEDCD6B0085; Wed, 13 Nov 2024 16:22:40 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id C9D6C6B0089; Wed, 13 Nov 2024 16:22:40 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id B64C66B008C; Wed, 13 Nov 2024 16:22:40 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 9AFB66B0085 for ; Wed, 13 Nov 2024 16:22:40 -0500 (EST) Received: from smtpin16.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id 4C7EA1A0CA8 for ; Wed, 13 Nov 2024 21:22:40 +0000 (UTC) X-FDA: 82782345198.16.596A742 Received: from mail-ej1-f49.google.com (mail-ej1-f49.google.com [209.85.218.49]) by imf30.hostedemail.com (Postfix) with ESMTP id 62B9480005 for ; Wed, 13 Nov 2024 21:21:16 +0000 (UTC) Authentication-Results: imf30.hostedemail.com; dkim=pass header.d=linux-foundation.org header.s=google header.b=PnefEInd; dmarc=none; spf=pass (imf30.hostedemail.com: domain of torvalds@linuxfoundation.org designates 209.85.218.49 as permitted sender) smtp.mailfrom=torvalds@linuxfoundation.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1731532781; a=rsa-sha256; cv=none; b=yfDbH7HLEP18GC7Ty+T56iupe9wOgHQAa7I5W9sgHGX39smy/R23hZUUouOpVn7Y26fNG9 CffYy9wCsKGlgxKOi4HdfYxXQ2X/q33g2hRl3x7KK83aL8KQnIQzV78WZZyB+bYooFONJY MQkXDvv3ExEqrpjMj0WkpTNcET4+4N4= ARC-Authentication-Results: i=1; imf30.hostedemail.com; dkim=pass header.d=linux-foundation.org header.s=google header.b=PnefEInd; dmarc=none; spf=pass (imf30.hostedemail.com: domain of torvalds@linuxfoundation.org designates 209.85.218.49 as permitted sender) smtp.mailfrom=torvalds@linuxfoundation.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1731532781; 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: in-reply-to:in-reply-to:references:references:dkim-signature; bh=lgXRlfeexjjlrQuWCwCm2fkGA5K7DLXozqP8RwYmyvA=; b=oCMTLq6H1BS2pSm2n6tcmb7ylNvK8XEg13T65T/CRkIFgzIfEVpZ0i7dOGs+9DSu670lTn zyAS7K/eZIkMPEy7zMiag4bTygrov41mplmBGx1f/BsRnNFKw+tO264s5SUTlOviw7TIbP f6Ldp2Ib8wCKCtR5MTtIMvapqD21sgs= Received: by mail-ej1-f49.google.com with SMTP id a640c23a62f3a-a9f1d76dab1so2843866b.0 for ; Wed, 13 Nov 2024 13:22:37 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux-foundation.org; s=google; t=1731532956; x=1732137756; darn=kvack.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=lgXRlfeexjjlrQuWCwCm2fkGA5K7DLXozqP8RwYmyvA=; b=PnefEIndWj0DjrOr3Q+4hvxRnxuyLUmXE4QCeMqOETyV07itW/zSKBApjnWVidMWeZ FLcKqBf9QKSX7iZO9nfAsQ3Lxs7isq5prwyG9wPAcWzHDQsbLg1F5F4XZakeUIhhpVP8 kx/TIfn6TMm7VqVM8yl3u2X2n9C8PPJxAkuCQ= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1731532956; x=1732137756; h=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=lgXRlfeexjjlrQuWCwCm2fkGA5K7DLXozqP8RwYmyvA=; b=CHPZ7VNm9hqSgzKdvkKtpKJoRPl4gw+3l11ES/Wshfo4owPJwq9UOfn/KusG2caTvX dyniJa6GOG5SX7MoFdPq3T9EwJlhS9c7xxLRWwjtzj9U6MT755MFOvMs/s1LrmUs1rmS RmLqYaq4pbk3b9oeF2iAhCo3pXKjTFMam+SMMfb6kjNtltEb22LFY5okWqC1WJeROZhK CexRBmlUwFSwDcQrt56Ei2cFLUizhMPmLolZz7tw7v0dthF/YQCBfLEYopmVdWq+xoI2 GGr0VAOERiqcIc+Q6gSYOWoN3udNcHkVokgH0/snKOEzEIDulRsDAkI1FgtvR16AdAPF Zxhg== X-Forwarded-Encrypted: i=1; AJvYcCV0iL0TOLuwhT+tefJ6A6lfQM9gVPAtOIvowqeSMZK4YzKI+znfUyYf2SIXK27YZgBv6ctpqWeb+A==@kvack.org X-Gm-Message-State: AOJu0YwsnwFnmd2M9jTTXGqFNLx7wA2rFMYNipUIBuIc75tUkTYV5JDb TKjTjD7BGSh8FpzgKZtl4eKXERwyD8K9lbzdCr4s1SCDOTafSvXjlvIWsqkEHUfwIt/0brfy1Mw zWh0= X-Google-Smtp-Source: AGHT+IHtMmzNo+STFzSdBI61uvsGBOiR/oNfEEGqiz3CUIJAx+1eM3qVEp6BN4h6G8rNEhYHZ75oVQ== X-Received: by 2002:a17:907:31c2:b0:a9e:c696:8f78 with SMTP id a640c23a62f3a-aa1c57ef4a3mr806861866b.51.1731532956375; Wed, 13 Nov 2024 13:22:36 -0800 (PST) Received: from mail-ej1-f49.google.com (mail-ej1-f49.google.com. [209.85.218.49]) by smtp.gmail.com with ESMTPSA id a640c23a62f3a-a9ee0e2ec75sm924293566b.188.2024.11.13.13.22.34 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 13 Nov 2024 13:22:34 -0800 (PST) Received: by mail-ej1-f49.google.com with SMTP id a640c23a62f3a-a9f1d76dab1so2838866b.0 for ; Wed, 13 Nov 2024 13:22:34 -0800 (PST) X-Forwarded-Encrypted: i=1; AJvYcCWvNXJ4qvor1ONiiDfnOAcxJfsY92CmYJK+b5xdyRBQ0xGc3ZQZOG7161uSau4wj8Fs74sY6VFqtg==@kvack.org X-Received: by 2002:a17:907:2dac:b0:a9e:c341:c896 with SMTP id a640c23a62f3a-aa1b10a4500mr814204866b.32.1731532954269; Wed, 13 Nov 2024 13:22:34 -0800 (PST) MIME-Version: 1.0 References: <141e2cc2dfac8b2f49c1c8d219dd7c20925b2cef.1731433903.git.josef@toxicpanda.com> In-Reply-To: From: Linus Torvalds Date: Wed, 13 Nov 2024 13:22:17 -0800 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v7 05/18] fsnotify: introduce pre-content permission events To: Amir Goldstein Cc: Josef Bacik , kernel-team@fb.com, linux-fsdevel@vger.kernel.org, jack@suse.cz, brauner@kernel.org, linux-xfs@vger.kernel.org, linux-btrfs@vger.kernel.org, linux-mm@kvack.org, linux-ext4@vger.kernel.org Content-Type: text/plain; charset="UTF-8" X-Stat-Signature: s4b87jmnxtiknk459wchjhsfne73697a X-Rspamd-Queue-Id: 62B9480005 X-Rspamd-Server: rspam08 X-Rspam-User: X-HE-Tag: 1731532876-681385 X-HE-Meta: U2FsdGVkX1/50fczGMfJjF1LC1eVgvShkOookl70XKmAc33sbfVwRoZFjkvT0ExIP0KleEKUBHx6mOYkQBzcRQJCqDchN2FpnwfWCh272RyBoyLotQtb0HW6UvgVLj6GyGtlYJ/07vNC7724zCo9cyqSnm8cP1QFi3E5Sv2F6yZ2W1pAvZ5wPRnSZnCWK9B7emhVXy+LzqOx/RRBDiPmQX0QtsJbma+KndnLvFUUcmzf3KtaAc+6OShQ9c2OAeiOuBYiBeeemcO8hGiiP9pQ+xMpWscLKi6VXcHF7G4uCWQMZ40GIdEpakz5pAbVGJIcZwcw+pi9HLQ+Vo7BTd/9s09ukNkAoz7DNaUpt8LtfJYb1p87qlWJNsJA0G9fCbVYHyaUw27TbKOYxEQX2bzcJMtP5g+c+Vn5ohJAjbzgp6x35lPwLG0al1Tm1bQ5gqQSlekhdlcVz/0pQDRYA5l8Bw6/g4R+bxfVSaf9ok39H4F/iHhLN3BwSE6VQv7msEmWbryrqO6lAO20LWyBpJ2ARhN7rP838L1rmncGxxiWzhfX+O6CNdITaUX3eWqMxs0bL0g+w9THrUXl36tookrTtgBZSHq/PprVAr+CCKIfFHDZxCmO7g3g/tNzFWKqG1wDXOFwYovar50WXAMqGKZp6NgZ3DTZOGcGP7uXsRw0cjfEDzOoJMLMYKMhoUOQ9fTlIdwK0uUHlZzfSD7ySoCoy75pLeVlFdtWCkh/j/LLTq7W4tTCCFGOMQWRFGgHQF3/LPEJvRnvIpz3/T/YzsS6tkpHf/IuaQxWtn6qP5QwZs3tVGHRIljk7JwnER5OEQx8I+3LF8WRLnqyIlTvxDN/xsvMvOD00XhnEpyYvDEkeqB3WEvSg6Hq5Tvkj+ZcjmtQWEIOKaq2OG0oXpKz+SaNjJbjgIoSjuy0pfHnfNqMyfdT9DElc2cUz4NQsLg4hhiJXsYC17aTuonUf5RVASE TZsYpt1a 40n9hFN8XRbACAihv1kLAHk4TDSy3eWnCEkC7of1JIOI3KSs/MbRMu4UcOcv/5eNUvoWZhh0GKCQ5Nf/lSP1M2LDGGu4d9tqJyb3EU5gBsBYAJR/eVvoroEqpgWoB1KkDUaerQmZV9k88H4Pw6IjmQfZHuh5qcARgZIAI2xzAOvzFmVUBcy36q5L+j0HmVoZZXkePEwvZcsixsHh29AChSix7IkJWd0FNYeD5QtWlOx7rzGP2eOkc3/+Nob9VJke3EyXjCgmPi94VGgiQ3Tksp7xMBZzTVVS/bgmEzPmwJDghQ4YYyGynsd/rkfs4VVlyYZB2BBv4Dvv3OaWdFiA2ML9I9qkCQ/7rEVMwalSXGS2d/TqLaCxlWJplA5G1L3m8K/ejbXAktzmlmNEhb0Y4eVvF6v8ju2QmB/oUrS7h5DCM21Uf/nDmblGXd9pmbJ7iWmvYEUI7hp3xDjRavN5uIFBvyiBbE1NA4MrAsywNrTf/hMJC8PYiBvtYaA== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000039, 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 Wed, 13 Nov 2024 at 11:11, Amir Goldstein wrote: > > > > > This whole "add another crazy inline function using another crazy > > helper needs to STOP. Later on in the patch series you do > > > > The patch that I sent did add another convenience helper > fsnotify_path(), but as long as it is not hiding crazy tests, > and does not expand to huge inlined code, I don't see the problem. So I don't mind adding a new inline function for convenience. But I do mind the whole "multiple levels of inline functions" model, and the thing I _particularly_ hate is the "mask is usually constant so that the effect of the inline function is practically two different things" as exemplified by "fsnotify_file()" and friends. At that point, the inline function isn't a helper any more, it's a hindrance to understanding what the heck is going on. Basically, as an example: fsnotify_file() is actually two very different things depending on the "mask" argument, an that argument is *typically* a constant. In fact, in fsnotify_file_area_perm() is very much is a constant, but to make it extra non-obvious, it's a *hidden* constant, using __u32 fsnotify_mask = FS_ACCESS_PERM; to hide the fact that it's actually calling fsnotify_file() with that constant argument. And in fsnotify_open() it's not exactly a constant, but it's kind of one: when you actually look at fsnotify_file(), it has that "I do a different filtering event based on mask", and the two different constants fsnotify_open() uses are actually the same for that mask. In other words, that whole "mask" test part of fsnotify_file() /* Permission events require group prio >= FSNOTIFY_PRIO_CONTENT */ if (mask & ALL_FSNOTIFY_PERM_EVENTS && !fsnotify_sb_has_priority_watchers(path->dentry->d_sb, FSNOTIFY_PRIO_CONTENT)) return 0; mess is actually STATICALLY TRUE OR FALSE, but it's made out to be somehow an "arghumenty" to the function, and it's really obfuscated. That is the kind of "helper inline" that I don't want to see in the new paths. Making that conditional more complicated was part of what I objected to in one of the patches. > Those convenience helpers help me to maintain readability and code > reuse. ABSOLUTELY NOT. That "convenience helkper" does exactly the opposite. It explicitly and actively obfuscates when the actual fsnotify_sb_has_priority_watchers() filtering is done. That helper is evil. Just go and look at the actual uses, let's take fsnotify_file_area_perm() as an example. As mentioned, as an extra level of obfuscation, that horrid "helper" function tries to hide how "mask" is constant by doing __u32 fsnotify_mask = FS_ACCESS_PERM; and then never modifying it, and then doing return fsnotify_file(file, fsnotify_mask); but if you walk through the logic, you now see that ok, that means that the "mask" conditional fsnotify_file() is actually just FS_ACCESS_PERM & ALL_FSNOTIFY_PERM_EVENTS which is always true, so it means that fsnotify_file_area_perm() unconditionally does that fsnotify_sb_has_priority_watchers(..) filitering. And dammit, you shouldn't have to walk through that pointless "helper" variable, and that pointless "helper" inline function to see that. It shouldn't be the case that fsnotify_file() does two completely different things based on a constant argument. It would have literally been much clearer to just have two explicitly different versions of that function, *WITHOUT* some kind of pseudo-conditional that isn't actually a conditional, and just have fsnotify_file_area_perm() be very explicit about the fact that it uses the fsnotify_sb_has_priority_watchers() logic. IOW, that conditional only makes it harder to see what the actual rules are. For no good reason. Look, magically for some reason fsnotify_name() could do the same thing without this kind of silly obfuscation. It just unconditonally calls fsnotify_sb_has_watchers() to filter the events. No silly games with doing two entirely different things based on a random constant argument. So this is why I say that any new fsnotify events will be NAK'ed and not merged by me unless it's all obvious, and unless it all obviously DOES NOT USE these inline garbage "helper" functions. The new logic had better be very obviously *only* using the file->f_mode bits, and just calling out-of-line to do the work. If I have to walk through several layers of inline functions, and look at what magic arguments those inline functions get just to see what the hell they actually do, I'm not going to merge it. Because I'm really tired of actively obfuscated VFS hooks that use inline functions to hide what the hell they are doing and whether they are expensive or not. Your fsnotify_file_range() uses fsnotify_parent(), which is another of those "it does two different things" functions that either call fsnotify() on the dentry, or call __fsnotify_parent() on it if it's an inode, which means that it's another case of "what does this actually do" which is pointlessly hard to follow, since clearly for a truncate event it can't be a directory. And to make matters worse, fsnotify_truncate_perm() actually checks truncate events for directories and regular files, when truncates can't actually happen for anything but regular files in the first place. So your helper function does a nonsensical cray-cray test that shouldn't exist. That makes it another "this is not a helper function, this is just obfuscation". Linus