Received-SPF: pass (google.com: domain of linux-kernel+bounces-4789-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <CAOQ4uxg-WvdcuCrQg7zp03ocNZoT-G2bpi=Y6nVxMTodyFAUbg@mail.gmail.com>
 <20231214220222.348101-1-vinicius.gomes@intel.com> <CAOQ4uxhJmjeSSM5iQyDadbj5UNjPqvh1QPLpSOVEYFbNbsjDQQ@mail.gmail.com>
 <87v88zp76v.fsf@intel.com> <CAOQ4uxiCVv7zbfn2BPrR9kh=DvGxQtXUmRvy2pDJ=G7rxjBrgg@mail.gmail.com>
 <CAOQ4uxhxvFt3_Wb3BGcjj4pGp=OFTBHNPJ4r4eH8245t-+CW+g@mail.gmail.com>
 <20231218-intim-lehrstellen-dbe053d6c3a8@brauner> <875y0vp41g.fsf@intel.com>
In-Reply-To: <875y0vp41g.fsf@intel.com>
From: Amir Goldstein <amir73il@gmail.com>
Date: Tue, 19 Dec 2023 09:15:52 +0200
Message-ID: <CAOQ4uxibYMQw0iszKhE5uxBnyayHWjqp4ZnOOiugO3GxMRS1eA@mail.gmail.com>
Subject: Re: [RFC] HACK: overlayfs: Optimize overlay/restore creds
To: Vinicius Costa Gomes <vinicius.gomes@intel.com>
Cc: Christian Brauner <brauner@kernel.org>, hu1.chen@intel.com, miklos@szeredi.hu, 
	malini.bhandaru@intel.com, tim.c.chen@intel.com, mikko.ylinen@intel.com, 
	lizhen.you@intel.com, linux-unionfs@vger.kernel.org, 
	linux-kernel@vger.kernel.org, linux-fsdevel <linux-fsdevel@vger.kernel.org>, 
	Linus Torvalds <torvalds@linux-foundation.org>, David Howells <dhowells@redhat.com>, 
	Seth Forshee <sforshee@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, Dec 18, 2023 at 11:57=E2=80=AFPM Vinicius Costa Gomes
<vinicius.gomes@intel.com> wrote:
>
> Christian Brauner <brauner@kernel.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 =3D 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 =3D 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 =3D 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.

The performance issues you observed are (probably) due to get/put
of cred refcount in the helpers {override,revert}_creds().

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.

Thanks,
Amir.