[PATCH v4 1/3] netfilter: log: protect nf_log_register against double registering

[PATCH v4 1/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>
---
 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           | 16 +++++++++++++---
 4 files changed, 46 insertions(+), 6 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 5eaf047ed37facad0df59e70428c08ef8717a70d..9562e393fdf7e178b0c48bff25ace88495c2224f 100644
--- a/net/netfilter/nf_log.c
+++ b/net/netfilter/nf_log.c
@@ -75,6 +75,7 @@ EXPORT_SYMBOL(nf_log_unset);
 int nf_log_register(u_int8_t pf, struct nf_logger *logger)
 {
 	int i;
+	int ret = 0;
 
 	if (pf >= ARRAY_SIZE(init_net.nf.nf_loggers))
 		return -EINVAL;
@@ -82,16 +83,25 @@ 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 (rcu_access_pointer(loggers[i][logger->type])) {
+				ret = -EEXIST;
+				goto unlock;
+			}
+		}
 		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 (rcu_access_pointer(loggers[pf][logger->type])) {
+			ret = -EEXIST;
+			goto unlock;
+		}
 		rcu_assign_pointer(loggers[pf][logger->type], logger);
 	}
 
+unlock:
 	mutex_unlock(&nf_log_mutex);
-
-	return 0;
+	return ret;
 }
 EXPORT_SYMBOL(nf_log_register);
 
-- 
1.9.3

[PATCH v4 2/3] Do error handling if __build_packet_message fails

[Marcelo Ricardo Leitner]

Currently, we don't check if __build_packet_message fails or not and
that leaves it prone to sending incomplete messages to userspace.

This commit fixes it by canceling the new message if there was any issue
while building it and makes sure the skb is freed if it was the only
message in it.

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

diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c
index b1e3a05794169283ed50d1c0fb4f44d9e7753eeb..045c5956611f61a3f5dad3e15ca7cf19365e5afa 100644
--- a/net/netfilter/nfnetlink_log.c
+++ b/net/netfilter/nfnetlink_log.c
@@ -568,10 +568,8 @@ __build_packet_message(struct nfnl_log_net *log,
 		struct nlattr *nla;
 		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");
-			return -1;
-		}
+		if (skb_tailroom(inst->skb) < nla_total_size(data_len))
+			goto nla_put_failure;
 
 		nla = (struct nlattr *)skb_put(inst->skb, nla_total_size(data_len));
 		nla->nla_type = NFULA_PAYLOAD;
@@ -586,6 +584,7 @@ __build_packet_message(struct nfnl_log_net *log,
 
 nla_put_failure:
 	PRINTR(KERN_ERR "nfnetlink_log: error creating log nlmsg\n");
+	nlmsg_cancel(skb, nlh);
 	return -1;
 }
 
@@ -708,8 +707,9 @@ nfulnl_log_packet(struct net *net,
 
 	inst->qlen++;
 
-	__build_packet_message(log, inst, skb, data_len, pf,
-				hooknum, in, out, prefix, plen);
+	if (__build_packet_message(log, inst, skb, data_len, pf,
+				   hooknum, in, out, prefix, plen))
+		goto build_failure;
 
 	if (inst->qlen >= qthreshold)
 		__nfulnl_flush(inst);
@@ -726,6 +726,15 @@ unlock_and_release:
 	instance_put(inst);
 	return;
 
+build_failure:
+	/* If no other messages in it, we're good to free it. */
+	if (!inst->skb->len) {
+		kfree_skb(inst->skb);
+		inst->skb = NULL;
+	}
+
+	inst->qlen--;
+
 alloc_failure:
 	/* FIXME: statistics */
 	goto unlock_and_release;
-- 
1.9.3

[PATCH v4 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 | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c
index 045c5956611f61a3f5dad3e15ca7cf19365e5afa..cb8088729b83b159fa99640aa5e149d9dba6e129 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);
 		}
 	}
 
@@ -583,7 +581,6 @@ __build_packet_message(struct nfnl_log_net *log,
 	return 0;
 
 nla_put_failure:
-	PRINTR(KERN_ERR "nfnetlink_log: error creating log nlmsg\n");
 	nlmsg_cancel(skb, nlh);
 	return -1;
 }
