Re: [PATCH 38/44] kdbus: Fix error path in kdbus_user_lookup()

[Sergei Zviagintsev <sergei@s15v.net>]

Hi David,

On Thu, Oct 08, 2015 at 05:06:57PM +0200, David Herrmann wrote:
> Hi
> 
> On Thu, Oct 8, 2015 at 1:32 PM, Sergei Zviagintsev <sergei@s15v.net> wrote:
> > If idr_alloc() fails, we shouldn't call idr_remove() as latter produces
> > warning when called on non-allocated ids. Split cleanup code into three
> > parts for differrent cleanup scenarios before and after idr_alloc().
> >
> > Signed-off-by: Sergei Zviagintsev <sergei@s15v.net>
> > ---
> >  ipc/kdbus/domain.c | 20 ++++++++++----------
> >  1 file changed, 10 insertions(+), 10 deletions(-)
> >
> > diff --git a/ipc/kdbus/domain.c b/ipc/kdbus/domain.c
> > index ac9f760c150d..31cd09fb572f 100644
> > --- a/ipc/kdbus/domain.c
> > +++ b/ipc/kdbus/domain.c
> > @@ -208,7 +208,7 @@ struct kdbus_user *kdbus_user_lookup(struct kdbus_domain *domain, kuid_t uid)
> >         u = kzalloc(sizeof(*u), GFP_KERNEL);
> >         if (!u) {
> >                 ret = -ENOMEM;
> > -               goto exit;
> > +               goto exit_unlock;
> >         }
> >
> >         kref_init(&u->kref);
> > @@ -225,7 +225,7 @@ struct kdbus_user *kdbus_user_lookup(struct kdbus_domain *domain, kuid_t uid)
> >                         ret = idr_alloc(&domain->user_idr, u, __kuid_val(uid),
> >                                         __kuid_val(uid) + 1, GFP_KERNEL);
> >                         if (ret < 0)
> > -                               goto exit;
> > +                               goto exit_free;
> >                 }
> >         }
> >
> > @@ -235,19 +235,19 @@ struct kdbus_user *kdbus_user_lookup(struct kdbus_domain *domain, kuid_t uid)
> >          */
> >         ret = ida_simple_get(&domain->user_ida, 1, 0, GFP_KERNEL);
> >         if (ret < 0)
> > -               goto exit;
> > +               goto exit_idr;
> >
> 
> Why not simply assign "u->uid = uid;" _after_ doing the idr operations?

If I understand it right, in this case we have to firstly assign
INVALID_UID to u->uid (to check it with uid_valid() in the error path)
and then do 'u->uid = uid'. But from the first sight it would be not so
obvious and may require adding some comment on it. Isn't it better to
stay explicit here by maintaining several goto labels?

> 
> Thanks
> David
> 
> >         u->id = ret;
> >         mutex_unlock(&domain->lock);
> >         return u;
> >
> > -exit:
> > -       if (u) {
> > -               if (uid_valid(u->uid))
> > -                       idr_remove(&domain->user_idr, __kuid_val(u->uid));
> > -               kdbus_domain_unref(u->domain);
> > -               kfree(u);
> > -       }
> > +exit_idr:
> > +       if (uid_valid(u->uid))
> > +               idr_remove(&domain->user_idr, __kuid_val(u->uid));
> > +exit_free:
> > +       kdbus_domain_unref(u->domain);
> > +       kfree(u);
> > +exit_unlock:
> >         mutex_unlock(&domain->lock);
> >         return ERR_PTR(ret);
> >  }
> > --
> > 1.8.3.1
> >