Re: Patch "fanotify: Release SRCU lock when waiting for userspace response" has been added to the 4.9-stable tree
On Fri, Apr 12, 2019 at 10:44:12AM +0200, Jan Kara wrote:
On Thu 11-04-19 11:26:27, Sasha Levin wrote:
This is a note to let you know that I've just added the patch titled
fanotify: Release SRCU lock when waiting for userspace responseto the 4.9-stable tree which can be found at: http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git%3Ba=su...
The filename of the patch is: fanotify-release-srcu-lock-when-waiting-for-userspac.patch and it can be found in the queue-4.9 subdirectory.
If you, or anyone else, feels it should not be added to the stable tree, please let stable@vger.kernel.org know about it.
I'd be careful with this series. You're missing at least the fixup series from Miklos culminating in f37650f1c7c7 "fanotify: fix fsnotify_prepare_user_wait() failure". And you seem to be missing also quite some prerequisites reworking lifetime of fsnotify marks (series culminating with f09b04a03e0 "fsnotify: Remove special handling of mark destruction on group shutdown"). So you're just introducing subtle use-after-free issues to fanotify code. Overall I think the chances for regressions here are much bigger than the problem you'll be fixing unless you'll go for something like wholesale update of fs/notify/* to state in f37650f1c7c7.
I've pulled this series based on the request here: https://lore.kernel.org/stable/20190411032430.17353-1-matthew.ruffell@canoni...
There are a few reasons why I'd prefer to keep it in:
1. It fixes a very real bug which has affected quite a few of our customers as well, so (at least for me) this is more than a minor bugfix.
2. It came with a straightforward testcase.
3. Given that Canonical pulled it in as well, it (hopefully) received more testing than some other random patches.
If there are missing patches here I'd be happy to take them in and re-test the kernel, but I'd really like to avoid *not* taking these patches just because we fear a regression but can't show it.
-- Thanks, Sasha
On Fri 12-04-19 10:43:44, Sasha Levin wrote:
On Fri, Apr 12, 2019 at 10:44:12AM +0200, Jan Kara wrote:
On Thu 11-04-19 11:26:27, Sasha Levin wrote:
This is a note to let you know that I've just added the patch titled
fanotify: Release SRCU lock when waiting for userspace responseto the 4.9-stable tree which can be found at: http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git%3Ba=su...
The filename of the patch is: fanotify-release-srcu-lock-when-waiting-for-userspac.patch and it can be found in the queue-4.9 subdirectory.
If you, or anyone else, feels it should not be added to the stable tree, please let stable@vger.kernel.org know about it.
I'd be careful with this series. You're missing at least the fixup series from Miklos culminating in f37650f1c7c7 "fanotify: fix fsnotify_prepare_user_wait() failure". And you seem to be missing also quite some prerequisites reworking lifetime of fsnotify marks (series culminating with f09b04a03e0 "fsnotify: Remove special handling of mark destruction on group shutdown"). So you're just introducing subtle use-after-free issues to fanotify code. Overall I think the chances for regressions here are much bigger than the problem you'll be fixing unless you'll go for something like wholesale update of fs/notify/* to state in f37650f1c7c7.
I've pulled this series based on the request here: https://lore.kernel.org/stable/20190411032430.17353-1-matthew.ruffell@canoni...
Thanks for reference! I've added Matthew into CC so that he's aware of the potential problems with the patches they backported.
There are a few reasons why I'd prefer to keep it in:
- It fixes a very real bug which has affected quite a few of our
customers as well, so (at least for me) this is more than a minor bugfix.
I have my reservations about it being a kernel bug :) Primarily, it is a problem in userspace not responding to fanotify permission events properly. With these patches, the misbehaving application will take down just the filesystem it is working on (processes blocked in D state), without them it will take down the whole machine. So sure the patches improve the situation but more often than not you have to reboot anyway.
And yes, it is bad that misbehaving userspace can take the kernel down rather easily but that's the problem with how fanotify permission events API has been designed and generally with the idea of AV vendors that they need to intercept and approve all writes / opens with their AV solution.
It came with a straightforward testcase.
Given that Canonical pulled it in as well, it (hopefully) received
more testing than some other random patches.
Sure, seeing the reference I don't blame you that you've included the patches.
If there are missing patches here I'd be happy to take them in and re-test the kernel, but I'd really like to avoid *not* taking these patches just because we fear a regression but can't show it.
So the three patches as you took them are definitely wrong because they introduce use after free issues. Ubuntu guys have backported the part that takes care of dropping SRCU when waiting for response but didn't backport the part that makes sure fanotify marks, inodes, and mounts stay allocated while we are waiting. This could be even exploitable as attacker can force inode eviction via rm(1). So please don't include them as they are into -stable.
Matthew, if you really want to backport the patches changing how fanotify uses SRCU (and honestly I'm not convinced you have to since without fixing the AV applications the system will not work good anyway), you have to also backport the series 5198adf649a0 "fsnotify: Remove unnecessary tests when showing fdinfo" .. f09b04a03e0 "fsnotify: Remove special handling of mark destruction on group shutdown") - yes, it is big and it completely reworks lifetime of notification marks and their inode / mount references in the kernel. And then as a cherry on top you also need to backport followup fixes 24c20305c7f "fsnotify: clean up fsnotify_prepare/finish_user_wait()" .. f37650f1c7c7 "fanotify: fix fsnotify_prepare_user_wait() failure". And as a warning these are only the prerequisites I'm aware of. Given the amount of patches, I might have easily forgotten about something.
Honza
On Mon, Apr 15, 2019 at 11:08:25AM +0200, Jan Kara wrote:
On Fri 12-04-19 10:43:44, Sasha Levin wrote:
On Fri, Apr 12, 2019 at 10:44:12AM +0200, Jan Kara wrote:
On Thu 11-04-19 11:26:27, Sasha Levin wrote:
This is a note to let you know that I've just added the patch titled
fanotify: Release SRCU lock when waiting for userspace responseto the 4.9-stable tree which can be found at: http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git%3Ba=su...
The filename of the patch is: fanotify-release-srcu-lock-when-waiting-for-userspac.patch and it can be found in the queue-4.9 subdirectory.
If you, or anyone else, feels it should not be added to the stable tree, please let stable@vger.kernel.org know about it.
I'd be careful with this series. You're missing at least the fixup series from Miklos culminating in f37650f1c7c7 "fanotify: fix fsnotify_prepare_user_wait() failure". And you seem to be missing also quite some prerequisites reworking lifetime of fsnotify marks (series culminating with f09b04a03e0 "fsnotify: Remove special handling of mark destruction on group shutdown"). So you're just introducing subtle use-after-free issues to fanotify code. Overall I think the chances for regressions here are much bigger than the problem you'll be fixing unless you'll go for something like wholesale update of fs/notify/* to state in f37650f1c7c7.
I've pulled this series based on the request here: https://lore.kernel.org/stable/20190411032430.17353-1-matthew.ruffell@canoni...
Thanks for reference! I've added Matthew into CC so that he's aware of the potential problems with the patches they backported.
There are a few reasons why I'd prefer to keep it in:
- It fixes a very real bug which has affected quite a few of our
customers as well, so (at least for me) this is more than a minor bugfix.
I have my reservations about it being a kernel bug :) Primarily, it is a problem in userspace not responding to fanotify permission events properly. With these patches, the misbehaving application will take down just the filesystem it is working on (processes blocked in D state), without them it will take down the whole machine. So sure the patches improve the situation but more often than not you have to reboot anyway.
And yes, it is bad that misbehaving userspace can take the kernel down rather easily but that's the problem with how fanotify permission events API has been designed and generally with the idea of AV vendors that they need to intercept and approve all writes / opens with their AV solution.
It came with a straightforward testcase.
Given that Canonical pulled it in as well, it (hopefully) received
more testing than some other random patches.
Sure, seeing the reference I don't blame you that you've included the patches.
If there are missing patches here I'd be happy to take them in and re-test the kernel, but I'd really like to avoid *not* taking these patches just because we fear a regression but can't show it.
So the three patches as you took them are definitely wrong because they introduce use after free issues. Ubuntu guys have backported the part that takes care of dropping SRCU when waiting for response but didn't backport the part that makes sure fanotify marks, inodes, and mounts stay allocated while we are waiting. This could be even exploitable as attacker can force inode eviction via rm(1). So please don't include them as they are into -stable.
Matthew, if you really want to backport the patches changing how fanotify uses SRCU (and honestly I'm not convinced you have to since without fixing the AV applications the system will not work good anyway), you have to also backport the series 5198adf649a0 "fsnotify: Remove unnecessary tests when showing fdinfo" .. f09b04a03e0 "fsnotify: Remove special handling of mark destruction on group shutdown") - yes, it is big and it completely reworks lifetime of notification marks and their inode / mount references in the kernel. And then as a cherry on top you also need to backport followup fixes 24c20305c7f "fsnotify: clean up fsnotify_prepare/finish_user_wait()" .. f37650f1c7c7 "fanotify: fix fsnotify_prepare_user_wait() failure". And as a warning these are only the prerequisites I'm aware of. Given the amount of patches, I might have easily forgotten about something.
Thank you for the detailed review and for catching this.
I've now dropped both of these series from the 4.4.y and 4.9.y trees.
Matthew, if you can fix these up properly, feel free to resend them.
thanks,
greg k-h
On 15/04/19 10:42 PM, Greg KH wrote:
On Mon, Apr 15, 2019 at 11:08:25AM +0200, Jan Kara wrote:
On Fri 12-04-19 10:43:44, Sasha Levin wrote:
On Fri, Apr 12, 2019 at 10:44:12AM +0200, Jan Kara wrote:
On Thu 11-04-19 11:26:27, Sasha Levin wrote:
This is a note to let you know that I've just added the patch titled
fanotify: Release SRCU lock when waiting for userspace responseto the 4.9-stable tree which can be found at: http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git%3Ba=su...
The filename of the patch is: fanotify-release-srcu-lock-when-waiting-for-userspac.patch and it can be found in the queue-4.9 subdirectory.
If you, or anyone else, feels it should not be added to the stable tree, please let stable@vger.kernel.org know about it.
I'd be careful with this series. You're missing at least the fixup series from Miklos culminating in f37650f1c7c7 "fanotify: fix fsnotify_prepare_user_wait() failure". And you seem to be missing also quite some prerequisites reworking lifetime of fsnotify marks (series culminating with f09b04a03e0 "fsnotify: Remove special handling of mark destruction on group shutdown"). So you're just introducing subtle use-after-free issues to fanotify code. Overall I think the chances for regressions here are much bigger than the problem you'll be fixing unless you'll go for something like wholesale update of fs/notify/* to state in f37650f1c7c7.
I've pulled this series based on the request here: https://lore.kernel.org/stable/20190411032430.17353-1-matthew.ruffell@canoni...
Thanks for reference! I've added Matthew into CC so that he's aware of the potential problems with the patches they backported.
There are a few reasons why I'd prefer to keep it in:
- It fixes a very real bug which has affected quite a few of our
customers as well, so (at least for me) this is more than a minor bugfix.
I have my reservations about it being a kernel bug :) Primarily, it is a problem in userspace not responding to fanotify permission events properly. With these patches, the misbehaving application will take down just the filesystem it is working on (processes blocked in D state), without them it will take down the whole machine. So sure the patches improve the situation but more often than not you have to reboot anyway.
And yes, it is bad that misbehaving userspace can take the kernel down rather easily but that's the problem with how fanotify permission events API has been designed and generally with the idea of AV vendors that they need to intercept and approve all writes / opens with their AV solution.
It came with a straightforward testcase.
Given that Canonical pulled it in as well, it (hopefully) received
more testing than some other random patches.
Sure, seeing the reference I don't blame you that you've included the patches.
If there are missing patches here I'd be happy to take them in and re-test the kernel, but I'd really like to avoid *not* taking these patches just because we fear a regression but can't show it.
So the three patches as you took them are definitely wrong because they introduce use after free issues. Ubuntu guys have backported the part that takes care of dropping SRCU when waiting for response but didn't backport the part that makes sure fanotify marks, inodes, and mounts stay allocated while we are waiting. This could be even exploitable as attacker can force inode eviction via rm(1). So please don't include them as they are into -stable.
Matthew, if you really want to backport the patches changing how fanotify uses SRCU (and honestly I'm not convinced you have to since without fixing the AV applications the system will not work good anyway), you have to also backport the series 5198adf649a0 "fsnotify: Remove unnecessary tests when showing fdinfo" .. f09b04a03e0 "fsnotify: Remove special handling of mark destruction on group shutdown") - yes, it is big and it completely reworks lifetime of notification marks and their inode / mount references in the kernel. And then as a cherry on top you also need to backport followup fixes 24c20305c7f "fsnotify: clean up fsnotify_prepare/finish_user_wait()" .. f37650f1c7c7 "fanotify: fix fsnotify_prepare_user_wait() failure". And as a warning these are only the prerequisites I'm aware of. Given the amount of patches, I might have easily forgotten about something.
Thank you for the detailed review and for catching this.
I've now dropped both of these series from the 4.4.y and 4.9.y trees.
Matthew, if you can fix these up properly, feel free to resend them.
thanks,
greg k-h
Jan, thank you for your detailed explanation and reasoning why just backporting those three commits is a bad idea.
I think I got a bit of tunnel vision trying to resolve the issue the customer was having, and when trying to stay within the -stable rules of keeping things small and to the point, I seemed to have accepted that those three commits were the answer, and did not consider the other commits on either side of those three commits, and the impact of not having them.
You have taught me that I need to take a moment and consider the whole picture and read around the commits on either side of the fix before creating patches, testing them and finding that the system no longer hangs, and accepting them as the answer.
If anyone is interested, I went and looked into the list of commits Jan mentioned, and if they need backporting for 4.9:
[ Fixup series] f37650f1c7c71cf5180b43229d13b421d81e7170 - cherry pick 9a31d7ad997f55768c687974ce36b759065b49e5 - cherry pick 0d6ec079d6aaa098b978d6395973bb027c752a03 - cherry pick 24c20305c7fc8959836211cb8c50aab93ae0e54f - cherry pick
[ Core series on dropping SRCU lock] 05f0e38724e8449184acd8fbf0473ee5a07adc6c - cherry pick 9385a84d7e1f658bb2d96ab798393e4b16268aaa - backport abc77577a669f424c5d0c185b9994f2621c52aa4 - cherry pick
[Mark lifetime series] f09b04a03e0239f65bd964a1de758e53cf6349e8 - cherry pick 6b3f05d24d355f50f3d9814304650fcab0efb482 - backport 11375145a70d69e871dd5b8fcadd5d1ee4162e7c - cherry pick e7253760587e8523fe1e8ede092a620f1403f2e8 - cherry pick 08991e83b7286635167bab40927665a90fb00d81 - cherry pick 04662cab59fc3e8421fd7a0539d304d51d2750a4 - cherry pick 2629718dd26f89e064dcdec6c8e5b9713502e1f8 - cherry pick 73cd3c33ab793325ebaae27fa58b4f713c16f12c - cherry pick 8212a6097a720896b4cdbe516487ad47f4296599 - cherry pick a03e2e4f078365428bb4317989cb5d1d6563cfe9 - cherry pick f06fd98759451876f51607f60abd74c89b141610 - cherry pick a242677bb1e6faa9bd82bd33afb2621071258231 - cherry pick 0810b4f9f207910d90aee56d312d25f334796363 - cherry pick 755b5bc681eb46de7bfaec196f85e30efd95bd9f - cherry pick e911d8af87dba7642138f4320ca3db80629989f2 - cherry pick 86ffe245c430f07f95d5d28d3b694ea72f4492e7 - backport 9dd813c15b2c101168808d4f5941a29985758973 - backport
[ Necessary infrastructure ] c97476400d3b73376fc055e828d7388d6b9ea99a - cherry pick 25c829afbd74fb9594d2351d9e41be05bacb9903 - cherry pick 5198adf649a0b7b0f9ddb98b264e57b41516116b - cherry pick e3ba730702af370563f66cb610b71aa0ca67955e - cherry pick
Now, this is starting to be a lot of commits, and 4.4 requires even more.
I think we might accept that this is far too major of a change for 4.4, 4.9 and there is probably a little too much risk of regression.
Greg, thanks for dropping the patches from 4.4.y and 4.9.y trees, I was starting to worry I might break some machines if they got through.
If anyone is still interested in a new patch series, I have some spare time which means I can keep working on this if needed, but for now, I think I will tell my customer to upgrade to the Ubuntu 4.15 LTS kernel, and move on.
For the record: the patches did not land in an Ubuntu kernel, since I never sent them to the Ubuntu kernel mailing list. I wanted some upstream feedback, and to try get this fixed for everyone. I am very happy with the feedback I got too =)
Anyway, I am happy that your review process works as intended, and this was caught early.
Thanks,
Matthew Ruffell
On Tue, Apr 16, 2019 at 05:11:08PM +1200, Matthew Ruffell wrote:
On 15/04/19 10:42 PM, Greg KH wrote:
On Mon, Apr 15, 2019 at 11:08:25AM +0200, Jan Kara wrote:
On Fri 12-04-19 10:43:44, Sasha Levin wrote:
On Fri, Apr 12, 2019 at 10:44:12AM +0200, Jan Kara wrote:
On Thu 11-04-19 11:26:27, Sasha Levin wrote:
This is a note to let you know that I've just added the patch titled
fanotify: Release SRCU lock when waiting for userspace responseto the 4.9-stable tree which can be found at: http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git%3Ba=su...
The filename of the patch is: fanotify-release-srcu-lock-when-waiting-for-userspac.patch and it can be found in the queue-4.9 subdirectory.
If you, or anyone else, feels it should not be added to the stable tree, please let stable@vger.kernel.org know about it.
I'd be careful with this series. You're missing at least the fixup series from Miklos culminating in f37650f1c7c7 "fanotify: fix fsnotify_prepare_user_wait() failure". And you seem to be missing also quite some prerequisites reworking lifetime of fsnotify marks (series culminating with f09b04a03e0 "fsnotify: Remove special handling of mark destruction on group shutdown"). So you're just introducing subtle use-after-free issues to fanotify code. Overall I think the chances for regressions here are much bigger than the problem you'll be fixing unless you'll go for something like wholesale update of fs/notify/* to state in f37650f1c7c7.
I've pulled this series based on the request here: https://lore.kernel.org/stable/20190411032430.17353-1-matthew.ruffell@canoni...
Thanks for reference! I've added Matthew into CC so that he's aware of the potential problems with the patches they backported.
There are a few reasons why I'd prefer to keep it in:
- It fixes a very real bug which has affected quite a few of our
customers as well, so (at least for me) this is more than a minor bugfix.
I have my reservations about it being a kernel bug :) Primarily, it is a problem in userspace not responding to fanotify permission events properly. With these patches, the misbehaving application will take down just the filesystem it is working on (processes blocked in D state), without them it will take down the whole machine. So sure the patches improve the situation but more often than not you have to reboot anyway.
And yes, it is bad that misbehaving userspace can take the kernel down rather easily but that's the problem with how fanotify permission events API has been designed and generally with the idea of AV vendors that they need to intercept and approve all writes / opens with their AV solution.
It came with a straightforward testcase.
Given that Canonical pulled it in as well, it (hopefully) received
more testing than some other random patches.
Sure, seeing the reference I don't blame you that you've included the patches.
If there are missing patches here I'd be happy to take them in and re-test the kernel, but I'd really like to avoid *not* taking these patches just because we fear a regression but can't show it.
So the three patches as you took them are definitely wrong because they introduce use after free issues. Ubuntu guys have backported the part that takes care of dropping SRCU when waiting for response but didn't backport the part that makes sure fanotify marks, inodes, and mounts stay allocated while we are waiting. This could be even exploitable as attacker can force inode eviction via rm(1). So please don't include them as they are into -stable.
Matthew, if you really want to backport the patches changing how fanotify uses SRCU (and honestly I'm not convinced you have to since without fixing the AV applications the system will not work good anyway), you have to also backport the series 5198adf649a0 "fsnotify: Remove unnecessary tests when showing fdinfo" .. f09b04a03e0 "fsnotify: Remove special handling of mark destruction on group shutdown") - yes, it is big and it completely reworks lifetime of notification marks and their inode / mount references in the kernel. And then as a cherry on top you also need to backport followup fixes 24c20305c7f "fsnotify: clean up fsnotify_prepare/finish_user_wait()" .. f37650f1c7c7 "fanotify: fix fsnotify_prepare_user_wait() failure". And as a warning these are only the prerequisites I'm aware of. Given the amount of patches, I might have easily forgotten about something.
Thank you for the detailed review and for catching this.
I've now dropped both of these series from the 4.4.y and 4.9.y trees.
Matthew, if you can fix these up properly, feel free to resend them.
thanks,
greg k-h
Jan, thank you for your detailed explanation and reasoning why just backporting those three commits is a bad idea.
I think I got a bit of tunnel vision trying to resolve the issue the customer was having, and when trying to stay within the -stable rules of keeping things small and to the point, I seemed to have accepted that those three commits were the answer, and did not consider the other commits on either side of those three commits, and the impact of not having them.
You have taught me that I need to take a moment and consider the whole picture and read around the commits on either side of the fix before creating patches, testing them and finding that the system no longer hangs, and accepting them as the answer.
If anyone is interested, I went and looked into the list of commits Jan mentioned, and if they need backporting for 4.9:
[ Fixup series] f37650f1c7c71cf5180b43229d13b421d81e7170 - cherry pick 9a31d7ad997f55768c687974ce36b759065b49e5 - cherry pick 0d6ec079d6aaa098b978d6395973bb027c752a03 - cherry pick 24c20305c7fc8959836211cb8c50aab93ae0e54f - cherry pick
[ Core series on dropping SRCU lock] 05f0e38724e8449184acd8fbf0473ee5a07adc6c - cherry pick 9385a84d7e1f658bb2d96ab798393e4b16268aaa - backport abc77577a669f424c5d0c185b9994f2621c52aa4 - cherry pick
[Mark lifetime series] f09b04a03e0239f65bd964a1de758e53cf6349e8 - cherry pick 6b3f05d24d355f50f3d9814304650fcab0efb482 - backport 11375145a70d69e871dd5b8fcadd5d1ee4162e7c - cherry pick e7253760587e8523fe1e8ede092a620f1403f2e8 - cherry pick 08991e83b7286635167bab40927665a90fb00d81 - cherry pick 04662cab59fc3e8421fd7a0539d304d51d2750a4 - cherry pick 2629718dd26f89e064dcdec6c8e5b9713502e1f8 - cherry pick 73cd3c33ab793325ebaae27fa58b4f713c16f12c - cherry pick 8212a6097a720896b4cdbe516487ad47f4296599 - cherry pick a03e2e4f078365428bb4317989cb5d1d6563cfe9 - cherry pick f06fd98759451876f51607f60abd74c89b141610 - cherry pick a242677bb1e6faa9bd82bd33afb2621071258231 - cherry pick 0810b4f9f207910d90aee56d312d25f334796363 - cherry pick 755b5bc681eb46de7bfaec196f85e30efd95bd9f - cherry pick e911d8af87dba7642138f4320ca3db80629989f2 - cherry pick 86ffe245c430f07f95d5d28d3b694ea72f4492e7 - backport 9dd813c15b2c101168808d4f5941a29985758973 - backport
[ Necessary infrastructure ] c97476400d3b73376fc055e828d7388d6b9ea99a - cherry pick 25c829afbd74fb9594d2351d9e41be05bacb9903 - cherry pick 5198adf649a0b7b0f9ddb98b264e57b41516116b - cherry pick e3ba730702af370563f66cb610b71aa0ca67955e - cherry pick
Now, this is starting to be a lot of commits, and 4.4 requires even more.
I think we might accept that this is far too major of a change for 4.4, 4.9 and there is probably a little too much risk of regression.
I agree.
Greg, thanks for dropping the patches from 4.4.y and 4.9.y trees, I was starting to worry I might break some machines if they got through.
If anyone is still interested in a new patch series, I have some spare time which means I can keep working on this if needed, but for now, I think I will tell my customer to upgrade to the Ubuntu 4.15 LTS kernel, and move on.
That is a good idea.
thanks,
greg k-h
linux-stable-mirror@lists.linaro.org
-
Greg KH -
Jan Kara -
Matthew Ruffell -
Sasha Levin