[PATCH] fanotify: update connector fsid cache on add mark

[PATCH] fanotify: update connector fsid cache on add mark

[Amir Goldstein]

When implementing connector fsid cache, we only initialized the cache
when the first mark added to object was added by FAN_REPORT_FID group.
We forgot to update conn->fsid when the second mark is added by
FAN_REPORT_FID group to an already attached connector without fsid
cache.

Reported-and-tested-by: syzbot+c277e8e2f46414645508@syzkaller.appspotmail.com
Fixes: 77115225acc6 ("fanotify: cache fsid in fsnotify_mark_connector")
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---

Jan,

This fix has been confirmed by syzbot to fix the issue as well as
by my modification to Matthew's LTP test:
https://github.com/amir73il/ltp/commits/fanotify_dirent

Thanks,
Amir.

 fs/notify/fanotify/fanotify.c    |  4 ++++
 fs/notify/mark.c                 | 14 +++++++++++---
 include/linux/fsnotify_backend.h |  2 ++
 3 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index e6fde1a5c072..b428c295d13f 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -355,6 +355,10 @@ static __kernel_fsid_t fanotify_get_fsid(struct fsnotify_iter_info *iter_info)
 		/* Mark is just getting destroyed or created? */
 		if (!conn)
 			continue;
+		if (!(conn->flags & FSNOTIFY_CONN_FLAG_HAS_FSID))
+			continue;
+		/* Pairs with smp_wmb() in fsnotify_add_mark_list() */
+		smp_rmb();
 		fsid = conn->fsid;
 		if (WARN_ON_ONCE(!fsid.val[0] && !fsid.val[1]))
 			continue;
