Johannes Weiner [off-list ref] writes:

On Wed, Jan 26, 2011 at 09:33:09AM -0800, Greg Thelen wrote:

quoted

Johannes Weiner [off-list ref] writes:

quoted

On Wed, Jan 26, 2011 at 12:29:15AM -0800, Greg Thelen wrote:

quoted

mem_cgroup_get_limit() returns a byte limit as a unsigned 64 bit value,
which is converted to a page count by mem_cgroup_out_of_memory().  Prior
to this patch the conversion could overflow on 32 bit platforms
yielding a limit of zero.

Balbir: It can truncate, because the conversion shrinks the required
bits of this 64-bit number by only PAGE_SHIFT (12).  Trying to store
the resulting up to 52 significant bits in a 32-bit integer will cut
up to 20 significant bits off.

quoted

Signed-off-by: Greg Thelen <redacted>
---
 mm/oom_kill.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 7dcca55..3fcac51 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c

@@ -538,7 +538,7 @@ void mem_cgroup_out_of_memory(struct mem_cgroup *mem, gfp_t gfp_mask)
 	struct task_struct *p;
 
 	check_panic_on_oom(CONSTRAINT_MEMCG, gfp_mask, 0, NULL);
-	limit = mem_cgroup_get_limit(mem) >> PAGE_SHIFT;
+	limit = min(mem_cgroup_get_limit(mem) >> PAGE_SHIFT, (u64)ULONG_MAX);

I would much prefer using min_t(u64, ...).  To make it really, really
explicit that this is 64-bit arithmetic.  But that is just me, no
correctness issue.

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

I agree that min_t() is clearer.  Does the following look better?

Sweet, thank you Greg!

quoted

Author: Greg Thelen [off-list ref]
Date:   Wed Jan 26 00:05:59 2011 -0800

    oom: handle truncation in mem_cgroup_out_of_memory()
    
    mem_cgroup_get_limit() returns a byte limit as an unsigned 64 bit value.
    mem_cgroup_out_of_memory() converts this byte limit to an unsigned long
    page count.  Prior to this patch, the 32 bit version of
    mem_cgroup_out_of_memory() would silently truncate the most significant
    20 bits from byte limit when constructing the limit as a page count.
    For byte limits with the lowest 44 bits set to zero, this truncation
    would compute a page limit of zero.
    
    This patch checks for such large byte limits that cannot be converted to
    page counts without loosing information.  In such situations, where a 32
    bit page counter is too small to represent the corresponding byte count,
    select a maximal page count.
    
    Signed-off-by: Greg Thelen [off-list ref]

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

That being said, does this have any practical impact at all?  I mean,
this code runs when the cgroup limit is breached.  But if the number
of allowed pages (not bytes!) can not fit into 32 bits, it means you
have a group of processes using more than 16T.  On a 32-bit machine.

The value of this patch is up for debate.  I do not have an example
situation where this truncation causes the wrong thing to happen.  I
suppose it might be possible for a racing update to
memory.limit_in_bytes which grows the limit from a reasonable (example:
100M) limit to a large limit (example 1<<45) could benefit from this
patch.  I admit that this case seems pathological and may not be likely
or even worth bothering over.  If neither the memcg nor the oom
maintainers want the patch, then feel free to drop it.  I just noticed
the issue and thought it might be worth addressing.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>