| Message ID | 20220725142048.30450-2-peterx@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/mprotect: Fix soft-dirty checks | expand |
Hi Peter and David, On 7/25/22 7:20 PM, Peter Xu wrote: > The check wanted to make sure when soft-dirty tracking is enabled we won't > grant write bit by accident, as a page fault is needed for dirty tracking. > The intention is correct but we didn't check it right because VM_SOFTDIRTY > set actually means soft-dirty tracking disabled. Fix it. [...] > +static inline bool vma_soft_dirty_enabled(struct vm_area_struct *vma) > +{ > + /* > + * NOTE: we must check this before VM_SOFTDIRTY on soft-dirty > + * enablements, because when without soft-dirty being compiled in, > + * VM_SOFTDIRTY is defined as 0x0, then !(vm_flags & VM_SOFTDIRTY) > + * will be constantly true. > + */ > + if (!IS_ENABLED(CONFIG_MEM_SOFT_DIRTY)) > + return false; > + > + /* > + * Soft-dirty is kind of special: its tracking is enabled when the > + * vma flags not set. > + */ > + return !(vma->vm_flags & VM_SOFTDIRTY); > +} I'm sorry. I'm unable to understand the inversion here. > its tracking is enabled when the vma flags not set. VM_SOFTDIRTY is set on the VMA when new VMA is allocated to mark is soft-dirty. When we write to clear_refs to clear soft-dirty bit, VM_SOFTDIRTY is cleared from the VMA as well. Then why do you say tracking is enabled when the vma flags not set? I'm missing some obvious thing. Maybe the meaning of tracking is to see if VM_SOFTDIRTY needs to be set. If VM_SOFTDIRTY is already set, tracking isn't needed. Can you give an example here?
On Sat, Nov 19, 2022 at 01:16:26AM +0500, Muhammad Usama Anjum wrote: > Hi Peter and David, Hi, Muhammad, > > On 7/25/22 7:20 PM, Peter Xu wrote: > > The check wanted to make sure when soft-dirty tracking is enabled we won't > > grant write bit by accident, as a page fault is needed for dirty tracking. > > The intention is correct but we didn't check it right because VM_SOFTDIRTY > > set actually means soft-dirty tracking disabled. Fix it. > [...] > > +static inline bool vma_soft_dirty_enabled(struct vm_area_struct *vma) > > +{ > > + /* > > + * NOTE: we must check this before VM_SOFTDIRTY on soft-dirty > > + * enablements, because when without soft-dirty being compiled in, > > + * VM_SOFTDIRTY is defined as 0x0, then !(vm_flags & VM_SOFTDIRTY) > > + * will be constantly true. > > + */ > > + if (!IS_ENABLED(CONFIG_MEM_SOFT_DIRTY)) > > + return false; > > + > > + /* > > + * Soft-dirty is kind of special: its tracking is enabled when the > > + * vma flags not set. > > + */ > > + return !(vma->vm_flags & VM_SOFTDIRTY); > > +} > I'm sorry. I'm unable to understand the inversion here. > > its tracking is enabled when the vma flags not set. > VM_SOFTDIRTY is set on the VMA when new VMA is allocated to mark is > soft-dirty. When we write to clear_refs to clear soft-dirty bit, > VM_SOFTDIRTY is cleared from the VMA as well. Then why do you say tracking > is enabled when the vma flags not set? Because only when 4>clear_refs happens would VM_SOFTDIRTY be cleared, and only until then the real tracking starts (by removing write bits on ptes). > I'm missing some obvious thing. Maybe the meaning of tracking is to see > if VM_SOFTDIRTY needs to be set. If VM_SOFTDIRTY is already set, tracking > isn't needed. Can you give an example here? If VM_SOFTDIRTY is set, pagemap will treat all pages as soft-dirty, please see pagemap_pmd_range(): if (vma->vm_flags & VM_SOFTDIRTY) flags |= PM_SOFT_DIRTY; So fundamentally it reports nothing useful when VM_SOFTDIRTY set. That's also why we need the clear_refs first before we can have anything useful. Feel free to reference to the doc page (admin-guide/mm/soft-dirty.rst): ---8<--- The soft-dirty is a bit on a PTE which helps to track which pages a task writes to. In order to do this tracking one should 1. Clear soft-dirty bits from the task's PTEs. This is done by writing "4" into the ``/proc/PID/clear_refs`` file of the task in question. 2. Wait some time. 3. Read soft-dirty bits from the PTEs. This is done by reading from the ``/proc/PID/pagemap``. The bit 55 of the 64-bit qword is the soft-dirty one. If set, the respective PTE was written to since step 1. ---8<--- The tracking starts at step 1, where is when the flag is cleared. Thanks,
Hi Peter, Thank you so much for replying. On 11/19/22 4:14 AM, Peter Xu wrote: > On Sat, Nov 19, 2022 at 01:16:26AM +0500, Muhammad Usama Anjum wrote: >> Hi Peter and David, > > Hi, Muhammad, > >> >> On 7/25/22 7:20 PM, Peter Xu wrote: >>> The check wanted to make sure when soft-dirty tracking is enabled we won't >>> grant write bit by accident, as a page fault is needed for dirty tracking. >>> The intention is correct but we didn't check it right because VM_SOFTDIRTY >>> set actually means soft-dirty tracking disabled. Fix it. >> [...] >>> +static inline bool vma_soft_dirty_enabled(struct vm_area_struct *vma) >>> +{ >>> + /* >>> + * NOTE: we must check this before VM_SOFTDIRTY on soft-dirty >>> + * enablements, because when without soft-dirty being compiled in, >>> + * VM_SOFTDIRTY is defined as 0x0, then !(vm_flags & VM_SOFTDIRTY) >>> + * will be constantly true. >>> + */ >>> + if (!IS_ENABLED(CONFIG_MEM_SOFT_DIRTY)) >>> + return false; >>> + >>> + /* >>> + * Soft-dirty is kind of special: its tracking is enabled when the >>> + * vma flags not set. >>> + */ >>> + return !(vma->vm_flags & VM_SOFTDIRTY); >>> +} >> I'm sorry. I'm unable to understand the inversion here. >>> its tracking is enabled when the vma flags not set. >> VM_SOFTDIRTY is set on the VMA when new VMA is allocated to mark is >> soft-dirty. When we write to clear_refs to clear soft-dirty bit, >> VM_SOFTDIRTY is cleared from the VMA as well. Then why do you say tracking >> is enabled when the vma flags not set? > > Because only when 4>clear_refs happens would VM_SOFTDIRTY be cleared, and > only until then the real tracking starts (by removing write bits on ptes). But even if the VM_SOFTDIRTY is set on the VMA, the individual pages are still marked soft-dirty. Both are independent. It means tracking is enabled all the time in individual pages. Only the soft-dirty bit status in individual page isn't significant if VM_SOFTDIRTY already is set. Right? > >> I'm missing some obvious thing. Maybe the meaning of tracking is to see >> if VM_SOFTDIRTY needs to be set. If VM_SOFTDIRTY is already set, tracking >> isn't needed. Can you give an example here? > > If VM_SOFTDIRTY is set, pagemap will treat all pages as soft-dirty, please > see pagemap_pmd_range(): > > if (vma->vm_flags & VM_SOFTDIRTY) > flags |= PM_SOFT_DIRTY; > > So fundamentally it reports nothing useful when VM_SOFTDIRTY set. That's > also why we need the clear_refs first before we can have anything useful. > > Feel free to reference to the doc page (admin-guide/mm/soft-dirty.rst): > > ---8<--- > The soft-dirty is a bit on a PTE which helps to track which pages a task > writes to. In order to do this tracking one should > > 1. Clear soft-dirty bits from the task's PTEs. > > This is done by writing "4" into the ``/proc/PID/clear_refs`` file of the > task in question. > > 2. Wait some time. > > 3. Read soft-dirty bits from the PTEs. > > This is done by reading from the ``/proc/PID/pagemap``. The bit 55 of the > 64-bit qword is the soft-dirty one. If set, the respective PTE was > written to since step 1. > ---8<--- > > The tracking starts at step 1, where is when the flag is cleared. > > Thanks, >
On Mon, Nov 21, 2022 at 07:57:05PM +0500, Muhammad Usama Anjum wrote: > Hi Peter, > > Thank you so much for replying. > > On 11/19/22 4:14 AM, Peter Xu wrote: > > On Sat, Nov 19, 2022 at 01:16:26AM +0500, Muhammad Usama Anjum wrote: > >> Hi Peter and David, > > > > Hi, Muhammad, > > > >> > >> On 7/25/22 7:20 PM, Peter Xu wrote: > >>> The check wanted to make sure when soft-dirty tracking is enabled we won't > >>> grant write bit by accident, as a page fault is needed for dirty tracking. > >>> The intention is correct but we didn't check it right because VM_SOFTDIRTY > >>> set actually means soft-dirty tracking disabled. Fix it. > >> [...] > >>> +static inline bool vma_soft_dirty_enabled(struct vm_area_struct *vma) > >>> +{ > >>> + /* > >>> + * NOTE: we must check this before VM_SOFTDIRTY on soft-dirty > >>> + * enablements, because when without soft-dirty being compiled in, > >>> + * VM_SOFTDIRTY is defined as 0x0, then !(vm_flags & VM_SOFTDIRTY) > >>> + * will be constantly true. > >>> + */ > >>> + if (!IS_ENABLED(CONFIG_MEM_SOFT_DIRTY)) > >>> + return false; > >>> + > >>> + /* > >>> + * Soft-dirty is kind of special: its tracking is enabled when the > >>> + * vma flags not set. > >>> + */ > >>> + return !(vma->vm_flags & VM_SOFTDIRTY); > >>> +} > >> I'm sorry. I'm unable to understand the inversion here. > >>> its tracking is enabled when the vma flags not set. > >> VM_SOFTDIRTY is set on the VMA when new VMA is allocated to mark is > >> soft-dirty. When we write to clear_refs to clear soft-dirty bit, > >> VM_SOFTDIRTY is cleared from the VMA as well. Then why do you say tracking > >> is enabled when the vma flags not set? > > > > Because only when 4>clear_refs happens would VM_SOFTDIRTY be cleared, and > > only until then the real tracking starts (by removing write bits on ptes). > But even if the VM_SOFTDIRTY is set on the VMA, the individual pages are > still marked soft-dirty. Both are independent. > > It means tracking is enabled all the time in individual pages. IMHO it depends on how we define "tracking enabled" - before clear_refs even if no pages written they'll also be reported as dirty, then the information is useless. > Only the soft-dirty bit status in individual page isn't significant if > VM_SOFTDIRTY already is set. Right? Yes. But I'd say it makes more sense to say "tracking enabled" if we really started tracking (by removing the write bits in ptes, otherwise we did nothing). When vma created we didn't track anything. I don't know the rational of why soft-dirty was defined like that. I think it's somehow related to the fact that we allow false positive dirty pages not false negative. IOW, it's a bug to leak a page being dirtied, but not vice versa if we report clean page dirty.
On 11/22/22 2:17 AM, Peter Xu wrote: > On Mon, Nov 21, 2022 at 07:57:05PM +0500, Muhammad Usama Anjum wrote: >> Hi Peter, >> >> Thank you so much for replying. >> >> On 11/19/22 4:14 AM, Peter Xu wrote: >>> On Sat, Nov 19, 2022 at 01:16:26AM +0500, Muhammad Usama Anjum wrote: >>>> Hi Peter and David, >>> >>> Hi, Muhammad, >>> >>>> >>>> On 7/25/22 7:20 PM, Peter Xu wrote: >>>>> The check wanted to make sure when soft-dirty tracking is enabled we won't >>>>> grant write bit by accident, as a page fault is needed for dirty tracking. >>>>> The intention is correct but we didn't check it right because VM_SOFTDIRTY >>>>> set actually means soft-dirty tracking disabled. Fix it. >>>> [...] >>>>> +static inline bool vma_soft_dirty_enabled(struct vm_area_struct *vma) >>>>> +{ >>>>> + /* >>>>> + * NOTE: we must check this before VM_SOFTDIRTY on soft-dirty >>>>> + * enablements, because when without soft-dirty being compiled in, >>>>> + * VM_SOFTDIRTY is defined as 0x0, then !(vm_flags & VM_SOFTDIRTY) >>>>> + * will be constantly true. >>>>> + */ >>>>> + if (!IS_ENABLED(CONFIG_MEM_SOFT_DIRTY)) >>>>> + return false; >>>>> + >>>>> + /* >>>>> + * Soft-dirty is kind of special: its tracking is enabled when the >>>>> + * vma flags not set. >>>>> + */ >>>>> + return !(vma->vm_flags & VM_SOFTDIRTY); >>>>> +} >>>> I'm sorry. I'm unable to understand the inversion here. >>>>> its tracking is enabled when the vma flags not set. >>>> VM_SOFTDIRTY is set on the VMA when new VMA is allocated to mark is >>>> soft-dirty. When we write to clear_refs to clear soft-dirty bit, >>>> VM_SOFTDIRTY is cleared from the VMA as well. Then why do you say tracking >>>> is enabled when the vma flags not set? >>> >>> Because only when 4>clear_refs happens would VM_SOFTDIRTY be cleared, and >>> only until then the real tracking starts (by removing write bits on ptes). >> But even if the VM_SOFTDIRTY is set on the VMA, the individual pages are >> still marked soft-dirty. Both are independent. >> >> It means tracking is enabled all the time in individual pages. Addition of vma_soft_dirty_enabled() has tinkered with the soft-dirty PTE bit status setting. The internal behavior has changed. The test case was shared by David (https://lore.kernel.org/all/bfcae708-db21-04b4-0bbe-712badd03071@redhat.com/). The explanation is as following: _Before_ addition of this patch(76aefad628aae), m = mmap(2 pages) clear_softdirty() mremap(m + pag_size) mprotect(READ) mprotect(READ | WRITE); memset(m) After memset(), PAGE-1 PAGE-2 VM_SOFTDIRTY set set PTE softdirty flag set set /proc//pagemap view set set _After_ addition of this patch(76aefad628aae) m = mmap(2 pages) clear_softdirty() mremap(m + page_size) mprotect(READ) mprotect(READ | WRITE); memset(m) After memset(), PAGE-1 PAGE-2 VM_SOFTDIRTY set set PTE softdirty flag *not set* set /proc//pagemap view set set The user's point of view hasn't changed. But internally after this patch, the soft-dirty tracking in PTEs gets turn off if VM_SOFTDIRTY is set. The soft-dirty tracking in the PTEs shouldn't be just turned off when mprotect is used. Why? Because soft-dirty tracking in the PTEs is always enabled regardless of VM_SOFTDIRTY is set or not. Example: m = mem(2 pages) At this point: PAGE-1 PAGE-2 VM_SOFTDIRTY set set PTE softdirty flag not set not set /proc//pagemap view set set memset(m) At this point: PAGE-1 PAGE-2 VM_SOFTDIRTY set set PTE softdirty flag set set /proc//pagemap view set set This example proves that soft-dirty flag on the PTE is set regardless of the VM_SOFTDIRTY. The simplest hack to get rid this changed behavior and revert to the previous behaviour is as following: --- a/mm/internal.h +++ b/mm/internal.h @@ -860,6 +860,8 @@ static inline bool vma_soft_dirty_enabled(struct vm_area_struct *vma) if (!IS_ENABLED(CONFIG_MEM_SOFT_DIRTY)) return false; + return true; + /* * Soft-dirty is kind of special: its tracking is enabled when the * vma flags not set. I was trying to verify this hack. But I couldn't previously until @Paul has mentioned this again. I've verified with limited tests that this hack in-deed works. We are unaware that does this hack create problems in other areas or not. We can think of better way to solve this. Once we get the comments from the community. This internal behavior change is affecting the new feature addition to the soft-dirty flag which is already delicate and has issues. (https://lore.kernel.org/all/20221109102303.851281-1-usama.anjum@collabora.com/) > > IMHO it depends on how we define "tracking enabled" - before clear_refs > even if no pages written they'll also be reported as dirty, then the > information is useless. > >> Only the soft-dirty bit status in individual page isn't significant if >> VM_SOFTDIRTY already is set. Right? > > Yes. But I'd say it makes more sense to say "tracking enabled" if we > really started tracking (by removing the write bits in ptes, otherwise we > did nothing). When vma created we didn't track anything. > > I don't know the rational of why soft-dirty was defined like that. I think > it's somehow related to the fact that we allow false positive dirty pages > not false negative. IOW, it's a bug to leak a page being dirtied, but not > vice versa if we report clean page dirty. >
On Mon, Dec 19, 2022 at 05:19:12PM +0500, Muhammad Usama Anjum wrote: > On 11/22/22 2:17 AM, Peter Xu wrote: > > On Mon, Nov 21, 2022 at 07:57:05PM +0500, Muhammad Usama Anjum wrote: > >> Hi Peter, > >> > >> Thank you so much for replying. > >> > >> On 11/19/22 4:14 AM, Peter Xu wrote: > >>> On Sat, Nov 19, 2022 at 01:16:26AM +0500, Muhammad Usama Anjum wrote: > >>>> Hi Peter and David, > >>> > >>> Hi, Muhammad, > >>> > >>>> > >>>> On 7/25/22 7:20 PM, Peter Xu wrote: > >>>>> The check wanted to make sure when soft-dirty tracking is enabled we won't > >>>>> grant write bit by accident, as a page fault is needed for dirty tracking. > >>>>> The intention is correct but we didn't check it right because VM_SOFTDIRTY > >>>>> set actually means soft-dirty tracking disabled. Fix it. > >>>> [...] > >>>>> +static inline bool vma_soft_dirty_enabled(struct vm_area_struct *vma) > >>>>> +{ > >>>>> + /* > >>>>> + * NOTE: we must check this before VM_SOFTDIRTY on soft-dirty > >>>>> + * enablements, because when without soft-dirty being compiled in, > >>>>> + * VM_SOFTDIRTY is defined as 0x0, then !(vm_flags & VM_SOFTDIRTY) > >>>>> + * will be constantly true. > >>>>> + */ > >>>>> + if (!IS_ENABLED(CONFIG_MEM_SOFT_DIRTY)) > >>>>> + return false; > >>>>> + > >>>>> + /* > >>>>> + * Soft-dirty is kind of special: its tracking is enabled when the > >>>>> + * vma flags not set. > >>>>> + */ > >>>>> + return !(vma->vm_flags & VM_SOFTDIRTY); > >>>>> +} > >>>> I'm sorry. I'm unable to understand the inversion here. > >>>>> its tracking is enabled when the vma flags not set. > >>>> VM_SOFTDIRTY is set on the VMA when new VMA is allocated to mark is > >>>> soft-dirty. When we write to clear_refs to clear soft-dirty bit, > >>>> VM_SOFTDIRTY is cleared from the VMA as well. Then why do you say tracking > >>>> is enabled when the vma flags not set? > >>> > >>> Because only when 4>clear_refs happens would VM_SOFTDIRTY be cleared, and > >>> only until then the real tracking starts (by removing write bits on ptes). > >> But even if the VM_SOFTDIRTY is set on the VMA, the individual pages are > >> still marked soft-dirty. Both are independent. > >> > >> It means tracking is enabled all the time in individual pages. > Addition of vma_soft_dirty_enabled() has tinkered with the soft-dirty PTE > bit status setting. The internal behavior has changed. The test case was > shared by David > (https://lore.kernel.org/all/bfcae708-db21-04b4-0bbe-712badd03071@redhat.com/). > The explanation is as following: > > _Before_ addition of this patch(76aefad628aae), > m = mmap(2 pages) > clear_softdirty() > mremap(m + pag_size) > mprotect(READ) > mprotect(READ | WRITE); > memset(m) > After memset(), > PAGE-1 PAGE-2 > VM_SOFTDIRTY set set > PTE softdirty flag set set > /proc//pagemap view set set > > > _After_ addition of this patch(76aefad628aae) > m = mmap(2 pages) > clear_softdirty() > mremap(m + page_size) > mprotect(READ) > mprotect(READ | WRITE); > memset(m) > After memset(), > PAGE-1 PAGE-2 > VM_SOFTDIRTY set set > PTE softdirty flag *not set* set > /proc//pagemap view set set > > The user's point of view hasn't changed. But internally after this patch, > the soft-dirty tracking in PTEs gets turn off if VM_SOFTDIRTY is set. The > soft-dirty tracking in the PTEs shouldn't be just turned off when mprotect > is used. Why? Because soft-dirty tracking in the PTEs is always enabled > regardless of VM_SOFTDIRTY is set or not. Example: > > m = mem(2 pages) > At this point: > PAGE-1 PAGE-2 > VM_SOFTDIRTY set set > PTE softdirty flag not set not set > /proc//pagemap view set set > memset(m) > At this point: > PAGE-1 PAGE-2 > VM_SOFTDIRTY set set > PTE softdirty flag set set > /proc//pagemap view set set > > This example proves that soft-dirty flag on the PTE is set regardless of > the VM_SOFTDIRTY. IMHO this is not a proof good enough - it's a kernel internal detail, and the userspace cannot detect it, right? Then it looks fine to not keep the same behavior on the ptes I think. After all currently the soft-dirty is designed as "taking either VM_SOFTDIRTY of pte soft-dirty as input of being dirty". Nothing violates that. Your approach introduced PAGEMAP_NO_REUSED_REGIONS but that special information is not remembered in vma, IIUC that's why you find things messed up. Fundamentally, it's because you're trying to reuse soft-dirty design but it's not completely soft-dirty anymore. That's also why I mentioned the other async uffd-wp approach because with that there's no fiddling with vma flags (since it'll be always set as pre-requisite), and this specific problem shouldn't exist either because uffd-wp was originally designed to be pte-based as I mentioned, so we can't grant write if pte is not checked. Your below change will resolve your problem for now, but it's definitely not wanted because it has a much broader impact on the whole system, for example, on vma_wants_writenotify(). We may still have some paths using default vm_page_prot (especially for file memories, not for the generic PF path but some others) that will start to lose write bits where we used to have them set. That's bad for performance because resolving each of them needs one more page fault after the change as it mostly invalidated the write bit in vm_page_prot. You can also introduce yet another flag in the vma so you can detect which vma has NEW soft-dirty enabled (your new approach) rather than the OLD (which still relies on vma flags besides ptes) but that'll really be ugly and making soft-dirty code unnecessarily complicated. > > The simplest hack to get rid this changed behavior and revert to the > previous behaviour is as following: > --- a/mm/internal.h > +++ b/mm/internal.h > @@ -860,6 +860,8 @@ static inline bool vma_soft_dirty_enabled(struct > vm_area_struct *vma) > if (!IS_ENABLED(CONFIG_MEM_SOFT_DIRTY)) > return false; > > + return true; > + > /* > * Soft-dirty is kind of special: its tracking is enabled when the > * vma flags not set. > I was trying to verify this hack. But I couldn't previously until @Paul has > mentioned this again. I've verified with limited tests that this hack > in-deed works. We are unaware that does this hack create problems in other > areas or not. We can think of better way to solve this. Once we get the > comments from the community. > > This internal behavior change is affecting the new feature addition to the > soft-dirty flag which is already delicate and has issues. > (https://lore.kernel.org/all/20221109102303.851281-1-usama.anjum@collabora.com/) > > > > > IMHO it depends on how we define "tracking enabled" - before clear_refs > > even if no pages written they'll also be reported as dirty, then the > > information is useless. > > > >> Only the soft-dirty bit status in individual page isn't significant if > >> VM_SOFTDIRTY already is set. Right? > > > > Yes. But I'd say it makes more sense to say "tracking enabled" if we > > really started tracking (by removing the write bits in ptes, otherwise we > > did nothing). When vma created we didn't track anything. > > > > I don't know the rational of why soft-dirty was defined like that. I think > > it's somehow related to the fact that we allow false positive dirty pages > > not false negative. IOW, it's a bug to leak a page being dirtied, but not > > vice versa if we report clean page dirty. > > > > -- > BR, > Muhammad Usama Anjum >
Hi Peter, Thank you for replying. On 12/20/22 9:03 PM, Peter Xu wrote: > On Mon, Dec 19, 2022 at 05:19:12PM +0500, Muhammad Usama Anjum wrote: >> On 11/22/22 2:17 AM, Peter Xu wrote: >>> On Mon, Nov 21, 2022 at 07:57:05PM +0500, Muhammad Usama Anjum wrote: >>>> Hi Peter, >>>> >>>> Thank you so much for replying. >>>> >>>> On 11/19/22 4:14 AM, Peter Xu wrote: >>>>> On Sat, Nov 19, 2022 at 01:16:26AM +0500, Muhammad Usama Anjum wrote: >>>>>> Hi Peter and David, >>>>> >>>>> Hi, Muhammad, >>>>> >>>>>> >>>>>> On 7/25/22 7:20 PM, Peter Xu wrote: >>>>>>> The check wanted to make sure when soft-dirty tracking is enabled we won't >>>>>>> grant write bit by accident, as a page fault is needed for dirty tracking. >>>>>>> The intention is correct but we didn't check it right because VM_SOFTDIRTY >>>>>>> set actually means soft-dirty tracking disabled. Fix it. >>>>>> [...] >>>>>>> +static inline bool vma_soft_dirty_enabled(struct vm_area_struct *vma) >>>>>>> +{ >>>>>>> + /* >>>>>>> + * NOTE: we must check this before VM_SOFTDIRTY on soft-dirty >>>>>>> + * enablements, because when without soft-dirty being compiled in, >>>>>>> + * VM_SOFTDIRTY is defined as 0x0, then !(vm_flags & VM_SOFTDIRTY) >>>>>>> + * will be constantly true. >>>>>>> + */ >>>>>>> + if (!IS_ENABLED(CONFIG_MEM_SOFT_DIRTY)) >>>>>>> + return false; >>>>>>> + >>>>>>> + /* >>>>>>> + * Soft-dirty is kind of special: its tracking is enabled when the >>>>>>> + * vma flags not set. >>>>>>> + */ >>>>>>> + return !(vma->vm_flags & VM_SOFTDIRTY); >>>>>>> +} >>>>>> I'm sorry. I'm unable to understand the inversion here. >>>>>>> its tracking is enabled when the vma flags not set. >>>>>> VM_SOFTDIRTY is set on the VMA when new VMA is allocated to mark is >>>>>> soft-dirty. When we write to clear_refs to clear soft-dirty bit, >>>>>> VM_SOFTDIRTY is cleared from the VMA as well. Then why do you say tracking >>>>>> is enabled when the vma flags not set? >>>>> >>>>> Because only when 4>clear_refs happens would VM_SOFTDIRTY be cleared, and >>>>> only until then the real tracking starts (by removing write bits on ptes). >>>> But even if the VM_SOFTDIRTY is set on the VMA, the individual pages are >>>> still marked soft-dirty. Both are independent. >>>> >>>> It means tracking is enabled all the time in individual pages. >> Addition of vma_soft_dirty_enabled() has tinkered with the soft-dirty PTE >> bit status setting. The internal behavior has changed. The test case was >> shared by David >> (https://lore.kernel.org/all/bfcae708-db21-04b4-0bbe-712badd03071@redhat.com/). >> The explanation is as following: >> >> _Before_ addition of this patch(76aefad628aae), >> m = mmap(2 pages) >> clear_softdirty() >> mremap(m + pag_size) >> mprotect(READ) >> mprotect(READ | WRITE); >> memset(m) >> After memset(), >> PAGE-1 PAGE-2 >> VM_SOFTDIRTY set set >> PTE softdirty flag set set >> /proc//pagemap view set set >> >> >> _After_ addition of this patch(76aefad628aae) >> m = mmap(2 pages) >> clear_softdirty() >> mremap(m + page_size) >> mprotect(READ) >> mprotect(READ | WRITE); >> memset(m) >> After memset(), >> PAGE-1 PAGE-2 >> VM_SOFTDIRTY set set >> PTE softdirty flag *not set* set >> /proc//pagemap view set set >> >> The user's point of view hasn't changed. But internally after this patch, >> the soft-dirty tracking in PTEs gets turn off if VM_SOFTDIRTY is set. The >> soft-dirty tracking in the PTEs shouldn't be just turned off when mprotect >> is used. Why? Because soft-dirty tracking in the PTEs is always enabled >> regardless of VM_SOFTDIRTY is set or not. Example: >> >> m = mem(2 pages) >> At this point: >> PAGE-1 PAGE-2 >> VM_SOFTDIRTY set set >> PTE softdirty flag not set not set >> /proc//pagemap view set set >> memset(m) >> At this point: >> PAGE-1 PAGE-2 >> VM_SOFTDIRTY set set >> PTE softdirty flag set set >> /proc//pagemap view set set >> >> This example proves that soft-dirty flag on the PTE is set regardless of >> the VM_SOFTDIRTY. > > IMHO this is not a proof good enough - it's a kernel internal detail, and > the userspace cannot detect it, right? Then it looks fine to not keep the > same behavior on the ptes I think. After all currently the soft-dirty is > designed as "taking either VM_SOFTDIRTY of pte soft-dirty as input of being > dirty". Nothing violates that. Nothing has changed for the userspace. But when the default soft-dirty feature always updates the soft-dirty flag in the PTEs regardless of VM_SOFTDIRTY is set or not, why does other components of the mm stop caring for soft-dirty flag in the PTE when VM_SOFTDIRTY is set? > > Your approach introduced PAGEMAP_NO_REUSED_REGIONS but that special > information is not remembered in vma, IIUC that's why you find things > messed up. Fundamentally, it's because you're trying to reuse soft-dirty > design but it's not completely soft-dirty anymore. Correct, that's why I'm trying to find a way to correct the soft-dirty support instead of using anything else. We should try and correct it. I've sent a RFC to track the soft-dirty flags for sub regions in the VMA. > > That's also why I mentioned the other async uffd-wp approach because with > that there's no fiddling with vma flags (since it'll be always set as > pre-requisite), and this specific problem shouldn't exist either because > uffd-wp was originally designed to be pte-based as I mentioned, so we can't > grant write if pte is not checked. > > Your below change will resolve your problem for now, but it's definitely > not wanted because it has a much broader impact on the whole system, for > example, on vma_wants_writenotify(). We may still have some paths using > default vm_page_prot (especially for file memories, not for the generic PF > path but some others) that will start to lose write bits where we used to > have them set. That's bad for performance because resolving each of them > needs one more page fault after the change as it mostly invalidated the > write bit in vm_page_prot. > > You can also introduce yet another flag in the vma so you can detect which > vma has NEW soft-dirty enabled (your new approach) rather than the OLD > (which still relies on vma flags besides ptes) but that'll really be ugly > and making soft-dirty code unnecessarily complicated. > >> >> The simplest hack to get rid this changed behavior and revert to the >> previous behaviour is as following: >> --- a/mm/internal.h >> +++ b/mm/internal.h >> @@ -860,6 +860,8 @@ static inline bool vma_soft_dirty_enabled(struct >> vm_area_struct *vma) >> if (!IS_ENABLED(CONFIG_MEM_SOFT_DIRTY)) >> return false; >> >> + return true; >> + >> /* >> * Soft-dirty is kind of special: its tracking is enabled when the >> * vma flags not set. >> I was trying to verify this hack. But I couldn't previously until @Paul has >> mentioned this again. I've verified with limited tests that this hack >> in-deed works. We are unaware that does this hack create problems in other >> areas or not. We can think of better way to solve this. Once we get the >> comments from the community. >> >> This internal behavior change is affecting the new feature addition to the >> soft-dirty flag which is already delicate and has issues. >> (https://lore.kernel.org/all/20221109102303.851281-1-usama.anjum@collabora.com/) >> >>> >>> IMHO it depends on how we define "tracking enabled" - before clear_refs >>> even if no pages written they'll also be reported as dirty, then the >>> information is useless. >>> >>>> Only the soft-dirty bit status in individual page isn't significant if >>>> VM_SOFTDIRTY already is set. Right? >>> >>> Yes. But I'd say it makes more sense to say "tracking enabled" if we >>> really started tracking (by removing the write bits in ptes, otherwise we >>> did nothing). When vma created we didn't track anything. >>> >>> I don't know the rational of why soft-dirty was defined like that. I think >>> it's somehow related to the fact that we allow false positive dirty pages >>> not false negative. IOW, it's a bug to leak a page being dirtied, but not >>> vice versa if we report clean page dirty. >>> >> >> -- >> BR, >> Muhammad Usama Anjum >> >
On Tue, Dec 20, 2022 at 11:15:00PM +0500, Muhammad Usama Anjum wrote: > Hi Peter, Hi, Muhammad, [...] > Nothing has changed for the userspace. But when the default soft-dirty > feature always updates the soft-dirty flag in the PTEs regardless of > VM_SOFTDIRTY is set or not But it was not? I don't see why the pte flags must be considered at all if VM_SOFTDIRTY is set in existing code base, or before this patch. > why does other components of the mm stop caring for soft-dirty flag in > the PTE when VM_SOFTDIRTY is set? > > > > > Your approach introduced PAGEMAP_NO_REUSED_REGIONS but that special > > information is not remembered in vma, IIUC that's why you find things > > messed up. Fundamentally, it's because you're trying to reuse soft-dirty > > design but it's not completely soft-dirty anymore. > Correct, that's why I'm trying to find a way to correct the soft-dirty > support instead of using anything else. We should try and correct it. I've > sent a RFC to track the soft-dirty flags for sub regions in the VMA. Note that I'm not against the change if that's servicing the purpose of the enhancement you're proposing. But again I wouldn't use "correct" as the word here because I still didn't see anything wrong with the old code. so IMHO the extra complexity on handling VM_SOFTDIRTY (even if to drop it and replace with other structures to maintain ranged soft-dirty) needs to be justified along with the feature introduced, not be justified as a fix. Thanks,
On 12/21/22 12:02 AM, Peter Xu wrote: > On Tue, Dec 20, 2022 at 11:15:00PM +0500, Muhammad Usama Anjum wrote: >> Hi Peter, > > Hi, Muhammad, > > [...] > >> Nothing has changed for the userspace. But when the default soft-dirty >> feature always updates the soft-dirty flag in the PTEs regardless of >> VM_SOFTDIRTY is set or not > > But it was not? I don't see why the pte flags must be considered at all if > VM_SOFTDIRTY is set in existing code base, or before this patch. I completely agree that setting pte flags isn't needed if VM_SOFTDIRTY is set. It is wasted effort. Before this patch, the effort was being wasted in the default part of the feature and in the other related components as well. After this patch, the effort is being wasted only in the default part of the feature and other related components have stopped doing this wasted effort which is a good thing. But now there is discrepancy that one part of code keeps on tracking pte while other has moved on/improved. > >> why does other components of the mm stop caring for soft-dirty flag in >> the PTE when VM_SOFTDIRTY is set? >> >>> >>> Your approach introduced PAGEMAP_NO_REUSED_REGIONS but that special >>> information is not remembered in vma, IIUC that's why you find things >>> messed up. Fundamentally, it's because you're trying to reuse soft-dirty >>> design but it's not completely soft-dirty anymore. >> Correct, that's why I'm trying to find a way to correct the soft-dirty >> support instead of using anything else. We should try and correct it. I've >> sent a RFC to track the soft-dirty flags for sub regions in the VMA. > > Note that I'm not against the change if that's servicing the purpose of the > enhancement you're proposing. But again I wouldn't use "correct" as the > word here because I still didn't see anything wrong with the old code. > > so IMHO the extra complexity on handling VM_SOFTDIRTY (even if to drop it > and replace with other structures to maintain ranged soft-dirty) needs to > be justified along with the feature introduced, not be justified as a fix. The question of tracking re-used regions should be answered by the original developers of the feature. I've been trying to get comments from them. But I've not got any. Maybe some conference talk can work where the maintainers/developers are present? Or I'll summarize the whole problem and ask Andrew directly. > > Thanks, >
On 12/19/22 5:19 PM, Muhammad Usama Anjum wrote: > Addition of vma_soft_dirty_enabled() has tinkered with the soft-dirty PTE > bit status setting. The internal behavior has changed. The test case was > shared by David > (https://lore.kernel.org/all/bfcae708-db21-04b4-0bbe-712badd03071@redhat.com/). > The explanation is as following: > > _Before_ addition of this patch(76aefad628aae), > m = mmap(2 pages) > clear_softdirty() > mremap(m + pag_size) > mprotect(READ) > mprotect(READ | WRITE); > memset(m) > After memset(), > PAGE-1 PAGE-2 > VM_SOFTDIRTY set set > PTE softdirty flag set set > /proc//pagemap view set set > > > _After_ addition of this patch(76aefad628aae) > m = mmap(2 pages) > clear_softdirty() > mremap(m + page_size) > mprotect(READ) > mprotect(READ | WRITE); > memset(m) > After memset(), > PAGE-1 PAGE-2 > VM_SOFTDIRTY set set > PTE softdirty flag *not set* set > /proc//pagemap view set set > > The user's point of view hasn't changed. But internally after this patch, > the soft-dirty tracking in PTEs gets turn off if VM_SOFTDIRTY is set. The > soft-dirty tracking in the PTEs shouldn't be just turned off when mprotect > is used. Why? Because soft-dirty tracking in the PTEs is always enabled > regardless of VM_SOFTDIRTY is set or not. Example: > > m = mem(2 pages) > At this point: > PAGE-1 PAGE-2 > VM_SOFTDIRTY set set > PTE softdirty flag not set not set > /proc//pagemap view set set > memset(m) > At this point: > PAGE-1 PAGE-2 > VM_SOFTDIRTY set set > PTE softdirty flag set set > /proc//pagemap view set set > > This example proves that soft-dirty flag on the PTE is set regardless of > the VM_SOFTDIRTY. Hi Andrew and Cyrill, Peter doesn't agree with me here that this change in behavior should be reverted etc. Please comment.
On 28.12.22 15:14, Muhammad Usama Anjum wrote: > On 12/19/22 5:19 PM, Muhammad Usama Anjum wrote: >> Addition of vma_soft_dirty_enabled() has tinkered with the soft-dirty PTE >> bit status setting. The internal behavior has changed. The test case was >> shared by David >> (https://lore.kernel.org/all/bfcae708-db21-04b4-0bbe-712badd03071@redhat.com/). >> The explanation is as following: >> >> _Before_ addition of this patch(76aefad628aae), >> m = mmap(2 pages) >> clear_softdirty() >> mremap(m + pag_size) >> mprotect(READ) >> mprotect(READ | WRITE); >> memset(m) >> After memset(), >> PAGE-1 PAGE-2 >> VM_SOFTDIRTY set set >> PTE softdirty flag set set >> /proc//pagemap view set set >> >> >> _After_ addition of this patch(76aefad628aae) >> m = mmap(2 pages) >> clear_softdirty() >> mremap(m + page_size) >> mprotect(READ) >> mprotect(READ | WRITE); >> memset(m) >> After memset(), >> PAGE-1 PAGE-2 >> VM_SOFTDIRTY set set >> PTE softdirty flag *not set* set >> /proc//pagemap view set set >> >> The user's point of view hasn't changed. But internally after this patch, >> the soft-dirty tracking in PTEs gets turn off if VM_SOFTDIRTY is set. The >> soft-dirty tracking in the PTEs shouldn't be just turned off when mprotect >> is used. Why? Because soft-dirty tracking in the PTEs is always enabled >> regardless of VM_SOFTDIRTY is set or not. Example: >> >> m = mem(2 pages) >> At this point: >> PAGE-1 PAGE-2 >> VM_SOFTDIRTY set set >> PTE softdirty flag not set not set >> /proc//pagemap view set set >> memset(m) >> At this point: >> PAGE-1 PAGE-2 >> VM_SOFTDIRTY set set >> PTE softdirty flag set set >> /proc//pagemap view set set >> >> This example proves that soft-dirty flag on the PTE is set regardless of >> the VM_SOFTDIRTY. > > Hi Andrew and Cyrill, > > Peter doesn't agree with me here that this change in behavior should be > reverted etc. Please comment. For the records, I agree with Peter: As 76aefad628aa ("mm/mprotect: fix soft-dirty check in can_change_pte_writable()") documents, this patch fixed real problems. /proc/pagemap works as expected right now such that we don't have an under-indication. Internal representation is an implementation detail. Whatever we do, there must not be an under-indication of softdirty. That is the ABI guaranteed (especially for anonymous memory). "No over-indication" was never the ABI guarantee. For your use case, you want to reduce over-indication. I suggested looked into alternatives.
diff --git a/mm/internal.h b/mm/internal.h index 15e8cb118832..e2d442e3c0b2 100644 --- a/mm/internal.h +++ b/mm/internal.h @@ -860,4 +860,22 @@ struct folio *try_grab_folio(struct page *page, int refs, unsigned int flags); DECLARE_PER_CPU(struct per_cpu_nodestat, boot_nodestats); +static inline bool vma_soft_dirty_enabled(struct vm_area_struct *vma) +{ + /* + * NOTE: we must check this before VM_SOFTDIRTY on soft-dirty + * enablements, because when without soft-dirty being compiled in, + * VM_SOFTDIRTY is defined as 0x0, then !(vm_flags & VM_SOFTDIRTY) + * will be constantly true. + */ + if (!IS_ENABLED(CONFIG_MEM_SOFT_DIRTY)) + return false; + + /* + * Soft-dirty is kind of special: its tracking is enabled when the + * vma flags not set. + */ + return !(vma->vm_flags & VM_SOFTDIRTY); +} + #endif /* __MM_INTERNAL_H */ diff --git a/mm/mmap.c b/mm/mmap.c index 125e8903c93c..93f9913409ea 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -1518,7 +1518,7 @@ int vma_wants_writenotify(struct vm_area_struct *vma, pgprot_t vm_page_prot) return 0; /* Do we need to track softdirty? */ - if (IS_ENABLED(CONFIG_MEM_SOFT_DIRTY) && !(vm_flags & VM_SOFTDIRTY)) + if (vma_soft_dirty_enabled(vma)) return 1; /* Specialty mapping? */ diff --git a/mm/mprotect.c b/mm/mprotect.c index 0420c3ed936c..c403e84129d4 100644 --- a/mm/mprotect.c +++ b/mm/mprotect.c @@ -49,7 +49,7 @@ static inline bool can_change_pte_writable(struct vm_area_struct *vma, return false; /* Do we need write faults for softdirty tracking? */ - if ((vma->vm_flags & VM_SOFTDIRTY) && !pte_soft_dirty(pte)) + if (vma_soft_dirty_enabled(vma) && !pte_soft_dirty(pte)) return false; /* Do we need write faults for uffd-wp tracking? */
The check wanted to make sure when soft-dirty tracking is enabled we won't grant write bit by accident, as a page fault is needed for dirty tracking. The intention is correct but we didn't check it right because VM_SOFTDIRTY set actually means soft-dirty tracking disabled. Fix it. There's another thing tricky about soft-dirty is that, we can't check the vma flag !(vma_flags & VM_SOFTDIRTY) directly but only check it after we checked CONFIG_MEM_SOFT_DIRTY because otherwise VM_SOFTDIRTY will be defined as zero, and !(vma_flags & VM_SOFTDIRTY) will constantly return true. To avoid misuse, introduce a helper for checking whether vma has soft-dirty tracking enabled. We can easily verify this with any exclusive anonymous page, like program below: =======8<====== #include <stdio.h> #include <unistd.h> #include <stdlib.h> #include <assert.h> #include <inttypes.h> #include <stdint.h> #include <sys/types.h> #include <sys/mman.h> #include <sys/types.h> #include <sys/stat.h> #include <unistd.h> #include <fcntl.h> #include <stdbool.h> #define BIT_ULL(nr) (1ULL << (nr)) #define PM_SOFT_DIRTY BIT_ULL(55) unsigned int psize; char *page; uint64_t pagemap_read_vaddr(int fd, void *vaddr) { uint64_t value; int ret; ret = pread(fd, &value, sizeof(uint64_t), ((uint64_t)vaddr >> 12) * sizeof(uint64_t)); assert(ret == sizeof(uint64_t)); return value; } void clear_refs_write(void) { int fd = open("/proc/self/clear_refs", O_RDWR); assert(fd >= 0); write(fd, "4", 2); close(fd); } #define check_soft_dirty(str, expect) do { \ bool dirty = pagemap_read_vaddr(fd, page) & PM_SOFT_DIRTY; \ if (dirty != expect) { \ printf("ERROR: %s, soft-dirty=%d (expect: %d)\n", str, dirty, expect); \ exit(-1); \ } \ } while (0) int main(void) { int fd = open("/proc/self/pagemap", O_RDONLY); assert(fd >= 0); psize = getpagesize(); page = mmap(NULL, psize, PROT_READ|PROT_WRITE, MAP_ANONYMOUS|MAP_PRIVATE, -1, 0); assert(page != MAP_FAILED); *page = 1; check_soft_dirty("Just faulted in page", 1); clear_refs_write(); check_soft_dirty("Clear_refs written", 0); mprotect(page, psize, PROT_READ); check_soft_dirty("Marked RO", 0); mprotect(page, psize, PROT_READ|PROT_WRITE); check_soft_dirty("Marked RW", 0); *page = 2; check_soft_dirty("Wrote page again", 1); munmap(page, psize); close(fd); printf("Test passed.\n"); return 0; } =======8<====== Here we attach a Fixes to commit 64fe24a3e05e only for easy tracking, as this patch won't apply to a tree before that point. However the commit wasn't the source of problem, but instead 64e455079e1b. It's just that after 64fe24a3e05e anonymous memory will also suffer from this problem with mprotect(). Fixes: 64e455079e1b ("mm: softdirty: enable write notifications on VMAs after VM_SOFTDIRTY cleared") Fixes: 64fe24a3e05e ("mm/mprotect: try avoiding write faults for exclusive anonymous pages when changing protection") Reviewed-by: David Hildenbrand <david@redhat.com> Signed-off-by: Peter Xu <peterx@redhat.com> --- mm/internal.h | 18 ++++++++++++++++++ mm/mmap.c | 2 +- mm/mprotect.c | 2 +- 3 files changed, 20 insertions(+), 2 deletions(-)