| Message ID | 20220630084124.691207-5-linus.walleij@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: Pass pointers to page accessors | expand |
On Thu, Jun 30, 2022 at 10:41:23AM +0200, Linus Walleij wrote: > Functions that work on a pointer to virtual memory such as > virt_to_pfn() and users of that function such as > virt_to_page() are supposed to pass a pointer to virtual > memory, ideally a (void *) or other pointer. However since > many architectures implement virt_to_pfn() as a macro, > this function becomes polymorphic and accepts both a > (unsigned long) and a (void *). I wonder if there is merit to convert x86 to use an inline after this goes in to prevent this polymorphic mistake? > If we instead implement a proper virt_to_pfn(void *addr) > function the following happens (occurred on arch/arm): > > mm/gup.c: In function '__get_user_pages_locked': > mm/gup.c:1599:49: warning: passing argument 1 of 'virt_to_pfn' > makes pointer from integer without a cast [-Wint-conversion] > pages[i] = virt_to_page(start); > > Fix this with an explicit cast. > > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: linux-mm@kvack.org > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > mm/gup.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/mm/gup.c b/mm/gup.c > index 551264407624..543c68da65f1 100644 > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -1672,7 +1672,7 @@ static long __get_user_pages_locked(struct mm_struct *mm, unsigned long start, > goto finish_or_fault; > > if (pages) { > - pages[i] = virt_to_page(start); > + pages[i] = virt_to_page((void *)start); That 'start' is actually a userspace addres so it technically is a __user pointer, but the missing context here is that this is a NOMMU special function, so I guess it is right as is? Still, it is NOP to what it is now so: Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> Jason
On Fri, Jul 1, 2022 at 2:29 AM Jason Gunthorpe <jgg@ziepe.ca> wrote: > On Thu, Jun 30, 2022 at 10:41:23AM +0200, Linus Walleij wrote: > > Functions that work on a pointer to virtual memory such as > > virt_to_pfn() and users of that function such as > > virt_to_page() are supposed to pass a pointer to virtual > > memory, ideally a (void *) or other pointer. However since > > many architectures implement virt_to_pfn() as a macro, > > this function becomes polymorphic and accepts both a > > (unsigned long) and a (void *). > > I wonder if there is merit to convert x86 to use an inline after this > goes in to prevent this polymorphic mistake? I have a patch like that, x86 uses the asm generic version IIUC: https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-integrator.git/patch/?id=ed4befbf783c06820e9d4620c7fd254a36d608fe There was also Xen: https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-integrator.git/patch/?id=43ab4e5d6738273055dca21031e23b53b3721889 The plan is to trickle in a few of those after I fixed up all the mm and drivers that were doing this with unsigned longs. I managed to convert all architectures except one to use static inlines for virt_to_pfn. I was actually a bit surprised how few sites there were. Yours, Linus Walleij
diff --git a/mm/gup.c b/mm/gup.c index 551264407624..543c68da65f1 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -1672,7 +1672,7 @@ static long __get_user_pages_locked(struct mm_struct *mm, unsigned long start, goto finish_or_fault; if (pages) { - pages[i] = virt_to_page(start); + pages[i] = virt_to_page((void *)start); if (pages[i]) get_page(pages[i]); }
Functions that work on a pointer to virtual memory such as virt_to_pfn() and users of that function such as virt_to_page() are supposed to pass a pointer to virtual memory, ideally a (void *) or other pointer. However since many architectures implement virt_to_pfn() as a macro, this function becomes polymorphic and accepts both a (unsigned long) and a (void *). If we instead implement a proper virt_to_pfn(void *addr) function the following happens (occurred on arch/arm): mm/gup.c: In function '__get_user_pages_locked': mm/gup.c:1599:49: warning: passing argument 1 of 'virt_to_pfn' makes pointer from integer without a cast [-Wint-conversion] pages[i] = virt_to_page(start); Fix this with an explicit cast. Cc: Andrew Morton <akpm@linux-foundation.org> Cc: linux-mm@kvack.org Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- mm/gup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)