[Cosmic][PATCH 0/1] fsnotify: Fix fsnotify_mark_connector race

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

[Cosmic][PATCH 0/1] fsnotify: Fix fsnotify_mark_connector race

Joseph Salisbury-3
BugLink: http://bugs.launchpad.net/bugs/1771075

== Cosmic Justification ==
A regression was introduced by commit 08991e83b728.  This regression
causes problems on hosts that make heavy use of inotify and causes panics / lockups
in the kernel in inotify-related code.

This regression is fixed buy commit d90a10e2444b as of v4.17-rc3.  Commit d90a10e2444b
was cc'd to stable and is already in Artful and Bionic but not Cosmic.
The commit was added to A and B by bug 1765564.

== Fix ==
d90a10e2444b ("fsnotify: Fix fsnotify_mark_connector race")

== Regression Potential ==
Low.  Fixes a current regression. The patch was cc'd to stable, so it has had
additional upstream review.  This commit is already applied to Artful
and Bionic.

== Test Case ==
A test kernel was built with this patch and tested by the original bug reporter.
The bug reporter states the test kernel resolved the bug.

Robert Kolchmeyer (1):
  fsnotify: Fix fsnotify_mark_connector race

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

--
2.7.4


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[Cosmic][PATCH 1/1] fsnotify: Fix fsnotify_mark_connector race

Joseph Salisbury-3
From: Robert Kolchmeyer <[hidden email]>

BugLink: http://bugs.launchpad.net/bugs/1771075

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.

References: https://bugzilla.kernel.org/show_bug.cgi?id=199437
Fixes: 08991e83b7286635167bab40927665a90fb00d81
CC: [hidden email]
Signed-off-by: Robert Kolchmeyer <[hidden email]>
Signed-off-by: Jan Kara <[hidden email]>
(cherry picked from commit d90a10e2444ba5a351fa695917258ff4c5709fa5)
Signed-off-by: Joseph Salisbury <[hidden email]>
---
 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 c6c6931..3504ac3 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -216,12 +216,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.7.4


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

Re: [Cosmic][PATCH 1/1] fsnotify: Fix fsnotify_mark_connector race

Joshua R. Poulson
I referred to this same issue and fix in
https://bugs.launchpad.net/ubuntu/+source/linux-azure/+bug/1765564

On Wed, May 23, 2018 at 11:27 AM, Joseph Salisbury
<[hidden email]> wrote:

> From: Robert Kolchmeyer <[hidden email]>
>
> BugLink: http://bugs.launchpad.net/bugs/1771075
>
> 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.
>
> References: https://bugzilla.kernel.org/show_bug.cgi?id=199437
> Fixes: 08991e83b7286635167bab40927665a90fb00d81
> CC: [hidden email]
> Signed-off-by: Robert Kolchmeyer <[hidden email]>
> Signed-off-by: Jan Kara <[hidden email]>
> (cherry picked from commit d90a10e2444ba5a351fa695917258ff4c5709fa5)
> Signed-off-by: Joseph Salisbury <[hidden email]>
> ---
>  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 c6c6931..3504ac3 100644
> --- a/include/linux/fsnotify_backend.h
> +++ b/include/linux/fsnotify_backend.h
> @@ -216,12 +216,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.7.4
>
>
> --
> kernel-team mailing list
> [hidden email]
> https://lists.ubuntu.com/mailman/listinfo/kernel-team

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

Re: [Cosmic][PATCH 0/1] fsnotify: Fix fsnotify_mark_connector race

Joseph Salisbury-3
In reply to this post by Joseph Salisbury-3
On 05/23/2018 02:27 PM, Joseph Salisbury wrote:

> BugLink: http://bugs.launchpad.net/bugs/1771075
>
> == Cosmic Justification ==
> A regression was introduced by commit 08991e83b728.  This regression
> causes problems on hosts that make heavy use of inotify and causes panics / lockups
> in the kernel in inotify-related code.
>
> This regression is fixed buy commit d90a10e2444b as of v4.17-rc3.  Commit d90a10e2444b
> was cc'd to stable and is already in Artful and Bionic but not Cosmic.
> The commit was added to A and B by bug 1765564.
>
> == Fix ==
> d90a10e2444b ("fsnotify: Fix fsnotify_mark_connector race")
>
> == Regression Potential ==
> Low.  Fixes a current regression. The patch was cc'd to stable, so it has had
> additional upstream review.  This commit is already applied to Artful
> and Bionic.
>
> == Test Case ==
> A test kernel was built with this patch and tested by the original bug reporter.
> The bug reporter states the test kernel resolved the bug.
>
> Robert Kolchmeyer (1):
>   fsnotify: Fix fsnotify_mark_connector race
>
>  include/linux/fsnotify_backend.h | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
This patch is also being requested in linux-gcp.  Should I send a v2, so
cosmic and linux-gcp are both listed in the subject?


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

