[PATCH 1/3] Introduce nft_log_dereference() macro

[PATCH 1/3] Introduce nft_log_dereference() macro

[Marcelo Ricardo Leitner]

Wrap up a common call pattern in an easier to handle call.

Signed-off-by: Marcelo Ricardo Leitner <mleitner@redhat.com>
---
 net/netfilter/nf_log.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
index daad6022c689c47a66a47e7a89a83c0c848c53d6..f1409d95f810c689ec70755eb8a85125d291ad47 100644
--- a/net/netfilter/nf_log.c
+++ b/net/netfilter/nf_log.c
@@ -19,6 +19,9 @@
 static struct nf_logger __rcu *loggers[NFPROTO_NUMPROTO][NF_LOG_TYPE_MAX] __read_mostly;
 static DEFINE_MUTEX(nf_log_mutex);
 
+#define nft_log_dereference(logger) \
+	rcu_dereference_protected(logger, lockdep_is_held(&nf_log_mutex))
+
 static struct nf_logger *__find_logger(int pf, const char *str_logger)
 {
 	struct nf_logger *log;
@@ -28,8 +31,7 @@ static struct nf_logger *__find_logger(int pf, const char *str_logger)
 		if (loggers[pf][i] == NULL)
 			continue;
 
-		log = rcu_dereference_protected(loggers[pf][i],
-						lockdep_is_held(&nf_log_mutex));
+		log = nft_log_dereference(loggers[pf][i]);
 		if (!strnicmp(str_logger, log->name, strlen(log->name)))
 			return log;
 	}
@@ -45,8 +47,7 @@ void nf_log_set(struct net *net, u_int8_t pf, const struct nf_logger *logger)
 		return;
 
 	mutex_lock(&nf_log_mutex);
-	log = rcu_dereference_protected(net->nf.nf_loggers[pf],
-					lockdep_is_held(&nf_log_mutex));
+	log = nft_log_dereference(net->nf.nf_loggers[pf]);
 	if (log == NULL)
 		rcu_assign_pointer(net->nf.nf_loggers[pf], logger);
 
@@ -61,8 +62,7 @@ void nf_log_unset(struct net *net, const struct nf_logger *logger)
 
 	mutex_lock(&nf_log_mutex);
 	for (i = 0; i < NFPROTO_NUMPROTO; i++) {
-		log = rcu_dereference_protected(net->nf.nf_loggers[i],
-				lockdep_is_held(&nf_log_mutex));
+		log = nft_log_dereference(net->nf.nf_loggers[i]);
 		if (log == logger)
 			RCU_INIT_POINTER(net->nf.nf_loggers[i], NULL);
 	}
@@ -297,8 +297,7 @@ static int seq_show(struct seq_file *s, void *v)
 	int i, ret;
 	struct net *net = seq_file_net(s);
 
-	logger = rcu_dereference_protected(net->nf.nf_loggers[*pos],
-					   lockdep_is_held(&nf_log_mutex));
+	logger = nft_log_dereference(net->nf.nf_loggers[*pos]);
 
 	if (!logger)
 		ret = seq_printf(s, "%2lld NONE (", *pos);
@@ -312,8 +311,7 @@ static int seq_show(struct seq_file *s, void *v)
 		if (loggers[*pos][i] == NULL)
 			continue;
 
-		logger = rcu_dereference_protected(loggers[*pos][i],
-					   lockdep_is_held(&nf_log_mutex));
+		logger = nft_log_dereference(loggers[*pos][i]);
 		ret = seq_printf(s, "%s", logger->name);
 		if (ret < 0)
 			return ret;
@@ -385,8 +383,7 @@ static int nf_log_proc_dostring(struct ctl_table *table, int write,
 		mutex_unlock(&nf_log_mutex);
 	} else {
 		mutex_lock(&nf_log_mutex);
-		logger = rcu_dereference_protected(net->nf.nf_loggers[tindex],
-						   lockdep_is_held(&nf_log_mutex));
+		logger = nft_log_dereference(net->nf.nf_loggers[tindex]);
 		if (!logger)
 			table->data = "NONE";
 		else
-- 
1.9.3

[PATCH v2 2/3] netfilter: log: protect nf_log_register against double registering

[Marcelo Ricardo Leitner]

Currently, despite the comment right before the function,
nf_log_register allows registering two loggers on with the same type and
end up overwriting the previous register.

Not a real issue today as current tree doesn't have two loggers for the
same type but it's better to get this protected.

Also make sure that all of its callers do error checking.

Signed-off-by: Marcelo Ricardo Leitner <mleitner@redhat.com>
---

Notes:
    v1->v2:
     - make use of pr_fmt
     - added err1 error handling flow
     - based on nft_log_dereference()

 net/ipv4/netfilter/nf_log_arp.c  | 12 +++++++++++-
 net/ipv4/netfilter/nf_log_ipv4.c | 12 +++++++++++-
 net/ipv6/netfilter/nf_log_ipv6.c | 12 +++++++++++-
 net/netfilter/nf_log.c           | 11 ++++++++++-
 4 files changed, 43 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/netfilter/nf_log_arp.c b/net/ipv4/netfilter/nf_log_arp.c
index ccfc78db12ee8acae68faf451f2cf6bc5597f2c1..0c8799a0c9e46df1bd414251c4d5661da024fae1 100644
--- a/net/ipv4/netfilter/nf_log_arp.c
+++ b/net/ipv4/netfilter/nf_log_arp.c
@@ -10,6 +10,7 @@
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
  */
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <linux/module.h>
 #include <linux/spinlock.h>
@@ -130,8 +131,17 @@ static int __init nf_log_arp_init(void)
 	if (ret < 0)
 		return ret;
 
-	nf_log_register(NFPROTO_ARP, &nf_arp_logger);
+	ret = nf_log_register(NFPROTO_ARP, &nf_arp_logger);
+	if (ret < 0) {
+		pr_err("failed to register logger\n");
+		goto err1;
+	}
+
 	return 0;
+
+err1:
+	unregister_pernet_subsys(&nf_log_arp_net_ops);
+	return ret;
 }
 
 static void __exit nf_log_arp_exit(void)
