On 19.08.24 19:12, Michal Hocko wrote:

On Mon 19-08-24 14:49:55, David Hildenbrand wrote:

quoted

On 19.08.24 14:48, Barry Song wrote:

[...]

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 60742d057b05..d2c37f8f8d09 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c

@@ -4668,8 +4668,10 @@ struct page *__alloc_pages_noprof(gfp_t gfp, unsigned int order,
           * There are several places where we assume that the order value is sane
           * so bail out early if the request is out of bound.
           */
-     if (WARN_ON_ONCE_GFP(order > MAX_PAGE_ORDER, gfp))
+     if (WARN_ON_ONCE_GFP(order > MAX_PAGE_ORDER, gfp)) {
+             BUG_ON(gfp & __GFP_NOFAIL);
                  return NULL;
+     }

[...]

quoted

quoted

Returning NULL doesn't necessarily crash the caller's process, p->field,
*(p + offset) deference could be used by hackers to exploit the system.

See my other reply to Michal: why do we even allow to specify them
separately and not simply let one enforce the other?

Are you replying to this patch? This is not about a combination of
flags. This is about the above (and other similar) boundary checks which
return NULL if the size is deemed incorrect. 
I think those are potential
problems because it could be a lack of input check which could be turned
into a potentially malicious code. Because unchecked (return value
because NOFAIL never fails, right?) return value might even not OOPs and
become a silent read/write into memory.

Right, that's a different kind of issue than the simple "don't pass 
stupid flag combinations" thing where "we'll fix that up for you" is 
more reasonable.

Possibly NOFAIL allocations with an allocation sizes that is not a small 
compile-time constant already has a bad smell to it, but I'm sure there 
are reasonable exceptions ...

Whether to BUG_ON or simply loop for ever in the allocator if somebody
requests non-sleeping NOFAIL allocation is a different story.

Right, "warn + loop forever" is one alternative where you could at least 
keep the system alive to some degree. Satisfying a large allocation 
might take a long time, satisfying a "too large" allocation would take 
forever.

But as Linus says, it's all workarounds for other buggy code, to make 
buggy code less exploitable, maybe, ...

-- 
Cheers,

David / dhildenb