[PATCH v1 0/3] fuse: a few small fixes

[PATCH v1 0/3] fuse: a few small fixes

[Alexander Mikhalitsyn]

Found by chance while working on support for idmapped mounts in fuse
(will send series soon :-)).

Cc: Miklos Szeredi <miklos@szeredi.hu>
Cc: <linux-fsdevel@vger.kernel.org>
Cc: <linux-kernel@vger.kernel.org>
Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>

Alexander Mikhalitsyn (3):
  fuse: fix typo for fuse_permission comment
  fuse: use GFP_KERNEL_ACCOUNT for allocations in fuse_dev_alloc
  fuse: __kuid_val/__kgid_val helpers in fuse_fill_attr_from_inode()

 fs/fuse/dir.c   | 2 +-
 fs/fuse/inode.c | 8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

-- 
2.34.1

[PATCH v1 1/3] fuse: fix typo for fuse_permission comment

[Alexander Mikhalitsyn]

Found by chance while working on support for idmapped mounts in fuse.

Cc: Miklos Szeredi <miklos@szeredi.hu>
Cc: <linux-fsdevel@vger.kernel.org>
Cc: <linux-kernel@vger.kernel.org>
Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
---
 fs/fuse/dir.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index d19cbf34c634..6f5f9ff95380 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -1485,7 +1485,7 @@ static int fuse_perm_getattr(struct inode *inode, int mask)
  *
  * 1) Local access checking ('default_permissions' mount option) based
  * on file mode.  This is the plain old disk filesystem permission
- * modell.
+ * model.
  *
  * 2) "Remote" access checking, where server is responsible for
  * checking permission in each inode operation.  An exception to this
-- 
2.34.1

[PATCH v1 2/3] fuse: use GFP_KERNEL_ACCOUNT for allocations in fuse_dev_alloc

[Alexander Mikhalitsyn]

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.

Cc: Miklos Szeredi <miklos@szeredi.hu>
Cc: Amir Goldstein <amir73il@gmail.com>
Cc: <linux-fsdevel@vger.kernel.org>
Cc: <linux-kernel@vger.kernel.org>
Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.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 2a6d44f91729..b8636b5e79dc 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -1415,11 +1415,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;
-- 
2.34.1

[PATCH v1 3/3] fuse: __kuid_val/__kgid_val helpers in fuse_fill_attr_from_inode()

[Alexander Mikhalitsyn]

For the sake of consistency, let's use these helpers to extract
{u,g}id_t values from k{u,g}id_t ones.

There are no functional changes, just to make code cleaner.

Cc: Christian Brauner <brauner@kernel.org>
Cc: Miklos Szeredi <miklos@szeredi.hu>
Cc: <linux-fsdevel@vger.kernel.org>
Cc: <linux-kernel@vger.kernel.org>
Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.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 b8636b5e79dc..ab824a8908b7 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -1489,8 +1489,8 @@ static void fuse_fill_attr_from_inode(struct fuse_attr *attr,
 		.ctimensec	= ctime.tv_nsec,
 		.mode		= fi->inode.i_mode,
 		.nlink		= fi->inode.i_nlink,
-		.uid		= fi->inode.i_uid.val,
-		.gid		= fi->inode.i_gid.val,
+		.uid		= __kuid_val(fi->inode.i_uid),
+		.gid		= __kgid_val(fi->inode.i_gid),
 		.rdev		= fi->inode.i_rdev,
 		.blksize	= 1u << fi->inode.i_blkbits,
 	};
-- 
2.34.1

Re: [PATCH v1 3/3] fuse: __kuid_val/__kgid_val helpers in fuse_fill_attr_from_inode()

[Christian Brauner]

On Fri, Jan 05, 2024 at 04:21:29PM +0100, Alexander Mikhalitsyn wrote:
> For the sake of consistency, let's use these helpers to extract
> {u,g}id_t values from k{u,g}id_t ones.
> 
> There are no functional changes, just to make code cleaner.
> 
> Cc: Christian Brauner <brauner@kernel.org>
> Cc: Miklos Szeredi <miklos@szeredi.hu>
> Cc: <linux-fsdevel@vger.kernel.org>
> Cc: <linux-kernel@vger.kernel.org>
> Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
> ---

Looks good to me,
Reviewed-by: Christian Brauner <brauner@kernel.org>

Re: [PATCH v1 0/3] fuse: a few small fixes

[Aleksandr Mikhalitsyn]

Gentle ping.

On Fri, Jan 5, 2024 at 4:21 PM Alexander Mikhalitsyn
<aleksandr.mikhalitsyn@canonical.com> wrote:
>
> Found by chance while working on support for idmapped mounts in fuse
> (will send series soon :-)).
>
> Cc: Miklos Szeredi <miklos@szeredi.hu>
> Cc: <linux-fsdevel@vger.kernel.org>
> Cc: <linux-kernel@vger.kernel.org>
> Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
>
> Alexander Mikhalitsyn (3):
>   fuse: fix typo for fuse_permission comment
>   fuse: use GFP_KERNEL_ACCOUNT for allocations in fuse_dev_alloc
>   fuse: __kuid_val/__kgid_val helpers in fuse_fill_attr_from_inode()
>
>  fs/fuse/dir.c   | 2 +-
>  fs/fuse/inode.c | 8 ++++----
>  2 files changed, 5 insertions(+), 5 deletions(-)
>
> --
> 2.34.1
>

