linux-kernel - Re: [PATCH -v2 -mm] memcg: do not trigger OOM from add_to_page_cache

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20121128152631.GT24381@cmpxchg.org>
Date:	Wed, 28 Nov 2012 10:26:31 -0500
From:	Johannes Weiner <hannes@...xchg.org>
To:	Michal Hocko <mhocko@...e.cz>
Cc:	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>,
	azurIt <azurit@...ox.sk>, linux-kernel@...r.kernel.org,
	linux-mm@...ck.org, cgroups mailinglist <cgroups@...r.kernel.org>
Subject: Re: [PATCH -v2 -mm] memcg: do not trigger OOM from
 add_to_page_cache_locked

On Tue, Nov 27, 2012 at 09:59:44PM +0100, Michal Hocko wrote:
> @@ -3863,7 +3862,7 @@ int mem_cgroup_cache_charge(struct page *page, struct mm_struct *mm,
>  		return 0;
>  
>  	if (!PageSwapCache(page))
> -		ret = mem_cgroup_charge_common(page, mm, gfp_mask, type);
> +		ret = mem_cgroup_charge_common(page, mm, gfp_mask, type, oom);
>  	else { /* page is swapcache/shmem */
>  		ret = __mem_cgroup_try_charge_swapin(mm, page,
>  						     gfp_mask, &memcg);

I think you need to pass it down the swapcache path too, as that is
what happens when the shmem page written to is in swap and has been
read into swapcache by the time of charging.

> @@ -1152,8 +1152,16 @@ repeat:
>  				goto failed;
>  		}
>  
> +		 /*
> +                  * Cannot trigger OOM even if gfp_mask would allow that
> +                  * normally because we might be called from a locked
> +                  * context (i_mutex held) if this is a write lock or
> +                  * fallocate and that could lead to deadlocks if the
> +                  * killed process is waiting for the same lock.
> +		  */

Indentation broken?

>  		error = mem_cgroup_cache_charge(page, current->mm,
> -						gfp & GFP_RECLAIM_MASK);
> +						gfp & GFP_RECLAIM_MASK,
> +						sgp < SGP_WRITE);

The code tests for read-only paths a bunch of times using

	sgp != SGP_WRITE && sgp != SGP_FALLOC

Would probably be more consistent and more robust to use this here as
well?

> @@ -1209,7 +1217,8 @@ repeat:
>  		SetPageSwapBacked(page);
>  		__set_page_locked(page);
>  		error = mem_cgroup_cache_charge(page, current->mm,
> -						gfp & GFP_RECLAIM_MASK);
> +						gfp & GFP_RECLAIM_MASK,
> +						sgp < SGP_WRITE);

Same.

Otherwise, the patch looks good to me, thanks for persisting :)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/