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 DDD99D6A220 for ; Thu, 14 Nov 2024 17:22:40 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 59C496B0098; Thu, 14 Nov 2024 12:22:40 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 524DA6B0099; Thu, 14 Nov 2024 12:22:40 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 382D86B009D; Thu, 14 Nov 2024 12:22:40 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id 1634C6B0098 for ; Thu, 14 Nov 2024 12:22:40 -0500 (EST) Received: from smtpin03.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id C710C161250 for ; Thu, 14 Nov 2024 17:22:39 +0000 (UTC) X-FDA: 82785367812.03.A1CEFF9 Received: from mail-qk1-f181.google.com (mail-qk1-f181.google.com [209.85.222.181]) by imf20.hostedemail.com (Postfix) with ESMTP id 910381C001E for ; Thu, 14 Nov 2024 17:21:42 +0000 (UTC) Authentication-Results: imf20.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=QjgM3UBg; spf=pass (imf20.hostedemail.com: domain of amir73il@gmail.com designates 209.85.222.181 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=1731604782; 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=OeFJjr3sAvxDHgXGKvBlBBH/HNXVr6YZMX5uUHaIfVg=; b=DPlQZcjf9XHC6RsKvySDwpU7oRrUYeHNqUUUpkGfsUrSBDbHQmvqKPaQE9KBlG1hBhpFfx mYxf0hXraBPCSf4ApjOnshPONZuFTSRHQFx4j1H/9pp/Hi5yuu+W5hmfgSWWWLXvwRe3Fg 1nKYI0qp9NiSirP552DM2sgkoUHrMr4= ARC-Authentication-Results: i=1; imf20.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=QjgM3UBg; spf=pass (imf20.hostedemail.com: domain of amir73il@gmail.com designates 209.85.222.181 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=1731604782; a=rsa-sha256; cv=none; b=Ai6oE029/d3rSywzQpphd/VZu03OswuX91a2s/ETDvtNVGJ+acJSsOlGveMWW0DDwM9ueE SCJJBFp7QV8b/OMGadsTN4SfVErL5lWl8nI3aO13XfWlDwy3fYosA4vJYndndGoRg+abzD mGTEr+sjnn0s1KaZjIzOLktvJ0o+rZc= Received: by mail-qk1-f181.google.com with SMTP id af79cd13be357-7b35b1eb7e3so74413185a.1 for ; Thu, 14 Nov 2024 09:22:37 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1731604957; x=1732209757; 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=OeFJjr3sAvxDHgXGKvBlBBH/HNXVr6YZMX5uUHaIfVg=; b=QjgM3UBghRjkHNBARs/eEu+4A3BmKaGwBi8Mf8cmtUAZKQP4/NPSWQN2os9AKAQpPh Oj3mefMmWz035b1SXCfu0KS1nNIQCQ3YlET24sn/fRcE9eJzN3T7KU6lswgXcV+9burH T7uZb8ni4FKmFybyDcvhtpzoXy7ZGbX7iso0FSglCX8f0voiolwEJ7wRW82o2YdRphUZ UM4GbKNFC3DY2SJGAlfchSq2s2e4tk2xw0Ldn5m5ZYshFNgqPYSSI7EWgONVK/O4cEP2 eT7H5nz3Ai1Taf4fJ/6reTE8rcuZSNXZrSBgKHS+I1D4m1gbKvkPwfYJcN+YuNfX3DP5 eOdA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1731604957; x=1732209757; 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=OeFJjr3sAvxDHgXGKvBlBBH/HNXVr6YZMX5uUHaIfVg=; b=dL5RHX64REBTflDVrl1rSFpVScKVCY06QpNg5LyEbdcO8qbbjoIhf2dVrY+Zk2Euuy /DwZuiGOCvsQtRl7KbTHI+86FtITOqvyAdFSE7InsSTChZfZaikLm8ddfGk8L1VmZn3B Aw38l2IKg1bHUXzlEFjBSwZA/gypRGa/GLNb7dQZ+mlgHSjCe3aO8yEEsajyk5viuLbY vhZqhgq5PpMsEgzhgiwgBJ2K5ge0KnfyPmAWu1fy+X0xBX0uDcikClB8WoV7vpmbTg2O wCh40iPfJG6W/SCXaMhBR/Z0vXyf7Oxfrqj8o9X/pCESJBZzjOpYEtvYundI2IX+iJOt IkfQ== X-Forwarded-Encrypted: i=1; AJvYcCUasL18cUsXoGKPRiG5ErS51N6x3b9+LdkDlsAuX35JNJnHEzyskHS+OYjq6qKiszbyKpxU6YZE7w==@kvack.org X-Gm-Message-State: AOJu0YxSzFr+JjGPjVfCxBsa7MdleMJf0xjUpvaDTLWhKJE8UH9m1Tuy HQGOvYEBwx7xXTjWgRzmZYgeDQXAE5emHcbUBj0l1gdHa6oDhmaFheAqeu9xNQgu/nUkokHpNH3 VWBm8y5iar+TNTRZnjwRS8+XPkcU= X-Google-Smtp-Source: AGHT+IGWy+vxnndz1151nIk1OoEYSziwAZ6Hll6nVis3K2RK0WbhxR9+FRkzy7ldvbq1Mbv1M4ojoumTX5Yaw3ABvqg= X-Received: by 2002:a05:620a:2894:b0:7a9:abdf:f517 with SMTP id af79cd13be357-7b35a545fc5mr637868285a.25.1731604956930; Thu, 14 Nov 2024 09:22:36 -0800 (PST) MIME-Version: 1.0 References: <141e2cc2dfac8b2f49c1c8d219dd7c20925b2cef.1731433903.git.josef@toxicpanda.com> <20241114150127.clibtrycjd3ke5ld@quack3> In-Reply-To: <20241114150127.clibtrycjd3ke5ld@quack3> From: Amir Goldstein Date: Thu, 14 Nov 2024 18:22:25 +0100 Message-ID: Subject: Re: [PATCH v7 05/18] fsnotify: introduce pre-content permission events To: Jan Kara Cc: Linus Torvalds , Josef Bacik , kernel-team@fb.com, linux-fsdevel@vger.kernel.org, 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: rspam09 X-Rspamd-Queue-Id: 910381C001E X-Stat-Signature: q8xob6qfazgaz3cr68rx6eg735a9c43c X-Rspam-User: X-HE-Tag: 1731604902-627856 X-HE-Meta: U2FsdGVkX1/iTqcw9/E3H7uG3Xjzb1MGTs9F1CZ50qZkTBY5MZtAyXjGoRduNOJ/VFhUvhtGTI1fnaNeNi373QdfeI6X1FTQpW16kFc1uGEe5jVAC58/sZkWbKb84zN26csMVypMr12GlH7dAEV/VdgpmsWvYrv1elZeLg/0giF+dqQCV4Ehtg4yWeEmhdJt8Tgne3TeWuWZa+lgSZGGBi67dFPDbrpPgWHLb55J8PvXbDmx/FdmqIx/L8Sp69R7XOrGO997JoLPxEtlYWs+kDSrWJ9R/MWy+/hYsycTSmhtFsqW4mFoAdTM8ewK4nV0AzT5dDGOKIOvJzQvH6hIveK5etYPdLvgkhJVBDePZyrpaaaDfs6dmvsXvAllaNKyvYcbS0LFr1sWySx/HZDpAwH9EpBjfsIJuVuzEQNh0pda4AH62/6j50fR+EsZJ2luTewY0o8Gkmf/t2t7+tt/2+evY0nGLztWr2W0RaIQ6jjmsMp20gSsG1tWJr6n27bsXYyvUaB2L8uNDTcti058QCybTuTC2snNzxeOPyhNvaI/Zpo/lCw7m/eTXbHAJkdkjXzBfmFc+1xYnQKvhnr186f37z2rHa0Ettk3hGy55rTqYlxBl9qmOi/uMSWWPH+bFCFcK+fLQ/4YYNojEmWc+owdNV9IACiYyT2gauQvcTzrU9YtTXE7PGe5ykHt4vkzBW7mxrp89xh4/Ye0YnIynGz6zZ2WYm+WJVh1pMh4R/FOxYtItBmhX6PaeU6K4w/RQs19L21vRU0V+ACxAflu93PucEK/DhAYjfrIDS4J2EJjqo+CstK2EmMXuJo3fJQB3wLUz4o1rEeCyVcf1kPNdDfACh/M9TincnQNi91HSrqc9wMn7gC3dzuS57yQfVeMUTh2jQQzt2p7NsJI4m8m7zYcxFz55seUJDUnJ1pK3p7m6MGRj/+1xv57giXs5xmADVDhPRjDOwyXTQLiqPz G9rgRGFa EL2rBKBSztC2hS5QkrUF1Tc7bxix3kfBYX3twNCglVIH6mgAyFtP8+wvm/xemX56/UcPeZub7HvuVYxPCAC8E8kP8S8UixM12Ey3zFMd3pKUi28KsrJdm9a0tFFjjBBlN3MVjzZElQ+37Pxa60myt7ZhgkT3DdrQt2o5n5iBw57AKauct25YytkYwcoBk7/eKCrJ1U72Eapche7MRYTwrmw1Gk+M3NEkGtwEUEkVwn9x9d07OLWulU7y7V+ayILKk/prQKj33yTyVYEdx2P5/ZkjANrZn5hgPsGd3ymtDSKiZJ56xnk9vyo+wMRyO8/RvnNXYBManTyO18/4qI90vmxRfDQYMgcinrK+CuuQHSwWswmaOZbx1H58i9Dy7j6bkJJk1OaN9c52nM1C89pO9OWujJqH++k0rx3UyOZFjzytVKNE= 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 Thu, Nov 14, 2024 at 4:01=E2=80=AFPM Jan Kara wrote: > > On Wed 13-11-24 19:49:31, Amir Goldstein wrote: > > From 7a2cd74654a53684d545b96c57c9091e420b3add Mon Sep 17 00:00:00 2001 > > From: Amir Goldstein > > Date: Tue, 12 Nov 2024 13:46:08 +0100 > > Subject: [PATCH] fsnotify: opt-in for permission events at file open ti= me > > > > Legacy inotify/fanotify listeners can add watches for events on inode, > > parent or mount and expect to get events (e.g. FS_MODIFY) on files that > > were already open at the time of setting up the watches. > > > > fanotify permission events are typically used by Anti-malware sofware, > > that is watching the entire mount and it is not common to have more tha= t > > one Anti-malware engine installed on a system. > > > > To reduce the overhead of the fsnotify_file_perm() hooks on every file > > access, relax the semantics of the legacy FAN_ACCESS_PERM event to gene= rate > > events only if there were *any* permission event listeners on the > > filesystem at the time that the file was opened. > > > > The new semantic is implemented by extending the FMODE_NONOTIFY bit int= o > > two FMODE_NONOTIFY_* bits, that are used to store a mode for which of t= he > > events types to report. > > > > This is going to apply to the new fanotify pre-content events in order > > to reduce the cost of the new pre-content event vfs hooks. > > > > Suggested-by: Linus Torvalds > > Link: https://lore.kernel.org/linux-fsdevel/CAHk-=3Dwj8L=3DmtcRTi=3DNEC= HMGfZQgXOp_uix1YVh04fEmrKaMnXA@mail.gmail.com/ > > Signed-off-by: Amir Goldstein > > Couple of notes below. > > > diff --git a/fs/open.c b/fs/open.c > > index 226aca8c7909..194c2c8d8cd4 100644 > > --- a/fs/open.c > > +++ b/fs/open.c > > @@ -901,7 +901,7 @@ static int do_dentry_open(struct file *f, > > f->f_sb_err =3D file_sample_sb_err(f); > > > > if (unlikely(f->f_flags & O_PATH)) { > > - f->f_mode =3D FMODE_PATH | FMODE_OPENED; > > + f->f_mode =3D FMODE_PATH | FMODE_OPENED | FMODE_NONOTIFY; > > f->f_op =3D &empty_fops; > > return 0; > > } > > @@ -929,6 +929,12 @@ static int do_dentry_open(struct file *f, > > if (error) > > goto cleanup_all; > > > > + /* > > + * Set FMODE_NONOTIFY_* bits according to existing permission wat= ches. > > + * If FMODE_NONOTIFY was already set for an fanotify fd, this doe= sn't > > + * change anything. > > + */ > > + f->f_mode |=3D fsnotify_file_mode(f); > > Maybe it would be obvious to do this like: > > file_set_fsnotify_mode(f); > > Because currently this depends on the details of how exactly FMODE_NONOTI= FY > is encoded. > ok. makes sense. > > diff --git a/include/linux/fs.h b/include/linux/fs.h > > index 70359dd669ff..dd583ce7dba8 100644 > > --- a/include/linux/fs.h > > +++ b/include/linux/fs.h > > @@ -173,13 +173,14 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, lo= ff_t offset, > > > > #define FMODE_NOREUSE ((__force fmode_t)(1 << 23)) > > > > -/* FMODE_* bit 24 */ > > - > > /* File is embedded in backing_file object */ > > -#define FMODE_BACKING ((__force fmode_t)(1 << 25)) > > +#define FMODE_BACKING ((__force fmode_t)(1 << 24)) > > + > > +/* File shouldn't generate fanotify pre-content events */ > > +#define FMODE_NONOTIFY_HSM ((__force fmode_t)(1 << 25)) > > > > -/* File was opened by fanotify and shouldn't generate fanotify events = */ > > -#define FMODE_NONOTIFY ((__force fmode_t)(1 << 26)) > > +/* File shouldn't generate fanotify permission events */ > > +#define FMODE_NONOTIFY_PERM ((__force fmode_t)(1 << 26)) > > > > /* File is capable of returning -EAGAIN if I/O will block */ > > #define FMODE_NOWAIT ((__force fmode_t)(1 << 27)) > > @@ -190,6 +191,21 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, lof= f_t offset, > > /* File does not contribute to nr_files count */ > > #define FMODE_NOACCOUNT ((__force fmode_t)(1 << 29)) > > > > +/* > > + * The two FMODE_NONOTIFY_ bits used together have a special meaning o= f > > + * not reporting any events at all including non-permission events. > > + * These are the possible values of FMODE_NOTIFY(f->f_mode) and their = meaning: > > + * > > + * FMODE_NONOTIFY_HSM - suppress only pre-content events. > > + * FMODE_NONOTIFY_PERM - suppress permission (incl. pre-content) event= s. > > + * FMODE_NONOTIFY - suppress all (incl. non-permission) events. > > + */ > > +#define FMODE_NONOTIFY_MASK \ > > + (FMODE_NONOTIFY_HSM | FMODE_NONOTIFY_PERM) > > +#define FMODE_NONOTIFY FMODE_NONOTIFY_MASK > > +#define FMODE_NOTIFY(mode) \ > > + ((mode) & FMODE_NONOTIFY_MASK) > > This looks a bit error-prone to me (FMODE_NONOTIFY looks like another FMO= DE > flag but in fact it is not which is an invitation for subtle bugs) and th= e > tests below which are sometimes done as (FMODE_NOTIFY(mode) =3D=3D xxx) a= nd > sometimes as (file->f_mode & xxx) are inconsistent and confusing (unless = you > understand what's happening under the hood). > > So how about defining macros like FMODE_FSNOTIFY_NORMAL(), > FMODE_FSNOTIFY_CONTENT() and FMODE_FSNOTIFY_PRE_CONTENT() which evaluate = to > true if we should be sending normal/content/pre-content events to the fil= e. > With appropriate comments this should make things more obvious. > ok, maybe something like this: #define FMODE_FSNOTIFY_NONE(mode) \ (FMODE_FSNOTIFY(mode) =3D=3D FMODE_NONOTIFY) #define FMODE_FSNOTIFY_NORMAL(mode) \ (FMODE_FSNOTIFY(mode) =3D=3D FMODE_NONOTIFY_PERM) #define FMODE_FSNOTIFY_PERM(mode) \ (!((mode) & FMODE_NONOTIFY_PERM)) #define FMODE_FSNOTIFY_HSM(mode) \ (FMODE_FSNOTIFY(mode) =3D=3D 0) At least keeping the double negatives contained in one place. And then we have these users in the final code: static inline bool fsnotify_file_has_pre_content_watches(struct file *file) { return file && unlikely(FMODE_FSNOTIFY_HSM(file->f_mode)); } static inline int fsnotify_open_perm(struct file *file) { int ret; if (likely(!FMODE_FSNOTIFY_PERM(file->f_mode))) return 0; ... static inline int fsnotify_file(struct file *file, __u32 mask) { if (FMODE_FSNOTIFY_NONE(file->f_mode)) return 0; ... BTW, I prefer using PERM,HSM instead of the FSNOTIFY_PRIO_ names for brevity, but also because at the moment of this patch FMODE_NONOTIFY_PERM means "suppress permission events if there are no listeners with priority >=3D FSNOTIFY_PRIO_CONTENT at all on any objects of the filesystem". It does NOT mean that there ARE permission events watchers on the file's sb/mnt/inode or parent, but what the users of the flag care about really is whether the specific file is being watched for permission events. I was contemplating if we should add the following check at open time as following patches add for pre-content watchers also for permission watchers on the specific file: /* * Permission events is a super set of pre-content events, so if th= ere * are no permission event watchers, there are also no pre-content = event * watchers and this is implied from the single FMODE_NONOTIFY_PERM= bit. */ if (likely(!fsnotify_sb_has_priority_watchers(sb, FSNOTIFY_PRIO_CONTENT))) return FMODE_NONOTIFY_PERM; + /* + * There are content watchers in the filesystem, but are there + * permission event watchers on this specific file? + */ + if (likely(!fsnotify_file_object_watched(file, + ALL_FSNOTIFY_PERM_EVENTS)= )) + return FMODE_NONOTIFY_PERM; + I decided not to stretch the behavior change too much and also since Anti-malware permission watchers often watch all the mounts of a filesystem, there is probably little to gain from this extra check. But we can reconsider this in the future. WDYT? In any case, IMO the language of FMODE_FSNOTIFY_PERM() matches the meaning of the users better and makes the code easier to understand. FMODE_FSNOTIFY_HSM() is debatable, but at least it is short ;) Anyway, I will send v2 with your suggestions. Thanks, Amir.