Re: [PATCH next] netfilter: nfqueue: batch verdict support

Re: [PATCH next] netfilter: nfqueue: batch verdict support

[Eric Dumazet]

Le vendredi 10 juin 2011 à 00:14 +0200, Florian Westphal a écrit :
> Introduces a new nfnetlink type that applies a given
> verdict to all queued packets with an id <= the id in the verdict
> message.
> 
> If a mark is provided it is applied to all matched packets.
> 
> This reduces the number of verdicts that have to be sent.
> Applications that make use of this feature need to maintain
> a timeout to send a batchverdict periodically to avoid starvation.
> 
> Signed-off-by: Florian Westphal <fw@strlen.de>

The real question hidden here is : "Should packet ids be monotonic" in
current implementation and all future ones ?

Before we accept this patch, we should make sure packets id are
monotonic, and I am afraid its not the case right now.

I suggest following patch then.

[PATCH] netfilter: nfqueue: assert monotonic packet ids

Packet identifier is currently setup in nfqnl_build_packet_message(),
using one atomic_inc_return().

Problem is that since several cpus might concurrently call
nfqnl_enqueue_packet() for the same queue, we can deliver packets to
consumer in non monotonic way (packet N+1 being delivered after packet
N)

This patch moves the packet id setup from nfqnl_build_packet_message()
to nfqnl_enqueue_packet() to guarantee correct delivery order.

This also removes one atomic operation.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
CC: Florian Westphal <fw@strlen.de>
CC: Pablo Neira Ayuso <pablo@netfilter.org>
CC: Eric Leblond <eric@regit.org>
---
 net/netfilter/nfnetlink_queue.c |   26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
index fdd2faf..bccaf3b 100644
--- a/net/netfilter/nfnetlink_queue.c
+++ b/net/netfilter/nfnetlink_queue.c
@@ -58,7 +58,7 @@ struct nfqnl_instance {
  */
 	spinlock_t	lock;
 	unsigned int	queue_total;
-	atomic_t	id_sequence;		/* 'sequence' of pkt ids */
+	unsigned int	id_sequence;		/* 'sequence' of pkt ids */
 	struct list_head queue_list;		/* packets in queue */
 };
 
