Message-ID: <20120923214152.GB1141@1984>
Date:	Sun, 23 Sep 2012 23:41:52 +0200
From:	Pablo Neira Ayuso <pablo@...filter.org>
To:	John Stultz <john.stultz@...aro.org>
Cc:	LKML <linux-kernel@...r.kernel.org>, JP Abgrall <jpa@...gle.com>,
	netdev@...r.kernel.org, Ashish Sharma <ashishsharma@...gle.com>,
	Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@...el.com>
Subject: Re: [PATCH 6/7][RFC] netfilter: xt_IDLETIMER: Add new netlink msg
 type

On Fri, Sep 21, 2012 at 10:10:52PM -0400, John Stultz wrote:
> From: JP Abgrall <jpa@...gle.com>
> 
> Send notifications when the label becomes active after an idle period.
> Send netlink message notifications in addition to sysfs notifications.
> Using a uevent with
>   subsystem=xt_idletimer
>   INTERFACE=...
>   STATE={active,inactive}
> 
> This is backport from common android-3.0
> commit: beb914e987cbbd368988d2b94a6661cb907c4d5a
> with uevent support instead of a new netlink message type.

I think this can be candidate to be included once some comestical
issues are fixed.

> Cc: netdev@...r.kernel.org
> Cc: JP Abgrall <jpa@...gle.com>
> Cc: Ashish Sharma <ashishsharma@...gle.com>
> Cc: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@...el.com>
> Signed-off-by: Ashish Sharma <ashishsharma@...gle.com>
> Signed-off-by: JP Abgrall <jpa@...gle.com>
> Signed-off-by: John Stultz <john.stultz@...aro.org>
> ---
>  include/linux/netfilter/xt_IDLETIMER.h |    8 ++++
>  net/netfilter/xt_IDLETIMER.c           |   78 +++++++++++++++++++++++++++++---
>  2 files changed, 79 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/netfilter/xt_IDLETIMER.h b/include/linux/netfilter/xt_IDLETIMER.h
> index 208ae93..faaa28b 100644
> --- a/include/linux/netfilter/xt_IDLETIMER.h
> +++ b/include/linux/netfilter/xt_IDLETIMER.h
> @@ -4,6 +4,7 @@
>   * Header file for Xtables timer target module.
>   *
>   * Copyright (C) 2004, 2010 Nokia Corporation
> + *

we can live without that extra line above.

>   * Written by Timo Teras <ext-timo.teras@...ia.com>
>   *
>   * Converted to x_tables and forward-ported to 2.6.34
> @@ -32,12 +33,19 @@
>  #include <linux/types.h>
>  
>  #define MAX_IDLETIMER_LABEL_SIZE 28
> +#define NLMSG_MAX_SIZE 64

better define this NLMSG_MAX_SIZE inside xt_IDLETIMER.c, this header
is exported to user-space and that definition is not useful.

> +
> +#define NL_EVENT_TYPE_INACTIVE 0
> +#define NL_EVENT_TYPE_ACTIVE 1
>  
>  struct idletimer_tg_info {
>  	__u32 timeout;
>  
>  	char label[MAX_IDLETIMER_LABEL_SIZE];
>  
> +	/* Use netlink messages for notification in addition to sysfs */
> +	__u8 send_nl_msg;
> +
>  	/* for kernel module internal use only */
>  	struct idletimer_tg *timer __attribute__((aligned(8)));
>  };

you have to define two structures:

struct idletimer_tg_info
struct idletimer_tg_info_v1

and register both revisions. This (rudimentary) mechanism allows us to
keep backward compatibility.

See net/netfilter/xt_socket.c for instance.