diff --git a/net/ipv4/netfilter/nf_log_ipv4.c b/net/ipv4/netfilter/nf_log_ipv4.c
index 078bdca1b607a167e05e7cf1bdfedccdd5aca92a..75101980eeee197a4f8413bbd7d29f4fd9e4bb74 100644
--- a/net/ipv4/netfilter/nf_log_ipv4.c
+++ b/net/ipv4/netfilter/nf_log_ipv4.c
@@ -5,6 +5,7 @@
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
  */
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <linux/module.h>
 #include <linux/spinlock.h>
@@ -366,8 +367,17 @@ static int __init nf_log_ipv4_init(void)
 	if (ret < 0)
 		return ret;
 
-	nf_log_register(NFPROTO_IPV4, &nf_ip_logger);
+	ret = nf_log_register(NFPROTO_IPV4, &nf_ip_logger);
+	if (ret < 0) {
+		pr_err("failed to register logger\n");
+		goto err1;
+	}
+
 	return 0;
+
+err1:
+	unregister_pernet_subsys(&nf_log_ipv4_net_ops);
+	return ret;
 }
 
 static void __exit nf_log_ipv4_exit(void)
diff --git a/net/ipv6/netfilter/nf_log_ipv6.c b/net/ipv6/netfilter/nf_log_ipv6.c
index 7b17a0be93e7eccb2a26cd3294713d0f1112158d..7fc34d1681a195ff071406811771b8327337db22 100644
--- a/net/ipv6/netfilter/nf_log_ipv6.c
+++ b/net/ipv6/netfilter/nf_log_ipv6.c
@@ -5,6 +5,7 @@
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
  */
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <linux/module.h>
 #include <linux/spinlock.h>
@@ -398,8 +399,17 @@ static int __init nf_log_ipv6_init(void)
 	if (ret < 0)
 		return ret;
 
-	nf_log_register(NFPROTO_IPV6, &nf_ip6_logger);
+	ret = nf_log_register(NFPROTO_IPV6, &nf_ip6_logger);
+	if (ret < 0) {
+		pr_err("failed to register logger\n");
+		goto err1;
+	}
+
 	return 0;
+
+err1:
+	unregister_pernet_subsys(&nf_log_ipv6_net_ops);
+	return ret;
 }
 
 static void __exit nf_log_ipv6_exit(void)
diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
index f1409d95f810c689ec70755eb8a85125d291ad47..e7c7439f48db590eba8f7f2eac61fafd9e571389 100644
--- a/net/netfilter/nf_log.c
+++ b/net/netfilter/nf_log.c
@@ -82,10 +82,19 @@ int nf_log_register(u_int8_t pf, struct nf_logger *logger)
 	mutex_lock(&nf_log_mutex);
 
 	if (pf == NFPROTO_UNSPEC) {
+		for (i = NFPROTO_UNSPEC; i < NFPROTO_NUMPROTO; i++) {
+			if (nft_log_dereference(loggers[i][logger->type])) {
+				mutex_unlock(&nf_log_mutex);
+				return -EEXIST;
+			}
+		}
 		for (i = NFPROTO_UNSPEC; i < NFPROTO_NUMPROTO; i++)
 			rcu_assign_pointer(loggers[i][logger->type], logger);
 	} else {
-		/* register at end of list to honor first register win */
+		if (nft_log_dereference(loggers[pf][logger->type])) {
+			mutex_unlock(&nf_log_mutex);
+			return -EEXIST;
+		}
 		rcu_assign_pointer(loggers[pf][logger->type], logger);
 	}
 
-- 
1.9.3

Re: [PATCH v2 2/3] netfilter: log: protect nf_log_register against double registering

[Pablo Neira Ayuso]

Hi Marcelo,

I just noticed another minor issue that I didn't notice in my previous
review.