Re: [Cosmic][PATCH 0/1] fsnotify: Fix fsnotify_mark_connector race

Thadeu Lima de Souza Cascardo-3
On Thu, May 24, 2018 at 07:32:53AM -0400, Joseph Salisbury wrote:

> On 05/23/2018 02:27 PM, Joseph Salisbury wrote:
> > BugLink: http://bugs.launchpad.net/bugs/1771075
> >
> > == Cosmic Justification ==
> > A regression was introduced by commit 08991e83b728.  This regression
> > causes problems on hosts that make heavy use of inotify and causes panics / lockups
> > in the kernel in inotify-related code.
> >
> > This regression is fixed buy commit d90a10e2444b as of v4.17-rc3.  Commit d90a10e2444b
> > was cc'd to stable and is already in Artful and Bionic but not Cosmic.
> > The commit was added to A and B by bug 1765564.
> >
> > == Fix ==
> > d90a10e2444b ("fsnotify: Fix fsnotify_mark_connector race")
> >
> > == Regression Potential ==
> > Low.  Fixes a current regression. The patch was cc'd to stable, so it has had
> > additional upstream review.  This commit is already applied to Artful
> > and Bionic.
> >
> > == Test Case ==
> > A test kernel was built with this patch and tested by the original bug reporter.
> > The bug reporter states the test kernel resolved the bug.
> >
> > Robert Kolchmeyer (1):
> >   fsnotify: Fix fsnotify_mark_connector race
> >
> >  include/linux/fsnotify_backend.h | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> >
> This patch is also being requested in linux-gcp.  Should I send a v2, so
> cosmic and linux-gcp are both listed in the subject?
>

Is this really targeting cosmic? Or bionic? Though cosmic is still carrying the
bionic kernel, it will soon have a 4.17 based kernel, which already has that
commit.

Cascardo.

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

Re: [Cosmic][PATCH 0/1] fsnotify: Fix fsnotify_mark_connector race

Joseph Salisbury-3
On 05/24/2018 07:59 AM, Thadeu Lima de Souza Cascardo wrote:

> On Thu, May 24, 2018 at 07:32:53AM -0400, Joseph Salisbury wrote:
>> On 05/23/2018 02:27 PM, Joseph Salisbury wrote:
>>> BugLink: http://bugs.launchpad.net/bugs/1771075
>>>
>>> == Cosmic Justification ==
>>> A regression was introduced by commit 08991e83b728.  This regression
>>> causes problems on hosts that make heavy use of inotify and causes panics / lockups
>>> in the kernel in inotify-related code.
>>>
>>> This regression is fixed buy commit d90a10e2444b as of v4.17-rc3.  Commit d90a10e2444b
>>> was cc'd to stable and is already in Artful and Bionic but not Cosmic.
>>> The commit was added to A and B by bug 1765564.
>>>
>>> == Fix ==
>>> d90a10e2444b ("fsnotify: Fix fsnotify_mark_connector race")
>>>
>>> == Regression Potential ==
>>> Low.  Fixes a current regression. The patch was cc'd to stable, so it has had
>>> additional upstream review.  This commit is already applied to Artful
>>> and Bionic.
>>>
>>> == Test Case ==
>>> A test kernel was built with this patch and tested by the original bug reporter.
>>> The bug reporter states the test kernel resolved the bug.
>>>
>>> Robert Kolchmeyer (1):
>>>   fsnotify: Fix fsnotify_mark_connector race
>>>
>>>  include/linux/fsnotify_backend.h | 4 +---
>>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>>
>> This patch is also being requested in linux-gcp.  Should I send a v2, so
>> cosmic and linux-gcp are both listed in the subject?
>>
> Is this really targeting cosmic? Or bionic? Though cosmic is still carrying the
> bionic kernel, it will soon have a 4.17 based kernel, which already has that
> commit.
>
> Cascardo.
It is targeting cosmic(And linux-gce).  Bionic is getting the commit via
bug 1765564, but that bug does not have cosmic as a bug task(And cosmic
was not requested in the SRU for that bug).  Cosmic will get this commit
when it is rebased, but I was not sure when that would happen.  I
figured I'd send a request to have the commit in cosmic in the mean time. 

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

Re: [Cosmic][PATCH 0/1] fsnotify: Fix fsnotify_mark_connector race

Seth Forshee
On Thu, May 24, 2018 at 10:39:05AM -0400, Joseph Salisbury wrote:

