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 58BB1D597B4 for ; Tue, 12 Nov 2024 23:06:23 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id A410F6B00B1; Tue, 12 Nov 2024 18:06:22 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 9C8236B00B2; Tue, 12 Nov 2024 18:06:22 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 842186B00B3; Tue, 12 Nov 2024 18:06:22 -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 608726B00B1 for ; Tue, 12 Nov 2024 18:06:22 -0500 (EST) Received: from smtpin25.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id E4E801C52A7 for ; Tue, 12 Nov 2024 23:06:21 +0000 (UTC) X-FDA: 82778976966.25.B70F95A Received: from mail-qv1-f49.google.com (mail-qv1-f49.google.com [209.85.219.49]) by imf07.hostedemail.com (Postfix) with ESMTP id B0A8340008 for ; Tue, 12 Nov 2024 23:05:20 +0000 (UTC) Authentication-Results: imf07.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=Ipybklrg; spf=pass (imf07.hostedemail.com: domain of amir73il@gmail.com designates 209.85.219.49 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=1731452546; 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=SIr16uc11UiqTl1Sx8j59V2P8BYfKtJ63uAHahcToIA=; b=hwxarvj9wwXr9GQa3jJ8QBLMJw4aZgKJ3uZlWcg2e5c/3uPzpIL5lhb3w6HC78RQZJk7aO efnTYmCCifNcco040RfBABZusDOVttcuTe6fGS+VhQgI9hhVW/r6WQIW81zrOzpT5tdaT1 fye0bXo0azQHRsRB32vHYZTF1UN7qfI= ARC-Authentication-Results: i=1; imf07.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=Ipybklrg; spf=pass (imf07.hostedemail.com: domain of amir73il@gmail.com designates 209.85.219.49 as permitted sender) smtp.mailfrom=amir73il@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1731452546; a=rsa-sha256; cv=none; b=OeeG+3i7YqxRMQbJqmteE7pQzO4c3rpPdMLHPwxuJMcAhyvJHtVH+/2eNXVsb02H4BUkwu Jtux2YN+emEPLKHjgr3//nsfMfrMoH3VGSkx/87hDFFHLcaufefhGn7KQ6NnVKr0FCf8w4 KIFxNe5kFcDZ1o6xyt4HCrnQToxYnys= Received: by mail-qv1-f49.google.com with SMTP id 6a1803df08f44-6cbe700dcc3so43431696d6.3 for ; Tue, 12 Nov 2024 15:06:19 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1731452779; x=1732057579; 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=SIr16uc11UiqTl1Sx8j59V2P8BYfKtJ63uAHahcToIA=; b=IpybklrgoTc4Bg61oP7sA+VMGWsEyHDBwHIMYvANgorLbCR80XMmFecCCpiFfpkTRX Pcwxyn12tQcYE2YFsJrThZrtsUkTs6CeX8UpxoCKPItES/2UqXJOEF15nMIkb3JfvAHh Fx4eWZcfdwI+JvK0A69ZkOd1pfOxqddZ6D95u59AiqP0LmSmvm4MBYkEMozOT12TME7S FRoMB8bqihlT+QaPa/MfQHpiOdXgl/dk1i1GAdw48o8BNP2vrGS37nUaUhErGcH1y2nY 9GRaGwnmnYDMb5TGulUU30bt28VUKDWvyhzH3TtGeSTx8wj9AqoNTrX0dre4dO8rry2p GCVQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1731452779; x=1732057579; 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=SIr16uc11UiqTl1Sx8j59V2P8BYfKtJ63uAHahcToIA=; b=uxvQmBKxhWlOPTk2o4OU83+ui9TYlJJ7kA3aIPrmLhH1AXaF4+tRuxi8rnutu244DQ JphIFWwgpGpLnvN5wHhuX+xe0ZwG6J7X69kJzIRwAuR0iGcqmCSpfAq6UyJ4lN+STDH/ UImlFS7zifILucGEWO6eo4DiZlMQ8bu06VnLRvOiZxxbVK0oOtao4Zwem3F7DVTTMLBM ZP3B1to1J5wv8cfcYXSa3hSNcBX/P9OAxY9KQLvpXR+DIpipnWWr6skJbSgy0LqtKWfg RebeDcgHIpVbgGRd1VxRe4YrrW5gKdGOpagzPdMYGYytfEPPXw6vv0MuvZohlkIp3FuS wx5Q== X-Forwarded-Encrypted: i=1; AJvYcCVkKt38Go7KDLqbldHYkt+ijWWSMuYDwnjdiKEN41yZUBVJkcuzR54qA8OCEL7i4K9epeVMGtYV0w==@kvack.org X-Gm-Message-State: AOJu0Yx02rC/q/tQR5L8NKVBNz3ily1NAAXgTmU6IMcenQ01dsbgJn6A 6iSEZlye6utlVfgXoBd2psxNbrZBbj3ve0DZnsalv9gjfxtFA1o7NZRyxcfOu17ncXPD5ntbjyq t4vlkVQGlNJnmgIcaqCQwKI2sj7s= X-Google-Smtp-Source: AGHT+IHxu77JKYYa/Yi0IgqgbzYETuy6BNeSMuPucl1B8xYxyyUZE8YdamGYeRduzkpm8Frlb0wJ61eZe0uRDQXpVlo= X-Received: by 2002:a05:6214:3287:b0:6d1:74d4:4ba2 with SMTP id 6a1803df08f44-6d39e15549cmr215580146d6.9.1731452779171; Tue, 12 Nov 2024 15:06:19 -0800 (PST) MIME-Version: 1.0 References: <141e2cc2dfac8b2f49c1c8d219dd7c20925b2cef.1731433903.git.josef@toxicpanda.com> In-Reply-To: From: Amir Goldstein Date: Wed, 13 Nov 2024 00:06:07 +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-Rspamd-Server: rspam06 X-Rspamd-Queue-Id: B0A8340008 X-Stat-Signature: e9ymcqb4soqx496nrytmboyjh55mhdhd X-Rspam-User: X-HE-Tag: 1731452720-279730 X-HE-Meta: U2FsdGVkX18A0QvS/pwlQqk0asxPxQelYKbO7jTRdwhloA1t3YBQKKswUuKQKB2lX7qr1zQnL2VQkR95g6GezhqKKheUZl2+3jSd+XoNscyYIpCGC82cuS5Y9NKLDSeqJ/F1BE27ImYHp6HQlMJA6f/DXfQnyy/YYStfYPwNgLIl9ROPoSCVf+KBRFsmjlBPZmlzXEBCbEmi3NSxpGBdhne9RCUaO0jZvj+zpyVtmOcAEWWZpmGgj2Sy6ciFeQQNAR6qx2t4GcBwNSKxXNoCAYeK6/cs/kAaxGk95gjI+pVEgJMh93o0xjj1oE3Vh3noUjbbozUEqUz2N2RHNxX1AmdACNKcQh0WnylU+QKotyn+7IhRigs/QEhNVEp2uIti9SXQ5/foR4P6mpDQjfuzt3cNjW7gzKrYsOorZhz0hAtNN6CoJwL8bRT6nr3oohL0qfDLJLAVf6D+1J9N+hzM4sMFJxoSLHCUWv/BbSQXxhKVH3Mg+hYbwzDQtmIKPwXQvq/3xv+ZbSGxeU9jx24ooqOkwbfMfl8C1U4alk6MJ9oaTEb3MEnU/TDsKh5eONji6lZW0nvgSiS5oUxBTmxgI4xDwQP7aP1Jx43blamZhUOMA84Lhg2RcfBIEkqetEXrNWBiy+t482cJxQ3KKNyPic/D4BQLzaUR0TJYB8vtLfMkRnwF0qm3zirFaIx9wjbxaaIqnyfzWWPXiTqKLbJVhdQPFgTlmaNk0SFRi/ucb/r73K4zRvT265eLAyWQ7h1XLUYMI5vR0GRyIGSGuAp7wTkYny/uYDKGh4bHcZyiN1/kaoE3yMFHdicdy0tCaeMy4PLxzerwLvYiLpHyUtVUccmIQg3C4MIJ0Z4oMNUT3O96iwZMOcJskUSayJNNlAcpFT6OQ4iPMomTv8PhzctajExrU9WRQ3rmT8FjpZjErIsVRgFdkc6TI6Kgu9tfhvyk2TM7EMbAc4ovU6xtzIZ EqXBkogE AWH0yB3qIQjoBW6LvyLdUBlzozTnYc6L6EBsGeWopdRpve+4Zs+fn0WYWyafucNwdf8nwSBRJy36w+ZnS+d3IXajNFs2949HXuHw6ZfHfj7Y3pAutjO6a/1yBx03GpQfNXcJmParLO0TH4Mk9ZM+j26jcq/CEzAXsn4ZXuf1be1i1j0bDgt01K8OYoXPXHghP+VOxllUmTu9Ome2cVybaJFNZtZpFpbSuHDEDYSHAXlBtlAar0kkr/tZDy+v6zbuRg9a2xh1sw6OKFm0N7XHzlsH19bLpo+pmd9S+aeLzlK+NVizFfigiHCn+cTLh4mgXiJlBDLcJVZRvdGjP3RsQC+jqHEPiRr+FxceaB2V5pDjvNPKtWkBBYYN5Qux5X+uinsNaw2yuDT5AplK9HzlbIMrbIuX3IB8DUfiXOpN9jvzH83eudFw0im/+yOlu2umMRIHs 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 Tue, Nov 12, 2024 at 9:12=E2=80=AFPM Linus Torvalds wrote: > > On Tue, 12 Nov 2024 at 09:56, Josef Bacik wrote: > > > > #ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS > > +static inline int fsnotify_pre_content(struct file *file) > > +{ > > + struct inode *inode =3D file_inode(file); > > + > > + /* > > + * Pre-content events are only reported for regular files and d= irs > > + * if there are any pre-content event watchers on this sb. > > + */ > > + if ((!S_ISDIR(inode->i_mode) && !S_ISREG(inode->i_mode)) || > > + !(inode->i_sb->s_iflags & SB_I_ALLOW_HSM) || > > + !fsnotify_sb_has_priority_watchers(inode->i_sb, > > + FSNOTIFY_PRIO_PRE_CONTEN= T)) > > + return 0; > > + > > + return fsnotify_file(file, FS_PRE_ACCESS); > > +} > > Yeah, no. > > None of this should check inode->i_sb->s_iflags at any point. > > The "is there a pre-content" thing should check one thing, and one > thing only: that "is this file watched" flag. > The whole indecipherable mess of inline functions that do random > things in needs to be cleaned up, not made even > more indecipherable. > > I'm NAKing this whole series until this is all sane and cleaned up, > and I don't want to see a new hacky version being sent out tomorrow > with just another layer of new hacks, with random new inline functions > that call other inline functions and have complex odd conditionals > that make no sense. > > Really. If the new hooks don't have that *SINGLE* bit test, they will > not get merged. > > And that *SINGLE* bit test had better not be hidden under multiple > layers of odd inline functions. > > You DO NOT get to use the same old broken complex function for the new > hooks that then mix these odd helpers. > > This whole "add another crazy inline function using another crazy > helper needs to STOP. Later on in the patch series you do > > +/* > + * fsnotify_truncate_perm - permission hook before file truncate > + */ > +static inline int fsnotify_truncate_perm(const struct path *path, > loff_t length) > +{ > + return fsnotify_pre_content(path, &length, 0); > +} > > or things like this: > > +static inline bool fsnotify_file_has_pre_content_watches(struct file *fi= le) > +{ > + if (!(file->f_mode & FMODE_NOTIFY_PERM)) > + return false; > + > + if (!(file_inode(file)->i_sb->s_iflags & SB_I_ALLOW_HSM)) > + return false; > + > + return fsnotify_file_object_watched(file, FSNOTIFY_PRE_CONTENT_EV= ENTS); > +} > > and no, NONE of that should be tested at runtime. > > I repeat: you should have *ONE* inline function that basically does > > static inline bool fsnotify_file_watched(struct file *file) > { > return file && unlikely(file->f_mode & FMODE_NOTIFY_PERM); > } > > and absolutely nothing else. If that file is set, the file has > notification events, and you go to an out-of-line slow case. You don't > inline the unlikely cases after that. > > And you make sure that you only set that special bit on files and > filesystems that support it. You most definitely don't check for > SB_I_ALLOW_HSM kind of flags at runtime in critical code. I understand your point. It makes sense. But it requires using another FMODE_HSM flag, because FMODE_NOTIFY_PERM covers also the legacy FS_ACCESS_PERM event, which has different semantics that I consider broken, but it is what it is. I am fine not optimizing out the legacy FS_ACCESS_PERM event and just making sure not to add new bad code, if that is what you prefer and I also am fine with using two FMODE_ flags if that is prefered. Thanks, Amir.