[1/8] isdn/gigaset: ratelimit CAPI message dumps

Introduce a global ratelimit for CAPI message dumps to protect
against possible log flood.
Drop the ratelimit for ignored messages which is now covered by the
global one.

Signed-off-by: Tilman Schmidt <tilman@imap.cc>
CC: stable <stable@kernel.org>
---
 drivers/isdn/gigaset/capi.c |   22 +++++++++-------------
 1 files changed, 9 insertions(+), 13 deletions(-)

Am 26.04.2012 01:02, schrieb Tilman Schmidt:
> Introduce a global ratelimit for CAPI message dumps to protect
> against possible log flood.
> Drop the ratelimit for ignored messages which is now covered by the
> global one.
> 

Hmm, I think the only CAPI messages which would need a ratelimit are
related to the DATA_B3 messages. If you need CAPI debug messages in most
cases you do not need all of the DATA_B3, but you do not want to miss
any other message related to the call control. With a general rate limit
you do not have the control, which messages are logged and which are not.
And here maybe some cases, when even the DATA_B3 are important (e.g.
searching bugs in flow control), so I would make it still conditional
to allow to print all messages.
And I'm not sure, if this is really something for stable.

> Signed-off-by: Tilman Schmidt <tilman@imap.cc>
> CC: stable <stable@kernel.org>
> ---
>  drivers/isdn/gigaset/capi.c |   22 +++++++++-------------
>  1 files changed, 9 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/isdn/gigaset/capi.c b/drivers/isdn/gigaset/capi.c
> index 343b5c8..292ca2f 100644
> --- a/drivers/isdn/gigaset/capi.c
> +++ b/drivers/isdn/gigaset/capi.c
> @@ -14,6 +14,7 @@
>  #include "gigaset.h"
>  #include <linux/proc_fs.h>
>  #include <linux/seq_file.h>
> +#include <linux/ratelimit.h>
>  #include <linux/isdn/capilli.h>
>  #include <linux/isdn/capicmd.h>
>  #include <linux/isdn/capiutil.h>
> @@ -223,10 +224,14 @@ get_appl(struct gigaset_capi_ctr *iif, u16 appl)
>  static inline void dump_cmsg(enum debuglevel level, const char *tag, _cmsg *p)
>  {
>  #ifdef CONFIG_GIGASET_DEBUG
> +	/* dump at most 20 messages in 20 secs */
> +	static DEFINE_RATELIMIT_STATE(msg_dump_ratelimit, 20 * HZ, 20);
>  	_cdebbuf *cdb;
>  
>  	if (!(gigaset_debuglevel & level))
>  		return;
> +	if (!___ratelimit(&msg_dump_ratelimit, tag))
> +		return;
>  
>  	cdb = capi_cmsg2str(p);
>  	if (cdb) {
> @@ -2059,12 +2064,6 @@ static void do_reset_b3_req(struct gigaset_capi_ctr *iif,
>  }
>  
>  /*
> - * dump unsupported/ignored messages at most twice per minute,
> - * some apps send those very frequently
> - */
> -static unsigned long ignored_msg_dump_time;
> -
> -/*
>   * unsupported CAPI message handler
>   */
>  static void do_unsupported(struct gigaset_capi_ctr *iif,
> @@ -2073,8 +2072,7 @@ static void do_unsupported(struct gigaset_capi_ctr *iif,
>  {
>  	/* decode message */
>  	capi_message2cmsg(&iif->acmsg, skb->data);
> -	if (printk_timed_ratelimit(&ignored_msg_dump_time, 30 * 1000))
> -		dump_cmsg(DEBUG_CMD, __func__, &iif->acmsg);
> +	dump_cmsg(DEBUG_CMD, __func__, &iif->acmsg);
>  	send_conf(iif, ap, skb, CapiMessageNotSupportedInCurrentState);
>  }
>  
> @@ -2085,11 +2083,9 @@ static void do_nothing(struct gigaset_capi_ctr *iif,
>  		       struct gigaset_capi_appl *ap,
>  		       struct sk_buff *skb)
>  {
> -	if (printk_timed_ratelimit(&ignored_msg_dump_time, 30 * 1000)) {
> -		/* decode message */
> -		capi_message2cmsg(&iif->acmsg, skb->data);
> -		dump_cmsg(DEBUG_CMD, __func__, &iif->acmsg);
> -	}
> +	/* decode message */
> +	capi_message2cmsg(&iif->acmsg, skb->data);
> +	dump_cmsg(DEBUG_CMD, __func__, &iif->acmsg);
>  	dev_kfree_skb_any(skb);
>  }
>

Am 26.04.2012 08:39, schrieb Karsten Keil:
> Am 26.04.2012 01:02, schrieb Tilman Schmidt:
>> Introduce a global ratelimit for CAPI message dumps to protect
>> against possible log flood.
>> Drop the ratelimit for ignored messages which is now covered by the
>> global one.
> 
> Hmm, I think the only CAPI messages which would need a ratelimit are
> related to the DATA_B3 messages. If you need CAPI debug messages in most
> cases you do not need all of the DATA_B3, but you do not want to miss
> any other message related to the call control. With a general rate limit
> you do not have the control, which messages are logged and which are not.

The ratelimit introduced by this patch only applies to messages
other than DATA_B3. Logging DATA_B3 messages is not done via
dump_cmsg().

I'd like to ratelimit specifically non-DATA_B3 messages because I
saw a (possibly buggy) CAPI application flooding the log with
FACILITY messages. Equally important, I'd like to make the
ratelimit in do_nothing() / do_unsupported() bursty because I had
a case where I needed to see several ignored/unhandled CAPI
messages in quick succession. So this patch is killing two birds
with one stone for me.

The burst limit of 20 messages in 20 seconds is chosen to allow a
complete call setup sequence to be logged, while limiting to one
message per second in the long run.

> And here maybe some cases, when even the DATA_B3 are important (e.g.
> searching bugs in flow control), so I would make it still conditional
> to allow to print all messages.

DATA_B3 dumps produce an enormous amount of log data and are
therefore controlled separately by the DEBUG_MCMD flag.
Someone who enables that should know what she or he does.
But if you need them, you need them all. A ratelimit doesn't
make sense there in my experience.

> And I'm not sure, if this is really something for stable.

It's pretty simple and localized, a net simplification, and only
affects generation of debugging messages, so I think it's safe.
But if you see a problem there I can drop the "CC: stable" line.

Thanks,
Tilman

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am 27.04.2012 12:29, schrieb Tilman Schmidt:
> Am 26.04.2012 08:39, schrieb Karsten Keil:
>> Am 26.04.2012 01:02, schrieb Tilman Schmidt:
>>> Introduce a global ratelimit for CAPI message dumps to protect
>>>  against possible log flood. Drop the ratelimit for ignored 
>>> messages which is now covered by the global one.
>> 
>> Hmm, I think the only CAPI messages which would need a ratelimit 
>> are related to the DATA_B3 messages. If you need CAPI debug 
>> messages in most cases you do not need all of the DATA_B3, but 
>> you do not want to miss any other message related to the call 
>> control. With a general rate limit you do not have the control, 
>> which messages are logged and which are not.
> 
> The ratelimit introduced by this patch only applies to messages 
> other than DATA_B3. Logging DATA_B3 messages is not done via 
> dump_cmsg().
> 

Thanks for the clarification, forget about my objection.
I ack this patch now.

> I'd like to ratelimit specifically non-DATA_B3 messages because I 
> saw a (possibly buggy) CAPI application flooding the log with 
> FACILITY messages. Equally important, I'd like to make the 
> ratelimit in do_nothing() / do_unsupported() bursty because I had
> a case where I needed to see several ignored/unhandled CAPI
> messages in quick succession. So this patch is killing two birds
> with one stone for me.
> 
> The burst limit of 20 messages in 20 seconds is chosen to allow a 
> complete call setup sequence to be logged, while limiting to one 
> message per second in the long run.
> 
>> And here maybe some cases, when even the DATA_B3 are important 
>> (e.g. searching bugs in flow control), so I would make it still 
>> conditional to allow to print all messages.
> 
> DATA_B3 dumps produce an enormous amount of log data and are 
> therefore controlled separately by the DEBUG_MCMD flag. Someone
> who enables that should know what she or he does. But if you need
> them, you need them all. A ratelimit doesn't make sense there in
> my experience.
> 
>> And I'm not sure, if this is really something for stable.
> 
> It's pretty simple and localized, a net simplification, and only 
> affects generation of debugging messages, so I think it's safe.
> But if you see a problem there I can drop the "CC: stable" line.
> 

I let the decision about it to you and the stable maintainers.

- -- 
Karsten Keil
Linux Kernel Development
Tel: +49 175 7249132
Mail: keil@b1-systems.de

B1 Systems GmbH
Osterfeldstraße 7 / 85088 Vohburg / http://www.b1-systems.de
GF: Ralph Dehner / Unternehmenssitz: Vohburg / AG: Ingolstadt,HRB 3537
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.16 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk+buHwACgkQo5VVC52CNcRoygCfWwPlWZ+A48OwEkr/MtK6PeNG
0UEAnipdxPSZDKa4s99LlGYwvggWIIAJ
=CLr6
-----END PGP SIGNATURE-----
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html