> On 05/24/2018 07:59 AM, Thadeu Lima de Souza Cascardo wrote:
> > On Thu, May 24, 2018 at 07:32:53AM -0400, Joseph Salisbury wrote:
> >> On 05/23/2018 02:27 PM, Joseph Salisbury wrote:
> >>> BugLink: http://bugs.launchpad.net/bugs/1771075
> >>>
> >>> == Cosmic Justification ==
> >>> A regression was introduced by commit 08991e83b728.  This regression
> >>> causes problems on hosts that make heavy use of inotify and causes panics / lockups
> >>> in the kernel in inotify-related code.
> >>>
> >>> This regression is fixed buy commit d90a10e2444b as of v4.17-rc3.  Commit d90a10e2444b
> >>> was cc'd to stable and is already in Artful and Bionic but not Cosmic.
> >>> The commit was added to A and B by bug 1765564.
> >>>
> >>> == Fix ==
> >>> d90a10e2444b ("fsnotify: Fix fsnotify_mark_connector race")
> >>>
> >>> == Regression Potential ==
> >>> Low.  Fixes a current regression. The patch was cc'd to stable, so it has had
> >>> additional upstream review.  This commit is already applied to Artful
> >>> and Bionic.
> >>>
> >>> == Test Case ==
> >>> A test kernel was built with this patch and tested by the original bug reporter.
> >>> The bug reporter states the test kernel resolved the bug.
> >>>
> >>> Robert Kolchmeyer (1):
> >>>   fsnotify: Fix fsnotify_mark_connector race
> >>>
> >>>  include/linux/fsnotify_backend.h | 4 +---
> >>>  1 file changed, 1 insertion(+), 3 deletions(-)
> >>>
> >> This patch is also being requested in linux-gcp.  Should I send a v2, so
> >> cosmic and linux-gcp are both listed in the subject?
> >>
> > Is this really targeting cosmic? Or bionic? Though cosmic is still carrying the
> > bionic kernel, it will soon have a 4.17 based kernel, which already has that
> > commit.
> >
> > Cascardo.
> It is targeting cosmic(And linux-gce).  Bionic is getting the commit via
> bug 1765564, but that bug does not have cosmic as a bug task(And cosmic
> was not requested in the SRU for that bug).  Cosmic will get this commit
> when it is rebased, but I was not sure when that would happen.  I
> figured I'd send a request to have the commit in cosmic in the mean time. 

I think the point is that there is no cosmic kernel distinct from the
bionic kernel yet, so it's ambiguous when you say "cosmic" whether you
mean the bionic kernel that gets copied forward to cosmic or the
unstable kernel which will eventually become the cosmic kernel.

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

Re: [Cosmic][PATCH 0/1] fsnotify: Fix fsnotify_mark_connector race

Joseph Salisbury-3
On 05/24/2018 11:03 AM, Seth Forshee wrote:

> On Thu, May 24, 2018 at 10:39:05AM -0400, Joseph Salisbury wrote:
>> On 05/24/2018 07:59 AM, Thadeu Lima de Souza Cascardo wrote:
>>> On Thu, May 24, 2018 at 07:32:53AM -0400, Joseph Salisbury wrote:
>>>> On 05/23/2018 02:27 PM, Joseph Salisbury wrote:
>>>>> BugLink: http://bugs.launchpad.net/bugs/1771075
>>>>>
>>>>> == Cosmic Justification ==
>>>>> A regression was introduced by commit 08991e83b728.  This regression
>>>>> causes problems on hosts that make heavy use of inotify and causes panics / lockups
>>>>> in the kernel in inotify-related code.
>>>>>
>>>>> This regression is fixed buy commit d90a10e2444b as of v4.17-rc3.  Commit d90a10e2444b
>>>>> was cc'd to stable and is already in Artful and Bionic but not Cosmic.
>>>>> The commit was added to A and B by bug 1765564.
>>>>>
>>>>> == Fix ==
>>>>> d90a10e2444b ("fsnotify: Fix fsnotify_mark_connector race")
>>>>>
>>>>> == Regression Potential ==
>>>>> Low.  Fixes a current regression. The patch was cc'd to stable, so it has had
>>>>> additional upstream review.  This commit is already applied to Artful
>>>>> and Bionic.
>>>>>
>>>>> == Test Case ==
>>>>> A test kernel was built with this patch and tested by the original bug reporter.
>>>>> The bug reporter states the test kernel resolved the bug.
>>>>>
>>>>> Robert Kolchmeyer (1):
>>>>>   fsnotify: Fix fsnotify_mark_connector race
>>>>>
>>>>>  include/linux/fsnotify_backend.h | 4 +---
>>>>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>>>>
>>>> This patch is also being requested in linux-gcp.  Should I send a v2, so
>>>> cosmic and linux-gcp are both listed in the subject?
>>>>
>>> Is this really targeting cosmic? Or bionic? Though cosmic is still carrying the
>>> bionic kernel, it will soon have a 4.17 based kernel, which already has that
>>> commit.
>>>
>>> Cascardo.
>> It is targeting cosmic(And linux-gce).  Bionic is getting the commit via
>> bug 1765564, but that bug does not have cosmic as a bug task(And cosmic
>> was not requested in the SRU for that bug).  Cosmic will get this commit
>> when it is rebased, but I was not sure when that would happen.  I
>> figured I'd send a request to have the commit in cosmic in the mean time. 
> I think the point is that there is no cosmic kernel distinct from the
> bionic kernel yet, so it's ambiguous when you say "cosmic" whether you
> mean the bionic kernel that gets copied forward to cosmic or the
> unstable kernel which will eventually become the cosmic kernel.

