linux-kernel - Re: [PATCH v2] fs, mm: account filp cache to kmemcg

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] [day] [month] [year] [list]

Message-ID: <20171012075451.3lfzctfusoctu3p2@dhcp22.suse.cz>
Date:   Thu, 12 Oct 2017 09:54:51 +0200
From:   Michal Hocko <mhocko@...nel.org>
To:     Shakeel Butt <shakeelb@...gle.com>
Cc:     Alexander Viro <viro@...iv.linux.org.uk>,
        Vladimir Davydov <vdavydov.dev@...il.com>,
        Greg Thelen <gthelen@...gle.com>,
        Johannes Weiner <hannes@...xchg.org>,
        Andrew Morton <akpm@...ux-foundation.org>, linux-mm@...ck.org,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] fs, mm: account filp cache to kmemcg

On Wed 11-10-17 12:03:59, Shakeel Butt wrote:
> The allocations from filp cache can be directly triggered by user
> space applications. A buggy application can consume a significant
> amount of unaccounted system memory. Though we have not noticed
> such buggy applications in our production but upon close inspection,
> we found that a lot of machines spend very significant amount of
> memory on these caches.
> 
> One way to limit allocations from filp cache is to set system level
> limit of maximum number of open files. However this limit is shared
> between different users on the system and one user can hog this
> resource. To cater that, we can charge filp to kmemcg and set the
> maximum limit very high and let the memory limit of each user limit
> the number of files they can open and indirectly limiting their
> allocations from filp cache.
> 
> One side effect of this change is that it will allow _sysctl() to
> return ENOMEM and the man page of _sysctl() does not specify that.
> However the man page also discourages to use _sysctl() at all.
> 
> Signed-off-by: Shakeel Butt <shakeelb@...gle.com>

OK, this makes more sense than the original patch. struct file is not
really large (248B on my system) so I am not sure how much this helps
though. Anyway, I have no objections to the patch but I do not feel
qualified to ack it either.

> ---
> 
> Changelog since v1:
> - removed names_cache charging to kmemcg
> 
>  fs/file_table.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/file_table.c b/fs/file_table.c
> index 61517f57f8ef..567888cdf7d3 100644
> --- a/fs/file_table.c
> +++ b/fs/file_table.c
> @@ -312,7 +312,7 @@ void put_filp(struct file *file)
>  void __init files_init(void)
>  {
>  	filp_cachep = kmem_cache_create("filp", sizeof(struct file), 0,
> -			SLAB_HWCACHE_ALIGN | SLAB_PANIC, NULL);
> +			SLAB_HWCACHE_ALIGN | SLAB_PANIC | SLAB_ACCOUNT, NULL);
>  	percpu_counter_init(&nr_files, 0, GFP_KERNEL);
>  }
>  
> -- 
> 2.15.0.rc0.271.g36b669edcc-goog

-- 
Michal Hocko
SUSE Labs