On 7/22/24 9:23 AM, Michal Hocko wrote: > On Sat 20-07-24 08:36:16, Barry Song wrote: >> On Fri, Jul 19, 2024 at 9:30 PM Michal Hocko <mhocko@xxxxxxxx> wrote: >> > >> > On Fri 19-07-24 21:02:10, Barry Song wrote: >> > > what about an earlier WARN_ON, for example, even before we begin to >> > > try the allocation? >> > >> > Page allocator is a hot path and adding checks for something that >> > shouldn't really even exist is not a great addition there IMHO. >> >> I would argue adding checks for something that shouldn't really >> even exist is precisely the point of having those checks. These >> checks help ensure the integrity and robustness of the system >> by catching unexpected conditions. I agree that the page allocator >> is a hot path, but adding one line might not cause noticeable >> overhead, especially considering that alloc_pages() already >> contains a few hundred lines of code. > > We do not add stuff like that into hot path. It should be acceptable with CONFIG_DEBUG_VM. Or in __alloc_pages_slowpath() it could be placed higher even without CONFIG_DEBUG_VM. Right now it's rather hard to reach, espcially for GPF_ATOMIC. > >> Regardless, let me try to summarize the discussions. Unless >> anyone has better ideas, v2 series might start with the following >> actions: >> >> 1. Update the documentation for GFP_NOFAIL to explicitly state >> that non-wait GFP_NOFAIL is not legal and should be avoided. >> This will provide a basis for other subsystems to explicitly >> reject anyone using GFP_ATOMIC | GFP_NOFAIL, etc. > > No objection to that. > >> 2. Add BUG_ON() at the existing points where we return NULL >> for GFP_NOFAIL - __alloc_pages_slowpath() , kmalloc, vmalloc >> to avoid exposing security vulnerabilities. > > Let's see how this goes. In __alloc_pages_noprof() we also have if (WARN_ON_ONCE_GFP(order > MAX_PAGE_ORDER, gfp)) return NULL; This should BUG_ON for __GFP_NOFAIL, same as the overflowing kmalloc_array(). > >> 3. Raise a bug report to the vdpa maintainer, requesting that they >> either drop GFP_ATOMIC or GFP_NOFAIL based on whether >> their context is atomic. If GFP_NOFAIL is dropped, ask for an >> explicit check on the return value. You could do it right now, based on the warning we have although it's hard to reach. > GPF_ATOMIC is likely used because of write_lock(&domain->bounce_lock) > vduse_domain_remove_user_bounce_pages is iself called with spin lock held > from vduse_domain_release. So you would need some pre-allocation.