diff mbox series

[01/10] fsnotify: rename fsnotify_{get,put}_sb_connectors()

Message ID 20240317184154.1200192-2-amir73il@gmail.com (mailing list archive)
State New, archived
Headers show
Series Further reduce overhead of fsnotify permission hooks | expand

Commit Message

Amir Goldstein March 17, 2024, 6:41 p.m. UTC
Instead of counting the number of connectors in an sb, we would like
to count the number of watched objects per priority group.

As a start, create an accessor fsnotify_sb_watched_objects() to
s_fsnotify_connectors and rename the fsnotify_{get,put}_sb_connectors()
helpers to fsnotify_{get,put}_sb_watchers() to better describes the
counter.

Increment the counter at the end of fsnotify_attach_connector_to_object()
if connector was attached instead of decrementing it on race to connect.

This is fine, because fsnotify_delete_sb() cannot be running in parallel
to fsnotify_attach_connector_to_object() which requires a reference to
a filesystem object.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fsnotify.c             |  4 +-
 fs/notify/mark.c                 | 67 ++++++++++++++++++--------------
 include/linux/fsnotify.h         |  2 +-
 include/linux/fsnotify_backend.h |  5 +++
 4 files changed, 45 insertions(+), 33 deletions(-)

Comments

Jan Kara March 27, 2024, 10:01 a.m. UTC | #1
On Sun 17-03-24 20:41:45, Amir Goldstein wrote:
> Instead of counting the number of connectors in an sb, we would like
> to count the number of watched objects per priority group.
> 
> As a start, create an accessor fsnotify_sb_watched_objects() to
> s_fsnotify_connectors and rename the fsnotify_{get,put}_sb_connectors()
> helpers to fsnotify_{get,put}_sb_watchers() to better describes the
> counter.
> 
> Increment the counter at the end of fsnotify_attach_connector_to_object()
> if connector was attached instead of decrementing it on race to connect.
> 
> This is fine, because fsnotify_delete_sb() cannot be running in parallel
> to fsnotify_attach_connector_to_object() which requires a reference to
> a filesystem object.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
...
> +static void fsnotify_put_inode_ref(struct inode *inode)
> +{
> +	iput(inode);
> +	fsnotify_put_sb_watched_objects(inode->i_sb);
> +}

This is a UAF issue. Will fix on commit. Otherwise the patch looks good.

								Honza
Amir Goldstein March 27, 2024, 10:05 a.m. UTC | #2
On Wed, Mar 27, 2024 at 12:01 PM Jan Kara <jack@suse.cz> wrote:
>
> On Sun 17-03-24 20:41:45, Amir Goldstein wrote:
> > Instead of counting the number of connectors in an sb, we would like
> > to count the number of watched objects per priority group.
> >
> > As a start, create an accessor fsnotify_sb_watched_objects() to
> > s_fsnotify_connectors and rename the fsnotify_{get,put}_sb_connectors()
> > helpers to fsnotify_{get,put}_sb_watchers() to better describes the
> > counter.
> >
> > Increment the counter at the end of fsnotify_attach_connector_to_object()
> > if connector was attached instead of decrementing it on race to connect.
> >
> > This is fine, because fsnotify_delete_sb() cannot be running in parallel
> > to fsnotify_attach_connector_to_object() which requires a reference to
> > a filesystem object.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ...
> > +static void fsnotify_put_inode_ref(struct inode *inode)
> > +{
> > +     iput(inode);
> > +     fsnotify_put_sb_watched_objects(inode->i_sb);
> > +}
>
> This is a UAF issue. Will fix on commit. Otherwise the patch looks good.

oops. Good catch!
Thanks,
Amir.
diff mbox series

Patch

diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index 2fc105a72a8f..503e7c75e777 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -92,8 +92,8 @@  void fsnotify_sb_delete(struct super_block *sb)
 	fsnotify_unmount_inodes(sb);
 	fsnotify_clear_marks_by_sb(sb);
 	/* Wait for outstanding object references from connectors */
