Re: [PATCH v2] ovl: untangle copy up call chain

[Vivek Goyal <vgoyal@redhat.com>]

On Thu, Oct 25, 2018 at 02:44:15PM +0200, Miklos Szeredi wrote:

[..]
> > -static struct dentry *ovl_get_tmpfile(struct ovl_copy_up_ctx *c)
> > +static struct dentry *ovl_get_workdir_temp(struct ovl_copy_up_ctx *c)
> >  {
> >         int err;
> >         struct dentry *temp;
> > @@ -484,10 +510,32 @@ static struct dentry *ovl_get_tmpfile(struct ovl_copy_up_ctx *c)
> >         if (new_creds)
> >                 old_creds = override_creds(new_creds);
> >
> > -       if (c->tmpfile)
> > -               temp = ovl_do_tmpfile(c->workdir, c->stat.mode);
> > -       else
> > -               temp = ovl_create_temp(c->workdir, &cattr);
> > +       temp = ovl_create_temp(c->workdir, &cattr);
> > +out:
> > +       if (new_creds) {
> > +               revert_creds(old_creds);
> 
> Not new in this patch, but it looks like this will Oops if old_creds
> is NULL, which happens if security_inode_copy_up() returns an error.
> 

If security_inode_copy_up() fails, then new_creds will be null too. And
in that case we will never call revert_creds(old_creds)?

IOW, new_creds should be returned by security_inode_copy_up() only if
it succeeds. Otherwise new_creds should be left untouched by hook.

> Trivial to fix, but I'm not sure we need the put_cred(new_creds) in
> the failed security_inode_copy_up() case.  Vivek, do you remember the
> reason for this error cleanup?

Same, In case of failure, new_creds should be NULL and we will never
call put_cred(new_creds).

So I can't see the bug. What am I missing?

Thanks
Vivek