Ahh, Ok.  So if Bionic fixes get carried forward, then this can probably
be NAK'd.


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

NACK: [Cosmic][PATCH 0/1] fsnotify: Fix fsnotify_mark_connector race

Khaled Elmously
On 2018-05-24 11:08:10 , Joseph Salisbury wrote:

> On 05/24/2018 11:03 AM, Seth Forshee wrote:
> > On Thu, May 24, 2018 at 10:39:05AM -0400, Joseph Salisbury wrote:
> >> On 05/24/2018 07:59 AM, Thadeu Lima de Souza Cascardo wrote:
> >>> On Thu, May 24, 2018 at 07:32:53AM -0400, Joseph Salisbury wrote:
> >>>> On 05/23/2018 02:27 PM, Joseph Salisbury wrote:
> >>>>> BugLink: http://bugs.launchpad.net/bugs/1771075
> >>>>>
> >>>>> == Cosmic Justification ==
> >>>>> A regression was introduced by commit 08991e83b728.  This regression
> >>>>> causes problems on hosts that make heavy use of inotify and causes panics / lockups
> >>>>> in the kernel in inotify-related code.
> >>>>>
> >>>>> This regression is fixed buy commit d90a10e2444b as of v4.17-rc3.  Commit d90a10e2444b
> >>>>> was cc'd to stable and is already in Artful and Bionic but not Cosmic.
> >>>>> The commit was added to A and B by bug 1765564.
> >>>>>
> >>>>> == Fix ==
> >>>>> d90a10e2444b ("fsnotify: Fix fsnotify_mark_connector race")
> >>>>>
> >>>>> == Regression Potential ==
> >>>>> Low.  Fixes a current regression. The patch was cc'd to stable, so it has had
> >>>>> additional upstream review.  This commit is already applied to Artful
> >>>>> and Bionic.
> >>>>>
> >>>>> == Test Case ==
> >>>>> A test kernel was built with this patch and tested by the original bug reporter.
> >>>>> The bug reporter states the test kernel resolved the bug.
> >>>>>
> >>>>> Robert Kolchmeyer (1):
> >>>>>   fsnotify: Fix fsnotify_mark_connector race
> >>>>>
> >>>>>  include/linux/fsnotify_backend.h | 4 +---
> >>>>>  1 file changed, 1 insertion(+), 3 deletions(-)
> >>>>>
> >>>> This patch is also being requested in linux-gcp.  Should I send a v2, so
> >>>> cosmic and linux-gcp are both listed in the subject?
> >>>>
> >>> Is this really targeting cosmic? Or bionic? Though cosmic is still carrying the
> >>> bionic kernel, it will soon have a 4.17 based kernel, which already has that
> >>> commit.
> >>>
> >>> Cascardo.
> >> It is targeting cosmic(And linux-gce).  Bionic is getting the commit via
> >> bug 1765564, but that bug does not have cosmic as a bug task(And cosmic
> >> was not requested in the SRU for that bug).  Cosmic will get this commit
> >> when it is rebased, but I was not sure when that would happen.  I
> >> figured I'd send a request to have the commit in cosmic in the mean time. 
> > I think the point is that there is no cosmic kernel distinct from the
> > bionic kernel yet, so it's ambiguous when you say "cosmic" whether you
> > mean the bionic kernel that gets copied forward to cosmic or the
> > unstable kernel which will eventually become the cosmic kernel.
>
> Ahh, Ok.  So if Bionic fixes get carried forward, then this can probably
> be NAK'd.
>
>

NACKing based on Joseph's last comment (same fix for bionic has been applied under 1765564).

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team