Re: [PATCH v1 3/3] fuse: __kuid_val/__kgid_val helpers in fuse_fill_attr_from_inode()

[Alexander Mikhalitsyn]

On Mon, 8 Jan 2024 12:37:07 +0100
Christian Brauner <brauner@kernel.org> wrote:

> On Fri, Jan 05, 2024 at 04:21:29PM +0100, Alexander Mikhalitsyn wrote:
> > For the sake of consistency, let's use these helpers to extract
> > {u,g}id_t values from k{u,g}id_t ones.
> > 
> > There are no functional changes, just to make code cleaner.
> > 
> > Cc: Christian Brauner <brauner@kernel.org>
> > Cc: Miklos Szeredi <miklos@szeredi.hu>
> > Cc: <linux-fsdevel@vger.kernel.org>
> > Cc: <linux-kernel@vger.kernel.org>
> > Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
> > ---
> 
> Looks good to me,
> Reviewed-by: Christian Brauner <brauner@kernel.org>

Thanks!

Kind regards,
Alex

Re: [PATCH v1 2/3] fuse: use GFP_KERNEL_ACCOUNT for allocations in fuse_dev_alloc

[Miklos Szeredi]

On Fri, 5 Jan 2024 at 16:21, Alexander Mikhalitsyn
<aleksandr.mikhalitsyn@canonical.com> 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.

Are the rules about when to use __GFP_ACCOUNT and when not documented somewhere?

I notice that most filesystem objects are allocated with
__GFP_ACCOUNT, but struct super_block isn't.  Is there a reason for
that?

Thanks,
Miklos

Re: [PATCH v1 1/3] fuse: fix typo for fuse_permission comment

[Miklos Szeredi]

On Fri, 5 Jan 2024 at 16:21, Alexander Mikhalitsyn
<aleksandr.mikhalitsyn@canonical.com> wrote:
>
> Found by chance while working on support for idmapped mounts in fuse.
>
> Cc: Miklos Szeredi <miklos@szeredi.hu>
> Cc: <linux-fsdevel@vger.kernel.org>
> Cc: <linux-kernel@vger.kernel.org>
> Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>

Applied, thanks.

Re: [PATCH v1 3/3] fuse: __kuid_val/__kgid_val helpers in fuse_fill_attr_from_inode()

[Miklos Szeredi]

On Fri, 5 Jan 2024 at 16:22, Alexander Mikhalitsyn
<aleksandr.mikhalitsyn@canonical.com> wrote:
>
> For the sake of consistency, let's use these helpers to extract
> {u,g}id_t values from k{u,g}id_t ones.
>
> There are no functional changes, just to make code cleaner.
>
> Cc: Christian Brauner <brauner@kernel.org>
> Cc: Miklos Szeredi <miklos@szeredi.hu>
> Cc: <linux-fsdevel@vger.kernel.org>
> Cc: <linux-kernel@vger.kernel.org>
> Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>

Applied, thanks.

Miklos

Re: [PATCH v1 2/3] fuse: use GFP_KERNEL_ACCOUNT for allocations in fuse_dev_alloc

[Aleksandr Mikhalitsyn]

On Mon, Feb 26, 2024 at 12:01 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Fri, 5 Jan 2024 at 16:21, Alexander Mikhalitsyn
> <aleksandr.mikhalitsyn@canonical.com> 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.

Hi Miklos,

Sorry, this thread just got lost in my inbox. I was revisiting and
rebasing fuse idmapped mounts support series and found this again.

>
> Are the rules about when to use __GFP_ACCOUNT and when not documented somewhere?

The only doc I found is this (memory-allocation.rst):
>Untrusted allocations triggered from userspace should be a subject
>of kmem accounting and must have ``__GFP_ACCOUNT`` bit set.

>
> I notice that most filesystem objects are allocated with
> __GFP_ACCOUNT, but struct super_block isn't.  Is there a reason for
> that?

I guess that it just wasn't yet covered with memcg accounting. I can
send a patch to account struct super_block too.

These days, it's pretty safe to use __GFP_ACCOUNT almost anywhere,
because even if memcg is not
determined in a current caller context then memcg charge will be
skipped (look into __memcg_slab_post_alloc_hook() function).

Let's ask what our friends who take care of mmcontrol.c think about this.
+CC Johannes Weiner <hannes@cmpxchg.org>
+CC Michal Hocko <mhocko@kernel.org>
+CC Roman Gushchin <roman.gushchin@linux.dev>
+CC Shakeel Butt <shakeel.butt@linux.dev>

I have also added Christian because he might be interested in
accounting for struct super_block.

Kind regards,
Alex

>
> Thanks,
> Miklos

Re: [PATCH v1 2/3] fuse: use GFP_KERNEL_ACCOUNT for allocations in fuse_dev_alloc

[Miklos Szeredi]

On Thu, 18 Jul 2024 at 12:01, Aleksandr Mikhalitsyn
<aleksandr.mikhalitsyn@canonical.com> wrote:

> I have also added Christian because he might be interested in
> accounting for struct super_block.

IMO doing it in the VFS as well makes much more sense than just the
fuse specific parts.

Thanks,
Miklos