diff --git a/fs/notify/mark.c b/fs/notify/mark.c
index 25eb247ea85a..99ddd126f6f0 100644
--- a/fs/notify/mark.c
+++ b/fs/notify/mark.c
@@ -482,10 +482,13 @@ static int fsnotify_attach_connector_to_object(fsnotify_connp_t *connp,
 	conn->type = type;
 	conn->obj = connp;
 	/* Cache fsid of filesystem containing the object */
-	if (fsid)
+	if (fsid) {
 		conn->fsid = *fsid;
-	else
+		conn->flags = FSNOTIFY_CONN_FLAG_HAS_FSID;
+	} else {
 		conn->fsid.val[0] = conn->fsid.val[1] = 0;
+		conn->flags = 0;
+	}
 	if (conn->type == FSNOTIFY_OBJ_TYPE_INODE)
 		inode = igrab(fsnotify_conn_inode(conn));
 	/*
@@ -560,7 +563,12 @@ static int fsnotify_add_mark_list(struct fsnotify_mark *mark,
 		if (err)
 			return err;
 		goto restart;
-	} else if (fsid && (conn->fsid.val[0] || conn->fsid.val[1]) &&
+	} else if (fsid && !(conn->flags & FSNOTIFY_CONN_FLAG_HAS_FSID)) {
+		conn->fsid = *fsid;
+		/* Pairs with smp_rmb() in fanotify_get_fsid() */
+		smp_wmb();
+		conn->flags |= FSNOTIFY_CONN_FLAG_HAS_FSID;
+	} else if (fsid && (conn->flags & FSNOTIFY_CONN_FLAG_HAS_FSID) &&
 		   (fsid->val[0] != conn->fsid.val[0] ||
 		    fsid->val[1] != conn->fsid.val[1])) {
 		/*
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index a9f9dcc1e515..da181dc05261 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -293,6 +293,8 @@ typedef struct fsnotify_mark_connector __rcu *fsnotify_connp_t;
 struct fsnotify_mark_connector {
 	spinlock_t lock;
 	unsigned int type;	/* Type of object [lock] */
+#define FSNOTIFY_CONN_FLAG_HAS_FSID	0x01
+	unsigned int flags;	/* flags [lock] */
 	__kernel_fsid_t fsid;	/* fsid of filesystem containing object */
 	union {
 		/* Object pointer [lock] */
-- 
2.17.1

Re: [PATCH] fanotify: update connector fsid cache on add mark

[Jan Kara]

On Wed 19-06-19 13:34:44, Amir Goldstein wrote:
> When implementing connector fsid cache, we only initialized the cache
> when the first mark added to object was added by FAN_REPORT_FID group.
> We forgot to update conn->fsid when the second mark is added by
> FAN_REPORT_FID group to an already attached connector without fsid
> cache.
> 
> Reported-and-tested-by: syzbot+c277e8e2f46414645508@syzkaller.appspotmail.com
> Fixes: 77115225acc6 ("fanotify: cache fsid in fsnotify_mark_connector")
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
> 
> Jan,
> 
> This fix has been confirmed by syzbot to fix the issue as well as
> by my modification to Matthew's LTP test:
> https://github.com/amir73il/ltp/commits/fanotify_dirent

Thanks for the fix Amir. I somewhat don't like the additional flags field
(which ends up growing fsnotify_mark_connector by one long) for just that
one special flag. If nothing else, can't we just store the flag inside
'type'? There's plenty of space there...

								Honza
> 
> Thanks,
> Amir.
> 
>  fs/notify/fanotify/fanotify.c    |  4 ++++
>  fs/notify/mark.c                 | 14 +++++++++++---
>  include/linux/fsnotify_backend.h |  2 ++
>  3 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index e6fde1a5c072..b428c295d13f 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -355,6 +355,10 @@ static __kernel_fsid_t fanotify_get_fsid(struct fsnotify_iter_info *iter_info)
>  		/* Mark is just getting destroyed or created? */
>  		if (!conn)
>  			continue;
> +		if (!(conn->flags & FSNOTIFY_CONN_FLAG_HAS_FSID))
> +			continue;
> +		/* Pairs with smp_wmb() in fsnotify_add_mark_list() */
> +		smp_rmb();
>  		fsid = conn->fsid;
>  		if (WARN_ON_ONCE(!fsid.val[0] && !fsid.val[1]))
>  			continue;
> diff --git a/fs/notify/mark.c b/fs/notify/mark.c
> index 25eb247ea85a..99ddd126f6f0 100644
> --- a/fs/notify/mark.c
> +++ b/fs/notify/mark.c
> @@ -482,10 +482,13 @@ static int fsnotify_attach_connector_to_object(fsnotify_connp_t *connp,
>  	conn->type = type;
>  	conn->obj = connp;
>  	/* Cache fsid of filesystem containing the object */
> -	if (fsid)
> +	if (fsid) {
>  		conn->fsid = *fsid;
> -	else
> +		conn->flags = FSNOTIFY_CONN_FLAG_HAS_FSID;
> +	} else {
>  		conn->fsid.val[0] = conn->fsid.val[1] = 0;
> +		conn->flags = 0;
> +	}
>  	if (conn->type == FSNOTIFY_OBJ_TYPE_INODE)
>  		inode = igrab(fsnotify_conn_inode(conn));
>  	/*
> @@ -560,7 +563,12 @@ static int fsnotify_add_mark_list(struct fsnotify_mark *mark,
>  		if (err)
>  			return err;
>  		goto restart;
> -	} else if (fsid && (conn->fsid.val[0] || conn->fsid.val[1]) &&
> +	} else if (fsid && !(conn->flags & FSNOTIFY_CONN_FLAG_HAS_FSID)) {
> +		conn->fsid = *fsid;
> +		/* Pairs with smp_rmb() in fanotify_get_fsid() */
> +		smp_wmb();
> +		conn->flags |= FSNOTIFY_CONN_FLAG_HAS_FSID;
> +	} else if (fsid && (conn->flags & FSNOTIFY_CONN_FLAG_HAS_FSID) &&
>  		   (fsid->val[0] != conn->fsid.val[0] ||
>  		    fsid->val[1] != conn->fsid.val[1])) {
>  		/*
> diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
> index a9f9dcc1e515..da181dc05261 100644
> --- a/include/linux/fsnotify_backend.h
> +++ b/include/linux/fsnotify_backend.h
> @@ -293,6 +293,8 @@ typedef struct fsnotify_mark_connector __rcu *fsnotify_connp_t;
>  struct fsnotify_mark_connector {
>  	spinlock_t lock;
>  	unsigned int type;	/* Type of object [lock] */
> +#define FSNOTIFY_CONN_FLAG_HAS_FSID	0x01
> +	unsigned int flags;	/* flags [lock] */
>  	__kernel_fsid_t fsid;	/* fsid of filesystem containing object */
>  	union {
>  		/* Object pointer [lock] */
> -- 
> 2.17.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] fanotify: update connector fsid cache on add mark

[Amir Goldstein]

On Wed, Jun 19, 2019 at 3:53 PM Jan Kara <jack@suse.cz> wrote:
>
> On Wed 19-06-19 13:34:44, Amir Goldstein wrote:
> > When implementing connector fsid cache, we only initialized the cache
> > when the first mark added to object was added by FAN_REPORT_FID group.
> > We forgot to update conn->fsid when the second mark is added by
> > FAN_REPORT_FID group to an already attached connector without fsid
> > cache.
> >
> > Reported-and-tested-by: syzbot+c277e8e2f46414645508@syzkaller.appspotmail.com
> > Fixes: 77115225acc6 ("fanotify: cache fsid in fsnotify_mark_connector")
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >
> > Jan,
> >
> > This fix has been confirmed by syzbot to fix the issue as well as
> > by my modification to Matthew's LTP test:
> > https://github.com/amir73il/ltp/commits/fanotify_dirent
>
> Thanks for the fix Amir. I somewhat don't like the additional flags field
> (which ends up growing fsnotify_mark_connector by one long) for just that
> one special flag. If nothing else, can't we just store the flag inside
> 'type'? There's plenty of space there...

I didn't think it mattered in the grand scheme of things, but
I did consider:
-        unsigned int type;      /* Type of object [lock] */
+        unsigned short type;      /* Type of object [lock] */
+#define FSNOTIFY_CONN_FLAG_HAS_FSID    0x01
+       unsigned short flags;     /* flags [lock] */

I think it makes sense.
Let me know if you want me to resend of if you can fix on commit.

Thanks,
Amir.

Re: [PATCH] fanotify: update connector fsid cache on add mark

[Jan Kara]

On Wed 19-06-19 16:04:04, Amir Goldstein wrote:
> On Wed, Jun 19, 2019 at 3:53 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Wed 19-06-19 13:34:44, Amir Goldstein wrote:
> > > When implementing connector fsid cache, we only initialized the cache
> > > when the first mark added to object was added by FAN_REPORT_FID group.
> > > We forgot to update conn->fsid when the second mark is added by
> > > FAN_REPORT_FID group to an already attached connector without fsid
> > > cache.
> > >
> > > Reported-and-tested-by: syzbot+c277e8e2f46414645508@syzkaller.appspotmail.com
> > > Fixes: 77115225acc6 ("fanotify: cache fsid in fsnotify_mark_connector")
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > ---
> > >
> > > Jan,
> > >
> > > This fix has been confirmed by syzbot to fix the issue as well as
> > > by my modification to Matthew's LTP test:
> > > https://github.com/amir73il/ltp/commits/fanotify_dirent
> >
> > Thanks for the fix Amir. I somewhat don't like the additional flags field
> > (which ends up growing fsnotify_mark_connector by one long) for just that
> > one special flag. If nothing else, can't we just store the flag inside
> > 'type'? There's plenty of space there...
> 
> I didn't think it mattered in the grand scheme of things, but

Well, the connector size usually isn't a huge concern but there can be lots
of connectors when someone watches large number of files so I prefer not to
waste space unnecessarily.

> I did consider:
> -        unsigned int type;      /* Type of object [lock] */
> +        unsigned short type;      /* Type of object [lock] */
> +#define FSNOTIFY_CONN_FLAG_HAS_FSID    0x01
> +       unsigned short flags;     /* flags [lock] */
> 
> I think it makes sense.
> Let me know if you want me to resend of if you can fix on commit.

Yes, this is even less intrusive than what I had in mind. I'll apply this
change on commit. Thanks!

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR