| Message ID | 1309688655.2523.16.camel@edumazet-laptop |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
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! -- 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
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. -- 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
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 = {