Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1242226 > unrolled thread
| Started by | Sergei Zviagintsev <sergei@s15v.net> |
|---|---|
| First post | 2015-10-08 13:40 +0200 |
| Last post | 2015-10-09 20:50 +0200 |
| Articles | 3 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
[PATCH 38/44] kdbus: Fix error path in kdbus_user_lookup() Sergei Zviagintsev <sergei@s15v.net> - 2015-10-08 13:40 +0200
Re: [PATCH 38/44] kdbus: Fix error path in kdbus_user_lookup() David Herrmann <dh.herrmann@gmail.com> - 2015-10-08 17:10 +0200
Re: [PATCH 38/44] kdbus: Fix error path in kdbus_user_lookup() Sergei Zviagintsev <sergei@s15v.net> - 2015-10-09 20:50 +0200
| From | Sergei Zviagintsev <sergei@s15v.net> |
|---|---|
| Date | 2015-10-08 13:40 +0200 |
| Subject | [PATCH 38/44] kdbus: Fix error path in kdbus_user_lookup() |
| Message-ID | <qhhId-2d4-13@gated-at.bofh.it> |
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;
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | David Herrmann <dh.herrmann@gmail.com> |
|---|---|
| Date | 2015-10-08 17:10 +0200 |
| Message-ID | <qhkZs-76Q-23@gated-at.bofh.it> |
| In reply to | #1242226 |
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?
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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Sergei Zviagintsev <sergei@s15v.net> |
|---|---|
| Date | 2015-10-09 20:50 +0200 |
| Message-ID | <qhKTU-231-23@gated-at.bofh.it> |
| In reply to | #1242503 |
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
> >
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web