Re: [PATCH v2 04/16] overlayfs: Document critical override_creds() operations

[Vinicius Costa Gomes]

Hi,

Amir Goldstein <amir73il@xxxxxxxxx> writes:

> On Wed, Sep 25, 2024 at 4:17 PM Vinicius Costa Gomes
> <vinicius.gomes@xxxxxxxxx> wrote:
>>
>> Christian Brauner <brauner@xxxxxxxxxx> writes:
>>
>> > On Tue, Sep 24, 2024 at 06:13:39PM GMT, Vinicius Costa Gomes wrote:
>> >> Vinicius Costa Gomes <vinicius.gomes@xxxxxxxxx> writes:
>> >>
>> >> > Miklos Szeredi <miklos@xxxxxxxxxx> writes:
>> >> >
>> >> >> On Thu, 22 Aug 2024 at 03:25, Vinicius Costa Gomes
>> >> >> <vinicius.gomes@xxxxxxxxx> 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.
>

Seems like a good idea. Will do that, and see how it looks.

> 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.
>

Thanks for the heads up. Will rebase my code on top of your branch.

> 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@xxxxxxxxx/
> [2] https://github.com/amir73il/linux/commits/ovl_real_file/


Cheers,
-- 
Vinicius