Date: Tue, 23 Jan 2024 16:39:31 +0100
From: Christian Brauner <brauner@...nel.org>
To: Vinicius Costa Gomes <vinicius.gomes@...el.com>
Cc: Amir Goldstein <amir73il@...il.com>, hu1.chen@...el.com, 
	miklos@...redi.hu, malini.bhandaru@...el.com, tim.c.chen@...el.com, 
	mikko.ylinen@...el.com, lizhen.you@...el.com, linux-unionfs@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-fsdevel <linux-fsdevel@...r.kernel.org>, 
	Linus Torvalds <torvalds@...ux-foundation.org>, David Howells <dhowells@...hat.com>, 
	Seth Forshee <sforshee@...nel.org>
Subject: Re: [RFC] HACK: overlayfs: Optimize overlay/restore creds

On Tue, Dec 19, 2023 at 06:33:59AM -0800, Vinicius Costa Gomes wrote:
> Amir Goldstein <amir73il@...il.com> writes:
> 
> > On Mon, Dec 18, 2023 at 11:57 PM Vinicius Costa Gomes
> > <vinicius.gomes@...el.com> wrote:
> >>
> >> Christian Brauner <brauner@...nel.org> writes:
> >>
> >> >> > Yes, the important thing is that an object cannot change
> >> >> > its non_refcount property during its lifetime -
> >> >>
> >> >> ... which means that put_creds_ref() should assert that
> >> >> there is only a single refcount - the one handed out by
> >> >> prepare_creds_ref() before removing non_refcount or
> >> >> directly freeing the cred object.
> >> >>
> >> >> I must say that the semantics of making a non-refcounted copy
> >> >> to an object whose lifetime is managed by the caller sounds a lot
> >> >> less confusing to me.
> >> >
> >> > So can't we do an override_creds() variant that is effectively just:
> >
> > Yes, I think that we can....
> >
> >> >
> >> > /* caller guarantees lifetime of @new */
> >> > const struct cred *foo_override_cred(const struct cred *new)
> >> > {
> >> >       const struct cred *old = current->cred;
> >> >       rcu_assign_pointer(current->cred, new);
> >> >       return old;
> >> > }
> >> >
> >> > /* caller guarantees lifetime of @old */
> >> > void foo_revert_creds(const struct cred *old)
> >> > {
> >> >       const struct cred *override = current->cred;
> >> >       rcu_assign_pointer(current->cred, old);
> >> > }
> >> >
> >
> > Even better(?), we can do this in the actual guard helpers to
> > discourage use without a guard:
> >
> > struct override_cred {
> >         struct cred *cred;
> > };
> >
> > DEFINE_GUARD(override_cred, struct override_cred *,
> >             override_cred_save(_T),
> >             override_cred_restore(_T));
> >
> > ...
> >
> > void override_cred_save(struct override_cred *new)
> > {
> >         new->cred = rcu_replace_pointer(current->cred, new->cred, true);
> > }
> >
> > void override_cred_restore(struct override_cred *old)
> > {
> >         rcu_assign_pointer(current->cred, old->cred);
> > }
> >
> >> > Maybe I really fail to understand this problem or the proposed solution:
> >> > the single reference that overlayfs keeps in ovl->creator_cred is tied
> >> > to the lifetime of the overlayfs superblock, no? And anyone who needs a
> >> > long term cred reference e.g, file->f_cred will take it's own reference
> >> > anyway. So it should be safe to just keep that reference alive until
> >> > overlayfs is unmounted, no? I'm sure it's something quite obvious why
> >> > that doesn't work but I'm just not seeing it currently.
> >>
> >> My read of the code says that what you are proposing should work. (what
> >> I am seeing is that in the "optimized" cases, the only practical effect
> >> of override/revert is the rcu_assign_pointer() dance)
> >>
> >> I guess that the question becomes: Do we want this property (that the
> >> 'cred' associated with a subperblock/similar is long lived and the
> >> "inner" refcount can be omitted) to be encoded in the constructor? Or do
> >> we want it to be "encoded" in a call by call basis?
> >>
> >
> > Neither.
> >
> > Christian's proposal does not involve marking the cred object as
> > long lived, which looks a much better idea to me.
> >
> 
> In my mind, I am reading his suggestion as the flag "long lived
> cred/lives long enough" is "in our brains" vs. what I proposed that the
> flag was "in the object". The effect of the "flag" is the same: when to
> use a lighter version (no refcount) of override/revert.
> 
> What I was thinking was more more under the covers, implicit. And I can
> see the advantages of having them more explicit.
> 
> > The performance issues you observed are (probably) due to get/put
> > of cred refcount in the helpers {override,revert}_creds().
> >
> 
> Yes, they are. Sorry that it was lost in the context. The original
> report is here:
> 
> https://lore.kernel.org/all/20231018074553.41333-1-hu1.chen@intel.com/
> 
> > Christian suggested lightweight variants of {override,revert}_creds()
> > that do not change refcount. Combining those with a guard and
> > I don't see what can go wrong (TM).
> >
> > If you try this out and post a patch, please be sure to include the
> > motivation for the patch along with performance numbers in the
> > commit message, even if only posting an RFC patch.
> >
> 
> Of course.
> 
> And to be sure, I will go with Christian's suggestion, it looks neat,
> and having a lighter version of references is a more common idiom.

Did this ever go anywhere?

Powered by blists - more mailing lists