@@ -1077,19 +1074,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 v4 1/3] netfilter: log: protect nf_log_register against double registering

[Pablo Neira Ayuso]

On Wed, Oct 29, 2014 at 10:51:13AM -0200, Marcelo Ricardo Leitner wrote:
> 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.

Also applied, thanks.

Re: [PATCH v4 2/3] Do error handling if __build_packet_message fails

[Pablo Neira Ayuso]

Hi Marcelo,

On Wed, Oct 29, 2014 at 10:51:14AM -0200, Marcelo Ricardo Leitner wrote:
> Currently, we don't check if __build_packet_message fails or not and
> that leaves it prone to sending incomplete messages to userspace.
> 
> This commit fixes it by canceling the new message if there was any issue
> while building it and makes sure the skb is freed if it was the only
> message in it.
> 
> Signed-off-by: Marcelo Ricardo Leitner <mleitner@redhat.com>
> ---
>  net/netfilter/nfnetlink_log.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c
> index b1e3a05794169283ed50d1c0fb4f44d9e7753eeb..045c5956611f61a3f5dad3e15ca7cf19365e5afa 100644
> --- a/net/netfilter/nfnetlink_log.c
> +++ b/net/netfilter/nfnetlink_log.c
> @@ -568,10 +568,8 @@ __build_packet_message(struct nfnl_log_net *log,
>  		struct nlattr *nla;
>  		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");
> -			return -1;
> -		}
> +		if (skb_tailroom(inst->skb) < nla_total_size(data_len))
> +			goto nla_put_failure;
>  
>  		nla = (struct nlattr *)skb_put(inst->skb, nla_total_size(data_len));
>  		nla->nla_type = NFULA_PAYLOAD;
> @@ -586,6 +584,7 @@ __build_packet_message(struct nfnl_log_net *log,
>  
>  nla_put_failure:
>  	PRINTR(KERN_ERR "nfnetlink_log: error creating log nlmsg\n");
> +	nlmsg_cancel(skb, nlh);
>  	return -1;
>  }
>  
> @@ -708,8 +707,9 @@ nfulnl_log_packet(struct net *net,
>  
>  	inst->qlen++;
>  
> -	__build_packet_message(log, inst, skb, data_len, pf,
> -				hooknum, in, out, prefix, plen);
> +	if (__build_packet_message(log, inst, skb, data_len, pf,
> +				   hooknum, in, out, prefix, plen))
> +		goto build_failure;
>  
>  	if (inst->qlen >= qthreshold)
>  		__nfulnl_flush(inst);
> @@ -726,6 +726,15 @@ unlock_and_release:
>  	instance_put(inst);
>  	return;
>  
> +build_failure:
> +	/* If no other messages in it, we're good to free it. */
> +	if (!inst->skb->len) {
> +		kfree_skb(inst->skb);
> +		inst->skb = NULL;
> +	}
> +
> +	inst->qlen--;

For each message that we put into the batch, we increase inst->qlen in
one, so I think this decrement isn't enough to leave things in
consistent state. If we at least have one message already in the
batch, I think it's good to give it a try to deliver it to userspace:

        if (inst->skb)
                __nfulnl_flush(inst);

Then, the WARN_ON that Florian added recently should catch that we
have size miscalculations from __nfulnl_send().

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

[Pablo Neira Ayuso]

On Wed, Oct 29, 2014 at 10:51:15AM -0200, 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().

I like this one, but it seems to depend on the previous :-(.

git am /tmp/v4-3-3-Make-use-of-pr_fmt-where-applicable.patch
Applying: Make use of pr_fmt where applicable
error: patch failed: net/netfilter/nfnetlink_log.c:583
error: net/netfilter/nfnetlink_log.c: patch does not apply
Patch failed at 0001 Make use of pr_fmt where applicable
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".

Would you resend? Thanks!

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

[Marcelo Ricardo Leitner]

On 04-11-2014 14:50, Pablo Neira Ayuso wrote:
> On Wed, Oct 29, 2014 at 10:51:15AM -0200, 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().
>
> I like this one, but it seems to depend on the previous :-(.
>
> git am /tmp/v4-3-3-Make-use-of-pr_fmt-where-applicable.patch
> Applying: Make use of pr_fmt where applicable
> error: patch failed: net/netfilter/nfnetlink_log.c:583
> error: net/netfilter/nfnetlink_log.c: patch does not apply
> Patch failed at 0001 Make use of pr_fmt where applicable
> 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".
>
> Would you resend? Thanks!

Sure. I'll resend just this one for now, as I need more time for the other one.

Thanks,
Marcelo

Re: [PATCH v4 2/3] Do error handling if __build_packet_message fails

[Marcelo Ricardo Leitner]

Hi Pablo,

On 04-11-2014 14:47, Pablo Neira Ayuso wrote:
> Hi Marcelo,
>
> On Wed, Oct 29, 2014 at 10:51:14AM -0200, Marcelo Ricardo Leitner wrote:
>> Currently, we don't check if __build_packet_message fails or not and
>> that leaves it prone to sending incomplete messages to userspace.
>>
>> This commit fixes it by canceling the new message if there was any issue
>> while building it and makes sure the skb is freed if it was the only
>> message in it.
>>
>> Signed-off-by: Marcelo Ricardo Leitner <mleitner@redhat.com>
>> ---
>>   net/netfilter/nfnetlink_log.c | 21 +++++++++++++++------
>>   1 file changed, 15 insertions(+), 6 deletions(-)
>>
>> diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c
>> index b1e3a05794169283ed50d1c0fb4f44d9e7753eeb..045c5956611f61a3f5dad3e15ca7cf19365e5afa 100644
>> --- a/net/netfilter/nfnetlink_log.c
>> +++ b/net/netfilter/nfnetlink_log.c
>> @@ -568,10 +568,8 @@ __build_packet_message(struct nfnl_log_net *log,
>>   		struct nlattr *nla;
>>   		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");
>> -			return -1;
>> -		}
>> +		if (skb_tailroom(inst->skb) < nla_total_size(data_len))
>> +			goto nla_put_failure;
>>
>>   		nla = (struct nlattr *)skb_put(inst->skb, nla_total_size(data_len));
>>   		nla->nla_type = NFULA_PAYLOAD;
>> @@ -586,6 +584,7 @@ __build_packet_message(struct nfnl_log_net *log,
>>
>>   nla_put_failure:
>>   	PRINTR(KERN_ERR "nfnetlink_log: error creating log nlmsg\n");
>> +	nlmsg_cancel(skb, nlh);
>>   	return -1;
>>   }
>>
>> @@ -708,8 +707,9 @@ nfulnl_log_packet(struct net *net,
>>
>>   	inst->qlen++;
>>
>> -	__build_packet_message(log, inst, skb, data_len, pf,
>> -				hooknum, in, out, prefix, plen);
>> +	if (__build_packet_message(log, inst, skb, data_len, pf,
>> +				   hooknum, in, out, prefix, plen))
>> +		goto build_failure;
>>
>>   	if (inst->qlen >= qthreshold)
>>   		__nfulnl_flush(inst);
>> @@ -726,6 +726,15 @@ unlock_and_release:
>>   	instance_put(inst);
>>   	return;
>>
>> +build_failure:
>> +	/* If no other messages in it, we're good to free it. */
>> +	if (!inst->skb->len) {
>> +		kfree_skb(inst->skb);
>> +		inst->skb = NULL;
>> +	}
>> +
>> +	inst->qlen--;
>
> For each message that we put into the batch, we increase inst->qlen in
> one, so I think this decrement isn't enough to leave things in
> consistent state. If we at least have one message already in the
> batch, I think it's good to give it a try to deliver it to userspace:
>
>          if (inst->skb)
>                  __nfulnl_flush(inst);

The idea was to undo just the last attempt and leave the older ones where they 
were...

Currently we:
- allocate skb, if needed
- increment inst->qlen
- call __build_packet_message
- if (inst->qlen >= qthreshold)
   we flush..

Considering __build_packet_message now will call nlmsg_cancel and undo all its 
work on fails, I thought on just dec'ing inst->qlen and releasing the skbuff, 
if it's empty.

I see your point on attempting the flush, good idea. Otherwise we could hit a 
situation that it would be stuck on undoing the last message and this 
situation would be fixed only after inst->timer expires.

It could be like:
build_failure:
     inst->qlen--;

     /* If no other messages in it, we're good to free it. */
     if (!inst->skb->len) {
         kfree_skb(inst->skb);
         inst->skb = NULL;
     }
     else {
         __nfulnl_flush(inst);
     }

What do you think?

> Then, the WARN_ON that Florian added recently should catch that we
> have size miscalculations from __nfulnl_send().

Good point. It may not catch it always. If the failed message was bigger than 
the DONE message, it may go on unnoticed. Actually, even if it warns, we would 
have no idea that a message was dropped a bit earlier, as the stats aren't 
there yet. Maybe another WARN_ONCE on build_failure label?

Thanks,
Marcelo

Re: [PATCH v4 2/3] Do error handling if __build_packet_message fails

[Pablo Neira Ayuso]

On Tue, Nov 04, 2014 at 04:01:54PM -0200, Marcelo Ricardo Leitner wrote:
> >>@@ -708,8 +707,9 @@ nfulnl_log_packet(struct net *net,
> >>
> >>  	inst->qlen++;
> >>
> >>-	__build_packet_message(log, inst, skb, data_len, pf,
> >>-				hooknum, in, out, prefix, plen);
> >>+	if (__build_packet_message(log, inst, skb, data_len, pf,
> >>+				   hooknum, in, out, prefix, plen))
> >>+		goto build_failure;
> >>
> >>  	if (inst->qlen >= qthreshold)
> >>  		__nfulnl_flush(inst);
> >>@@ -726,6 +726,15 @@ unlock_and_release:
> >>  	instance_put(inst);
> >>  	return;
> >>
> >>+build_failure:
> >>+	/* If no other messages in it, we're good to free it. */
> >>+	if (!inst->skb->len) {
> >>+		kfree_skb(inst->skb);
> >>+		inst->skb = NULL;
> >>+	}
> >>+
> >>+	inst->qlen--;
> >
> >For each message that we put into the batch, we increase inst->qlen in
> >one, so I think this decrement isn't enough to leave things in
> >consistent state. If we at least have one message already in the
> >batch, I think it's good to give it a try to deliver it to userspace:
> >
> >         if (inst->skb)
> >                 __nfulnl_flush(inst);
> 
> The idea was to undo just the last attempt and leave the older ones
> where they were...
>
> Currently we:
> - allocate skb, if needed
> - increment inst->qlen
> - call __build_packet_message
> - if (inst->qlen >= qthreshold)
>   we flush..
> 
> Considering __build_packet_message now will call nlmsg_cancel and
> undo all its work on fails, I thought on just dec'ing inst->qlen and
> releasing the skbuff, if it's empty.
>
> I see your point on attempting the flush, good idea. Otherwise we
> could hit a situation that it would be stuck on undoing the last
> message and this situation would be fixed only after inst->timer
> expires.
> 
> It could be like:
> build_failure:
>     inst->qlen--;

We can inst->qlen++ after build_packet_message, so you can skip this
line above.

>     /* If no other messages in it, we're good to free it. */
>     if (!inst->skb->len) {

Now I'd suggest:

      if (inst->qlen == 0) {

>         kfree_skb(inst->skb);
>         inst->skb = NULL;
>     }
>     else {
>         __nfulnl_flush(inst);
>     }
> 
> What do you think?
> 
> >Then, the WARN_ON that Florian added recently should catch that we
> >have size miscalculations from __nfulnl_send().
> 
> Good point. It may not catch it always. If the failed message was
> bigger than the DONE message, it may go on unnoticed. Actually, even
> if it warns, we would have no idea that a message was dropped a bit
> earlier, as the stats aren't there yet. Maybe another WARN_ONCE on
> build_failure label?

That will catch possible miscalculations too, I don't like we're
polluting this with many WARN_ONCE, but I don't see any better way to
catch this size miscalculation bugs, so ahead add it.

BTW, we should also signal the userspace when we fail to build the
message via:

nfnetlink_set_err(net, 0, group, -ENOBUFS);

so it knows that we're losing log messages for whatever reason.
Basically, userspace hits -ENOBUFS when calling recv(), which means
netlink is losing messages. I don't think we really need the
statistics.

I can also see this line:

nlh->nlmsg_len = inst->skb->tail - old_tail;

that can be replaced by nlmsg_end(skb, nlh);

It seems this code needs some care.

Re: [PATCH v4 2/3] Do error handling if __build_packet_message fails

[Marcelo Ricardo Leitner]

On 04-11-2014 16:26, Pablo Neira Ayuso wrote:
> On Tue, Nov 04, 2014 at 04:01:54PM -0200, Marcelo Ricardo Leitner wrote:
>>>> @@ -708,8 +707,9 @@ nfulnl_log_packet(struct net *net,
>>>>
>>>>   	inst->qlen++;
>>>>
>>>> -	__build_packet_message(log, inst, skb, data_len, pf,
>>>> -				hooknum, in, out, prefix, plen);
>>>> +	if (__build_packet_message(log, inst, skb, data_len, pf,
>>>> +				   hooknum, in, out, prefix, plen))
>>>> +		goto build_failure;
>>>>
>>>>   	if (inst->qlen >= qthreshold)
>>>>   		__nfulnl_flush(inst);
>>>> @@ -726,6 +726,15 @@ unlock_and_release:
>>>>   	instance_put(inst);
>>>>   	return;
>>>>
>>>> +build_failure:
>>>> +	/* If no other messages in it, we're good to free it. */
>>>> +	if (!inst->skb->len) {
>>>> +		kfree_skb(inst->skb);
>>>> +		inst->skb = NULL;
>>>> +	}
>>>> +
>>>> +	inst->qlen--;
>>>
>>> For each message that we put into the batch, we increase inst->qlen in
>>> one, so I think this decrement isn't enough to leave things in
>>> consistent state. If we at least have one message already in the
>>> batch, I think it's good to give it a try to deliver it to userspace:
>>>
>>>          if (inst->skb)
>>>                  __nfulnl_flush(inst);
>>
>> The idea was to undo just the last attempt and leave the older ones
>> where they were...
>>
>> Currently we:
>> - allocate skb, if needed
>> - increment inst->qlen
>> - call __build_packet_message
>> - if (inst->qlen >= qthreshold)
>>    we flush..
>>
>> Considering __build_packet_message now will call nlmsg_cancel and
>> undo all its work on fails, I thought on just dec'ing inst->qlen and
>> releasing the skbuff, if it's empty.
>>
>> I see your point on attempting the flush, good idea. Otherwise we
>> could hit a situation that it would be stuck on undoing the last
>> message and this situation would be fixed only after inst->timer
>> expires.
>>
>> It could be like:
>> build_failure:
>>      inst->qlen--;
>
> We can inst->qlen++ after build_packet_message, so you can skip this
> line above.

Ack, okay

>
>>      /* If no other messages in it, we're good to free it. */
>>      if (!inst->skb->len) {
>
> Now I'd suggest:
>
>        if (inst->qlen == 0) {

ack

>
>>          kfree_skb(inst->skb);
>>          inst->skb = NULL;
>>      }
>>      else {
>>          __nfulnl_flush(inst);
>>      }
>>
>> What do you think?
>>
>>> Then, the WARN_ON that Florian added recently should catch that we
>>> have size miscalculations from __nfulnl_send().
>>
>> Good point. It may not catch it always. If the failed message was
>> bigger than the DONE message, it may go on unnoticed. Actually, even
>> if it warns, we would have no idea that a message was dropped a bit
>> earlier, as the stats aren't there yet. Maybe another WARN_ONCE on
>> build_failure label?
>
> That will catch possible miscalculations too, I don't like we're
> polluting this with many WARN_ONCE, but I don't see any better way to
> catch this size miscalculation bugs, so ahead add it.
>
> BTW, we should also signal the userspace when we fail to build the
> message via:
>
> nfnetlink_set_err(net, 0, group, -ENOBUFS);
>
> so it knows that we're losing log messages for whatever reason.
> Basically, userspace hits -ENOBUFS when calling recv(), which means
> netlink is losing messages. I don't think we really need the
> statistics.

Cool. Then we probably don't need the WARN_ON unless we really want those 
numbers, or even so. As we will be calling nfnetlink_set_err, we have other 
ways of debugging. Wouldn't be as ready as a WARN_ONCE in there, yes.. but a 
systemtap script can easily get a hold in there.

So no WARN_ONCE, just the nfnetlink_set_err() call? Like this?

 >        if (inst->qlen == 0) {
 >>          kfree_skb(inst->skb);
 >>          inst->skb = NULL;
 >>      }
 >>      else {
 >>          __nfulnl_flush(inst);
 >>      }
         nfnetlink_set_err(net, 0, group, -ENOBUFS);

So we send whatever we had, and signal the failure..

>
> I can also see this line:
>
> nlh->nlmsg_len = inst->skb->tail - old_tail;
>
> that can be replaced by nlmsg_end(skb, nlh);
>
> It seems this code needs some care.

Agreed. I'll try :)

Thanks,
Marcelo

Re: [PATCH v4 2/3] Do error handling if __build_packet_message fails

[Pablo Neira Ayuso]

On Tue, Nov 04, 2014 at 04:52:31PM -0200, Marcelo Ricardo Leitner wrote:
> >That will catch possible miscalculations too, I don't like we're
> >polluting this with many WARN_ONCE, but I don't see any better way to
> >catch this size miscalculation bugs, so ahead add it.
> >
> >BTW, we should also signal the userspace when we fail to build the
> >message via:
> >
> >nfnetlink_set_err(net, 0, group, -ENOBUFS);
> >
> >so it knows that we're losing log messages for whatever reason.
> >Basically, userspace hits -ENOBUFS when calling recv(), which means
> >netlink is losing messages. I don't think we really need the
> >statistics.
> 
> Cool. Then we probably don't need the WARN_ON unless we really want
> those numbers, or even so. As we will be calling nfnetlink_set_err,
> we have other ways of debugging. Wouldn't be as ready as a WARN_ONCE
> in there, yes.. but a systemtap script can easily get a hold in
> there.
> 
> So no WARN_ONCE, just the nfnetlink_set_err() call? Like this?

The ENOBUFS notice won't be enough. This can be triggered if the rate
of log message is too high, or the userspace process is too slow to
empty the socket queue.

I think we need both WARN_ONCE (tells us that miscalculation is
happening, ie. we have a bug) and the nfnetlink_set_err (tells
userspace it's losing log messages, so logging becomes
unreliable/approximative).

Re: [PATCH v4 2/3] Do error handling if __build_packet_message fails

[Marcelo Ricardo Leitner]

On 04-11-2014 17:04, Pablo Neira Ayuso wrote:
> On Tue, Nov 04, 2014 at 04:52:31PM -0200, Marcelo Ricardo Leitner wrote:
>>> That will catch possible miscalculations too, I don't like we're
>>> polluting this with many WARN_ONCE, but I don't see any better way to
>>> catch this size miscalculation bugs, so ahead add it.
>>>
>>> BTW, we should also signal the userspace when we fail to build the
>>> message via:
>>>
>>> nfnetlink_set_err(net, 0, group, -ENOBUFS);
>>>
>>> so it knows that we're losing log messages for whatever reason.
>>> Basically, userspace hits -ENOBUFS when calling recv(), which means
>>> netlink is losing messages. I don't think we really need the
>>> statistics.
>>
>> Cool. Then we probably don't need the WARN_ON unless we really want
>> those numbers, or even so. As we will be calling nfnetlink_set_err,
>> we have other ways of debugging. Wouldn't be as ready as a WARN_ONCE
>> in there, yes.. but a systemtap script can easily get a hold in
>> there.
>>
>> So no WARN_ONCE, just the nfnetlink_set_err() call? Like this?
>
> The ENOBUFS notice won't be enough. This can be triggered if the rate
> of log message is too high, or the userspace process is too slow to
> empty the socket queue.
>
> I think we need both WARN_ONCE (tells us that miscalculation is
> happening, ie. we have a bug) and the nfnetlink_set_err (tells
> userspace it's losing log messages, so logging becomes
> unreliable/approximative).

yeah.. okay, will put it in too.

Thanks,
Marcelo

Re: [PATCH v4 2/3] Do error handling if __build_packet_message fails

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> BTW, we should also signal the userspace when we fail to build the
> message via:
> 
> nfnetlink_set_err(net, 0, group, -ENOBUFS);
> 
> so it knows that we're losing log messages for whatever reason.
> Basically, userspace hits -ENOBUFS when calling recv(), which means
> netlink is losing messages. I don't think we really need the
> statistics.

Not sure if this is a good idea.

a) __build_packet_message must never fail.
If it does, the kernel has a size accoutning bug somewhere.
b) I see no meaningful way for userspace to handle this error;
there is nothing it can do about it.
c) If it happens, it might be that some userspace logging daemon
suddently dies because it sees an unexpected 'fatal' error.

> It seems this code needs some care.

Agreed...

Re: [PATCH v4 2/3] Do error handling if __build_packet_message fails

[Pablo Neira Ayuso]

On Tue, Nov 04, 2014 at 08:11:20PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > BTW, we should also signal the userspace when we fail to build the
> > message via:
> > 
> > nfnetlink_set_err(net, 0, group, -ENOBUFS);
> > 
> > so it knows that we're losing log messages for whatever reason.
> > Basically, userspace hits -ENOBUFS when calling recv(), which means
> > netlink is losing messages. I don't think we really need the
> > statistics.
> 
> Not sure if this is a good idea.
> 
> a) __build_packet_message must never fail.
> If it does, the kernel has a size accoutning bug somewhere.
> b) I see no meaningful way for userspace to handle this error;
> there is nothing it can do about it.
> c) If it happens, it might be that some userspace logging daemon
> suddently dies because it sees an unexpected 'fatal' error.

userspace should be handling -ENOBUFS already, netlink reports this if
the buffer overruns. Although there's nothing userspace can do to
recover lost messages, this reports that the logging became
unreliable. People that don't mind about this can disable it via
NETLINK_NO_ENOBUFS socket option.

The new nfnetlink_set_err() will also report skb allocation failures
to userspace, which is missing. I would remove those printk there to
report OOM, there's nothing userspace can do with that.

For the unlikely size miscalculation case, this probably will make it
more evident to userspace that we have a bug since userspace will
receive very frequent ENOBUFS report, even under very low netlink
traffic. I would also add a WARN_ONCE there as you added if
__build_packet_message returns an error, so we have two ways to rise
alarms.

Re: [PATCH v4 2/3] Do error handling if __build_packet_message fails

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Tue, Nov 04, 2014 at 08:11:20PM +0100, Florian Westphal wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > BTW, we should also signal the userspace when we fail to build the
> > > message via:
> > > 
> > > nfnetlink_set_err(net, 0, group, -ENOBUFS);
> > > 
> > > so it knows that we're losing log messages for whatever reason.
> > > Basically, userspace hits -ENOBUFS when calling recv(), which means
> > > netlink is losing messages. I don't think we really need the
> > > statistics.
> > 
> > Not sure if this is a good idea.
> > 
> > a) __build_packet_message must never fail.
> > If it does, the kernel has a size accoutning bug somewhere.
> > b) I see no meaningful way for userspace to handle this error;
> > there is nothing it can do about it.
> > c) If it happens, it might be that some userspace logging daemon
> > suddently dies because it sees an unexpected 'fatal' error.
> 
> userspace should be handling -ENOBUFS already, netlink reports this if
> the buffer overruns.

You're right.

> I would remove those printk there to
> report OOM, there's nothing userspace can do with that.

Agreed.