linux-kernel - Re: [PATCH] fuse: use GFP_KERNEL_ACCOUNT for allocations in fuse_dev

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: <6emhfruzu3fujdkpld3j44qz5x2sg54xe7vjfqms552cammrhs@pef3mqv5p3qy>
Date: Wed, 14 Aug 2024 09:54:00 -0700
From: Shakeel Butt <shakeel.butt@...ux.dev>
To: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@...onical.com>
Cc: miklos@...redi.hu, Amir Goldstein <amir73il@...il.com>, 
	Christian Brauner <brauner@...nel.org>, Johannes Weiner <hannes@...xchg.org>, 
	Michal Hocko <mhocko@...nel.org>, Roman Gushchin <roman.gushchin@...ux.dev>, 
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] fuse: use GFP_KERNEL_ACCOUNT for allocations in
 fuse_dev_alloc

Hi Alexander,

On Wed, Aug 14, 2024 at 01:23:56PM GMT, Alexander Mikhalitsyn wrote:
> fuse_dev_alloc() is called from the process context and it makes
> sense to properly account allocated memory to the kmemcg as these
> allocations are for long living objects.
> 
> Link: https://lore.kernel.org/all/20240105152129.196824-3-aleksandr.mikhalitsyn@canonical.com/
> 
> Cc: Miklos Szeredi <miklos@...redi.hu>
> Cc: Amir Goldstein <amir73il@...il.com>
> Cc: Christian Brauner <brauner@...nel.org>
> Cc: Johannes Weiner <hannes@...xchg.org>
> Cc: Michal Hocko <mhocko@...nel.org>
> Cc: Roman Gushchin <roman.gushchin@...ux.dev>
> Cc: Shakeel Butt <shakeel.butt@...ux.dev>
> Cc: <linux-fsdevel@...r.kernel.org>
> Cc: <linux-kernel@...r.kernel.org>
> Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@...onical.com>
> ---
>  fs/fuse/inode.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> index ed4c2688047f..6dae007186e1 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -1486,11 +1486,11 @@ struct fuse_dev *fuse_dev_alloc(void)
>  	struct fuse_dev *fud;
>  	struct list_head *pq;
>  
> -	fud = kzalloc(sizeof(struct fuse_dev), GFP_KERNEL);
> +	fud = kzalloc(sizeof(struct fuse_dev), GFP_KERNEL_ACCOUNT);
>  	if (!fud)
>  		return NULL;
>  
> -	pq = kcalloc(FUSE_PQ_HASH_SIZE, sizeof(struct list_head), GFP_KERNEL);
> +	pq = kcalloc(FUSE_PQ_HASH_SIZE, sizeof(struct list_head), GFP_KERNEL_ACCOUNT);
>  	if (!pq) {
>  		kfree(fud);
>  		return NULL;

No objection from me but let me ask couple of questions to make sure we
know the impact of this change. It seems like this function is called
during mount() operation. Is that correct? If yes then basically the
admin process or node controller is being charged for this memory.
Nothing bad but this info should be in commit message. Also what is the
lifetime of these objects? From mount to unmount? Please add that info
as well. There are other unaccounted allocations in the fuse fs. Is
there a followup plan to include those as well?

thanks,
Shakeel