| Message ID | 20190601074959.14036-4-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/16] uaccess: add untagged_addr definition for other arches | expand |
On Sat, Jun 1, 2019 at 12:50 AM Christoph Hellwig <hch@lst.de> wrote: > > Pass in the already calculated end value instead of recomputing it, and > leave the end > start check in the callers instead of duplicating them > in the arch code. Good cleanup, except it's wrong. > - if (nr_pages <= 0) > + if (end < start) > return 0; You moved the overflow test to generic code - good. You removed the sign and zero test on nr_pages - bad. The zero test in particular is _important_ - the GUP range operators know and depend on the fact that they are passed a non-empty range. The sign test it less so, but is definitely appropriate. It might be even better to check that the "<< PAGE_SHIFT" doesn't overflow in "long", of course, but with callers being supposed to be trusted, the sign test at least checks for stupid underflow issues. So at the very least that "(end < start)" needs to be "(end <= start)", but honestly, I think the sign of the nr_pages should be continued to be checked. Linus
On Sat, Jun 01, 2019 at 09:14:17AM -0700, Linus Torvalds wrote: > On Sat, Jun 1, 2019 at 12:50 AM Christoph Hellwig <hch@lst.de> wrote: > > > > Pass in the already calculated end value instead of recomputing it, and > > leave the end > start check in the callers instead of duplicating them > > in the arch code. > > Good cleanup, except it's wrong. > > > - if (nr_pages <= 0) > > + if (end < start) > > return 0; > > You moved the overflow test to generic code - good. > > You removed the sign and zero test on nr_pages - bad. I only removed a duplicate of it. The full (old) code in get_user_pages_fast() looks like this: if (nr_pages <= 0) return 0; if (unlikely(!access_ok((void __user *)start, len))) return -EFAULT; if (gup_fast_permitted(start, nr_pages)) {
On Mon, Jun 3, 2019 at 12:41 AM Christoph Hellwig <hch@lst.de> wrote: > > I only removed a duplicate of it. I don't see any remaining cases. > The full (old) code in get_user_pages_fast() looks like this: > > if (nr_pages <= 0) > return 0; > > if (unlikely(!access_ok((void __user *)start, len))) > return -EFAULT; > > if (gup_fast_permitted(start, nr_pages)) { Yes, and that code was correct. The new code has no test at all for "nr_pages == 0", afaik. Linus
On Mon, Jun 3, 2019 at 9:08 AM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > The new code has no test at all for "nr_pages == 0", afaik. Note that it really is important to check for that, because right now we do if (gup_fast_permitted(start, nr_pages)) { local_irq_save(flags); gup_pgd_range(start, end, write ? FOLL_WRITE : 0, pages, &nr); local_irq_restore(flags); } and that gup_pgd_range() function *depends* on the range being non-zero, and does pgdp = pgd_offset(current->mm, addr); do { pgd_t pgd = READ_ONCE(*pgdp); ... } while (pgdp++, addr = next, addr != end); Note how a zero range would turn into an infinite range here. And the only check for 0 was that if (nr_pages <= 0) return 0; in get_user_pages_fast() that you removed. (Admittedly, it would be much better to have that check in __get_user_pages_fast() itself, because we do have callers that call the double-underscore version) Now, I sincerely hope that we don't have anybody that passes in a zero nr_pages (or a negative one), but we do actually have a comment saying it's ok. Note that the check for "if (end < start)" not only does not check for 0, it also doesn't really check for negative. It checks for _overflow_. Admittedly most negative values would be expected to overflow, but it's still a very different issue. Maybe you added the check for negative somewhere else (in another patch), but I don't see it. Linus
On Mon, Jun 03, 2019 at 10:02:10AM -0700, Linus Torvalds wrote: > On Mon, Jun 3, 2019 at 9:08 AM Linus Torvalds > <torvalds@linux-foundation.org> wrote: > > > > The new code has no test at all for "nr_pages == 0", afaik. > > Note that it really is important to check for that, because right now we do True. The 0 check got lost. I'll make sure we do the right thing for the next version.
diff --git a/arch/s390/include/asm/pgtable.h b/arch/s390/include/asm/pgtable.h index 9f0195d5fa16..9b274fcaacb6 100644 --- a/arch/s390/include/asm/pgtable.h +++ b/arch/s390/include/asm/pgtable.h @@ -1270,14 +1270,8 @@ static inline pte_t *pte_offset(pmd_t *pmd, unsigned long address) #define pte_offset_map(pmd, address) pte_offset_kernel(pmd, address) #define pte_unmap(pte) do { } while (0) -static inline bool gup_fast_permitted(unsigned long start, int nr_pages) +static inline bool gup_fast_permitted(unsigned long start, unsigned long end) { - unsigned long len, end; - - len = (unsigned long) nr_pages << PAGE_SHIFT; - end = start + len; - if (end < start) - return false; return end <= current->mm->context.asce_limit; } #define gup_fast_permitted gup_fast_permitted diff --git a/arch/x86/include/asm/pgtable_64.h b/arch/x86/include/asm/pgtable_64.h index 0bb566315621..4990d26dfc73 100644 --- a/arch/x86/include/asm/pgtable_64.h +++ b/arch/x86/include/asm/pgtable_64.h @@ -259,14 +259,8 @@ extern void init_extra_mapping_uc(unsigned long phys, unsigned long size); extern void init_extra_mapping_wb(unsigned long phys, unsigned long size); #define gup_fast_permitted gup_fast_permitted -static inline bool gup_fast_permitted(unsigned long start, int nr_pages) +static inline bool gup_fast_permitted(unsigned long start, unsigned long end) { - unsigned long len, end; - - len = (unsigned long)nr_pages << PAGE_SHIFT; - end = start + len; - if (end < start) - return false; if (end >> __VIRTUAL_MASK_SHIFT) return false; return true; diff --git a/mm/gup.c b/mm/gup.c index 9775f7675653..e7566f5ff9cf 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -2122,13 +2122,9 @@ static void gup_pgd_range(unsigned long addr, unsigned long end, * Check if it's allowed to use __get_user_pages_fast() for the range, or * we need to fall back to the slow version: */ -bool gup_fast_permitted(unsigned long start, int nr_pages) +static bool gup_fast_permitted(unsigned long start, unsigned long end) { - unsigned long len, end; - - len = (unsigned long) nr_pages << PAGE_SHIFT; - end = start + len; - return end >= start; + return true; } #endif @@ -2149,6 +2145,8 @@ int __get_user_pages_fast(unsigned long start, int nr_pages, int write, len = (unsigned long) nr_pages << PAGE_SHIFT; end = start + len; + if (end < start) + return 0; if (unlikely(!access_ok((void __user *)start, len))) return 0; @@ -2164,7 +2162,7 @@ int __get_user_pages_fast(unsigned long start, int nr_pages, int write, * block IPIs that come from THPs splitting. */ - if (gup_fast_permitted(start, nr_pages)) { + if (gup_fast_permitted(start, end)) { local_irq_save(flags); gup_pgd_range(start, end, write ? FOLL_WRITE : 0, pages, &nr); local_irq_restore(flags); @@ -2223,13 +2221,12 @@ int get_user_pages_fast(unsigned long start, int nr_pages, len = (unsigned long) nr_pages << PAGE_SHIFT; end = start + len; - if (nr_pages <= 0) + if (end < start) return 0; - if (unlikely(!access_ok((void __user *)start, len))) return -EFAULT; - if (gup_fast_permitted(start, nr_pages)) { + if (gup_fast_permitted(start, end)) { local_irq_disable(); gup_pgd_range(addr, end, gup_flags, pages, &nr); local_irq_enable();
Pass in the already calculated end value instead of recomputing it, and leave the end > start check in the callers instead of duplicating them in the arch code. Signed-off-by: Christoph Hellwig <hch@lst.de> --- arch/s390/include/asm/pgtable.h | 8 +------- arch/x86/include/asm/pgtable_64.h | 8 +------- mm/gup.c | 17 +++++++---------- 3 files changed, 9 insertions(+), 24 deletions(-)