> diff --git a/net/netfilter/xt_IDLETIMER.c b/net/netfilter/xt_IDLETIMER.c
> index f407ebc1..df91e26 100644
> --- a/net/netfilter/xt_IDLETIMER.c
> +++ b/net/netfilter/xt_IDLETIMER.c
> @@ -5,6 +5,7 @@
>   * After timer expires a kevent will be sent.
>   *
>   * Copyright (C) 2004, 2010 Nokia Corporation
> + *
>   * Written by Timo Teras <ext-timo.teras@...ia.com>
>   *
>   * Converted to x_tables and reworked for upstream inclusion
> @@ -38,8 +39,10 @@
>  #include <linux/netfilter/xt_IDLETIMER.h>
>  #include <linux/kdev_t.h>
>  #include <linux/kobject.h>
> +#include <linux/skbuff.h>
>  #include <linux/workqueue.h>
>  #include <linux/sysfs.h>
> +#include <net/net_namespace.h>
>  
>  struct idletimer_tg_attr {
>  	struct attribute attr;
> @@ -56,6 +59,8 @@ struct idletimer_tg {
>  	struct idletimer_tg_attr attr;
>  
>  	unsigned int refcnt;
> +	bool send_nl_msg;
> +	bool active;
>  };
>  
>  static LIST_HEAD(idletimer_tg_list);
> @@ -63,6 +68,32 @@ static DEFINE_MUTEX(list_mutex);
>  
>  static struct kobject *idletimer_tg_kobj;
>  
> +static void notify_netlink_uevent(const char *iface, struct idletimer_tg *timer)
> +{
> +	char iface_msg[NLMSG_MAX_SIZE];
> +	char state_msg[NLMSG_MAX_SIZE];
> +	char *envp[] = { iface_msg, state_msg, NULL };
> +	int res;
> +
> +	res = snprintf(iface_msg, NLMSG_MAX_SIZE, "INTERFACE=%s",
> +		       iface);
> +	if (NLMSG_MAX_SIZE <= res) {
> +		pr_err("message too long (%d)", res);
> +		return;
> +	}
> +	res = snprintf(state_msg, NLMSG_MAX_SIZE, "STATE=%s",
> +		       timer->active ? "active" : "inactive");
> +	if (NLMSG_MAX_SIZE <= res) {
> +		pr_err("message too long (%d)", res);
> +		return;
> +	}
> +	pr_debug("putting nlmsg: <%s> <%s>\n", iface_msg, state_msg);
> +	kobject_uevent_env(idletimer_tg_kobj, KOBJ_CHANGE, envp);
> +	return;
> +
> +

extra lines.

> +}
> +
>  static
>  struct idletimer_tg *__idletimer_tg_find_by_label(const char *label)
>  {
> @@ -83,6 +114,7 @@ static ssize_t idletimer_tg_show(struct kobject *kobj, struct attribute *attr,
>  {
>  	struct idletimer_tg *timer;
>  	unsigned long expires = 0;
> +	unsigned long now = jiffies;
>  
>  	mutex_lock(&list_mutex);
>  
> @@ -92,11 +124,15 @@ static ssize_t idletimer_tg_show(struct kobject *kobj, struct attribute *attr,
>  
>  	mutex_unlock(&list_mutex);
>  
> -	if (time_after(expires, jiffies))
> +	if (time_after(expires, now))
>  		return sprintf(buf, "%u\n",
> -			       jiffies_to_msecs(expires - jiffies) / 1000);
> +			       jiffies_to_msecs(expires - now) / 1000);
>  
> -	return sprintf(buf, "0\n");
> +	if (timer->send_nl_msg)
> +		return sprintf(buf, "0 %d\n",
> +			jiffies_to_msecs(now - expires) / 1000);
> +	else
> +		return sprintf(buf, "0\n");
>  }
>  
>  static void idletimer_tg_work(struct work_struct *work)
> @@ -105,6 +141,9 @@ static void idletimer_tg_work(struct work_struct *work)
>  						  work);
>  
>  	sysfs_notify(idletimer_tg_kobj, NULL, timer->attr.attr.name);
> +
> +	if (timer->send_nl_msg)
> +		notify_netlink_uevent(timer->attr.attr.name, timer);
>  }
>  
>  static void idletimer_tg_expired(unsigned long data)
> @@ -113,6 +152,7 @@ static void idletimer_tg_expired(unsigned long data)
>  
>  	pr_debug("timer %s expired\n", timer->attr.attr.name);
>  
> +	timer->active = false;
>  	schedule_work(&timer->work);
>  }
>  
> @@ -145,6 +185,8 @@ static int idletimer_tg_create(struct idletimer_tg_info *info)
>  	setup_timer(&info->timer->timer, idletimer_tg_expired,
>  		    (unsigned long) info->timer);
>  	info->timer->refcnt = 1;
> +	info->timer->send_nl_msg = (info->send_nl_msg == 0) ? false : true;
> +	info->timer->active = true;
>  
>  	mod_timer(&info->timer->timer,
>  		  msecs_to_jiffies(info->timeout * 1000) + jiffies);
> @@ -168,14 +210,24 @@ static unsigned int idletimer_tg_target(struct sk_buff *skb,
>  					 const struct xt_action_param *par)
>  {
>  	const struct idletimer_tg_info *info = par->targinfo;
> +	unsigned long now = jiffies;
>  
>  	pr_debug("resetting timer %s, timeout period %u\n",
>  		 info->label, info->timeout);
>  
>  	BUG_ON(!info->timer);
>  
> +	info->timer->active = true;
> +
> +	if (time_before(info->timer->timer.expires, now)) {
> +		schedule_work(&info->timer->work);
> +		pr_debug("Starting timer %s (Expired, Jiffies): %lu, %lu\n",
> +			 info->label, info->timer->timer.expires, now);
> +	}

AFAICS, this seems to fix the case in which timer has expired and we
refresh it without sending the notification.

Should go in a different patch.

> +
> +	/* TODO: Avoid modifying timers on each packet */
>  	mod_timer(&info->timer->timer,
> -		  msecs_to_jiffies(info->timeout * 1000) + jiffies);
> +		  msecs_to_jiffies(info->timeout * 1000) + now);
>  
>  	return XT_CONTINUE;
>  }
> @@ -184,8 +236,9 @@ static int idletimer_tg_checkentry(const struct xt_tgchk_param *par)
>  {
>  	struct idletimer_tg_info *info = par->targinfo;
>  	int ret;
> +	unsigned long now = jiffies;
>  
> -	pr_debug("checkentry targinfo%s\n", info->label);
> +	pr_debug("checkentry targinfo %s\n", info->label);
>  
>  	if (info->timeout == 0) {
>  		pr_debug("timeout value is zero\n");
> @@ -204,8 +257,16 @@ static int idletimer_tg_checkentry(const struct xt_tgchk_param *par)
>  	info->timer = __idletimer_tg_find_by_label(info->label);
>  	if (info->timer) {
>  		info->timer->refcnt++;
> +		info->timer->active = true;
> +
> +		if (time_before(info->timer->timer.expires, now)) {
> +			schedule_work(&info->timer->work);
> +			pr_debug("Starting Checkentry timer (Expired, Jiffies): %lu, %lu\n",
> +				info->timer->timer.expires, now);
> +		}
> +
>  		mod_timer(&info->timer->timer,
> -			  msecs_to_jiffies(info->timeout * 1000) + jiffies);
> +			  msecs_to_jiffies(info->timeout * 1000) + now);
>  
>  		pr_debug("increased refcnt of timer %s to %u\n",
>  			 info->label, info->timer->refcnt);
> @@ -219,6 +280,7 @@ static int idletimer_tg_checkentry(const struct xt_tgchk_param *par)
>  	}
>  
>  	mutex_unlock(&list_mutex);
> +

we don't need this.

>  	return 0;
>  }
>  
> @@ -240,7 +302,7 @@ static void idletimer_tg_destroy(const struct xt_tgdtor_param *par)
>  		kfree(info->timer);
>  	} else {
>  		pr_debug("decreased refcnt of timer %s to %u\n",
> -			 info->label, info->timer->refcnt);
> +		info->label, info->timer->refcnt);

remove this change.

>  	}
>  
>  	mutex_unlock(&list_mutex);
> @@ -248,6 +310,7 @@ static void idletimer_tg_destroy(const struct xt_tgdtor_param *par)
>  
>  static struct xt_target idletimer_tg __read_mostly = {
>  	.name		= "IDLETIMER",
> +	.revision	= 1,
>  	.family		= NFPROTO_UNSPEC,
>  	.target		= idletimer_tg_target,
>  	.targetsize     = sizeof(struct idletimer_tg_info),
> @@ -313,3 +376,4 @@ MODULE_DESCRIPTION("Xtables: idle time monitor");
>  MODULE_LICENSE("GPL v2");
>  MODULE_ALIAS("ipt_IDLETIMER");
>  MODULE_ALIAS("ip6t_IDLETIMER");
> +MODULE_ALIAS("arpt_IDLETIMER");

Interesting, they are really using this with arptables?

> -- 
> 1.7.9.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists