Messages in this thread | | | From | Linus Torvalds <> | Date | Fri, 12 Jun 2020 13:16:24 -0700 | Subject | Re: [GIT PULL] proc fixes v2 for v5.8-rc1 |
| |
On Fri, Jun 12, 2020 at 1:06 PM Eric W. Biederman <ebiederm@xmission.com> wrote: > > I have a sense that a use after free that anyone can trigger could be a > bit dangerous, and despite not being the only virtual filesystem in the > kernel proc is the only virtual filesystem that called new_inode_pseudo.
So the reason I pulled that change despite my questions was that I do agree with the whole "there's probably little point to use new_inode_pseudo() here" argument.
But at the same time, I ghet the feeling that this partly just is papering over the problem. If fsnotify causes problems with a new_inode_pseudo() inode, then fsnotify should be _checking_ for that case.
And if fsnotify were to check for it, then the reason for /proc to use it would largely go away. Maybe the debug check for umount matters, but honestly it doesn't really seem to be a big deal.
A pseudo-inode is basically independent of the filesystem it was mounted as, so the generic_shutdown_super() check not triggering for them is sensible, I feel.
But yeah, we could also make the rule go the other way, and simply make sure that "new_inode_pseudo()" itself checks that the super-block you give it is something fundamenally unmountable and was created with 'kern_mount()'.
That would have also figured out that the /proc case was broken.
So the main objection I have here is really that this fix feels incomplete, and doesn't really reflect the underlying issue, just fixes the symptom.
Either the underlying issue is that you shouldn't call 'fsnotify' on /proc, or the underlying issue is that /proc was using a bad inode and nobody even noticed until the fsnotify issue.
This is not a huge deal. I think you've fixed the bug, I just have this itch that the thing that triggered it shouldn't have happened in the first place.
Linus
| |