Messages in this thread | | | Date | Fri, 28 Apr 2023 17:41:33 +0200 | From | David Hildenbrand <> | Subject | Re: [PATCH v5] mm/gup: disallow GUP writing to file-backed mappings by default |
| |
On 28.04.23 17:27, Jason Gunthorpe wrote: > On Fri, Apr 28, 2023 at 05:08:27PM +0200, David Hildenbrand wrote: > >>> I think this is broken today and we should block it. We know from >>> experiments with RDMA that doing exactly this triggers kernel oop's. >> >> I never saw similar reports in the wild (especially targeted at RHEL), so is >> this still a current issue that has not been mitigated? Or is it just so >> hard to actually trigger? > > People send RDMA related bug reports to us, and we tell them not to do > this stuff :) > >>> I'm skeptical that anyone can actually do this combination of things >>> successfully without getting kernel crashes or file data corruption - >>> ie there is no real user to break. >> >> I am pretty sure that there are such VM users, because on the libvirt level >> it's completely unclear which features trigger what behavior :/ > > IDK, why on earth would anyone want to do this? Using VFIO forces all > the memory to become resident so what was the point of making it file > backed in the first place?
As I said, copy-and paste, incremental changes to domain XMLs. I've seen some crazy domain XMLs in bug reports.
> > I'm skeptical there are real users even if it now requires special > steps to be crashy/corrupty.
In any case, I think we should document the possible implications of this patch. I gave one use case that could be broken.
> >>>> Sure, we could warn, or convert individual users using a flag (io_uring). >>>> But maybe we should invest more energy on a fix? >>> >>> It has been years now, I think we need to admit a fix is still years >>> away. Blocking the security problem may even motivate more people to >>> work on a fix. >> >> Maybe we should make this a topic this year at LSF/MM (again?). At least we >> learned a lot about GUP, what might work, what might not work, and got a >> depper understanding (+ motivation to fix? :) ) the issue at hand. > > We keep having the topic.. This is the old argument that the FS people > say the MM isn't following its inode and dirty lifetime rules and the > MM people say the FS isn't following its refcounting rules <shrug>
:/ so we have to discuss it ... again I guess.
> >>> Security is the primary case where we have historically closed uAPI >>> items. >> >> As this patch >> >> 1) Does not tackle GUP-fast >> 2) Does not take care of !FOLL_LONGTERM >> >> I am not convinced by the security argument in regard to this patch. > > It is incremental and a temperature check to see what kind of real > users exist. We have no idea right now, just speculation.
Right, but again, if we start talking about security it's a different thing IMHO.
>> Everything else sounds like band-aids to me, is insufficient, and might >> cause more harm than actually help IMHO. Especially the gup-fast case is >> extremely easy to work-around in malicious user space. > > It is true this patch should probably block gup_fast when using > FOLL_LONGTERM as well, just like we used to do for the DAX check.
Then we'd at least fix the security issue for all FOLL_LONGTERM completely.
-- Thanks,
David / dhildenb
| |