On Fri, Oct 24, 2014 at 10:59:50AM -0200, Marcelo Ricardo Leitner wrote:
> diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
> index f1409d95f810c689ec70755eb8a85125d291ad47..e7c7439f48db590eba8f7f2eac61fafd9e571389 100644
> --- a/net/netfilter/nf_log.c
> +++ b/net/netfilter/nf_log.c
> @@ -82,10 +82,19 @@ int nf_log_register(u_int8_t pf, struct nf_logger *logger)
>  	mutex_lock(&nf_log_mutex);
>  
>  	if (pf == NFPROTO_UNSPEC) {
> +		for (i = NFPROTO_UNSPEC; i < NFPROTO_NUMPROTO; i++) {
> +			if (nft_log_dereference(loggers[i][logger->type])) {

Given that we're not dereferencing, I think you can use
rcu_access_pointer() instead here.

> +				mutex_unlock(&nf_log_mutex);
> +				return -EEXIST;
> +			}
> +		}
>  		for (i = NFPROTO_UNSPEC; i < NFPROTO_NUMPROTO; i++)
>  			rcu_assign_pointer(loggers[i][logger->type], logger);
>  	} else {
> -		/* register at end of list to honor first register win */
> +		if (nft_log_dereference(loggers[pf][logger->type])) {

Same thing here.

Re: [PATCH v2 2/3] netfilter: log: protect nf_log_register against double registering

[Marcelo Ricardo Leitner]

On 27-10-2014 20:09, Pablo Neira Ayuso wrote:
> Hi Marcelo,
>
> I just noticed another minor issue that I didn't notice in my previous
> review.
>
> On Fri, Oct 24, 2014 at 10:59:50AM -0200, Marcelo Ricardo Leitner wrote:
>> diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
>> index f1409d95f810c689ec70755eb8a85125d291ad47..e7c7439f48db590eba8f7f2eac61fafd9e571389 100644
>> --- a/net/netfilter/nf_log.c
>> +++ b/net/netfilter/nf_log.c
>> @@ -82,10 +82,19 @@ int nf_log_register(u_int8_t pf, struct nf_logger *logger)
>>   	mutex_lock(&nf_log_mutex);
>>
>>   	if (pf == NFPROTO_UNSPEC) {
>> +		for (i = NFPROTO_UNSPEC; i < NFPROTO_NUMPROTO; i++) {
>> +			if (nft_log_dereference(loggers[i][logger->type])) {
>
> Given that we're not dereferencing, I think you can use
> rcu_access_pointer() instead here.

Right! I'll update it (this and the below one) and send in a few, thanks.

Marcelo

>> +				mutex_unlock(&nf_log_mutex);
>> +				return -EEXIST;
>> +			}
>> +		}
>>   		for (i = NFPROTO_UNSPEC; i < NFPROTO_NUMPROTO; i++)
>>   			rcu_assign_pointer(loggers[i][logger->type], logger);
>>   	} else {
>> -		/* register at end of list to honor first register win */
>> +		if (nft_log_dereference(loggers[pf][logger->type])) {
>
> Same thing here.
>

[PATCH 3/3] Make use of pr_fmt where applicable

[Marcelo Ricardo Leitner]

And also remove PRINTR macro, as it was used only once, wasn't helping
much and was actually making it harder to use pr_err().

Signed-off-by: Marcelo Ricardo Leitner <mleitner@redhat.com>
---
 net/netfilter/nfnetlink_log.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c
index b1e3a05794169283ed50d1c0fb4f44d9e7753eeb..1c6c970e5d1baa5517c5589bd52345d0ae971c1a 100644
--- a/net/netfilter/nfnetlink_log.c
+++ b/net/netfilter/nfnetlink_log.c
@@ -12,6 +12,8 @@
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
  */
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #include <linux/module.h>
 #include <linux/skbuff.h>
 #include <linux/if_arp.h>
@@ -45,9 +47,6 @@
 #define NFULNL_QTHRESH_DEFAULT 	100	/* 100 packets */
 #define NFULNL_COPY_RANGE_MAX	0xFFFF	/* max packet size is limited by 16-bit struct nfattr nfa_len field */
 
-#define PRINTR(x, args...)	do { if (net_ratelimit()) \
-				     printk(x, ## args); } while (0);
-
 struct nfulnl_instance {
 	struct hlist_node hlist;	/* global list of instances */
 	spinlock_t lock;
@@ -335,8 +334,7 @@ nfulnl_alloc_skb(struct net *net, u32 peer_portid, unsigned int inst_size,
 			skb = nfnetlink_alloc_skb(net, pkt_size,
 						  peer_portid, GFP_ATOMIC);
 			if (!skb)
-				pr_err("nfnetlink_log: can't even alloc %u bytes\n",
-				       pkt_size);
+				pr_err("can't even alloc %u bytes\n", pkt_size);
 		}
 	}
 
@@ -569,7 +567,7 @@ __build_packet_message(struct nfnl_log_net *log,
 		int size = nla_attr_size(data_len);
 
 		if (skb_tailroom(inst->skb) < nla_total_size(data_len)) {
-			printk(KERN_WARNING "nfnetlink_log: no tailroom!\n");
+			pr_warn("no tailroom!\n");
 			return -1;
 		}
 
@@ -585,7 +583,8 @@ __build_packet_message(struct nfnl_log_net *log,
 	return 0;
 
 nla_put_failure:
-	PRINTR(KERN_ERR "nfnetlink_log: error creating log nlmsg\n");
+	if (net_ratelimit())
+		pr_err("error creating log nlmsg\n");
 	return -1;
 }
 
@@ -1068,19 +1067,19 @@ static int __init nfnetlink_log_init(void)
 	netlink_register_notifier(&nfulnl_rtnl_notifier);
 	status = nfnetlink_subsys_register(&nfulnl_subsys);
 	if (status < 0) {
-		pr_err("log: failed to create netlink socket\n");
+		pr_err("failed to create netlink socket\n");
 		goto cleanup_netlink_notifier;
 	}
 
 	status = nf_log_register(NFPROTO_UNSPEC, &nfulnl_logger);
 	if (status < 0) {
-		pr_err("log: failed to register logger\n");
+		pr_err("failed to register logger\n");
 		goto cleanup_subsys;
 	}
 
 	status = register_pernet_subsys(&nfnl_log_net_ops);
 	if (status < 0) {
-		pr_err("log: failed to register pernet ops\n");
+		pr_err("failed to register pernet ops\n");
 		goto cleanup_logger;
 	}
 	return status;
-- 
1.9.3

Re: [PATCH 3/3] Make use of pr_fmt where applicable

[Marcelo Ricardo Leitner]

On 24-10-2014 10:59, Marcelo Ricardo Leitner wrote:
> And also remove PRINTR macro, as it was used only once, wasn't helping
> much and was actually making it harder to use pr_err().
>
> Signed-off-by: Marcelo Ricardo Leitner <mleitner@redhat.com>
> ---
>   net/netfilter/nfnetlink_log.c | 19 +++++++++----------
>   1 file changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c
> index b1e3a05794169283ed50d1c0fb4f44d9e7753eeb..1c6c970e5d1baa5517c5589bd52345d0ae971c1a 100644
> --- a/net/netfilter/nfnetlink_log.c
> +++ b/net/netfilter/nfnetlink_log.c
> @@ -12,6 +12,8 @@
>    * it under the terms of the GNU General Public License version 2 as
>    * published by the Free Software Foundation.
>    */
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
>   #include <linux/module.h>
>   #include <linux/skbuff.h>
>   #include <linux/if_arp.h>
> @@ -45,9 +47,6 @@
>   #define NFULNL_QTHRESH_DEFAULT 	100	/* 100 packets */
>   #define NFULNL_COPY_RANGE_MAX	0xFFFF	/* max packet size is limited by 16-bit struct nfattr nfa_len field */
>
> -#define PRINTR(x, args...)	do { if (net_ratelimit()) \
> -				     printk(x, ## args); } while (0);
> -
>   struct nfulnl_instance {
>   	struct hlist_node hlist;	/* global list of instances */
>   	spinlock_t lock;
> @@ -335,8 +334,7 @@ nfulnl_alloc_skb(struct net *net, u32 peer_portid, unsigned int inst_size,
>   			skb = nfnetlink_alloc_skb(net, pkt_size,
>   						  peer_portid, GFP_ATOMIC);
>   			if (!skb)
> -				pr_err("nfnetlink_log: can't even alloc %u bytes\n",
> -				       pkt_size);
> +				pr_err("can't even alloc %u bytes\n", pkt_size);
>   		}
>   	}
>
> @@ -569,7 +567,7 @@ __build_packet_message(struct nfnl_log_net *log,
>   		int size = nla_attr_size(data_len);
>
>   		if (skb_tailroom(inst->skb) < nla_total_size(data_len)) {
> -			printk(KERN_WARNING "nfnetlink_log: no tailroom!\n");
> +			pr_warn("no tailroom!\n");
>   			return -1;
>   		}
>
> @@ -585,7 +583,8 @@ __build_packet_message(struct nfnl_log_net *log,
>   	return 0;
>
>   nla_put_failure:
> -	PRINTR(KERN_ERR "nfnetlink_log: error creating log nlmsg\n");
> +	if (net_ratelimit())
> +		pr_err("error creating log nlmsg\n");

I'll send a v2 using pr_err_ratelimited() instead

Marcelo

Re: [PATCH 3/3] Make use of pr_fmt where applicable

[Marcelo Ricardo Leitner]

On 24-10-2014 16:11, Marcelo Ricardo Leitner wrote:
> On 24-10-2014 10:59, Marcelo Ricardo Leitner wrote:
>> And also remove PRINTR macro, as it was used only once, wasn't helping
>> much and was actually making it harder to use pr_err().
>>
>> Signed-off-by: Marcelo Ricardo Leitner <mleitner@redhat.com>
>> ---
>>   net/netfilter/nfnetlink_log.c | 19 +++++++++----------
>>   1 file changed, 9 insertions(+), 10 deletions(-)
>>
>> diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c
>> index
>> b1e3a05794169283ed50d1c0fb4f44d9e7753eeb..1c6c970e5d1baa5517c5589bd52345d0ae971c1a
>> 100644
>> --- a/net/netfilter/nfnetlink_log.c
>> +++ b/net/netfilter/nfnetlink_log.c
>> @@ -12,6 +12,8 @@
>>    * it under the terms of the GNU General Public License version 2 as
>>    * published by the Free Software Foundation.
>>    */
>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>> +
>>   #include <linux/module.h>
>>   #include <linux/skbuff.h>
>>   #include <linux/if_arp.h>
>> @@ -45,9 +47,6 @@
>>   #define NFULNL_QTHRESH_DEFAULT     100    /* 100 packets */
>>   #define NFULNL_COPY_RANGE_MAX    0xFFFF    /* max packet size is limited
>> by 16-bit struct nfattr nfa_len field */
>>
>> -#define PRINTR(x, args...)    do { if (net_ratelimit()) \
>> -                     printk(x, ## args); } while (0);
>> -
>>   struct nfulnl_instance {
>>       struct hlist_node hlist;    /* global list of instances */
>>       spinlock_t lock;
>> @@ -335,8 +334,7 @@ nfulnl_alloc_skb(struct net *net, u32 peer_portid,
>> unsigned int inst_size,
>>               skb = nfnetlink_alloc_skb(net, pkt_size,
>>                             peer_portid, GFP_ATOMIC);
>>               if (!skb)
>> -                pr_err("nfnetlink_log: can't even alloc %u bytes\n",
>> -                       pkt_size);
>> +                pr_err("can't even alloc %u bytes\n", pkt_size);
>>           }
>>       }
>>
>> @@ -569,7 +567,7 @@ __build_packet_message(struct nfnl_log_net *log,
>>           int size = nla_attr_size(data_len);
>>
>>           if (skb_tailroom(inst->skb) < nla_total_size(data_len)) {
>> -            printk(KERN_WARNING "nfnetlink_log: no tailroom!\n");
>> +            pr_warn("no tailroom!\n");
>>               return -1;
>>           }
>>
>> @@ -585,7 +583,8 @@ __build_packet_message(struct nfnl_log_net *log,
>>       return 0;
>>
>>   nla_put_failure:
>> -    PRINTR(KERN_ERR "nfnetlink_log: error creating log nlmsg\n");
>> +    if (net_ratelimit())
>> +        pr_err("error creating log nlmsg\n");
>
> I'll send a v2 using pr_err_ratelimited() instead

Or not.. WDYT? Because we are currently using net_ratelimit() and 
pr_err_ratelimited() would create a new limit just for this message.

Thanks,
Marcelo

[PATCH v2 3/3] Make use of pr_fmt where applicable

[Marcelo Ricardo Leitner]

And also remove PRINTR macro in favor of net_*_ratelimited().

Signed-off-by: Marcelo Ricardo Leitner <mleitner@redhat.com>
---

Notes:
    v1-v2:
      make use of net_err_ratelimited()

 net/netfilter/nfnetlink_log.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c
index b1e3a05794169283ed50d1c0fb4f44d9e7753eeb..0eb10bf295e92ad29541f58ca1de67e3380157c0 100644
--- a/net/netfilter/nfnetlink_log.c
+++ b/net/netfilter/nfnetlink_log.c
@@ -12,6 +12,8 @@
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
  */
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #include <linux/module.h>
 #include <linux/skbuff.h>
 #include <linux/if_arp.h>
@@ -45,9 +47,6 @@
 #define NFULNL_QTHRESH_DEFAULT 	100	/* 100 packets */
 #define NFULNL_COPY_RANGE_MAX	0xFFFF	/* max packet size is limited by 16-bit struct nfattr nfa_len field */
 
-#define PRINTR(x, args...)	do { if (net_ratelimit()) \
-				     printk(x, ## args); } while (0);
-
 struct nfulnl_instance {
 	struct hlist_node hlist;	/* global list of instances */
 	spinlock_t lock;
@@ -335,8 +334,7 @@ nfulnl_alloc_skb(struct net *net, u32 peer_portid, unsigned int inst_size,
 			skb = nfnetlink_alloc_skb(net, pkt_size,
 						  peer_portid, GFP_ATOMIC);
 			if (!skb)
-				pr_err("nfnetlink_log: can't even alloc %u bytes\n",
-				       pkt_size);
+				pr_err("can't even alloc %u bytes\n", pkt_size);
 		}
 	}
 
@@ -569,7 +567,7 @@ __build_packet_message(struct nfnl_log_net *log,
 		int size = nla_attr_size(data_len);
 
 		if (skb_tailroom(inst->skb) < nla_total_size(data_len)) {
-			printk(KERN_WARNING "nfnetlink_log: no tailroom!\n");
+			pr_warn("no tailroom!\n");
 			return -1;
 		}
 
@@ -585,7 +583,7 @@ __build_packet_message(struct nfnl_log_net *log,
 	return 0;
 
 nla_put_failure:
-	PRINTR(KERN_ERR "nfnetlink_log: error creating log nlmsg\n");
+	net_err_ratelimited("error creating log nlmsg\n");
 	return -1;
 }
 
@@ -1068,19 +1066,19 @@ static int __init nfnetlink_log_init(void)
 	netlink_register_notifier(&nfulnl_rtnl_notifier);
 	status = nfnetlink_subsys_register(&nfulnl_subsys);
 	if (status < 0) {
-		pr_err("log: failed to create netlink socket\n");
+		pr_err("failed to create netlink socket\n");
 		goto cleanup_netlink_notifier;
 	}
 
 	status = nf_log_register(NFPROTO_UNSPEC, &nfulnl_logger);
 	if (status < 0) {
-		pr_err("log: failed to register logger\n");
+		pr_err("failed to register logger\n");
 		goto cleanup_subsys;
 	}
 
 	status = register_pernet_subsys(&nfnl_log_net_ops);
 	if (status < 0) {
-		pr_err("log: failed to register pernet ops\n");
+		pr_err("failed to register pernet ops\n");
 		goto cleanup_logger;
 	}
 	return status;
-- 
1.9.3

Re: [PATCH v2 3/3] Make use of pr_fmt where applicable

[Pablo Neira Ayuso]

On Fri, Oct 24, 2014 at 04:46:32PM -0200, Marcelo Ricardo Leitner wrote:
> And also remove PRINTR macro in favor of net_*_ratelimited().
> 
> Signed-off-by: Marcelo Ricardo Leitner <mleitner@redhat.com>
> ---
> 
> Notes:
>     v1-v2:
>       make use of net_err_ratelimited()
> 
>  net/netfilter/nfnetlink_log.c | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c
> index b1e3a05794169283ed50d1c0fb4f44d9e7753eeb..0eb10bf295e92ad29541f58ca1de67e3380157c0 100644
> --- a/net/netfilter/nfnetlink_log.c
> +++ b/net/netfilter/nfnetlink_log.c
> @@ -12,6 +12,8 @@
>   * it under the terms of the GNU General Public License version 2 as
>   * published by the Free Software Foundation.
>   */
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
>  #include <linux/module.h>
>  #include <linux/skbuff.h>
>  #include <linux/if_arp.h>
> @@ -45,9 +47,6 @@
>  #define NFULNL_QTHRESH_DEFAULT 	100	/* 100 packets */
>  #define NFULNL_COPY_RANGE_MAX	0xFFFF	/* max packet size is limited by 16-bit struct nfattr nfa_len field */
>  
> -#define PRINTR(x, args...)	do { if (net_ratelimit()) \
> -				     printk(x, ## args); } while (0);
> -
>  struct nfulnl_instance {
>  	struct hlist_node hlist;	/* global list of instances */
>  	spinlock_t lock;
> @@ -335,8 +334,7 @@ nfulnl_alloc_skb(struct net *net, u32 peer_portid, unsigned int inst_size,
>  			skb = nfnetlink_alloc_skb(net, pkt_size,
>  						  peer_portid, GFP_ATOMIC);
>  			if (!skb)
> -				pr_err("nfnetlink_log: can't even alloc %u bytes\n",
> -				       pkt_size);
> +				pr_err("can't even alloc %u bytes\n", pkt_size);
>  		}
>  	}
>  
> @@ -569,7 +567,7 @@ __build_packet_message(struct nfnl_log_net *log,
>  		int size = nla_attr_size(data_len);
>  
>  		if (skb_tailroom(inst->skb) < nla_total_size(data_len)) {
> -			printk(KERN_WARNING "nfnetlink_log: no tailroom!\n");
> +			pr_warn("no tailroom!\n");
>  			return -1;

I think we can just goto nla_put_failure and remove this warning.

>  		}
>  
> @@ -585,7 +583,7 @@ __build_packet_message(struct nfnl_log_net *log,
>  	return 0;
>  
>  nla_put_failure:
> -	PRINTR(KERN_ERR "nfnetlink_log: error creating log nlmsg\n");
> +	net_err_ratelimited("error creating log nlmsg\n");

I'd remove this one too.

But before doing any error message cleanup, we should also call
nlmsg_cancel() here to leave the nflog netlink batch message in
consistent state in case of errors. And we don't check for errors
after __build_packet_message() which may result in sending an
incomplete message to userspace.

Would you send us patches to address this? Thanks.

Re: [PATCH v2 3/3] Make use of pr_fmt where applicable

[Marcelo Ricardo Leitner]

On 27-10-2014 20:23, Pablo Neira Ayuso wrote:
> On Fri, Oct 24, 2014 at 04:46:32PM -0200, Marcelo Ricardo Leitner wrote:
>> And also remove PRINTR macro in favor of net_*_ratelimited().
>>
>> Signed-off-by: Marcelo Ricardo Leitner <mleitner@redhat.com>
>> ---
>>
>> Notes:
>>      v1-v2:
>>        make use of net_err_ratelimited()
>>
>>   net/netfilter/nfnetlink_log.c | 18 ++++++++----------
>>   1 file changed, 8 insertions(+), 10 deletions(-)
>>
>> diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c
>> index b1e3a05794169283ed50d1c0fb4f44d9e7753eeb..0eb10bf295e92ad29541f58ca1de67e3380157c0 100644
>> --- a/net/netfilter/nfnetlink_log.c
>> +++ b/net/netfilter/nfnetlink_log.c
>> @@ -12,6 +12,8 @@
>>    * it under the terms of the GNU General Public License version 2 as
>>    * published by the Free Software Foundation.
>>    */
>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>> +
>>   #include <linux/module.h>
>>   #include <linux/skbuff.h>
>>   #include <linux/if_arp.h>
>> @@ -45,9 +47,6 @@
>>   #define NFULNL_QTHRESH_DEFAULT 	100	/* 100 packets */
>>   #define NFULNL_COPY_RANGE_MAX	0xFFFF	/* max packet size is limited by 16-bit struct nfattr nfa_len field */
>>
>> -#define PRINTR(x, args...)	do { if (net_ratelimit()) \
>> -				     printk(x, ## args); } while (0);
>> -
>>   struct nfulnl_instance {
>>   	struct hlist_node hlist;	/* global list of instances */
>>   	spinlock_t lock;
>> @@ -335,8 +334,7 @@ nfulnl_alloc_skb(struct net *net, u32 peer_portid, unsigned int inst_size,
>>   			skb = nfnetlink_alloc_skb(net, pkt_size,
>>   						  peer_portid, GFP_ATOMIC);
>>   			if (!skb)
>> -				pr_err("nfnetlink_log: can't even alloc %u bytes\n",
>> -				       pkt_size);
>> +				pr_err("can't even alloc %u bytes\n", pkt_size);
>>   		}
>>   	}
>>
>> @@ -569,7 +567,7 @@ __build_packet_message(struct nfnl_log_net *log,
>>   		int size = nla_attr_size(data_len);
>>
>>   		if (skb_tailroom(inst->skb) < nla_total_size(data_len)) {
>> -			printk(KERN_WARNING "nfnetlink_log: no tailroom!\n");
>> +			pr_warn("no tailroom!\n");
>>   			return -1;
>
> I think we can just goto nla_put_failure and remove this warning.

ok

>>   		}
>>
>> @@ -585,7 +583,7 @@ __build_packet_message(struct nfnl_log_net *log,
>>   	return 0;
>>
>>   nla_put_failure:
>> -	PRINTR(KERN_ERR "nfnetlink_log: error creating log nlmsg\n");
>> +	net_err_ratelimited("error creating log nlmsg\n");
>
> I'd remove this one too.
>
> But before doing any error message cleanup, we should also call
> nlmsg_cancel() here to leave the nflog netlink batch message in
> consistent state in case of errors. And we don't check for errors
> after __build_packet_message() which may result in sending an
> incomplete message to userspace.

okay

> Would you send us patches to address this? Thanks.

Sure thing. Thanks Pablo.

Marcelo

Re: [PATCH v2 3/3] Make use of pr_fmt where applicable

[Marcelo Ricardo Leitner]

On 28-10-2014 10:59, Marcelo Ricardo Leitner wrote:
> On 27-10-2014 20:23, Pablo Neira Ayuso wrote:
>> On Fri, Oct 24, 2014 at 04:46:32PM -0200, Marcelo Ricardo Leitner wrote:
>>> And also remove PRINTR macro in favor of net_*_ratelimited().
>>>
>>> Signed-off-by: Marcelo Ricardo Leitner <mleitner@redhat.com>
>>> ---
>>>
>>> Notes:
>>>      v1-v2:
>>>        make use of net_err_ratelimited()
>>>
>>>   net/netfilter/nfnetlink_log.c | 18 ++++++++----------
>>>   1 file changed, 8 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c
>>> index
>>> b1e3a05794169283ed50d1c0fb4f44d9e7753eeb..0eb10bf295e92ad29541f58ca1de67e3380157c0
>>> 100644
>>> --- a/net/netfilter/nfnetlink_log.c
>>> +++ b/net/netfilter/nfnetlink_log.c
>>> @@ -12,6 +12,8 @@
>>>    * it under the terms of the GNU General Public License version 2 as
>>>    * published by the Free Software Foundation.
>>>    */
>>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>>> +
>>>   #include <linux/module.h>
>>>   #include <linux/skbuff.h>
>>>   #include <linux/if_arp.h>
>>> @@ -45,9 +47,6 @@
>>>   #define NFULNL_QTHRESH_DEFAULT     100    /* 100 packets */
>>>   #define NFULNL_COPY_RANGE_MAX    0xFFFF    /* max packet size is limited
>>> by 16-bit struct nfattr nfa_len field */
>>>
>>> -#define PRINTR(x, args...)    do { if (net_ratelimit()) \
>>> -                     printk(x, ## args); } while (0);
>>> -
>>>   struct nfulnl_instance {
>>>       struct hlist_node hlist;    /* global list of instances */
>>>       spinlock_t lock;
>>> @@ -335,8 +334,7 @@ nfulnl_alloc_skb(struct net *net, u32 peer_portid,
>>> unsigned int inst_size,
>>>               skb = nfnetlink_alloc_skb(net, pkt_size,
>>>                             peer_portid, GFP_ATOMIC);
>>>               if (!skb)
>>> -                pr_err("nfnetlink_log: can't even alloc %u bytes\n",
>>> -                       pkt_size);
>>> +                pr_err("can't even alloc %u bytes\n", pkt_size);
>>>           }
>>>       }
>>>
>>> @@ -569,7 +567,7 @@ __build_packet_message(struct nfnl_log_net *log,
>>>           int size = nla_attr_size(data_len);
>>>
>>>           if (skb_tailroom(inst->skb) < nla_total_size(data_len)) {
>>> -            printk(KERN_WARNING "nfnetlink_log: no tailroom!\n");
>>> +            pr_warn("no tailroom!\n");
>>>               return -1;
>>
>> I think we can just goto nla_put_failure and remove this warning.
>
> ok
>
>>>           }
>>>
>>> @@ -585,7 +583,7 @@ __build_packet_message(struct nfnl_log_net *log,
>>>       return 0;
>>>
>>>   nla_put_failure:
>>> -    PRINTR(KERN_ERR "nfnetlink_log: error creating log nlmsg\n");
>>> +    net_err_ratelimited("error creating log nlmsg\n");
>>
>> I'd remove this one too.
>>
>> But before doing any error message cleanup, we should also call
>> nlmsg_cancel() here to leave the nflog netlink batch message in
>> consistent state in case of errors. And we don't check for errors
>> after __build_packet_message() which may result in sending an
>> incomplete message to userspace.
>
> okay
>
>> Would you send us patches to address this? Thanks.
>
> Sure thing. Thanks Pablo.
>

Hi Pablo,

While we are at this, what about this one, does it really have to be a BUG() one?

__build_packet_message()
...
     if (data_len) {
         struct nlattr *nla;
         int size = nla_attr_size(data_len);

         if (skb_tailroom(inst->skb) < nla_total_size(data_len))
             goto nla_put_failure;           <-- already changed

         nla = (struct nlattr *)skb_put(inst->skb, nla_total_size(data_len));
         nla->nla_type = NFULA_PAYLOAD;
         nla->nla_len = size;

         if (skb_copy_bits(skb, 0, nla_data(nla), data_len))
             BUG();         <--
     }

Seems we could just put a goto nla_put_failure there too instead of bringing 
everything down. skb_copy_bits will only fail if we try to copy too much from 
the skb.. We could leave a WARN_ONCE in there if the idea is to catch nasty 
bugs in there. WDYT? I'm thinking in just sticking with a goto in there too.

Marcelo

Re: [PATCH v2 3/3] Make use of pr_fmt where applicable

[Pablo Neira Ayuso]

On Tue, Oct 28, 2014 at 05:56:31PM -0200, Marcelo Ricardo Leitner wrote:
> Hi Pablo,
> 
> While we are at this, what about this one, does it really have to be a BUG() one?
> 
> __build_packet_message()
> ...
>     if (data_len) {
>         struct nlattr *nla;
>         int size = nla_attr_size(data_len);
> 
>         if (skb_tailroom(inst->skb) < nla_total_size(data_len))
>             goto nla_put_failure;           <-- already changed
> 
>         nla = (struct nlattr *)skb_put(inst->skb, nla_total_size(data_len));
>         nla->nla_type = NFULA_PAYLOAD;
>         nla->nla_len = size;
> 
>         if (skb_copy_bits(skb, 0, nla_data(nla), data_len))
>             BUG();         <--
>     }
> 
> Seems we could just put a goto nla_put_failure there too instead of
> bringing everything down. skb_copy_bits will only fail if we try to
> copy too much from the skb.. We could leave a WARN_ONCE in there if
> the idea is to catch nasty bugs in there. WDYT? I'm thinking in just
> sticking with a goto in there too.

I think this is most likely catching a malformed skbuff coming from
the driver, in that case we shouldn't go further. There's similar
handling in other part of the kernel.

This may also trigger the BUG() if data_len is miscalculated as you
said, but that seems less likely to happen to me.

I would leave that one as it is.

Re: [PATCH v2 3/3] Make use of pr_fmt where applicable

[Marcelo Ricardo Leitner]

On 28-10-2014 18:12, Pablo Neira Ayuso wrote:
> On Tue, Oct 28, 2014 at 05:56:31PM -0200, Marcelo Ricardo Leitner wrote:
>> Hi Pablo,
>>
>> While we are at this, what about this one, does it really have to be a BUG() one?
>>
>> __build_packet_message()
>> ...
>>      if (data_len) {
>>          struct nlattr *nla;
>>          int size = nla_attr_size(data_len);
>>
>>          if (skb_tailroom(inst->skb) < nla_total_size(data_len))
>>              goto nla_put_failure;           <-- already changed
>>
>>          nla = (struct nlattr *)skb_put(inst->skb, nla_total_size(data_len));
>>          nla->nla_type = NFULA_PAYLOAD;
>>          nla->nla_len = size;
>>
>>          if (skb_copy_bits(skb, 0, nla_data(nla), data_len))
>>              BUG();         <--
>>      }
>>
>> Seems we could just put a goto nla_put_failure there too instead of
>> bringing everything down. skb_copy_bits will only fail if we try to
>> copy too much from the skb.. We could leave a WARN_ONCE in there if
>> the idea is to catch nasty bugs in there. WDYT? I'm thinking in just
>> sticking with a goto in there too.
>
> I think this is most likely catching a malformed skbuff coming from
> the driver, in that case we shouldn't go further. There's similar
> handling in other part of the kernel.
>
> This may also trigger the BUG() if data_len is miscalculated as you
> said, but that seems less likely to happen to me.
>
> I would leave that one as it is.

Cool, thanks.

Marcelo

Re: [PATCH 1/3] Introduce nft_log_dereference() macro

[Pablo Neira Ayuso]

On Fri, Oct 24, 2014 at 10:59:49AM -0200, Marcelo Ricardo Leitner wrote:
> Wrap up a common call pattern in an easier to handle call.

Patch doesn't apply for some reason.

nf-next$ git am /tmp/1-3-Introduce-nft_log_dereference-macro.patch -s
Applying: Introduce nft_log_dereference() macro
error: patch failed: net/netfilter/nf_log.c:28
error: net/netfilter/nf_log.c: patch does not apply
Patch failed at 0001 Introduce nft_log_dereference() macro
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".

Re: [PATCH 1/3] Introduce nft_log_dereference() macro

[Marcelo Ricardo Leitner]

On 27-10-2014 20:03, Pablo Neira Ayuso wrote:
> On Fri, Oct 24, 2014 at 10:59:49AM -0200, Marcelo Ricardo Leitner wrote:
>> Wrap up a common call pattern in an easier to handle call.
>
> Patch doesn't apply for some reason.
>
> nf-next$ git am /tmp/1-3-Introduce-nft_log_dereference-macro.patch -s
> Applying: Introduce nft_log_dereference() macro
> error: patch failed: net/netfilter/nf_log.c:28
> error: net/netfilter/nf_log.c: patch does not apply
> Patch failed at 0001 Introduce nft_log_dereference() macro
> When you have resolved this problem run "git am --resolved".
> If you would prefer to skip this patch, instead run "git am --skip".
> To restore the original branch and stop patching run "git am --abort".
>

Oh that's weird, but I'll send it again together with the other ones.

Marcelo