[PATCH] fsnotify: Fix fsnotify_mark_connector race

[PATCH] fsnotify: Fix fsnotify_mark_connector race

[Robert Kolchmeyer]

fsnotify() acquires a reference to a fsnotify_mark_connector through
the SRCU-protected pointer to_tell->i_fsnotify_marks. However, it
appears that no precautions are taken in fsnotify_put_mark() to
ensure that fsnotify() drops its reference to this
fsnotify_mark_connector before assigning a value to its 'destroy_next'
field. This can result in fsnotify_put_mark() assigning a value
to a connector's 'destroy_next' field right before fsnotify() tries to
traverse the linked list referenced by the connector's 'list' field.
Since these two fields are members of the same union, this behavior
results in a kernel panic.

This issue is resolved by calling synchronize_srcu() before updating
the connector's 'destroy_next' field in fsnotify_put_mark(). Since the
relevant section of fsnotify() occurs in a SRCU read-side critical
section, this will force fsnotify_put_mark() to wait for fsnotify()
to finish operating on the connector before updating its 'destroy_next'
field. Since fsnotify_put_mark() removes references to the connector
before assigning its 'destroy_next' field, it shouldn't be possible for
another thread to acquire a reference to the connector while
fsnotify_put_mark() waits for fsnotify() to finish its work.

The offending behavior here is extremely unlikely; since
fsnotify_put_mark() removes references to a connector (specifically,
it ensures that the connector is unreachable from the inode it was
formerly attached to) before updating its 'destroy_next' field, a
sizeable chunk of code in fsnotify_put_mark() has to execute in the
short window between when fsnotify() acquires the connector reference
and saves the value of its 'list' field. On the HEAD kernel, I've only
been able to reproduce this by inserting a udelay(1) in fsnotify().
However, I've been able to reproduce this issue without inserting a
udelay(1) anywhere on older unmodified release kernels, so I believe
it's worth fixing at HEAD.

Fixes bug 199437: https://bugzilla.kernel.org/show_bug.cgi?id=199437

Fixes: 08991e83b7286635167bab40927665a90fb00d81
Signed-off-by: Robert Kolchmeyer <rkolchmeyer@google.com>
---
 fs/notify/mark.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/notify/mark.c b/fs/notify/mark.c
index e9191b416434..358fc7de1e86 100644
--- a/fs/notify/mark.c
+++ b/fs/notify/mark.c
@@ -227,6 +227,7 @@ void fsnotify_put_mark(struct fsnotify_mark *mark)
 	iput(inode);
 
 	if (free_conn) {
+		synchronize_srcu(&fsnotify_mark_srcu);
 		spin_lock(&destroy_lock);
 		conn->destroy_next = connector_destroy_list;
 		connector_destroy_list = conn;
-- 
2.17.0.484.g0c8726318c-goog

Re: [PATCH] fsnotify: Fix fsnotify_mark_connector race

[Jan Kara]

On Wed 18-04-18 17:05:25, Robert Kolchmeyer wrote:
> fsnotify() acquires a reference to a fsnotify_mark_connector through
> the SRCU-protected pointer to_tell->i_fsnotify_marks. However, it
> appears that no precautions are taken in fsnotify_put_mark() to
> ensure that fsnotify() drops its reference to this
> fsnotify_mark_connector before assigning a value to its 'destroy_next'
> field. This can result in fsnotify_put_mark() assigning a value
> to a connector's 'destroy_next' field right before fsnotify() tries to
> traverse the linked list referenced by the connector's 'list' field.
> Since these two fields are members of the same union, this behavior
> results in a kernel panic.

Ahh, I was reading this code just two days ago trying to find some issues
(due to reports of softlockups in fsnotify()) but this didn't occur to me.
Great spotting!

> This issue is resolved by calling synchronize_srcu() before updating
> the connector's 'destroy_next' field in fsnotify_put_mark(). Since the
> relevant section of fsnotify() occurs in a SRCU read-side critical
> section, this will force fsnotify_put_mark() to wait for fsnotify()
> to finish operating on the connector before updating its 'destroy_next'
> field. Since fsnotify_put_mark() removes references to the connector
> before assigning its 'destroy_next' field, it shouldn't be possible for
> another thread to acquire a reference to the connector while
> fsnotify_put_mark() waits for fsnotify() to finish its work.
> 
> The offending behavior here is extremely unlikely; since
> fsnotify_put_mark() removes references to a connector (specifically,
> it ensures that the connector is unreachable from the inode it was
> formerly attached to) before updating its 'destroy_next' field, a
> sizeable chunk of code in fsnotify_put_mark() has to execute in the
> short window between when fsnotify() acquires the connector reference
> and saves the value of its 'list' field. On the HEAD kernel, I've only
> been able to reproduce this by inserting a udelay(1) in fsnotify().
> However, I've been able to reproduce this issue without inserting a
> udelay(1) anywhere on older unmodified release kernels, so I believe
> it's worth fixing at HEAD.

Oh, the bug definitely needs fixing. I somewhat dislike synchronize_srcu()
when destroying the connector though. The whole point of
connector_destroy_list is to batch synchronize_srcu() calls and move them
out of the relative fast path. So what I'd do instead is to move
'destroy_next' from a union with 'list' to a union with 'inode' and 'mnt'.
Those are not safe to access under srcu anyway - you have to hold
conn->lock to get them and use conn->flags to determine the type under the
same lock. So in that union we are actually safe to use the space for
anything once conn->flags are cleared by
fsnotify_detach_connector_from_object(). Will you send a patch for that so
that you get a proper credit for all the debugging you did? :) Thanks!

								Honza