-	wait_var_event(&sb->s_fsnotify_connectors,
-		       !atomic_long_read(&sb->s_fsnotify_connectors));
+	wait_var_event(fsnotify_sb_watched_objects(sb),
+		       !atomic_long_read(fsnotify_sb_watched_objects(sb)));
 }
 
 /*
diff --git a/fs/notify/mark.c b/fs/notify/mark.c
index d6944ff86ffa..8339d77b1aa2 100644
--- a/fs/notify/mark.c
+++ b/fs/notify/mark.c
@@ -116,10 +116,43 @@  __u32 fsnotify_conn_mask(struct fsnotify_mark_connector *conn)
 	return *fsnotify_conn_mask_p(conn);
 }
 
+static void fsnotify_get_sb_watched_objects(struct super_block *sb)
+{
+	atomic_long_inc(fsnotify_sb_watched_objects(sb));
+}
+
+static void fsnotify_put_sb_watched_objects(struct super_block *sb)
+{
+	if (atomic_long_dec_and_test(fsnotify_sb_watched_objects(sb)))
+		wake_up_var(fsnotify_sb_watched_objects(sb));
+}
+
 static void fsnotify_get_inode_ref(struct inode *inode)
 {
 	ihold(inode);
-	atomic_long_inc(&inode->i_sb->s_fsnotify_connectors);
+	fsnotify_get_sb_watched_objects(inode->i_sb);
+}
+
+static void fsnotify_put_inode_ref(struct inode *inode)
+{
+	iput(inode);
+	fsnotify_put_sb_watched_objects(inode->i_sb);
+}
+
+static void fsnotify_get_sb_watchers(struct fsnotify_mark_connector *conn)
+{
+	struct super_block *sb = fsnotify_connector_sb(conn);
+
+	if (sb)
+		fsnotify_get_sb_watched_objects(sb);
+}
+
+static void fsnotify_put_sb_watchers(struct fsnotify_mark_connector *conn)
+{
+	struct super_block *sb = fsnotify_connector_sb(conn);
+
+	if (sb)
+		fsnotify_put_sb_watched_objects(sb);
 }
 
 /*
@@ -213,31 +246,6 @@  static void fsnotify_connector_destroy_workfn(struct work_struct *work)
 	}
 }
 
-static void fsnotify_put_inode_ref(struct inode *inode)
-{
-	struct super_block *sb = inode->i_sb;
-
-	iput(inode);
-	if (atomic_long_dec_and_test(&sb->s_fsnotify_connectors))
-		wake_up_var(&sb->s_fsnotify_connectors);
-}
-
-static void fsnotify_get_sb_connectors(struct fsnotify_mark_connector *conn)
-{
-	struct super_block *sb = fsnotify_connector_sb(conn);
-
-	if (sb)
-		atomic_long_inc(&sb->s_fsnotify_connectors);
-}
-
-static void fsnotify_put_sb_connectors(struct fsnotify_mark_connector *conn)
-{
-	struct super_block *sb = fsnotify_connector_sb(conn);
-
-	if (sb && atomic_long_dec_and_test(&sb->s_fsnotify_connectors))
-		wake_up_var(&sb->s_fsnotify_connectors);
-}
-
 static void *fsnotify_detach_connector_from_object(
 					struct fsnotify_mark_connector *conn,
 					unsigned int *type)
@@ -261,7 +269,7 @@  static void *fsnotify_detach_connector_from_object(
 		fsnotify_conn_sb(conn)->s_fsnotify_mask = 0;
 	}
 
-	fsnotify_put_sb_connectors(conn);
+	fsnotify_put_sb_watchers(conn);
 	rcu_assign_pointer(*(conn->obj), NULL);
 	conn->obj = NULL;
 	conn->type = FSNOTIFY_OBJ_TYPE_DETACHED;
@@ -549,8 +557,6 @@  static int fsnotify_attach_connector_to_object(fsnotify_connp_t *connp,
 	conn->flags = 0;
 	conn->type = obj_type;
 	conn->obj = connp;
-	conn->flags = 0;
-	fsnotify_get_sb_connectors(conn);
 
 	/*
 	 * cmpxchg() provides the barrier so that readers of *connp can see
@@ -558,10 +564,11 @@  static int fsnotify_attach_connector_to_object(fsnotify_connp_t *connp,
 	 */
 	if (cmpxchg(connp, NULL, conn)) {
 		/* Someone else created list structure for us */
-		fsnotify_put_sb_connectors(conn);
 		kmem_cache_free(fsnotify_mark_connector_cachep, conn);
+		return 0;
 	}
 
+	fsnotify_get_sb_watchers(conn);
 	return 0;
 }
 
diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
index 1a9de119a0f7..e470bb67c9a3 100644
--- a/include/linux/fsnotify.h
+++ b/include/linux/fsnotify.h
@@ -20,7 +20,7 @@ 
 /* Are there any inode/mount/sb objects that are being watched at all? */
 static inline bool fsnotify_sb_has_watchers(struct super_block *sb)
 {
-	return atomic_long_read(&sb->s_fsnotify_connectors);
+	return atomic_long_read(fsnotify_sb_watched_objects(sb));
 }
 
 /*
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 8f40c349b228..d4e3bc55d174 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -483,6 +483,11 @@  struct fsnotify_mark_connector {
 	struct hlist_head list;
 };
 
+static inline atomic_long_t *fsnotify_sb_watched_objects(struct super_block *sb)
+{
+	return &sb->s_fsnotify_connectors;
+}
+
 /*
  * A mark is simply an object attached to an in core inode which allows an
  * fsnotify listener to indicate they are either no longer interested in events