peer_pid checking in ip_queue

Hi,

I patched ip_queue.c to modify the default behaviour if peer_pid is 0. In this case, there's no program in user space to receive the skbuff via netlink, but ip_queue will allocate space for the skbuff and after checking if peer_pid is 0, it will destroy this skbuff allocated. Am I missing anything?

best regards,
Pablo

P.S: BTW, Harald, I noticed that I had some problems with my smtp server and some email got lost, I sent you an email with a "digest" of all my recent patches, did you receive it? If not, please let me know.

------------------------------------------------------------------------

net/ipv4/netfilter/ip_queue.c +3 -6

@@ -277,6 +277,9 @@
 	struct sk_buff *nskb;
 	struct ipq_queue_entry *entry;
 
+	if (!peer_pid)
+		return -EINVAL;
+
 	if (copy_mode == IPQ_COPY_NONE)
 		return -EAGAIN;
 

@@ -303,9 +306,6 @@
 
 	write_lock_bh(&queue_lock);
 
-	if (!peer_pid)
-		goto err_out_free_nskb; 
-
  	/* netlink_unicast will either free the nskb or attach it to a socket */ 
 	status = netlink_unicast(ipqnl, nskb, peer_pid, MSG_DONTWAIT);
 	if (status < 0)

@@ -318,9 +318,6 @@
 	write_unlock_bh(&queue_lock);
 	return status;
 
-err_out_free_nskb:
-	kfree_skb(nskb); 
-	
 err_out_unlock:
 	write_unlock_bh(&queue_lock);
 

peer_pid is protected by queue_lock, you move it out of the locked
section. This leads to a small race which can cause message loss. Not sure if it's bad though as netlink is unreliable itself.

Hi Patrick,

Quoted 2 lines · Patrick McHardy

peer_pid is protected by queue_lock, you move it out of the locked
section. This leads to a small race which can cause message loss. Not sure if it's bad though as netlink is unreliable itself.

yes it's true, well if i'm not wrong, this small race could happen for first packets which are going to be enqueued, is it?.

For example, two packets arrive almost at the same time, ip_queue allocates space for both new entries to be enqueued, first packet is not enqueued because peer_pid is 0 but, in that moment, the user space program binds to ip_queue's netlink sockets, but entry for second packet is already created and peer_pid is not 0 anymore, so this packet is enqueue. With my patch, both packets will be lost, with current implementation one is lost and the other one is enqueued.

By other hand, I think that if user programs hungs or dies, we stress the kernel allocating and releasing memory all the time. So, my question is, is really a problem losing these packets?

thanks for the feedback,
Pablo

It can happen if peer_pid is checked outside the locked section on one CPU and changed on another at the same time. I don't consider it important, as the exact point in time at which the first packet will be received is non-deterministic for userspace anyway.

What about when the user program closes? How bad it is to have read the pid and then there is no userspace there?

Hi,

Quoted 3 lines · Henrik Nordstrom

What about when the user program closes? How bad it is to have read the pid and then there is no userspace there?

As Patrick pointed out before, netlink is not reliable, so it will drop the packets sent to an user space which doesn't exist without notification. But if the user space program closes properly, peer_pid will be reset. Have a look at ipq_rcv_nl_event and netlink_release. AFAIK, if the user space program hangs or dies, kernel will release the socket later, so for quite some time netlink will drop packets. Am I right?

regards,
Pablo

Quoted 4 lines · Patrick McHardy

It can happen if peer_pid is checked outside the locked section on one CPU and changed on another at the same time. I don't consider it important, as the exact point in time at which the first packet will be received is non-deterministic for userspace anyway.

What about when the user program closes? How bad it is to have read the pid and then there is no userspace there?

Regards
Henrik

It can happen if peer_pid is checked outside the locked section on one CPU and changed on another at the same time. I don't consider it important, as the exact point in time at which the first packet will be received is non-deterministic for userspace anyway. But we can probably also do it without a race-conditions, ipq_build_packet_message read_locks queue_lock, directly after that it is write_locked by ipq_enqueue_packet. As multiple threads will queue at the write_lock anyway it seems we can just replace it by a spin_lock and lock the entire section. But let me think some more about it ..

Regards
Patrick

Quoted 9 lines · Patrick McHardy

It can happen if peer_pid is checked outside the locked section on one CPU and changed on another at the same time. I don't consider it important, as the exact point in time at which the first packet will be received is non-deterministic for userspace anyway. But we can probably also do it without a race-conditions, ipq_build_packet_message read_locks queue_lock, directly after that it is write_locked by ipq_enqueue_packet. As multiple threads will queue at the write_lock anyway it seems we can just replace it by a spin_lock and lock the entire section. But let me think some more about it ..

any results so far? If not, I will just postpone this minor issue.

Quoted 4 lines

Regards
Patrick

Hi,

I patched ip_queue.c to modify the default behaviour if peer_pid is 0. In this case, there's no program in user space to receive the skbuff via netlink, but ip_queue will allocate space for the skbuff and after checking if peer_pid is 0, it will destroy this skbuff allocated. Am I missing anything?

Pablo