> Fixes bug 199437: https://bugzilla.kernel.org/show_bug.cgi?id=199437
> 
> Fixes: 08991e83b7286635167bab40927665a90fb00d81
> Signed-off-by: Robert Kolchmeyer <rkolchmeyer@google.com>
> ---
>  fs/notify/mark.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/notify/mark.c b/fs/notify/mark.c
> index e9191b416434..358fc7de1e86 100644
> --- a/fs/notify/mark.c
> +++ b/fs/notify/mark.c
> @@ -227,6 +227,7 @@ void fsnotify_put_mark(struct fsnotify_mark *mark)
>  	iput(inode);
>  
>  	if (free_conn) {
> +		synchronize_srcu(&fsnotify_mark_srcu);
>  		spin_lock(&destroy_lock);
>  		conn->destroy_next = connector_destroy_list;
>  		connector_destroy_list = conn;
> -- 
> 2.17.0.484.g0c8726318c-goog
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] fsnotify: Fix fsnotify_mark_connector race

[Robert Kolchmeyer]

Sounds great. I'll send out that patch shortly. Thanks!

On Thu, Apr 19, 2018 at 2:23 AM, Jan Kara <jack@suse.cz> wrote:
> On Wed 18-04-18 17:05:25, Robert Kolchmeyer wrote:
>> fsnotify() acquires a reference to a fsnotify_mark_connector through
>> the SRCU-protected pointer to_tell->i_fsnotify_marks. However, it
>> appears that no precautions are taken in fsnotify_put_mark() to
>> ensure that fsnotify() drops its reference to this
>> fsnotify_mark_connector before assigning a value to its 'destroy_next'
>> field. This can result in fsnotify_put_mark() assigning a value
>> to a connector's 'destroy_next' field right before fsnotify() tries to
>> traverse the linked list referenced by the connector's 'list' field.
>> Since these two fields are members of the same union, this behavior
>> results in a kernel panic.
>
> Ahh, I was reading this code just two days ago trying to find some issues
> (due to reports of softlockups in fsnotify()) but this didn't occur to me.
> Great spotting!
>
>> This issue is resolved by calling synchronize_srcu() before updating
>> the connector's 'destroy_next' field in fsnotify_put_mark(). Since the
>> relevant section of fsnotify() occurs in a SRCU read-side critical
>> section, this will force fsnotify_put_mark() to wait for fsnotify()
>> to finish operating on the connector before updating its 'destroy_next'
>> field. Since fsnotify_put_mark() removes references to the connector
>> before assigning its 'destroy_next' field, it shouldn't be possible for
>> another thread to acquire a reference to the connector while
>> fsnotify_put_mark() waits for fsnotify() to finish its work.
>>
>> The offending behavior here is extremely unlikely; since
>> fsnotify_put_mark() removes references to a connector (specifically,
>> it ensures that the connector is unreachable from the inode it was
>> formerly attached to) before updating its 'destroy_next' field, a
>> sizeable chunk of code in fsnotify_put_mark() has to execute in the
>> short window between when fsnotify() acquires the connector reference
>> and saves the value of its 'list' field. On the HEAD kernel, I've only
>> been able to reproduce this by inserting a udelay(1) in fsnotify().
>> However, I've been able to reproduce this issue without inserting a
>> udelay(1) anywhere on older unmodified release kernels, so I believe
>> it's worth fixing at HEAD.
>
> Oh, the bug definitely needs fixing. I somewhat dislike synchronize_srcu()
> when destroying the connector though. The whole point of
> connector_destroy_list is to batch synchronize_srcu() calls and move them
> out of the relative fast path. So what I'd do instead is to move
> 'destroy_next' from a union with 'list' to a union with 'inode' and 'mnt'.
> Those are not safe to access under srcu anyway - you have to hold
> conn->lock to get them and use conn->flags to determine the type under the
> same lock. So in that union we are actually safe to use the space for
> anything once conn->flags are cleared by
> fsnotify_detach_connector_from_object(). Will you send a patch for that so
> that you get a proper credit for all the debugging you did? :) Thanks!
>
>                                                                 Honza
>
>> Fixes bug 199437: https://bugzilla.kernel.org/show_bug.cgi?id=199437
>>
>> Fixes: 08991e83b7286635167bab40927665a90fb00d81
>> Signed-off-by: Robert Kolchmeyer <rkolchmeyer@google.com>
>> ---
>>  fs/notify/mark.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/fs/notify/mark.c b/fs/notify/mark.c
>> index e9191b416434..358fc7de1e86 100644
>> --- a/fs/notify/mark.c
>> +++ b/fs/notify/mark.c
>> @@ -227,6 +227,7 @@ void fsnotify_put_mark(struct fsnotify_mark *mark)
>>       iput(inode);
>>
>>       if (free_conn) {
>> +             synchronize_srcu(&fsnotify_mark_srcu);
>>               spin_lock(&destroy_lock);
>>               conn->destroy_next = connector_destroy_list;
>>               connector_destroy_list = conn;
>> --
>> 2.17.0.484.g0c8726318c-goog
>>
> --
> Jan Kara <jack@suse.com>
> SUSE Labs, CR

[PATCH v2] fsnotify: Fix fsnotify_mark_connector race

[Robert Kolchmeyer]

fsnotify() acquires a reference to a fsnotify_mark_connector through
the SRCU-protected pointer to_tell->i_fsnotify_marks. However, it
appears that no precautions are taken in fsnotify_put_mark() to
ensure that fsnotify() drops its reference to this
fsnotify_mark_connector before assigning a value to its 'destroy_next'
field. This can result in fsnotify_put_mark() assigning a value
to a connector's 'destroy_next' field right before fsnotify() tries to
traverse the linked list referenced by the connector's 'list' field.
Since these two fields are members of the same union, this behavior
results in a kernel panic.

This issue is resolved by moving the connector's 'destroy_next' field
into the object pointer union. This should work since the object pointer
access is protected by both a spinlock and the value of the 'flags'
field, and the 'flags' field is cleared while holding the spinlock in
fsnotify_put_mark() before 'destroy_next' is updated. It shouldn't be
possible for another thread to accidentally read from the object pointer
after the 'destroy_next' field is updated.

The offending behavior here is extremely unlikely; since
fsnotify_put_mark() removes references to a connector (specifically,
it ensures that the connector is unreachable from the inode it was
formerly attached to) before updating its 'destroy_next' field, a
sizeable chunk of code in fsnotify_put_mark() has to execute in the
short window between when fsnotify() acquires the connector reference
and saves the value of its 'list' field. On the HEAD kernel, I've only
been able to reproduce this by inserting a udelay(1) in fsnotify().
However, I've been able to reproduce this issue without inserting a
udelay(1) anywhere on older unmodified release kernels, so I believe
it's worth fixing at HEAD.

Fixes bug 199437: https://bugzilla.kernel.org/show_bug.cgi?id=199437

Fixes: 08991e83b7286635167bab40927665a90fb00d81
Signed-off-by: Robert Kolchmeyer <rkolchmeyer@google.com>
---
Changelog since v1:
- Solve problem by moving 'destroy_next' field into a different union instead
  of calling synchronize_srcu().

 include/linux/fsnotify_backend.h | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 9f1edb92c97e..a3d13d874fd1 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -217,12 +217,10 @@ struct fsnotify_mark_connector {
 	union {	/* Object pointer [lock] */
 		struct inode *inode;
 		struct vfsmount *mnt;
-	};
-	union {
-		struct hlist_head list;
 		/* Used listing heads to free after srcu period expires */
 		struct fsnotify_mark_connector *destroy_next;
 	};
+	struct hlist_head list;
 };
 
 /*
-- 
2.17.0.484.g0c8726318c-goog

Re: [PATCH v2] fsnotify: Fix fsnotify_mark_connector race

[Jan Kara]

On Thu 19-04-18 10:44:33, Robert Kolchmeyer wrote:
> fsnotify() acquires a reference to a fsnotify_mark_connector through
> the SRCU-protected pointer to_tell->i_fsnotify_marks. However, it
> appears that no precautions are taken in fsnotify_put_mark() to
> ensure that fsnotify() drops its reference to this
> fsnotify_mark_connector before assigning a value to its 'destroy_next'
> field. This can result in fsnotify_put_mark() assigning a value
> to a connector's 'destroy_next' field right before fsnotify() tries to
> traverse the linked list referenced by the connector's 'list' field.
> Since these two fields are members of the same union, this behavior
> results in a kernel panic.
> 
> This issue is resolved by moving the connector's 'destroy_next' field
> into the object pointer union. This should work since the object pointer
> access is protected by both a spinlock and the value of the 'flags'
> field, and the 'flags' field is cleared while holding the spinlock in
> fsnotify_put_mark() before 'destroy_next' is updated. It shouldn't be
> possible for another thread to accidentally read from the object pointer
> after the 'destroy_next' field is updated.
> 
> The offending behavior here is extremely unlikely; since
> fsnotify_put_mark() removes references to a connector (specifically,
> it ensures that the connector is unreachable from the inode it was
> formerly attached to) before updating its 'destroy_next' field, a
> sizeable chunk of code in fsnotify_put_mark() has to execute in the
> short window between when fsnotify() acquires the connector reference
> and saves the value of its 'list' field. On the HEAD kernel, I've only
> been able to reproduce this by inserting a udelay(1) in fsnotify().
> However, I've been able to reproduce this issue without inserting a
> udelay(1) anywhere on older unmodified release kernels, so I believe
> it's worth fixing at HEAD.
> 
> Fixes bug 199437: https://bugzilla.kernel.org/show_bug.cgi?id=199437
> 
> Fixes: 08991e83b7286635167bab40927665a90fb00d81
> Signed-off-by: Robert Kolchmeyer <rkolchmeyer@google.com>

Thanks! I've added the patch to my tree and will push it to Linus for -rc3.

								Honza

> ---
> Changelog since v1:
> - Solve problem by moving 'destroy_next' field into a different union instead
>   of calling synchronize_srcu().
> 
>  include/linux/fsnotify_backend.h | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
> index 9f1edb92c97e..a3d13d874fd1 100644
> --- a/include/linux/fsnotify_backend.h
> +++ b/include/linux/fsnotify_backend.h
> @@ -217,12 +217,10 @@ struct fsnotify_mark_connector {
>  	union {	/* Object pointer [lock] */
>  		struct inode *inode;
>  		struct vfsmount *mnt;
> -	};
> -	union {
> -		struct hlist_head list;
>  		/* Used listing heads to free after srcu period expires */
>  		struct fsnotify_mark_connector *destroy_next;
>  	};
> +	struct hlist_head list;
>  };
>  
>  /*
> -- 
> 2.17.0.484.g0c8726318c-goog
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR