[PATCH] peer_pid checking in ip_queue

[PATCH] peer_pid checking in ip_queue

[Pablo Neira]

[-- Attachment #1: Type: text/plain, Size: 541 bytes --]

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.

[-- Attachment #2: ip_queue-peer_pid.patch --]
[-- Type: text/plain, Size: 769 bytes --]

--- linux-2.6.3-old/net/ipv4/netfilter/ip_queue.c	2004-02-18 04:59:59.000000000 +0100
+++ linux-2.6.3/net/ipv4/netfilter/ip_queue.c	2004-03-23 10:31:16.000000000 +0100
@@ -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);
 

Re: [PATCH] peer_pid checking in ip_queue

[Patrick McHardy]

Hi Pablo,

Pablo Neira wrote:
> 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?

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.

Regards
Patrick

> 
> 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.
> 
> 
> ------------------------------------------------------------------------
> 
> --- linux-2.6.3-old/net/ipv4/netfilter/ip_queue.c	2004-02-18 04:59:59.000000000 +0100
> +++ linux-2.6.3/net/ipv4/netfilter/ip_queue.c	2004-03-23 10:31:16.000000000 +0100
> @@ -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);
>  

Re: [PATCH] peer_pid checking in ip_queue

[Pablo Neira]

Hi Patrick,

Patrick McHardy wrote:

> 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

Re: [PATCH] peer_pid checking in ip_queue

[Patrick McHardy]

Pablo Neira wrote:
> Hi Patrick,
> 
> Patrick McHardy wrote:
> 
>> 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?.

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

> 
> 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
> 

Re: [PATCH] peer_pid checking in ip_queue

[Henrik Nordstrom]

On Tue, 23 Mar 2004, Patrick McHardy wrote:

> 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

Re: [PATCH] peer_pid checking in ip_queue

[Pablo Neira]

Hi,

Henrik Nordstrom wrote:

>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

Re: [PATCH] peer_pid checking in ip_queue

[Patrick McHardy]

Henrik Nordstrom wrote:
> On Tue, 23 Mar 2004, Patrick McHardy wrote:
> 
> 
>>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?

It's no problem, netlink_unicast checks for existance of the socket,
which will be gone with the reader. If no socket is found it returns
-ECONNREFUSED.

Regards
Patrick


> 
> Regards
> Henrik
> 

Re: [PATCH] peer_pid checking in ip_queue

[Patrick McHardy]

Pablo Neira wrote:
> Hi,
> 
> Henrik Nordstrom wrote:
> 
>> 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?

Well, if it hangs the socket buffer will fill over time until
it reaches its limits, but there is nothing we can do. But
we can do something for the almost-known-no-listener case :)

Regards
Patrick

> 
> regards,
> Pablo
> 

Re: [PATCH] peer_pid checking in ip_queue

[Harald Welte]

[-- Attachment #1: Type: text/plain, Size: 839 bytes --]

On Tue, Mar 23, 2004 at 12:09:24PM +0100, Pablo Neira wrote:
> 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?

seems fine to me, I'll put this in my list of pending patches

> Pablo
-- 
- Harald Welte <laforge@netfilter.org>             http://www.netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] peer_pid checking in ip_queue

[Harald Welte]

[-- Attachment #1: Type: text/plain, Size: 1162 bytes --]

On Tue, Mar 23, 2004 at 11:44:15PM +0100, Patrick McHardy wrote:
> 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.

> Regards
> Patrick


-- 
- Harald Welte <laforge@netfilter.org>             http://www.netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] peer_pid checking in ip_queue

[Patrick McHardy]

Harald Welte wrote:
> On Tue, Mar 23, 2004 at 11:44:15PM +0100, Patrick McHardy wrote:
> 
>>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.

Not yet, but I will get to it next week.

> 
> 
>>Regards
>>Patrick
> 
> 
>