| Message ID | 20210320093701.12829-4-linmiaohe@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Cleanup and fixup for mm/migrate.c | expand |
On 20.03.21 10:36, Miaohe Lin wrote: > If the zone device page does not belong to un-addressable device memory, > the variable entry will be uninitialized and lead to indeterminate pte > entry ultimately. Fix this unexpectant case and warn about it. s/unexpectant/unexpected/ > > Fixes: df6ad69838fc ("mm/device-public-memory: device memory cache coherent with CPU") > Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> > --- > mm/migrate.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/mm/migrate.c b/mm/migrate.c > index 20a3bf75270a..271081b014cb 100644 > --- a/mm/migrate.c > +++ b/mm/migrate.c > @@ -2972,6 +2972,13 @@ static void migrate_vma_insert_page(struct migrate_vma *migrate, > > swp_entry = make_device_private_entry(page, vma->vm_flags & VM_WRITE); > entry = swp_entry_to_pte(swp_entry); > + } else { > + /* > + * For now we only support migrating to un-addressable > + * device memory. > + */ > + WARN_ON(1); > + goto abort; Fix it by crashing the kernel with panic_on_warn? :) If this case can actual happen, than no WARN_ON() - rather a pr_warn_once(). If this case cannot happen, why do we even care (it's not a fix then)?
On Tuesday, 23 March 2021 9:26:43 PM AEDT David Hildenbrand wrote: > On 20.03.21 10:36, Miaohe Lin wrote: > > If the zone device page does not belong to un-addressable device memory, > > the variable entry will be uninitialized and lead to indeterminate pte > > entry ultimately. Fix this unexpectant case and warn about it. > > s/unexpectant/unexpected/ > > > > > Fixes: df6ad69838fc ("mm/device-public-memory: device memory cache coherent with CPU") > > Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> > > --- > > mm/migrate.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/mm/migrate.c b/mm/migrate.c > > index 20a3bf75270a..271081b014cb 100644 > > --- a/mm/migrate.c > > +++ b/mm/migrate.c > > @@ -2972,6 +2972,13 @@ static void migrate_vma_insert_page(struct migrate_vma *migrate, > > > > swp_entry = make_device_private_entry(page, vma->vm_flags & VM_WRITE); > > entry = swp_entry_to_pte(swp_entry); > > + } else { > > + /* > > + * For now we only support migrating to un-addressable > > + * device memory. > > + */ > > + WARN_ON(1); > > + goto abort; > > Fix it by crashing the kernel with panic_on_warn? :) > > If this case can actual happen, than no WARN_ON() - rather a > pr_warn_once(). If this case cannot happen, why do we even care (it's > not a fix then)? There is also already a check for this case in migrate_vma_pages(). The problem is it happens after the call to migrate_vma_insert_page(). I wonder if instead it would be better just to move the existing check to before that call?
Hi: On 2021/3/23 18:26, David Hildenbrand wrote: > On 20.03.21 10:36, Miaohe Lin wrote: >> If the zone device page does not belong to un-addressable device memory, >> the variable entry will be uninitialized and lead to indeterminate pte >> entry ultimately. Fix this unexpectant case and warn about it. > > s/unexpectant/unexpected/ > >> >> Fixes: df6ad69838fc ("mm/device-public-memory: device memory cache coherent with CPU") >> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> >> --- >> mm/migrate.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/mm/migrate.c b/mm/migrate.c >> index 20a3bf75270a..271081b014cb 100644 >> --- a/mm/migrate.c >> +++ b/mm/migrate.c >> @@ -2972,6 +2972,13 @@ static void migrate_vma_insert_page(struct migrate_vma *migrate, >> swp_entry = make_device_private_entry(page, vma->vm_flags & VM_WRITE); >> entry = swp_entry_to_pte(swp_entry); >> + } else { >> + /* >> + * For now we only support migrating to un-addressable >> + * device memory. >> + */ >> + WARN_ON(1); >> + goto abort; > > Fix it by crashing the kernel with panic_on_warn? :) > Sorry, my bad. :( > If this case can actual happen, than no WARN_ON() - rather a pr_warn_once(). If this case cannot happen, why do we even care (it's not a fix then)? Yep, this case can actual happen. Many thanks for providing alternative pr_warn_once(). > >
On 2021/3/23 19:07, Alistair Popple wrote: > On Tuesday, 23 March 2021 9:26:43 PM AEDT David Hildenbrand wrote: >> On 20.03.21 10:36, Miaohe Lin wrote: >>> If the zone device page does not belong to un-addressable device memory, >>> the variable entry will be uninitialized and lead to indeterminate pte >>> entry ultimately. Fix this unexpectant case and warn about it. >> >> s/unexpectant/unexpected/ >> >>> >>> Fixes: df6ad69838fc ("mm/device-public-memory: device memory cache > coherent with CPU") >>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> >>> --- >>> mm/migrate.c | 7 +++++++ >>> 1 file changed, 7 insertions(+) >>> >>> diff --git a/mm/migrate.c b/mm/migrate.c >>> index 20a3bf75270a..271081b014cb 100644 >>> --- a/mm/migrate.c >>> +++ b/mm/migrate.c >>> @@ -2972,6 +2972,13 @@ static void migrate_vma_insert_page(struct > migrate_vma *migrate, >>> >>> swp_entry = make_device_private_entry(page, vma->vm_flags & > VM_WRITE); >>> entry = swp_entry_to_pte(swp_entry); >>> + } else { >>> + /* >>> + * For now we only support migrating to un-addressable >>> + * device memory. >>> + */ >>> + WARN_ON(1); >>> + goto abort; >> >> Fix it by crashing the kernel with panic_on_warn? :) >> >> If this case can actual happen, than no WARN_ON() - rather a >> pr_warn_once(). If this case cannot happen, why do we even care (it's >> not a fix then)? > > There is also already a check for this case in migrate_vma_pages(). The > problem is it happens after the call to migrate_vma_insert_page(). I wonder if > instead it would be better just to move the existing check to before that > call? > Yes, sounds good! Many thanks for your advice! :) > > > . >
On 2021/3/23 19:28, Miaohe Lin wrote: > On 2021/3/23 19:07, Alistair Popple wrote: >> On Tuesday, 23 March 2021 9:26:43 PM AEDT David Hildenbrand wrote: >>> On 20.03.21 10:36, Miaohe Lin wrote: >>>> If the zone device page does not belong to un-addressable device memory, >>>> the variable entry will be uninitialized and lead to indeterminate pte >>>> entry ultimately. Fix this unexpectant case and warn about it. >>> >>> s/unexpectant/unexpected/ >>> >>>> >>>> Fixes: df6ad69838fc ("mm/device-public-memory: device memory cache >> coherent with CPU") >>>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> >>>> --- >>>> mm/migrate.c | 7 +++++++ >>>> 1 file changed, 7 insertions(+) >>>> >>>> diff --git a/mm/migrate.c b/mm/migrate.c >>>> index 20a3bf75270a..271081b014cb 100644 >>>> --- a/mm/migrate.c >>>> +++ b/mm/migrate.c >>>> @@ -2972,6 +2972,13 @@ static void migrate_vma_insert_page(struct >> migrate_vma *migrate, >>>> >>>> swp_entry = make_device_private_entry(page, vma->vm_flags & >> VM_WRITE); >>>> entry = swp_entry_to_pte(swp_entry); >>>> + } else { >>>> + /* >>>> + * For now we only support migrating to un-addressable >>>> + * device memory. >>>> + */ >>>> + WARN_ON(1); >>>> + goto abort; >>> >>> Fix it by crashing the kernel with panic_on_warn? :) >>> >>> If this case can actual happen, than no WARN_ON() - rather a >>> pr_warn_once(). If this case cannot happen, why do we even care (it's >>> not a fix then)? >> >> There is also already a check for this case in migrate_vma_pages(). The >> problem is it happens after the call to migrate_vma_insert_page(). I wonder if >> instead it would be better just to move the existing check to before that >> call? >> > While look at this more closely, move the existing check to before migrate_vma_insert_page() could potentially change the current mmu_notifier semantics against anonymous zero page. :( So check zone device page inside migrate_vma_insert_page() would be more acceptable. Many thanks. > Yes, sounds good! Many thanks for your advice! :) > >>> >> . >> >
diff --git a/mm/migrate.c b/mm/migrate.c index 20a3bf75270a..271081b014cb 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -2972,6 +2972,13 @@ static void migrate_vma_insert_page(struct migrate_vma *migrate, swp_entry = make_device_private_entry(page, vma->vm_flags & VM_WRITE); entry = swp_entry_to_pte(swp_entry); + } else { + /* + * For now we only support migrating to un-addressable + * device memory. + */ + WARN_ON(1); + goto abort; } } else { entry = mk_pte(page, vma->vm_page_prot);
If the zone device page does not belong to un-addressable device memory, the variable entry will be uninitialized and lead to indeterminate pte entry ultimately. Fix this unexpectant case and warn about it. Fixes: df6ad69838fc ("mm/device-public-memory: device memory cache coherent with CPU") Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> --- mm/migrate.c | 7 +++++++ 1 file changed, 7 insertions(+)