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 8F4CDD637D0 for ; Wed, 13 Nov 2024 22:35:32 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 226FD6B009D; Wed, 13 Nov 2024 17:35:32 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 1D6F96B009C; Wed, 13 Nov 2024 17:35:32 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 09E556B009D; Wed, 13 Nov 2024 17:35:32 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id DD4C46B009B for ; Wed, 13 Nov 2024 17:35:31 -0500 (EST) Received: from smtpin16.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id 9E637C0DED for ; Wed, 13 Nov 2024 22:35:31 +0000 (UTC) X-FDA: 82782528780.16.0CB9FF1 Received: from mail-ot1-f42.google.com (mail-ot1-f42.google.com [209.85.210.42]) by imf19.hostedemail.com (Postfix) with ESMTP id 6FAE11A0014 for ; Wed, 13 Nov 2024 22:34:35 +0000 (UTC) Authentication-Results: imf19.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=dpj49die; spf=pass (imf19.hostedemail.com: domain of amir73il@gmail.com designates 209.85.210.42 as permitted sender) smtp.mailfrom=amir73il@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=1731537186; 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=+KDLg9To/YkPL+lMewcxLGFQQ9zp/mgylVTsqbiOTEQ=; b=iCZzDXlfsflHmExAdudGeJDp/+APMzshlNRQminXxtX3GTjfRtvfra++EL0tdxV4DaolUH HP1c0NdeC4VAIX0QJzVARZkQTqO/ND53m5tuZICgS27ku/6iX3GnCpCXzxbTepRTi6sGKy PM9vaJyexXEmDHinlPEDdzO0sETKe6E= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1731537186; a=rsa-sha256; cv=none; b=vrWnkHBEQ6aeGtdUkaY4rxW9D7inkNTOqmfmA/9IMRfHi8csoumBod8IIlVARgeFo8o8kQ UVAMQKdK1p1iMvPg6FUc2eAth+FNs+ADSfnutmYVfJC1OTopFpCPd6l8j5kIhypi1i2Wqv Q6QIDon0DHt2bGfYa9IWXu2a5BzLtGc= ARC-Authentication-Results: i=1; imf19.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=dpj49die; spf=pass (imf19.hostedemail.com: domain of amir73il@gmail.com designates 209.85.210.42 as permitted sender) smtp.mailfrom=amir73il@gmail.com; dmarc=pass (policy=none) header.from=gmail.com Received: by mail-ot1-f42.google.com with SMTP id 46e09a7af769-7180ab89c58so3783015a34.1 for ; Wed, 13 Nov 2024 14:35:29 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1731537329; x=1732142129; 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=+KDLg9To/YkPL+lMewcxLGFQQ9zp/mgylVTsqbiOTEQ=; b=dpj49diev1J1H59mFHY+nvctj16zXv/tIH1xMrPCw70QFLe9IV7H0BzPCGm9LC3VPy dKTIdnWn9/L139ielX4fmQMHxIebdjtzfUFwRY4zKm31arMHaSz5Ye2yh29JrTyfBmzx +uPBjX1CftPsA9oq9uH6haxqAlYvcAKv5ZXLrVvnMx9a38DqBUDKjMnLni5aDvdv1nLO bPCR7jC0HvicMcqHcRHp4EF8QArDt6ovewi3ZdNMhUX0QAQW2G3ZgfuClwmDcZJdR91O zEm2j3jfYk43wsfPby2gTHN/amReH/gb8uP/dfXkFANdOPEzWsdxmqOH7mOoqF2HG4yd AddA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1731537329; x=1732142129; 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=+KDLg9To/YkPL+lMewcxLGFQQ9zp/mgylVTsqbiOTEQ=; b=Np+DaeGDlQnV3uoMV6ROXOkI46sO6aKXk5y6fzeioNVihcRlaCezgppRtxqC9Si9Fx 5tSWx9KaMHeoHl0LmWVSR00QlHxXTMsA8FcuDxV9BmFrjM+IwUqw6pMWtvjHYbUo3ZEk rph02NRMJ73j8Q5KLV4iIJRcSzhOwNVMBsGS9lEcMbhHGcimBm0vosyRFcbjcENh21+C VsjwtZk0O3Lny2trE3JTOsP3p3Azqojr00A54BjGR4X7S5aeA+2wchTQpXtcyJuHkDe2 jM9AQchmEou5PkgGipYa0oQBnKt8UuNgXLlUm7/06cLOmnpmmzZDa+t1dl2ib36PZKgK 8x7A== X-Forwarded-Encrypted: i=1; AJvYcCWs39I3YWD2qje+xfAEITyUNjP4K28woZ8HD7AVKUrj8q/DVTp1VNdtcowIUVBEmipsHkREONPCtA==@kvack.org X-Gm-Message-State: AOJu0YzBFVhUCOassnPJQO8P4/+3sAuezcH5RmnxopprMEZk4QQLygVa eBzeF+dVPap2a+WzazCjpp7m6+xXkn45O6EmotNwhhKi17idjnGNePXe8nE5dWFa4VRPLvyxJL1 vOF7MUITddc8JbZpfuBlSPkqBYqU= X-Google-Smtp-Source: AGHT+IEdZlQxt48o8J8iDRR0Lg99n+z9ualxe1wY20DVHLuL74dL16q0iW+CsMahVNFRcOyIZXBweMNj0cx54B5GAec= X-Received: by 2002:a05:6830:2117:b0:718:18d6:a447 with SMTP id 46e09a7af769-71a6020c4c3mr4694846a34.24.1731537328656; Wed, 13 Nov 2024 14:35:28 -0800 (PST) MIME-Version: 1.0 References: <141e2cc2dfac8b2f49c1c8d219dd7c20925b2cef.1731433903.git.josef@toxicpanda.com> In-Reply-To: From: Amir Goldstein Date: Wed, 13 Nov 2024 23:35:16 +0100 Message-ID: Subject: Re: [PATCH v7 05/18] fsnotify: introduce pre-content permission events To: Linus Torvalds 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" Content-Transfer-Encoding: quoted-printable X-Rspam-User: X-Rspamd-Server: rspam12 X-Rspamd-Queue-Id: 6FAE11A0014 X-Stat-Signature: ez1i1p6toofz8go5hu1qc4wmdk9achzd X-HE-Tag: 1731537275-828672 X-HE-Meta: U2FsdGVkX1/3EamAXxGNYVGJpqLf6FZQp1ib3tHV+iZU63+hyt56g/ElMybPA2ST6oSbLxwMSxsw8qnkBHS+cTFiYyGjTygYiFwwNCSUET/ub8GGx8WPGiSrP56B5jfo73rqx6ph+inmpYylVmSSYXruNFpXmJ9TwUX156NxxzO/mbiYpoQV9/H61QS7esfKe0Xe2Fh8RCK+FmZ6QOz6af6zMABT771QnmakHttMriWf0qK0P++RG4lUBzJZLMFJdFSUErTJtbXd3/PEXyDRZKHfSkRBJFMyzMA3ByTrq3P9xLA1lOpo4MABUbvScfLX8PgxCYaW8ofKBS/fBbrDwvfZS8+lb96Vh9oRB7PB/U48L3i9y/BTtpWm1QLhyAlke7ZJoHi1DbeJ932rgW+1pWw6ID3M9sNXU1qNfo65FezyCAiOVZ/vkThJcgzDAcbxJVLHy/24E1/cdWSWofieO/CqqlSLkMrVKsxRByaEvDPCRFL265V3rMCmFq/6faZsFUKgD8ChKFJ4M58bh8j6SmtPaQ5veZSkBh8VAKF42vxgIgx0FxH3qzinYC5vKHbUykdFD9JUkDJ5JQ5Onlb8gX/4pVmAoKfqp28F2yU8mACwnnEFnLDsA7Jg7azEBsLZdZQ5eVEiNUqRMSYqqekyCSgwfPYiaMZfAfetb6YAVn/pLf9mKSZjzF4c3KuiEOIOKWbaoew5kN8PYMSo7Crx5km9Yk1x36dLsAiNBCNX3XaL7S3NZu9qcwquOFYNevFDJKusKCKSIVivX405AzPNoXmLaZ2hPEqmmEnFNr437NKrsby85KXFLKqX+U24RAJ/CBCZP2dh6K5F25m82ZoKuQ5AR7T5cUTcq1J31KQvu9+AkwsvP9L8+yeedvih9iS0utE3EChNA9M3/UdiJhabeB1+ga/EvAc0P7ifUdlE/wizK20U2BoFva1O8La7wklOfL0IylXV9OGcHKdgKAB HQgAMWJp ZO+Mg+cxqIvs5MKHUMNdwdtzRGrX067R+44HL5opV/Zej4bxdKYHS0M9P5F0zaLln8Y352nr3XYQWw5kIHhz5rB6hdSSfsdRhpJhMX5quQDoLiKrjFkCMblUUrq8bfUyE1NuW4ndA0bZxIpTj7O53leFePFSPXxYeYNJOsM3Qieb7WxnQKUaVMpBkdo3OiujxigMpZEFu4bYaYiYJmFll3IKvjen7C1m48YoWY6DRPYUIeNjRveS8EoY+RJT8Sy33erHV4mHRoCPY9ff2e3V7sJ03di8c1jhraNUaAo7vosEMYhe7ZKjWQcD5NnXjwzRqI49ozPh+77EOZcBvbz8fovw+/DTihM+6XuukNduHY86FQ4jD5d7GsMfxomI8GViJG2EiGJ3m4FZGlxx6jU/2RUVceQ== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000099, 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, Nov 13, 2024 at 10:22=E2=80=AFPM Linus Torvalds wrote: > > 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 =3D FS_ACCESS_PERM; > > to hide the fact that it's actually calling fsnotify_file() with that > constant argument. Yeh, that specific "obfuscation" is a leftover from history. It is already gone in the patches that we sent. > > 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 >=3D FSNOTIFY_PRIO_CONTEN= T */ > 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. > Yeh. I see that problem absolutely. This is already gone in the patch that I send you today: - All the old hooks call fsnotify_file() that only checks FMODE_NONOTIFY and calls fsnotify_path() - The permission hooks now check FMODE_NONOTIFY_PERM and call fsnotify_path() > 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 =3D 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. > ok. that's going to be history soon. I will send this cleanup patch regardless of the pre-content series. > 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. Sure for new hooks with new check-on-open semantics that is going to be easy to do. The historic reason for the heavy inlining is trying to optimize out indirect calls when we do not have the luxury of using the check-on-open semantics. > > 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. Ha, right, that's a stupid copy&paste braino. Easy to fix. The simplest thing to do for the new hooks I think is to make fsnotify_file_range() extern and then you won't need to look beyond it, because it already comes after the unlikley FMODE_NONOTIFY_ bits check. Will work on the rest of the series following those guidelines. Let me know if the patch I sent you has taken a wrong direction. Thanks, Amir.