Message-ID: <CAOQ4uxhuvBtSrbw2RAGKnO6O9dXH2DZ-fHJ=z8v+T+5PariZ0w@mail.gmail.com>
Date: Mon, 7 Oct 2024 13:12:39 +0200
From: Amir Goldstein <amir73il@...il.com>
To: Vinicius Costa Gomes <vinicius.gomes@...el.com>
Cc: Christian Brauner <brauner@...nel.org>, Miklos Szeredi <miklos@...redi.hu>, hu1.chen@...el.com, 
	malini.bhandaru@...el.com, tim.c.chen@...el.com, mikko.ylinen@...el.com, 
	lizhen.you@...el.com, linux-unionfs@...r.kernel.org, 
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 04/16] overlayfs: Document critical override_creds() operations

On Wed, Sep 25, 2024 at 4:17 PM Vinicius Costa Gomes
<vinicius.gomes@...el.com> wrote:
>
> Christian Brauner <brauner@...nel.org> writes:
>
> > On Tue, Sep 24, 2024 at 06:13:39PM GMT, Vinicius Costa Gomes wrote:
> >> Vinicius Costa Gomes <vinicius.gomes@...el.com> writes:
> >>
> >> > Miklos Szeredi <miklos@...redi.hu> writes:
> >> >
> >> >> On Thu, 22 Aug 2024 at 03:25, Vinicius Costa Gomes
> >> >> <vinicius.gomes@...el.com> wrote:
> >> >>>
> >> >>> Add a comment to these operations that cannot use the _light version
> >> >>> of override_creds()/revert_creds(), because during the critical
> >> >>> section the struct cred .usage counter might be modified.
> >> >>
> >> >> Why is it a problem if the usage counter is modified?  Why is the
> >> >> counter modified in each of these cases?
> >> >>
> >> >
> >> > Working on getting some logs from the crash that I get when I convert
> >> > the remaining cases to use the _light() functions.
> >> >
> >>
> >> See the log below.
> >>
> >> > Perhaps I was wrong on my interpretation of the crash.
> >> >
> >>
> >> What I am seeing is that ovl_setup_cred_for_create() has a "side
> >> effect", it creates another set of credentials, runs the security hooks
> >> with this new credentials, and the side effect is that when it returns,
> >> by design, 'current->cred' is this new credentials (a third set of
> >> credentials).
> >
> > Well yes, during ovl_setup_cred_for_create() the fs{g,u}id needs to be
> > overwritten. But I'm stil confused what the exact problem is as it was
> > always clear that ovl_setup_cred_for_create() wouldn't be ported to
> > light variants.
> >
> > /me looks...
> >
> >>
> >> And this implies that refcounting for this is somewhat tricky, as said
> >> in commit d0e13f5bbe4b ("ovl: fix uid/gid when creating over whiteout").
> >>
> >> I see two ways forward:
> >>
> >> 1. Keep using the non _light() versions in functions that call
> >>    ovl_setup_cred_for_create().
> >> 2. Change ovl_setup_cred_for_create() so it doesn't drop the "extra"
> >>    refcount.
> >>
> >> I went with (1), and it still sounds to me like the best way, but I
> >> agree that my explanation was not good enough, will add the information
> >> I just learned to the commit message and to the code.
> >>
> >> Do you see another way forward? Or do you think that I should go with
> >> (2)?
> >
> > ... ok, I understand. Say we have:
> >
> > ovl_create_tmpfile()
> > /* current->cred == ovl->creator_cred without refcount bump /*
> > old_cred = ovl_override_creds_light()
> > -> ovl_setup_cred_for_create()
> >    /* Copy current->cred == ovl->creator_cred */
> >    modifiable_cred = prepare_creds()
> >
> >    /* Override current->cred == modifiable_cred */
> >    mounter_creds = override_creds(modifiable_cred)
> >
> >    /*
> >     * And here's the BUG BUG BUG where we decrement the refcount on the
> >     * constant mounter_creds.
> >     */
> >    put_cred(mounter_creds) // BUG BUG BUG
> >
> >    put_cred(modifiable_creds)
> >
> > So (1) is definitely the wrong option given that we can get rid of
> > refcount decs and incs in the creation path.
> >
> > Imo, you should do (2) and add a WARN_ON_ONC(). Something like the
> > __completely untested__:
> >
>
> > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> > index ab65e98a1def..e246e0172bb6 100644
> > --- a/fs/overlayfs/dir.c
> > +++ b/fs/overlayfs/dir.c
> > @@ -571,7 +571,12 @@ static int ovl_setup_cred_for_create(struct dentry *dentry, struct inode *inode,
> >                 put_cred(override_cred);
> >                 return err;
> >         }
> > -       put_cred(override_creds(override_cred));
> > +
> > +       /*
> > +        * We must be called with creator creds already, otherwise we risk
> > +        * leaking creds.
> > +        */
> > +       WARN_ON_ONCE(override_creds(override_cred) != ovl_creds(dentry->d_sb));
> >         put_cred(override_cred);
> >
> >         return 0;
> >
>
> At first glance, looks good. Going to test it and see how it works.
> Thank you.
>
> For the next version of the series, my plan is to include this
> suggestion/change and remove the guard()/scoped_guard() conversion
> patches from the series.
>

Vinicius,

I have a request. Since the plan is to keep the _light() helpers around for the
time being, please make the existing helper ovl_override_creds() use the
light version and open code the non-light versions in the few places where
they are needed and please replace all the matching call sites of
revert_creds() to
a helper ovl_revert_creds() that is a wrapper for the light version.

Also, depending on when you intend to post your work for review,
I have a feeling that the review of my patches is going to be done
before your submit your patches for review, so you may want to consider
already basing your patches on top of my development branch [2] to avoid
conflicts later.

Anyway, the parts of my patches that conflict with yours (s/real.file/realfile/)
are not likely to change anymore.

Thanks,
Amir.

[1] https://lore.kernel.org/linux-unionfs/20241006082359.263755-1-amir73il@gmail.com/
[2] https://github.com/amir73il/linux/commits/ovl_real_file/

Powered by blists - more mailing lists