@@ -213,13 +213,15 @@ nfqnl_flush(struct nfqnl_instance *queue, nfqnl_cmpfn cmpfn, unsigned long data)
 
 static struct sk_buff *
 nfqnl_build_packet_message(struct nfqnl_instance *queue,
-			   struct nf_queue_entry *entry)
+			   struct nf_queue_entry *entry,
+			   __be32 **packet_id_ptr)
 {
 	sk_buff_data_t old_tail;
 	size_t size;
 	size_t data_len = 0;
 	struct sk_buff *skb;
-	struct nfqnl_msg_packet_hdr pmsg;
+	struct nlattr *nla;
+	struct nfqnl_msg_packet_hdr *pmsg;
 	struct nlmsghdr *nlh;
 	struct nfgenmsg *nfmsg;
 	struct sk_buff *entskb = entry->skb;
@@ -272,12 +274,11 @@ nfqnl_build_packet_message(struct nfqnl_instance *queue,
 	nfmsg->version = NFNETLINK_V0;
 	nfmsg->res_id = htons(queue->queue_num);
 
-	entry->id = atomic_inc_return(&queue->id_sequence);
-	pmsg.packet_id 		= htonl(entry->id);
-	pmsg.hw_protocol	= entskb->protocol;
-	pmsg.hook		= entry->hook;
-
-	NLA_PUT(skb, NFQA_PACKET_HDR, sizeof(pmsg), &pmsg);
+	nla = __nla_reserve(skb, NFQA_PACKET_HDR, sizeof(*pmsg));
+	pmsg = nla_data(nla);
+	pmsg->hw_protocol	= entskb->protocol;
+	pmsg->hook		= entry->hook;
+	*packet_id_ptr		= &pmsg->packet_id;
 
 	indev = entry->indev;
 	if (indev) {
@@ -389,6 +390,7 @@ nfqnl_enqueue_packet(struct nf_queue_entry *entry, unsigned int queuenum)
 	struct sk_buff *nskb;
 	struct nfqnl_instance *queue;
 	int err = -ENOBUFS;
+	__be32 *packet_id_ptr;
 
 	/* rcu_read_lock()ed by nf_hook_slow() */
 	queue = instance_lookup(queuenum);
@@ -402,7 +404,7 @@ nfqnl_enqueue_packet(struct nf_queue_entry *entry, unsigned int queuenum)
 		goto err_out;
 	}
 
-	nskb = nfqnl_build_packet_message(queue, entry);
+	nskb = nfqnl_build_packet_message(queue, entry, &packet_id_ptr);
 	if (nskb == NULL) {
 		err = -ENOMEM;
 		goto err_out;
@@ -421,6 +423,8 @@ nfqnl_enqueue_packet(struct nf_queue_entry *entry, unsigned int queuenum)
 				 queue->queue_total);
 		goto err_out_free_nskb;
 	}
+	entry->id = ++queue->id_sequence;
+	*packet_id_ptr = htonl(entry->id);
 
 	/* nfnetlink_unicast will either free the nskb or add it to a socket */
 	err = nfnetlink_unicast(nskb, &init_net, queue->peer_pid, MSG_DONTWAIT);
@@ -870,7 +874,7 @@ static int seq_show(struct seq_file *s, void *v)
 			  inst->peer_pid, inst->queue_total,
 			  inst->copy_mode, inst->copy_range,
 			  inst->queue_dropped, inst->queue_user_dropped,
-			  atomic_read(&inst->id_sequence), 1);
+			  inst->id_sequence, 1);
 }
 
 static const struct seq_operations nfqnl_seq_ops = {

Re: [PATCH next] netfilter: nfqueue: batch verdict support

[Florian Westphal]

Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > Introduces a new nfnetlink type that applies a given
> > verdict to all queued packets with an id <= the id in the verdict
> > message.
> > 
> > If a mark is provided it is applied to all matched packets.
> > 
> > This reduces the number of verdicts that have to be sent.
> > Applications that make use of this feature need to maintain
> > a timeout to send a batchverdict periodically to avoid starvation.
> > 
> > Signed-off-by: Florian Westphal <fw@strlen.de>
> 
> The real question hidden here is : "Should packet ids be monotonic" in
> current implementation and all future ones ?
> 
> Before we accept this patch, we should make sure packets id are
> monotonic, and I am afraid its not the case right now.

You're right, good catch.

I was fooled by atomic_inc being monotonically increasing, but since
packet building is not protected by the queue spin lock
reordering can indeed happen.

> [PATCH] netfilter: nfqueue: assert monotonic packet ids
> 
> Packet identifier is currently setup in nfqnl_build_packet_message(),
> using one atomic_inc_return().
> 
> Problem is that since several cpus might concurrently call
> nfqnl_enqueue_packet() for the same queue, we can deliver packets to
> consumer in non monotonic way (packet N+1 being delivered after packet
> N)

I would actually consider your patch a bug fix...

Thanks a lot for spending time on this!

Re: [PATCH next] netfilter: nfqueue: batch verdict support

[Patrick McHardy]

On 03.07.2011 12:24, Eric Dumazet wrote:
> The real question hidden here is : "Should packet ids be monotonic" in
> current implementation and all future ones ?
> 
> Before we accept this patch, we should make sure packets id are
> monotonic, and I am afraid its not the case right now.
> 
> I suggest following patch then.
> 
> [PATCH] netfilter: nfqueue: assert monotonic packet ids
> 
> Packet identifier is currently setup in nfqnl_build_packet_message(),
> using one atomic_inc_return().
> 
> Problem is that since several cpus might concurrently call
> nfqnl_enqueue_packet() for the same queue, we can deliver packets to
> consumer in non monotonic way (packet N+1 being delivered after packet
> N)
> 
> This patch moves the packet id setup from nfqnl_build_packet_message()
> to nfqnl_enqueue_packet() to guarantee correct delivery order.
> 
> This also removes one atomic operation.
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> CC: Florian Westphal <fw@strlen.de>
> CC: Pablo Neira Ayuso <pablo@netfilter.org>
> CC: Eric Leblond <eric@regit.org>

Applied, thanks Eric.