linux-ext4 - Re: [PATCH] fscrypt: Copy the memcg information to the ciphertext page

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: <Y9mH0PCcZoGPryXw@slm.duckdns.org>
Date:   Tue, 31 Jan 2023 11:27:44 -1000
From:   Tejun Heo <tj@...nel.org>
To:     Matthew Wilcox <willy@...radead.org>
Cc:     Eric Biggers <ebiggers@...nel.org>,
        "Theodore Y . Ts'o" <tytso@....edu>,
        Jaegeuk Kim <jaegeuk@...nel.org>,
        linux-fscrypt@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        linux-ext4@...r.kernel.org, linux-f2fs-devel@...ts.sourceforge.net,
        stable@...r.kernel.org, cgroups@...r.kernel.org
Subject: Re: [PATCH] fscrypt: Copy the memcg information to the ciphertext
 page

Hello,

On Sun, Jan 29, 2023 at 09:26:57PM +0000, Matthew Wilcox wrote:
> > > diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
> > > index e78be66bbf01..a4e76f96f291 100644
> > > --- a/fs/crypto/crypto.c
> > > +++ b/fs/crypto/crypto.c
> > > @@ -205,6 +205,9 @@ struct page *fscrypt_encrypt_pagecache_blocks(struct page *page,
> > >  	}
> > >  	SetPagePrivate(ciphertext_page);
> > >  	set_page_private(ciphertext_page, (unsigned long)page);
> > > +#ifdef CONFIG_MEMCG
> > > +	ciphertext_page->memcg_data = page->memcg_data;
> > > +#endif
> > >  	return ciphertext_page;
> > >  }
> > 
> > Nothing outside mm/ and include/linux/memcontrol.h does anything with memcg_data
> > directly.  Are you sure this is the right thing to do here?
> 
> Nope ;-)  Happy to hear from people who know more about cgroups than I
> do.  Adding some more ccs.
> 
> > Also, this patch causes the following:
> 
> Oops.  Clearly memcg_data needs to be set to NULL before we free it.

These can usually be handled by explicitly associating the bio's to the
desired cgroups using one of bio_associate_blkg*() or
bio_clone_blkg_association(). It is possible to go through memcg ownership
too using set_active_memcg() so that the page is owned by the target cgroup;
however, the page ownership doesn't directly map to IO ownership as the
relationship depends on the type of the page (e.g. IO ownership for
pagecache writeback is determined per-inode, not per-page). If the in-flight
pages are limited, it probably is better to set bio association directly.

Thanks.

-- 
tejun