[PATCH] [SCHED] Fix unlock spinlock in error path

[PATCH] [SCHED] Fix unlock spinlock in error path

[Catalin(ux aka Dino) BOIE]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 394 bytes --]

Hello!

In act_api.c::tcf_action_copy_stats, in error path (get_stats
returns < 0), the spinlock taken in gnet_stats_start_copy_compat
or gnet_stats_start_copy is not unlocked.

The attached patch fixes this.

Please, apply.

Thank you very much!

Signed-off-by: Catalin(ux aka Dino) BOIE <catab at umbrella.ro>

---
Catalin(ux aka Dino) BOIE
catab at deuroconsult.ro
http://kernel.umbrella.ro/

[-- Attachment #2: Type: TEXT/PLAIN, Size: 587 bytes --]

--- linux/net/sched/act_api.c	2005-03-02 09:38:33.000000000 +0200
+++ tovbug/net/sched/act_api.c	2005-04-15 13:27:54.000000000 +0300
@@ -401,7 +401,7 @@ int tcf_action_copy_stats(struct sk_buff
 
 	if (a->ops != NULL && a->ops->get_stats != NULL)
 		if (a->ops->get_stats(skb, a) < 0)
-			goto errout;
+			goto errout_stop_stats;
 
 	if (gnet_stats_copy_basic(&d, &h->bstats) < 0 ||
 #ifdef CONFIG_NET_ESTIMATOR
@@ -415,6 +415,9 @@ int tcf_action_copy_stats(struct sk_buff
 
 	return 0;
 
+errout_stop_stats:
+	gnet_stats_finish_copy(&d);
+
 errout:
 	return -1;
 }

Re: [PATCH] [SCHED] Fix unlock spinlock in error path

[Thomas Graf]

* Catalin(ux aka Dino) BOIE <Pine.LNX.4.62.0505161555080.13294@webhosting.rdsbv.ro> 2005-05-16 16:01
> In act_api.c::tcf_action_copy_stats, in error path (get_stats
> returns < 0), the spinlock taken in gnet_stats_start_copy_compat
> or gnet_stats_start_copy is not unlocked.

This is on purpose, get_stats() is nothing more than a placeholder
at the moment for something that should really be called dump_stats()
and have struct gnet_dump as one of its arguments. The unlocking
must be done in the internal error handling and if we ever have an
operation that could fail between _start() and _stop() for
another reason we'll need a gnet_stats_abort() (skb_trim()'ing
properly) to avoid double unlocking. But since there is no
user of this API yet it doesn't make sense to bloat the code.

Re: [PATCH] [SCHED] Fix unlock spinlock in error path

[Catalin(ux aka Dino) BOIE]

On Mon, 16 May 2005, Thomas Graf wrote:

> * Catalin(ux aka Dino) BOIE <Pine.LNX.4.62.0505161555080.13294@webhosting.rdsbv.ro> 2005-05-16 16:01
>> In act_api.c::tcf_action_copy_stats, in error path (get_stats
>> returns < 0), the spinlock taken in gnet_stats_start_copy_compat
>> or gnet_stats_start_copy is not unlocked.
>
> This is on purpose, get_stats() is nothing more than a placeholder
> at the moment for something that should really be called dump_stats()
> and have struct gnet_dump as one of its arguments. The unlocking
> must be done in the internal error handling and if we ever have an
> operation that could fail between _start() and _stop() for
> another reason we'll need a gnet_stats_abort() (skb_trim()'ing
> properly) to avoid double unlocking. But since there is no
> user of this API yet it doesn't make sense to bloat the code.


Thanks Thomas for the explication!
Sorry for the noise.

---
Catalin(ux aka Dino) BOIE
catab at deuroconsult.ro
http://kernel.umbrella.ro/

Re: [PATCH] [SCHED] Fix unlock spinlock in error path

[Thomas Graf]

* Catalin(ux aka Dino) BOIE <Pine.LNX.4.62.0505162224420.20204@webhosting.rdsbv.ro> 2005-05-16 22:25
> Thanks Thomas for the explication!
> Sorry for the noise.

Don't worry, I should have documented this weird code part. ;->